Skip to content

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

Merged
merged 15 commits into from
Oct 14, 2024
Merged

feat: add wsproxy implementation for key fetching #14917

merged 15 commits into from
Oct 14, 2024

Conversation

sreya
Copy link
Collaborator

@sreya sreya commented Oct 1, 2024

This PR implements the wsproxy variant for a key cache. Along with jwt PR these are the final pieces before we can start wiring everything up.

@sreya sreya marked this pull request as ready for review October 1, 2024 23:30
sreya added 7 commits October 2, 2024 22:17
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
@sreya sreya requested a review from spikecurtis October 2, 2024 23:02
@Emyrk
Copy link
Member

Emyrk commented Oct 3, 2024

Spike's comments I think highlight the need to implement this once. With the changes #14928, can we reuse 1 implementation?

@sreya
Copy link
Collaborator Author

sreya commented Oct 3, 2024

@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?

@Emyrk
Copy link
Member

Emyrk commented Oct 3, 2024

@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 👍

sreya added 4 commits October 4, 2024 06:46
- 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 {
Copy link
Contributor

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.
@sreya sreya requested a review from spikecurtis October 4, 2024 15:59
@github-actions github-actions bot added the stale This issue is like stale bread. label Oct 12, 2024
@matifali matifali removed the stale This issue is like stale bread. label Oct 12, 2024
@sreya sreya merged commit 384873a into main Oct 14, 2024
26 checks passed
@sreya sreya deleted the jon/wspkeys branch October 14, 2024 19:04
@github-actions github-actions bot locked and limited conversation to collaborators Oct 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants