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

Ensure the proxy service shuts down properly in case of partial init #50710

Merged
merged 2 commits into from
Jan 7, 2025

Conversation

hugoShaka
Copy link
Contributor

@hugoShaka hugoShaka commented Jan 2, 2025

While fixing a flaky test in #50702 I observed that bothprocess.Close() and process.Shutdown() were hanging.

Down the rabbit hole was the initProxyEndpoint doing an incomplete startup and not cleaning up the services it started.

This PR ensures that proxy.shutdown is registered before we start running the proxy.* components so we have a chance to cleanup even if something goes wrong and we never finish the init.

Explanation:

In the PR I linked, I tried to wait for shutdown before cleaning up channels created by the test. This failed because initProxyEndpoint:

  • creates the proxy.web service without process.ExitContext() (http.Server doesn't take any context)
  • tries to create many components including db:proxy
  • those component use authClient to retrieve information and can fail to get created
  • finally registers the OnExit() function closing the proxy.web server and causing the proxy.web service to exit.

Now if anything else bad happens between the proxy:web registration and the proxy.shutdown registration (e.g. we do a shutdown and close the auth client), the proxy.web service will continue to run and block the shutdown. This also leaves dangling routines and resources. This is an issue in tests as we quickly start/stop teleport instances.

@hugoShaka hugoShaka added the bug label Jan 2, 2025
@hugoShaka hugoShaka requested a review from zmb3 January 2, 2025 21:38
@hugoShaka hugoShaka changed the title Ensure the proxy service shuts dowqn properly in case of partial init Ensure the proxy service shuts down properly in case of partial init Jan 2, 2025
@hugoShaka hugoShaka added the no-changelog Indicates that a PR does not require a changelog entry label Jan 2, 2025
lib/service/service.go Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
lib/service/service.go Outdated Show resolved Hide resolved
Align logging punctuation with the new log style

Co-authored-by: rosstimothy <[email protected]>
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from zmb3 January 7, 2025 10:00
@hugoShaka hugoShaka added this pull request to the merge queue Jan 7, 2025
Merged via the queue into master with commit 0ebacac Jan 7, 2025
41 checks passed
@hugoShaka hugoShaka deleted the hugo/ensure-proxy-shutdown-on-failed-init branch January 7, 2025 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug no-changelog Indicates that a PR does not require a changelog entry size/sm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants