feat: sync the served environment set from the proxy config - #18
Open
gagantrivedi wants to merge 41 commits into
Open
feat: sync the served environment set from the proxy config#18gagantrivedi wants to merge 41 commits into
gagantrivedi wants to merge 41 commits into
Conversation
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 exhaustive literal broke compilation whenever AppSettings gained a field; the other test files already spread from default().
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 endpoint response cache is deprecated and disabled by default; without it every request passes the key gate, so a cache write racing a removal is not servable residue worth guarding against. The poll re-insertion guard on the main environments cache stays — that cache is load-bearing.
Credential for the proxy config endpoint. The empty-config startup warning now fires only when neither static pairs nor a proxy key are configured.
Declares only the fields the proxy acts on; the endpoint's other fields are ignored by serde.
The diff/apply policy lives beside the index whose invariants it enforces: protected (statically configured) client keys are never overridden or removed by the proxy config; unchanged environments are untouched; displaced and removed versions are returned so the caller can invalidate request caches.
When proxy_key is set, every poll first fetches the proxy config and reconciles the served set against it: new environments get their documents in the same pass, rotations and removals clear everything cached under keys that stopped resolving, and statically configured environments are never overridden or removed. A failed fetch reports through the poll result and removes nothing — an environment is only dropped by a successful response that omits it.
active/expires_at only gated the upstream fetch; the request path indexed and served every key regardless, so a deactivation delivered by the proxy config — the mechanism revocation is meant to propagate through — and an expiry passing between polls were both ignored. resolve now rejects a presented server key that is no longer valid; client keys are unaffected.
A brand-new environment has no server-side keys yet, and the config reports it anyway. Indexing it made every poll fail — nothing to fetch a document with — which held /health red for the whole proxy from the moment anyone created an environment. Such environments are now filtered from the desired set (and dropped if previously served), picked up automatically by the sync after their first key is created.
Matches the layering: sync_proxy_config fetches, the index syncs to what was fetched. SyncOutcome follows.
Consistent with the engine's EvaluationResult vocabulary.
SyncResult had been inserted between the index's doc block and the struct, so rustdoc attached the whole thing — lock discipline and all — to SyncResult and left EnvironmentIndex undocumented.
from_settings already receives the static pairs, so remembering them makes 'static config wins' an unconditional property of the type instead of a per-call obligation, and sync_to needs no protected parameter. Protection now covers the full key namespace: a config environment whose keys collide with a static environment's keys is skipped entirely instead of silently hijacking the index entry.
'Stop serving = purge both cache layers' was written in three places; a fourth cache would have been missed in one of them. Also records that removal of a config-managed environment is transient.
Some("") counted as configured — suppressing the no-environments
startup warning — while sending an empty X-Proxy-Key header and
failing every sync, loud only once /health went red.
The endpoint doesn't guarantee key order, and sync_to detects change by equality; an upstream query-plan change would otherwise cause spurious displacement and cache clearing every poll.
gagantrivedi
force-pushed
the
feat/proxy-config-discovery
branch
from
August 22, 2026 10:35
d49bc46 to
134525a
Compare
gagantrivedi
marked this pull request as ready for review
August 22, 2026 10:36
|
@CodeRabbit review |
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change adds an optional validated Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟠 High · up to The change can allow expired environment keys to access cached documents, flags, or identity data, while overlapping refreshes may restore removed environments or stale keys. These security and correctness risks should be fixed before merging. 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 |
Member
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![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: 7a4fca1a-71b2-460e-9ff3-9c61e10aa615
📒 Files selected for processing (7)
src/config/settings.rssrc/environments.rssrc/main.rssrc/models/mod.rssrc/models/proxy_config.rssrc/services/environment.rstests/test_proxy_config_discovery.rs
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Cached document, flags, and identity responses returned before resolve_key ran, so a dynamic server key that expired between proxy config polls kept reading cached data until eviction. Validate the presented key before every endpoint-cache lookup.
The poll task was spawned before the initial refresh and tokio's interval delivers its first tick immediately, so two refresh_environment_caches runs overlapped at startup. A delayed older proxy-config response could then resurrect a removed environment or a rotated key set. Run the initial refresh first and skip the interval's immediate first tick; the poll loop itself is serial, so refreshes can no longer overlap.
matthewelwell
left a comment
matthewelwell
left a comment
There was a problem hiding this comment.
Full disclosure: I haven't reviewed the tests
Comment on lines
-106
to
+110
| self.cache.remove_environment(&keys.client_key).await; | ||
| self.clear_endpoint_caches(&keys).await; | ||
| self.purge_environment_caches(&keys).await; |
There was a problem hiding this comment.
I guess this question is valid for the previous code here, but I don't understand the 'purge' here - shouldn't we rebuild instead?
There was a problem hiding this comment.
Also, if it simplifies it, I think we should just deprecate the endpoint caches - they are not worth keeping imo.
gagantrivedi
force-pushed
the
feat/environment-discovery
branch
from
September 5, 2026 04:58
6eeada4 to
fbec84a
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
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
Stacked on #17. With a
proxy_keyconfigured, the proxy keeps its served environments in sync with the proxy config endpoint (GET /api/v1/proxy/config/,X-Proxy-Keyheader): a newly created environment is served within one poll interval, rotations and removals propagate automatically, and per-environment configuration is no longer needed.proxy_keysetting (a single key — organisation-level grants make one key multi-org).models/proxy_config.rs: serde DTOs declaring only the fields acted on; everything else the endpoint sends is ignored.EnvironmentIndex::sync_to(desired): inserts new/changed environments, removes absent ones — statically configured environments (the index remembers their keys) are never overridden, removed, or key-hijacked by the config.How did you test this code?
cargo test: 77 green — reconcile unit tests plustests/test_proxy_config_discovery.rs, wiremock against the frozen contract shape: environment added / dropped / key-rotated, static-never-removed, and fetch-failure-removes-nothing. Clippy and fmt clean.