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 19 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 |
|
📝 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.
| /// environment's keys. A server-side key resolves only while it is | ||
| /// valid, so a deactivation delivered by the proxy config and an | ||
| /// expiry passing between polls both take effect on the next request. | ||
| pub fn resolve(&self, key: &str) -> Option<Arc<EnvironmentKeys>> { | ||
| self.by_key | ||
| let keys = self | ||
| .by_key | ||
| .read() | ||
| .expect("environment index lock poisoned") | ||
| .get(key) | ||
| .cloned() | ||
| .cloned()?; | ||
|
|
||
| if key != keys.client_key { | ||
| let presented = keys | ||
| .server_keys | ||
| .iter() | ||
| .find(|server_key| server_key.key == key)?; | ||
| if !presented.is_valid() { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| Some(keys) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Gate endpoint-cache hits with server-key validity.
resolve rejects an expired server key, but endpoint-cache hits do not call it. At src/services/environment.rs Line 341, Line 393, and Line 472, cached document, flags, and identity responses return before resolve_key.
If a dynamic server key expires after the last proxy-config poll, that key can still read cached data until the cache is cleared. Call resolve_key before each endpoint-cache lookup. Add a test that primes each cache before expiry and then verifies that the expired key is rejected.
| // Sync first so an environment added to the proxy config gets its | ||
| // document fetched in the same pass. | ||
| let mut all_success = self.sync_proxy_config().await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- declared Tokio dependency ---'
rg -n -C 3 'tokio' Cargo.toml Cargo.lock
printf '%s\n' '--- relevant source files ---'
ast-grep outline src/services/environment.rs
ast-grep outline src/main.rs
rg -n -C 6 'refresh_environment_caches|poll_environments|sync_proxy_config|discard_environment_if_removed|sync_to|Interval|interval' src
printf '%s\n' '--- targeted source sections ---'
sed -n '1,180p' src/services/environment.rs
sed -n '1,180p' src/main.rsRepository: Flagsmith/edge-proxy-rs
Length of output: 35880
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- refresh and fetch implementation ---'
sed -n '180,330p' src/services/environment.rs
printf '%s\n' '--- EnvironmentIndex implementation and locking ---'
sed -n '1,225p' src/environments.rs
printf '%s\n' '--- cache trait and local cache implementation ---'
rg -n -C 8 'trait EnvironmentsCache|struct LocalMemEnvironmentsCache|put_environment|get_environment|remove_environment' src
printf '%s\n' '--- current tests and test dependencies ---'
sed -n '620,735p' src/services/environment.rs
sed -n '1,80p' Cargo.tomlRepository: Flagsmith/edge-proxy-rs
Length of output: 49042
🌐 Web query:
Tokio 1.48.0 time interval first tick immediately ready documentation
💡 Result:
In Tokio 1.48.0, when you create an interval using tokio::time::interval, the first call to .tick().await will complete immediately [1][2]. This is because interval(period) is equivalent to interval_at(Instant::now(), period), which schedules the first tick to trigger as soon as the interval is initialized [2][3]. If you prefer the first tick to wait for the full duration, you should use tokio::time::interval_at with a start time set to the current time plus the duration (Instant::now() + period) [1][4]. Alternatively, if you have already created the interval and need to reset its timing, you can use interval.reset() to set the next tick to occur one full period after the current time [5].
Citations:
- 1: https://docs.rs/tokio/1.48.0/tokio/time/index.html
- 2: https://docs.rs/tokio/latest/tokio/time/fn.interval.html
- 3: https://docs.rs/tokio/latest/src/tokio/time/interval.rs.html
- 4: https://docs.rs/tokio/latest/tokio/time/index.html
- 5: https://docs.rs/tokio_wasi/latest/tokio/time/struct.Interval.html
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(frozen=True)
class Environment:
client: str
server: str
document: str
class Model:
def __init__(self):
self.index = {}
self.cache = {}
def sync_to(self, environment):
if environment is None:
old = self.index.pop("client", None)
if old is not None:
self.cache.pop("client", None)
return
self.index["client"] = environment
self.index[environment.server] = environment
def refresh_after_config_response(self, environment):
self.sync_to(environment)
current = self.index["client"]
self.cache[current.client] = current.document
# Exact discard_environment_if_removed check:
if self.index.get(current.client) is None:
self.cache.pop(current.client, None)
def rotation_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
new = Environment("client", "ser.new", "new-document")
model.sync_to(old)
# Refresh 1 has already started and is paused before applying old config.
# Refresh 2 applies the rotated config and fetches the new document.
model.refresh_after_config_response(new)
# Release refresh 1's delayed old config and let it finish.
model.refresh_after_config_response(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
return "rotation: stale server key and document are restored"
def removal_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
model.sync_to(old)
# Refresh 2 applies removal first.
model.refresh_after_config_response(None)
# Refresh 1 then applies its delayed old config.
model.refresh_after_config_response(old)
assert "client" in model.index
assert model.cache["client"] == "old-document"
return "removal: removed environment and document are restored"
print(rotation_interleaving())
print(removal_interleaving())
PYRepository: Flagsmith/edge-proxy-rs
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(frozen=True)
class Environment:
client: str
server: str
document: str
class Model:
def __init__(self):
self.index = {}
self.cache = {}
def sync_to(self, environment):
if environment is None:
old = self.index.pop("client", None)
if old is not None:
self.cache.pop("client", None)
return
self.index["client"] = environment
self.index[environment.server] = environment
def refresh_after_config_response(self, environment):
self.sync_to(environment)
current = self.index["client"]
self.cache[current.client] = current.document
if self.index.get(current.client) is None:
self.cache.pop(current.client, None)
def rotation_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
new = Environment("client", "ser.new", "new-document")
model.sync_to(old)
model.refresh_after_config_response(new)
model.refresh_after_config_response(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
return "rotation: stale server key and document are restored"
def removal_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
model.sync_to(old)
model.refresh_after_config_response(None)
model.refresh_after_config_response(old)
assert "client" in model.index
assert model.cache["client"] == "old-document"
return "removal: removed environment and document are restored"
print(rotation_interleaving())
print(removal_interleaving())
PYRepository: Flagsmith/edge-proxy-rs
Length of output: 418
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(frozen=True)
class Environment:
client: str
server: str
document: str
class Model:
def __init__(self):
self.index = {}
self.cache = {}
def apply_config(self, environment):
if environment is None:
old = self.index.pop("client", None)
if old is not None:
self.index.pop(old.server, None)
self.cache.pop("client", None)
return
self.index["client"] = environment
self.index[environment.server] = environment
def fetch_and_store_snapshot(self, snapshot):
self.cache[snapshot.client] = snapshot.document
# Mirrors discard_environment_if_removed: it checks only the
# client-key lookup, not whether this snapshot is still current.
if self.index.get(snapshot.client) is None:
self.cache.pop(snapshot.client, None)
def rotation_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
new = Environment("client", "ser.new", "new-document")
model.apply_config(old)
# Refresh 2 applies the newer config and completes its document fetch.
model.apply_config(new)
model.fetch_and_store_snapshot(new)
# Refresh 1 now applies its delayed older config and completes.
model.apply_config(old)
model.fetch_and_store_snapshot(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
def removal_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
model.apply_config(old)
# Refresh 1 is delayed before applying its old config.
# Refresh 2 removes the environment and completes.
model.apply_config(None)
# Refresh 1 then applies the delayed old config and fetches its snapshot.
model.apply_config(old)
model.fetch_and_store_snapshot(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
rotation_interleaving()
removal_interleaving()
print("controlled refresh interleavings reproduce stale key and document restoration")
PYRepository: Flagsmith/edge-proxy-rs
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(frozen=True)
class Environment:
client: str
server: str
document: str
class Model:
def __init__(self):
self.index = {}
self.cache = {}
def apply_config(self, environment):
if environment is None:
old = self.index.pop("client", None)
if old is not None:
self.index.pop(old.server, None)
self.cache.pop("client", None)
return
self.index["client"] = environment
self.index[environment.server] = environment
def fetch_and_store_snapshot(self, snapshot):
self.cache[snapshot.client] = snapshot.document
if self.index.get(snapshot.client) is None:
self.cache.pop(snapshot.client, None)
def rotation_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
new = Environment("client", "ser.new", "new-document")
model.apply_config(old)
model.apply_config(new)
model.fetch_and_store_snapshot(new)
model.apply_config(old)
model.fetch_and_store_snapshot(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
def removal_interleaving():
model = Model()
old = Environment("client", "ser.old", "old-document")
model.apply_config(old)
model.apply_config(None)
model.apply_config(old)
model.fetch_and_store_snapshot(old)
assert model.index["client"].server == "ser.old"
assert model.cache["client"] == "old-document"
rotation_interleaving()
removal_interleaving()
print("controlled refresh interleavings reproduce stale key and document restoration")
PYRepository: Flagsmith/edge-proxy-rs
Length of output: 239
Serialise complete environment refreshes.
main.rs starts poll_environments before the initial refresh, and tokio::time::interval delivers its first tick immediately. Two refresh_environment_caches calls can therefore overlap. A delayed older config can restore a removed environment or rotated key set, then overwrite the cache because discard_environment_if_removed checks only whether the client key resolves. Guard the complete refresh with a shared async mutex, or start polling after the initial refresh completes. Add controlled rotation and removal tests.
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.