chore: move app proxying code to workspaceapps pkg #6998
Conversation
Moves path-app, subdomain-app and reconnecting PTY proxying to the new
workspaceapps.WorkspaceAppServer struct. This is 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 for simplicity sake.
Tests will remain in their original package, pending being moved to an
apptest package (or similar).
Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
| @@ -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.
I assume this will be part of moons?
There was a problem hiding this comment.
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.
This will cause leaks if merged before fixing this, right? We could just add a Close func to this server, which has it's own mutex and wait group for this.
There was a problem hiding this comment.
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 in 7e0ea10
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.
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
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.
We should rename this now that it does both signing/encryption.
How about, "SecurityKey"?
There was a problem hiding this comment.
Other ideas:
- "CryptoKey"
- "AuthenticityKey" (since we only use this key to guarantee authenticity of payloads)
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