Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug/Minor: Remove sockpair from HAPROXY_MASTER_CLI #330

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

git001
Copy link

@git001 git001 commented Apr 3, 2024

With commit haproxy/haproxy@8a02257 was the sockpair@ added to the master socket.

fix: #329

With commit haproxy/haproxy@8a02257
was the `sockpair@` added to the master socket.

fix: haproxytech#329
@mjuraga
Copy link
Collaborator

mjuraga commented Apr 10, 2024

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.

@georgijd-form3
Copy link
Contributor

georgijd-form3 commented Apr 10, 2024

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:

masterRuntime := os.Getenv("HAPROXY_MASTER_CLI")
if misc.IsUnixSocketAddr(masterRuntime) {

because misc.IsUnixSocket returns true for empty strings:

dataplaneapi/misc/misc.go

Lines 156 to 166 in 2e6d658

func IsUnixSocketAddr(addr string) bool {
if strings.HasPrefix(addr, "ipv4@") || strings.HasPrefix(addr, "ipv6@") {
return false
}
// check if it has semicolon
if strings.Contains(addr, ":") {
return false
}
return true
}

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
Copy link
Author

git001 commented Apr 10, 2024

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
Copy link
Collaborator

mjuraga commented Apr 12, 2024

@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
Copy link
Author

git001 commented Apr 13, 2024

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

client, err := client_native.New(cyx, opt...)
if err != nil {
log.Fatalf("Error initializing configuration client: %v", err)
}

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

@georgijd-form3
Copy link
Contributor

Hi everyone,

Any updates on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Plane API is killed by SIGTERM in openshift
3 participants