feat: sync the served environment set from the proxy config - #18
feat: sync the served environment set from the proxy config#18gagantrivedi wants to merge 21 commits into
Conversation
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.
d49bc46 to
134525a
Compare
|
@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 |
|
@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.
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
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.