haproxytech / dataplaneapi

HAProxy Data Plane API
https://www.haproxy.com/documentation/dataplaneapi/
Apache License 2.0
328 stars 76 forks source link

Bug/Minor: Remove sockpair from HAPROXY_MASTER_CLI #330

Open git001 opened 7 months ago

git001 commented 7 months ago

With commit https://github.com/haproxy/haproxy/commit/8a02257d88276e2f2f10c407d2961995f74a192c was the sockpair@ added to the master socket.

fix: https://github.com/haproxytech/dataplaneapi/issues/329

mjuraga commented 7 months ago

Hey, this will work for sure. But to make a proper fix, we would need to do the following:

With this logic we would be safe that any other sockets, or types of sockets added to the env variable won't break our parsing and we will always use the one we actually need.

georgijd-form3 commented 7 months ago

I would also like to add that this code (not the PR itself) would override the haproxyOptions.MasterRuntime even if the environment variable is not set:

https://github.com/haproxytech/dataplaneapi/blob/2e6d65889706f9c0553e4083edd99d829e0062f5/configure_data_plane.go#L123-L124

because misc.IsUnixSocket returns true for empty strings:

https://github.com/haproxytech/dataplaneapi/blob/2e6d65889706f9c0553e4083edd99d829e0062f5/misc/misc.go#L156-L166

So if someone tries to set the MasterRuntime with the--master-runtime/-m flag or haproxy.master_runtime in the config file that value will be overwritten with an empty string

I guess it relies on the fact that HAPROXY_MASTER_CLI is always set when HAPROXY_MWORKER is set but I don't know if that's a safe assumption

git001 commented 7 months ago

Hey, this will work for sure. But to make a proper fix, we would need to do the following:

* the variable `HAPROXY_MASTER_CLI` actually returns values separated by `;`, in the future there could be multiple of those, so we need to spilt the string by `;`

* we need to iterate through it and find the one with `unix@` prefix as we currently only work with unix stats sockets, check if it works and use it

With this logic we would be safe that any other sockets, or types of sockets added to the env variable won't break our parsing and we will always use the one we actually need.

Okay. How should then the haproxyOptions.MasterRuntime be set for more the one socket?

mjuraga commented 7 months ago

@georgijd-form3 is right, the IsUnixSocket should be improved to properly check whether the string is unix@, we should check wheter the exists also.

If we have multiple sockets set, we should use the first one that is valid. As it is good enough for what we need it for.

git001 commented 7 months ago

@mjuraga

If we have multiple sockets set, we should use the first one that is valid. As it is good enough for what we need it for.

Looks like there is a code which checks if the socket is usable.

https://github.com/haproxytech/dataplaneapi/blob/f36e1efd9875a9cf23bf46eea6f1518aa1fe7484/configure_data_plane.go#L1014-L1019

maybe there is something similar to canUseMasterSocketReload or can this function be used to check if a master socket is usable?

georgijd-form3 commented 5 months ago

Hi everyone,

Any updates on this?