dataplaneapi icon indicating copy to clipboard operation
dataplaneapi copied to clipboard

Bug/Minor: Remove sockpair from HAPROXY_MASTER_CLI

Open git001 opened this issue 10 months ago • 6 comments

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

git001 avatar Apr 03 '24 13:04 git001

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.

mjuraga avatar Apr 10 '24 08:04 mjuraga

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

georgijd-form3 avatar Apr 10 '24 09:04 georgijd-form3

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?

git001 avatar Apr 10 '24 12:04 git001

@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.

mjuraga avatar Apr 12 '24 14:04 mjuraga

@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?

git001 avatar Apr 13 '24 12:04 git001

Hi everyone,

Any updates on this?

georgijd-form3 avatar Jun 03 '24 11:06 georgijd-form3