refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable - #17
refactor: introduce EnvironmentIndex to make the served environment set runtime-mutable#17gagantrivedi wants to merge 26 commits into
Conversation
gagantrivedi
changed the title
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📝 WalkthroughWalkthroughThe proxy now uses Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The runtime-mutable environment index can currently mishandle colliding keys and can briefly serve data fetched for a replaced environment. These are concrete correctness risks that could expose the wrong environment’s flags or identities, so the PR is not merge-ready until collision handling and replacement validation are fixed. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7f797395-a335-4f14-b32b-be624b0136a6
📒 Files selected for processing (8)
src/cache/environment.rssrc/config/settings.rssrc/environments.rssrc/lib.rssrc/main.rssrc/services/environment.rstests/test_lru_cache.rstests/test_remove_environment.rs
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
Full disclosure: I've not reviewed the tests.
| // An environment stuck here fails every poll, keeping /health red | ||
| // until a config change — or, once reconciliation exists, until it | ||
| // removes or re-keys the environment. |
There was a problem hiding this comment.
This doesn't make any sense to me. How / why would an environment get 'stuck' here?
There was a problem hiding this comment.
It can't happen here but it can happen in: #18
I will move the comment to that with how
| .await? | ||
| { | ||
| Some(document) => Ok(document), | ||
| // 304: upstream confirmed the cached copy is current. |
There was a problem hiding this comment.
What about other non-200 responses?
There was a problem hiding this comment.
Those are returned as error handled by the caller of this fucntion
| )); | ||
| } | ||
| // Validation only — lookups below still use the raw presented key, so | ||
| // a server-side key 503s on this endpoint (contexts are stored under |
There was a problem hiding this comment.
Errr... this is supposed to be a todo
| pub fn resolve(&self, key: &str) -> Option<Arc<EnvironmentKeys>> { | ||
| self.by_key | ||
| .read() | ||
| .expect("environment index lock poisoned") |
There was a problem hiding this comment.
This appears a lot.
- Does it need to? It feels odd.
- Should it be a constant, instead of a hard-coded string?
There was a problem hiding this comment.
That was mostly forced syntax: std's lock returns a Result we had to unwrap. Switched to parking_lot's RwLock (already in our dependency tree), which doesn't poison, so it's gone.
| /// 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. |
There was a problem hiding this comment.
This docstring doesn't make much sense to me. Some examples:
dropping index entries for server keys the previous version no longer has- what is the 'previous version' here? The index? Why would it no longer have some server key?key gate- what is the key gate?so the caller owns invalidating whatever is cached under keys that stopped resolving- this seems like unnecessary slop?
There was a problem hiding this comment.
another victim of endoint cache! let me know if it makes sense now
| /// synchronous code. | ||
| #[derive(Default)] | ||
| pub struct EnvironmentIndex { | ||
| by_key: RwLock<HashMap<String, Arc<EnvironmentKeys>>>, |
There was a problem hiding this comment.
Perhaps it's my lack of familiarity with rust, but I don't really understand what by_key is. It's used a lot in the code below, so it might be worth some specific documentation here?
6eeada4 to
fbec84a
Compare
Environments are now held in an EnvironmentRegistry that indexes each record under its client key and every server key, replacing the immutable key_mapping/server_to_client maps built at startup. Records carry an EnvSource so statically configured environments can never be evicted by future inventory reconciliation. Adds EnvironmentsCache::remove_environment and EnvironmentService::evict_environment so an environment can be forgotten at runtime, including the endpoint caches that are consulted before the key gate. Splits fetch_environment into a server-key picker and a reusable fetch_document(server_key, if_modified_since). No behaviour change for existing configs; the poll-failure log now prints the client key instead of the server-side secret.
An omitted field now behaves like an explicitly empty list (which was already accepted), so a future discovery-only config needs no static pairs. Statically configured pairs keep working exactly as before.
main() now only loads settings and logging before delegating, so an alternative binary can compose the proxy from the library crate.
The type is the domain concept of which environments the proxy serves, not a service, so it moves out of services/ to src/environments.rs beside cache/ — matching how comparable Rust proxies place such state (e.g. unleash-edge's feature_cache). Drops the services re-export: EnvironmentService is its only consumer.
Names the content: the key set of one environment (client key, server keys, provenance). Also unabbreviates EnvSource to EnvironmentSource and renames records() to snapshot() so no 'record' vocabulary is left.
Nothing reads it: an environment's provenance only matters to the reconciliation that later phases introduce, and staticness is derivable there from the immutable environment_key_pairs settings. Reintroduce an explicit field if that derivation proves awkward.
Evict implies cache-pressure expulsion; this is a deliberate removal from the served set, and the cache trait already calls its half remove_environment, so the whole family now shares the verb.
A config that lists the same server key for two environments is a misconfiguration we choose not to defend against; removal now just drops the entry's keys unconditionally.
Endpoint-cache lookups run before the key gate, so an in-flight request that passed the gate could write its result after remove_environment's clears and have it served indefinitely; the poll loop, iterating a pre-removal snapshot, could likewise re-insert a removed environment's document and pin it until restart. Every cache write now re-checks the index afterwards and clears what it wrote if the key no longer resolves — remove_environment un-indexes before clearing, so one of the two clears always runs last. Removal also clears caches even when the key is unknown, so repeating it cleans any residue. The interleavings themselves aren't deterministically testable without injection points; the tests pin each guard's behaviour instead.
Mirrors HashMap::insert and makes insert symmetric with remove: when a later phase rotates keys via insert, the caller needs the dropped server keys to invalidate request caches, which are keyed by presented key and consulted before the key gate.
The duplicated-server-key failure shape, the /health-stays-red state a key-less environment creates, and the actual reason run() lives in the library were all design decisions living only in review threads.
A typo'd environment_key_pairs field name silently parses as an empty
set (serde ignores unknown fields), leaving a healthy-looking proxy
that rejects everything. Also pins the serde default with a test that
{} parses to an empty, valid config.
Nothing besides main.rs ever called it; the composition-binary story it served is speculative, and it can return the day something real needs it. The empty-config warning moves with the body.
The cache is gone (chore/drop-endpoint-cache); the per-endpoint tests stay as plain removed-key rejections, and the residue test moves to the environment cache.
It guarded a removal racing an in-flight fetch, which cannot happen: every index and cache mutation runs on the serial refresh task, and requests resolve the key before touching the cache, so an orphaned document could not be served anyway. The unknown-key residue clear in remove_environment goes with it — same race, same reasoning.
fbdd769 to
21dc629
Compare
No poisoning, so the repeated expect() at every lock site goes away.
The replaced record only served endpoint-cache invalidation, which is gone.
2 participants
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature.Changes
Contributes to Flagsmith/edge-proxy#128
Groundwork for environment auto-discovery: the served environment set becomes runtime-mutable. No behaviour change — static
environment_key_pairsconfigs work exactly as today; discovery itself is a follow-up PR.EnvironmentIndex(src/environments.rs) replaces the startup-built key maps. Each environment is indexed under its client key and every server key;insert/removereturn the replaced/removed entry so callers can invalidate the cache.resolve_key()replaces the three unknown-key gates (ser.keys still 503 on/flags//identitiesas today — fixed in a follow-up).remove_environmentstops serving an environment and clears its cached document. All index and cache mutation happens on the serial refresh task, so no re-insertion guards are needed.environment_key_pairsis now optional; a startup warning fires when nothing is configured.ser.secret.Accepted, deliberately: duplicated server keys are last-one-wins.
How did you test this code?
cargo test: 51 green, including index unit tests andtests/test_remove_environment.rs(removed environments are rejected on every endpoint, for both key kinds). No existing test expectations changed. Clippy and fmt clean.