-
Notifications
You must be signed in to change notification settings - Fork 911
feat: add wsproxy implementation for key fetching #14917
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
Conversation
This change streamlines the CryptoKey handling by utilizing the codersdk package, thereby reducing code duplication and potentially simplifying maintenance efforts in the future.
- Implement `cryptokeys.Keycache` interface in `CryptoKeyCache`. - Introduce context management for graceful shutdowns. - Simplify function signatures and improve concurrency handling. - Ensure functions return errors when cache is closed.
- Use `cryptokeys.ErrKeyNotFound` for missing keys - Use `cryptokeys.ErrKeyInvalid` for invalid keys - Replace `xerrors` for streamlined error codes
Spike's comments I think highlight the need to implement this once. With the changes #14928, can we reuse 1 implementation? |
@Emyrk I'm game to refactor this to use one implementation but honestly I'd like to get this initiative over the finish line. I'd consider the refactor tech debt which I can tackle concurrently to ensuring there are no larger issues with the overall feature while we dogfood it in dev. You chill with that? |
I can live with it 👍 |
- Replaced sync/atomic with sync.Mutex and sync.Cond for better concurrency control, avoiding frequent lock contention and reducing potential for data races. - Removed separate fetch logic and integrated it into the main crypto key retrieval flow to handle concurrent fetches properly. - Optimized the Close method to minimize wait time by broadcasting on cond to notify waiting fetches when closed.
The unused `cryptokeys.Keycache` interface implementation check was removed from the `CryptoKeyCache` struct, as it was unnecessary and did not contribute to functionality. This streamlines the code and adheres to best practices for interface checks.
} | ||
|
||
func (k *CryptoKeyCache) key(sequence int32) (codersdk.CryptoKey, bool) { | ||
if sequence == latestSequence { |
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.
if you're going to use a special int32 to represent latest, you might as well go whole hog and just insert the crypto key into the map under that special value, and drop k.latest
The `latest` field in `CryptoKeyCache` was redundant and has been removed to simplify the code. Instead, the latest crypto key is now directly accessed from the `keys` map using `latestSequence`. This change reduces unnecessary state management and potential for data inconsistency.
This PR implements the
wsproxy
variant for a key cache. Along withjwt
PR these are the final pieces before we can start wiring everything up.