-
Notifications
You must be signed in to change notification settings - Fork 1
refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable #17
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
Open
gagantrivedi
wants to merge
20
commits into
main
Choose a base branch
from
feat/environment-discovery
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
87d93c5
refactor: replace static key maps with a runtime environment registry
gagantrivedi de62e6b
refactor: allow environment_key_pairs to be omitted from config
gagantrivedi 1b260fb
refactor: move server startup into lib::run
gagantrivedi e633269
test: build AppSettings with struct spread in LRU cache tests
gagantrivedi 3172495
chore: trim redundant half of the environment_key_pairs comment
gagantrivedi ed24723
refactor: rename EnvironmentRegistry to EnvironmentIndex at top level
gagantrivedi 9c1d3a2
refactor: rename EnvRecord to EnvironmentKeys
gagantrivedi 3b4f5b7
refactor: drop the source field from EnvironmentKeys
gagantrivedi c78f64d
refactor: rename evict_environment to remove_environment
gagantrivedi 790db10
docs: say proxy config endpoint, not inventory
gagantrivedi 94a8bd2
refactor: drop the shared-server-key guard from index removal
gagantrivedi 643e312
fix: clear cache writes that race environment removal
gagantrivedi f7fafad
refactor: return the displaced entry from EnvironmentIndex::insert
gagantrivedi cf82197
docs: record index assumptions and the failing-poll health obligation
gagantrivedi 6ca6e9f
fix: warn at startup when no environments are configured
gagantrivedi f888c26
refactor: fold run() back into main.rs
gagantrivedi 34a3abd
refactor: drop the endpoint-cache write guard
gagantrivedi 24100f4
docs: plainer wording for remove_environment
gagantrivedi b7d582a
docs: clearer given-comment in the poll-reinsertion test
gagantrivedi 6eeada4
refactor: say replaced, not displaced, in insert's contract
gagantrivedi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,286 @@ | ||
| use std::collections::HashMap; | ||
| use std::sync::{Arc, RwLock}; | ||
|
|
||
| use chrono::{DateTime, Utc}; | ||
|
|
||
| use crate::config::settings::EnvironmentKeyPair; | ||
|
|
||
| /// A server-side (`ser.`) key together with the validity metadata the | ||
| /// proxy config endpoint reports. Statically configured keys carry no | ||
| /// metadata and are always valid. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct ServerKey { | ||
| pub key: String, | ||
| pub active: bool, | ||
| pub expires_at: Option<DateTime<Utc>>, | ||
| } | ||
|
|
||
| impl ServerKey { | ||
| pub fn is_valid(&self) -> bool { | ||
| self.active && self.expires_at.is_none_or(|at| at > Utc::now()) | ||
| } | ||
| } | ||
|
|
||
| /// The key set of one environment the proxy serves: its client-side key | ||
| /// and every server-side key that can authenticate for it upstream | ||
| /// (multiple during rotation). | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct EnvironmentKeys { | ||
| pub client_key: String, | ||
| pub server_keys: Vec<ServerKey>, | ||
| } | ||
|
|
||
| impl EnvironmentKeys { | ||
| /// The first server-side key still usable for upstream fetches. | ||
| pub fn valid_server_key(&self) -> Option<&ServerKey> { | ||
| self.server_keys.iter().find(|key| key.is_valid()) | ||
| } | ||
| } | ||
|
|
||
| /// The runtime-mutable set of environments the proxy serves. | ||
| /// | ||
| /// Every environment is indexed under its client key *and* each of its | ||
| /// server keys, so a single lookup resolves whichever kind of key a request | ||
| /// presents. | ||
| /// | ||
| /// Server keys are assumed unique across environments: a key duplicated in | ||
| /// the config is last-one-wins on insert, and removing either environment | ||
| /// un-indexes the shared key for both. | ||
| /// | ||
| /// Uses `std::sync::RwLock`, not tokio's: guards are held only for a map | ||
| /// operation, never across an await, and lookups stay callable from | ||
| /// synchronous code. | ||
| #[derive(Default)] | ||
| pub struct EnvironmentIndex { | ||
| by_key: RwLock<HashMap<String, Arc<EnvironmentKeys>>>, | ||
| } | ||
|
|
||
| impl EnvironmentIndex { | ||
| pub fn from_settings(pairs: &[EnvironmentKeyPair]) -> Self { | ||
| let index = Self::default(); | ||
| for pair in pairs { | ||
| index.insert(EnvironmentKeys { | ||
| client_key: pair.client_side_key.clone(), | ||
| server_keys: vec![ServerKey { | ||
| key: pair.server_side_key.clone(), | ||
| active: true, | ||
| expires_at: None, | ||
| }], | ||
| }); | ||
| } | ||
| index | ||
| } | ||
|
|
||
| /// Resolve a presented key — client- or server-side — to its | ||
| /// environment's keys. | ||
| pub fn resolve(&self, key: &str) -> Option<Arc<EnvironmentKeys>> { | ||
| self.by_key | ||
| .read() | ||
| .expect("environment index lock poisoned") | ||
| .get(key) | ||
| .cloned() | ||
| } | ||
|
|
||
| /// Insert or replace an environment's keys, dropping index entries | ||
| /// for server keys the previous version no longer has. Returns the | ||
| /// replaced version, if any: request caches are keyed by presented | ||
| /// key and consulted before the key gate, so the caller owns | ||
| /// invalidating whatever is cached under keys that stopped resolving. | ||
| pub fn insert(&self, keys: EnvironmentKeys) -> Option<Arc<EnvironmentKeys>> { | ||
| let keys = Arc::new(keys); | ||
| let mut by_key = self | ||
| .by_key | ||
| .write() | ||
| .expect("environment index lock poisoned"); | ||
|
|
||
| let previous = by_key.get(&keys.client_key).cloned(); | ||
| if let Some(previous) = &previous { | ||
| for server_key in &previous.server_keys { | ||
| by_key.remove(&server_key.key); | ||
| } | ||
| } | ||
|
|
||
| for server_key in &keys.server_keys { | ||
| by_key.insert(server_key.key.clone(), Arc::clone(&keys)); | ||
| } | ||
| by_key.insert(keys.client_key.clone(), keys); | ||
| previous | ||
| } | ||
|
|
||
| /// Remove the environment `key` resolves to (any of its keys works), | ||
| /// returning its keys so the caller can clear per-key caches. | ||
| pub fn remove(&self, key: &str) -> Option<Arc<EnvironmentKeys>> { | ||
| let mut by_key = self | ||
| .by_key | ||
| .write() | ||
| .expect("environment index lock poisoned"); | ||
| let keys = by_key.get(key).cloned()?; | ||
|
|
||
| by_key.remove(&keys.client_key); | ||
| for server_key in &keys.server_keys { | ||
| by_key.remove(&server_key.key); | ||
| } | ||
|
|
||
| Some(keys) | ||
| } | ||
|
|
||
| /// Point-in-time snapshot of every environment's keys, ordered by | ||
| /// client key so callers iterate deterministically. | ||
| pub fn snapshot(&self) -> Vec<Arc<EnvironmentKeys>> { | ||
| let by_key = self.by_key.read().expect("environment index lock poisoned"); | ||
| let mut snapshot: Vec<Arc<EnvironmentKeys>> = by_key | ||
| .iter() | ||
| .filter(|(key, keys)| key.as_str() == keys.client_key) | ||
| .map(|(_, keys)| Arc::clone(keys)) | ||
| .collect(); | ||
| snapshot.sort_by(|a, b| a.client_key.cmp(&b.client_key)); | ||
| snapshot | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use chrono::TimeDelta; | ||
|
|
||
| fn pair(client: &str, server: &str) -> EnvironmentKeyPair { | ||
| EnvironmentKeyPair { | ||
| client_side_key: client.to_string(), | ||
| server_side_key: server.to_string(), | ||
| } | ||
| } | ||
|
|
||
| fn server_key(key: &str) -> ServerKey { | ||
| ServerKey { | ||
| key: key.to_string(), | ||
| active: true, | ||
| expires_at: None, | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn from_settings_resolves_both_keys_to_the_same_environment() { | ||
| // Given | ||
| let index = EnvironmentIndex::from_settings(&[pair("client_a", "ser.a")]); | ||
|
|
||
| // When | ||
| let by_client = index.resolve("client_a").unwrap(); | ||
| let by_server = index.resolve("ser.a").unwrap(); | ||
|
|
||
| // Then | ||
| assert!(Arc::ptr_eq(&by_client, &by_server)); | ||
| assert_eq!(by_client.client_key, "client_a"); | ||
| assert!(by_client.valid_server_key().is_some()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn resolve_unknown_key_returns_none() { | ||
| let index = EnvironmentIndex::from_settings(&[pair("client_a", "ser.a")]); | ||
| assert!(index.resolve("nope").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn insert_replaces_keys_and_drops_stale_server_key_index() { | ||
| // Given | ||
| let index = EnvironmentIndex::from_settings(&[pair("client_a", "ser.old")]); | ||
|
|
||
| // When the environment's server key is rotated | ||
| let replaced = index.insert(EnvironmentKeys { | ||
| client_key: "client_a".to_string(), | ||
| server_keys: vec![server_key("ser.new")], | ||
| }); | ||
|
|
||
| // Then the caller learns which version (and keys) it replaced | ||
| assert_eq!(replaced.unwrap().server_keys[0].key, "ser.old"); | ||
| assert!(index.resolve("ser.old").is_none()); | ||
| assert_eq!(index.resolve("ser.new").unwrap().client_key, "client_a"); | ||
| assert_eq!(index.snapshot().len(), 1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn insert_returns_none_for_a_new_environment() { | ||
| let index = EnvironmentIndex::default(); | ||
| let replaced = index.insert(EnvironmentKeys { | ||
| client_key: "client_a".to_string(), | ||
| server_keys: vec![server_key("ser.a")], | ||
| }); | ||
| assert!(replaced.is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn remove_by_any_key_clears_every_index_entry() { | ||
| // Given an environment with two server keys | ||
| let index = EnvironmentIndex::default(); | ||
| index.insert(EnvironmentKeys { | ||
| client_key: "client_a".to_string(), | ||
| server_keys: vec![server_key("ser.one"), server_key("ser.two")], | ||
| }); | ||
|
|
||
| // When removed via one of its server keys | ||
| let removed = index.remove("ser.two").unwrap(); | ||
|
|
||
| // Then | ||
| assert_eq!(removed.client_key, "client_a"); | ||
| assert!(index.resolve("client_a").is_none()); | ||
| assert!(index.resolve("ser.one").is_none()); | ||
| assert!(index.resolve("ser.two").is_none()); | ||
| assert!(index.remove("client_a").is_none()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn snapshot_returns_one_entry_per_environment_sorted_by_client_key() { | ||
| // Given | ||
| let index = EnvironmentIndex::from_settings(&[ | ||
| pair("client_b", "ser.b"), | ||
| pair("client_a", "ser.a"), | ||
| ]); | ||
|
|
||
| // When | ||
| let snapshot = index.snapshot(); | ||
|
|
||
| // Then | ||
| let client_keys: Vec<&str> = snapshot.iter().map(|r| r.client_key.as_str()).collect(); | ||
| assert_eq!(client_keys, vec!["client_a", "client_b"]); | ||
| } | ||
|
|
||
| #[test] | ||
| fn valid_server_key_skips_inactive_and_expired_keys() { | ||
| // Given | ||
| let keys = EnvironmentKeys { | ||
| client_key: "client_a".to_string(), | ||
| server_keys: vec![ | ||
| ServerKey { | ||
| key: "ser.inactive".to_string(), | ||
| active: false, | ||
| expires_at: None, | ||
| }, | ||
| ServerKey { | ||
| key: "ser.expired".to_string(), | ||
| active: true, | ||
| expires_at: Some(Utc::now() - TimeDelta::days(1)), | ||
| }, | ||
| ServerKey { | ||
| key: "ser.valid".to_string(), | ||
| active: true, | ||
| expires_at: Some(Utc::now() + TimeDelta::days(1)), | ||
| }, | ||
| ], | ||
| }; | ||
|
|
||
| // When / Then | ||
| assert_eq!(keys.valid_server_key().unwrap().key, "ser.valid"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn valid_server_key_returns_none_when_no_key_is_usable() { | ||
| let keys = EnvironmentKeys { | ||
| client_key: "client_a".to_string(), | ||
| server_keys: vec![ServerKey { | ||
| key: "ser.inactive".to_string(), | ||
| active: false, | ||
| expires_at: None, | ||
| }], | ||
| }; | ||
| assert!(keys.valid_server_key().is_none()); | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| pub mod cache; | ||
| pub mod config; | ||
| pub mod environments; | ||
| pub mod error; | ||
| pub mod models; | ||
| pub mod routes; | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
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.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject key collisions before modifying
by_key.EnvironmentKeyPairpermits duplicate server keys and client keys that match another indexed key. Line 104 overwrites the existing owner. A later removal of the original environment removes that shared entry, even when it now belongs to the replacement environment.Require every client key and server key to be unique across the index. Reject an update atomically when it conflicts with another environment.