This repository was archived by the owner on Sep 30, 2024. It is now read-only.
fix(appliance): cache authorization status#64213
Merged
Merged
Conversation
In order to reduce the cost of calls to auth-gated endpoints, cache valid admin passwords in-memory. The appliance's frontend calls auth-gated endpoints in a tight loop, and bcrypt checking is intentionally an expensive operation. This could occasionally cause the appliance-frontend to disconnect from the backend. We observed frontend's nginx reporting an upstream connection close, and exec'ing into its pod and curling the backend regularly hung.
craigfurman
referenced
this pull request
Aug 1, 2024
When the admin has first installed Code Search Suite, the appliance waits for the admin to click an "I'm ready" button. This causes the appliance to unblock a background thread that periodically checks the health of sg-frontend. When it is healthy, it ensures that the ingress-facing frontend is pointed to sg-frontend. And when it is not, it points to the appliance-frontend. Pointing to the appliance-frontend is its initial state pre-install, and given that we've just installed sg, the appliance switches the service over quickly. Meanwhile, clicking this button transitions the frontend to a "refresh" state (this being one of the states in its state machine). This causes the UI to reload the web page. The reason we have to do this is that it is a way to "redirect to yourself". If the ingress-facing service has been repointed, refreshing like this will show site-admin, which is the desired behavior. The issue this commit fixes, is that this is racy: upon refresh, the browser tab queries the appliance (via an nginx proxy hosted on the same domain serving appliance-frontend) for its state. We have to store state on the backend (specifically, we use a ConfigMap annotation), so that the appliance can do the right thing if it has been rebooted at any time. This will help power future features such as UI-driven upgrades. The race occurs if, upon refresh, the ingress-facing service has been flipped over to sg-frontend. The appliance API that answered the state questions is no longer available! In general, we can't tell the difference between this expected turn of events, and a state in which the backend can't be reached. This commit mitigates the race by setting the appliance UI to refresh if it cannot reach the appliance API. This looks no different to a "disconnected" state if things really are broken, but in the expected path, it will resolve the race by retrying. This commit reliably causes the appliance-driven installation flow to redirect to site-admin after clicking "ready", according to my experimentation in minikube. I suspect that this would be the case even without https://github.com/sourcegraph/sourcegraph/pull/64213, which fixes an unrelated performance issue. I suspect we need both, otherwise the appliance UI will regularly disconnect for prolonged periods of time, which is confusing.
craigfurman
referenced
this pull request
Aug 1, 2024
When the admin has first installed Code Search Suite, the appliance waits for the admin to click an "I'm ready" button. This causes the appliance to unblock a background thread that periodically checks the health of sg-frontend. When it is healthy, it ensures that the ingress-facing frontend is pointed to sg-frontend. And when it is not, it points to the appliance-frontend. Pointing to the appliance-frontend is its initial state pre-install, and given that we've just installed sg, the appliance switches the service over quickly. Meanwhile, clicking this button transitions the frontend to a "refresh" state (this being one of the states in its state machine). This causes the UI to reload the web page. The reason we have to do this is that it is a way to "redirect to yourself". If the ingress-facing service has been repointed, refreshing like this will show site-admin, which is the desired behavior. The issue this commit fixes, is that this is racy: upon refresh, the browser tab queries the appliance (via an nginx proxy hosted on the same domain serving appliance-frontend) for its state. We have to store state on the backend (specifically, we use a ConfigMap annotation), so that the appliance can do the right thing if it has been rebooted at any time. This will help power future features such as UI-driven upgrades. The race occurs if, upon refresh, the ingress-facing service has been flipped over to sg-frontend. The appliance API that answered the state questions is no longer available! In general, we can't tell the difference between this expected turn of events, and a state in which the backend can't be reached. This commit mitigates the race by setting the appliance UI to refresh if it cannot reach the appliance API. This looks no different to a "disconnected" state if things really are broken, but in the expected path, it will resolve the race by retrying. This commit reliably causes the appliance-driven installation flow to redirect to site-admin after clicking "ready", according to my experimentation in minikube. I suspect that this would be the case even without https://github.com/sourcegraph/sourcegraph/pull/64213, which fixes an unrelated performance issue. I suspect we need both, otherwise the appliance UI will regularly disconnect for prolonged periods of time, which is confusing. Closes https://linear.app/sourcegraph/issue/REL-308/appliance-frontend-seems-to-disconnect-the-backend-during-installation
Chickensoupwithrice
approved these changes
Aug 1, 2024
sourcegraph-release-bot
pushed a commit
that referenced
this pull request
Aug 1, 2024
In order to reduce the cost of calls to auth-gated endpoints, cache valid admin passwords in-memory. The appliance's frontend calls auth-gated endpoints in a tight loop, and bcrypt checking is intentionally an expensive operation. This could occasionally cause the appliance-frontend to disconnect from the backend. We observed frontend's nginx reporting an upstream connection close, and exec'ing into its pod and curling the backend regularly hung. (cherry picked from commit 156aa5a)
craigfurman
pushed a commit
that referenced
this pull request
Aug 1, 2024
craigfurman
referenced
this pull request
Aug 1, 2024
When the admin has first installed Code Search Suite, the appliance waits for the admin to click an "I'm ready" button. This causes the appliance to unblock a background thread that periodically checks the health of sg-frontend. When it is healthy, it ensures that the ingress-facing frontend is pointed to sg-frontend. And when it is not, it points to the appliance-frontend. Pointing to the appliance-frontend is its initial state pre-install, and given that we've just installed sg, the appliance switches the service over quickly. Meanwhile, clicking this button transitions the frontend to a "refresh" state (this being one of the states in its state machine). This causes the UI to reload the web page. The reason we have to do this is that it is a way to "redirect to yourself". If the ingress-facing service has been repointed, refreshing like this will show site-admin, which is the desired behavior. The issue this commit fixes, is that this is racy: upon refresh, the browser tab queries the appliance (via an nginx proxy hosted on the same domain serving appliance-frontend) for its state. We have to store state on the backend (specifically, we use a ConfigMap annotation), so that the appliance can do the right thing if it has been rebooted at any time. This will help power future features such as UI-driven upgrades. The race occurs if, upon refresh, the ingress-facing service has been flipped over to sg-frontend. The appliance API that answered the state questions is no longer available! In general, we can't tell the difference between this expected turn of events, and a state in which the backend can't be reached. This commit mitigates the race by setting the appliance UI to refresh if it cannot reach the appliance API. This looks no different to a "disconnected" state if things really are broken, but in the expected path, it will resolve the race by retrying. This commit reliably causes the appliance-driven installation flow to redirect to site-admin after clicking "ready", according to my experimentation in minikube. I suspect that this would be the case even without https://github.com/sourcegraph/sourcegraph/pull/64213, which fixes an unrelated performance issue. I suspect we need both, otherwise the appliance UI will regularly disconnect for prolonged periods of time, which is confusing. Closes https://linear.app/sourcegraph/issue/REL-308/appliance-frontend-seems-to-disconnect-the-backend-during-installation (cherry picked from commit e54407d)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In order to reduce the cost of calls to auth-gated endpoints, cache valid admin passwords in-memory. The appliance's frontend calls auth-gated endpoints in a tight loop, and bcrypt checking is intentionally an expensive operation.
This could occasionally cause the appliance-frontend to disconnect from the backend. We observed frontend's nginx reporting an upstream connection close, and exec'ing into its pod and curling the backend regularly hung.
I collected two CPU profiles from the appliance backend: one without this commit, one with it. In both cases, SG was not being installed - the frontend was running, and I had a browser tab open, so that the browser was hitting the backend frequently via the nginx API proxy.
Without this fix:
With this fix:
See the test plan below for how I obtained these CPU profiles.
2 things stand out between them: without this fix, the total CPU time consumed over the 30-second profiling period is 1000s of times larger! On my mac (so not even contending with other processes on a kubernetes node), it used 25 seconds of CPU time - almost saturating a core. We can also see that calls to
bcrypt.CompareHashAndPassword()are responsible for all of this.It's perhaps not ideal from a security perspective to memory-cache the password, but subjectively this trade-off seems like a reasonable way to get moving. Let me know what you think though.
This is a necessary step for https://linear.app/sourcegraph/issue/REL-308/appliance-frontend-seems-to-disconnect-the-backend-during-installation but does not close it. This is because the disconnection bug still occurs, after clicking wait-for-admin, but I think this instance of it is for a different reason. See https://github.com/sourcegraph/sourcegraph/pull/64216 for an explanation and fix of that reason.
Test plan
Starting on the https://github.com/sourcegraph/sourcegraph/pull/64211 branch, not this one:
In one terminal:
In another:
Navigate to localhost:8889 in a web browser and log into the appliance. You don't need to begin installing SG, just leave the tab open.
In another terminal:
go tool pprof -png -output appliance-cpu-main.png 'http://localhost:6061/debug/pprof/profile?seconds=30Repeat the experiment for this branch (but with https://github.com/sourcegraph/sourcegraph/pull/64211 merged into it, for pprof), and compare profiles.
Finally, I deployed this branch to my local minikube environment to see how it interacted with the ingress stack:
I saw no disconnections during SG's installation, until the race condition I described further up kicked in after clicking wait-for-admin, and I had to refresh the page in order to see site-admin.
Changelog