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

[FIXED] Monitoring: tls configuration not updated on reload #2714

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Conversation

kozlovic
Copy link
Member

When creating the http server, we need to provide a TLS configuration.
After a config reload, the new TLS config would not be reflected.

We had the same issue with Websocket and was fixed with the use
of tls.Config.GetConfigForClient API, which makes the TLS handshake
to ask for a TLS config. That fix for websocket was simply not applied
to the HTTPs monitoring case.

I have also fixed some flappers due to the use of localhost instead
of 127.0.0.1 (connections possibly would resolve to some IPv6 address
that the server would not accept, etc..)

Signed-off-by: Ivan Kozlovic ivan@synadia.com

When creating the http server, we need to provide a TLS configuration.
After a config reload, the new TLS config would not be reflected.

We had the same issue with Websocket and was fixed with the use
of tls.Config.GetConfigForClient API, which makes the TLS handshake
to ask for a TLS config. That fix for websocket was simply not applied
to the HTTPs monitoring case.

I have also fixed some flappers due to the use of localhost instead
of 127.0.0.1 (connections possibly would resolve to some IPv6 address
that the server would not accept, etc..)

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic
Copy link
Member Author

@matthiashanel I have fixed some tests that tended to flap because of the use of "localhost" since it can resolve to IPv6 (::1 or some other IPv6 local ips) which server may not accept. You will remember that I showed you that once on a google meet because you had a flapper and that was the reason.

Copy link
Contributor

@matthiashanel matthiashanel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kozlovic kozlovic merged commit 4f37cbd into main Nov 30, 2021
@kozlovic kozlovic deleted the fix_2713 branch November 30, 2021 19:56
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.

3 participants