- Notifications
You must be signed in to change notification settings - Fork1k
chore: move app proxying code to workspaceapps pkg#6998
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Moves path-app, subdomain-app and reconnecting PTY proxying to the newworkspaceapps.WorkspaceAppServer struct. This is in preparation forexternal workspace proxies.Updates app logout flow to avoid redirecting to coder-logout.${app_host}on logout. Instead, all subdomain app tokens owned by the logging-outuser will be deleted every time you logout for simplicity sake.Tests will remain in their original package, pending being moved to anapptest package (or similar).Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1,8 @@ | |||
| package workspaceapps_test | |||
| // NOTE: for now, app proxying tests are still in their old locations, pending | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I assume this will be part of moons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TBD exactly, but the unit tests will live in APGL and called from both APGL and enterprise coderd with embedded and external moons/workspace proxies.
coderd/workspaceapps/proxy.go Outdated
| // TODO: Fix this later | ||
| // s.WebsocketWaitMutex.Lock() | ||
| // s.WebsocketWaitGroup.Add(1) | ||
| // s.WebsocketWaitMutex.Unlock() | ||
| // defer s.WebsocketWaitGroup.Done() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This will cause leaks if merged before fixing this, right? We could just add aClose func to this server, which has it's own mutex and wait group for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can do that.
Question, why do we need a mutex and a wait group? Aren't wait groups already thread safe?
This code in the close locks the mutex then waits.
Lines 809 to 811 in7e0ea10
| api.WebsocketWaitMutex.Lock() | |
| api.WebsocketWaitGroup.Wait() | |
| api.WebsocketWaitMutex.Unlock() |
So if a new websocket comes in while we are closing, the new websocket conn gets block (trys to lock). Then the wait group finises, then the new websocket conn continues as the lock is freed.
I think this logic has some flaws, and just a waitgroup wouldn't fix all race conditions, but it keeps the same behavior and reduces some complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've implemented it the exact same way as coderd does. I do agree that the mutex seems pointless and we can fix that if you want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is this crazy?
#7008
coderd/workspaceapps/token.go Outdated
| // | ||
| // The first 64 bytes of the key are used for signing tokens with HMAC-SHA256, | ||
| // and the last 32 bytes are used for encrypting API keys with AES-256-GCM. | ||
| type SigningKey [96]byte |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should rename this now that it does both signing/encryption.
How about, "SecurityKey"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Other ideas:
- "CryptoKey"
- "AuthenticityKey" (since we only use this key to guarantee authenticity of payloads)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Rename is here:fe28e42
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
This all looks good. Things will likely change a bit as we make this support external proxies.
Uh oh!
There was an error while loading.Please reload this page.
Moves path-app, subdomain-app and reconnecting PTY proxying to the new
workspaceapps.WorkspaceAppServerstruct. This struct does not interface with the database ever, in preparation for external workspace proxies.Updates app logout flow to avoid redirecting to
coder-logout.${app_host}on logout. Instead, all subdomain app tokens owned by the logging-out user will be deleted every time you logout and your browser will not do redirecting to each app host. This is in preparation for workspace proxies so we don't have to redirect the user to every single moon in a loop.Tests will remain in their original package, pending being moved to an apptest package (or similar).
Closes#6914