fix: Do not remove features cache if upstream goes away #362
Conversation
If upstream goes away, edge should remain durable and keep the features it has gotten for the tokens. Since we already keep the authentication status for the tokens we've seen this fix allows you to start edge, manage to talk to upstream once, then sever connection to upstream, but keep serving feature toggles for known tokens
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
server/src/http/feature_refresher.rs
Outdated
| assert!(feature_refresher.tokens_to_refresh.is_empty()); | ||
| assert!(!feature_refresher.features_cache.is_empty()); | ||
| assert!(!feature_refresher.engine_cache.is_empty()); |
There was a problem hiding this comment.
The crux is here. Previously, when we receive 404 or connection failed we'd remove the features_cache and the engine_cache as well, thus removing all the durability built into Edge for when upstream goes away.
server/src/http/feature_refresher.rs
Outdated
| @@ -392,23 +392,15 @@ impl FeatureRefresher { | |||
| } | |||
| } | |||
| FeatureError::NotFound => { | |||
| warn!("Had a bad URL when trying to fetch features. Removing ourselves"); | |||
| info!("Had a bad URL when trying to fetch features. Removing ourselves from the list of refresh tasks"); | |||
There was a problem hiding this comment.
What happens when upstream is back online? I think 404 response code depends on each server/LB implementation, but maybe one reason for 404 is no pods available in a k8s cluster.
If we consider the backoff implementation we did in the SDKs I think this is correct as it's aligned with that. I'm just wondering if this behavior changes when edge is still not ready (i.e. before receiving at least 1 successful response from upstream)
There was a problem hiding this comment.
This sounds relevant, but it's a little early for me to understand exactly what you're saying 🥱 Would you be willing to explain this comment a little more for me?
There was a problem hiding this comment.
So, every time the same key gets used we do the evaluation in the following order
- Is key even allowed to access - this is stored in the TokenCache
2a. We have equal or wider access to the token's environment already, if so, we should be able to get features from the feature cache
2b We do not have equal or wider access, add the token to the list of tokens our FeatureRefresher cares about - Once we have features, give them to Yggdrasil to build an EngineState
- Evaluate toggles if /api/frontend, return toggles if /api/client/features
What Gaston is worrying about is whether or not we repeat step 2, if upstream comes back online. From what I gather, we'll attempt to register it for refresh once every hour for normal tokens. I'm starting to think we should change the default here to 5 minutes or so.
For startup tokens, we try once every second until they achieve TokenStatus::Validated, then they too get moved into the refresh once per hour.
There was a problem hiding this comment.
Right, and by step 2, you're referring to
- Once we have features, give them to Yggdrasil to build an EngineState
Right? Or 2a/2b? Anyway, it sounds like you've got it covered?
server/src/http/feature_refresher.rs
Outdated
| @@ -392,23 +392,15 @@ impl FeatureRefresher { | |||
| } | |||
| } | |||
| FeatureError::NotFound => { | |||
| warn!("Had a bad URL when trying to fetch features. Removing ourselves"); | |||
| info!("Had a bad URL when trying to fetch features. Removing ourselves from the list of refresh tasks"); | |||
There was a problem hiding this comment.
This sounds relevant, but it's a little early for me to understand exactly what you're saying 🥱 Would you be willing to explain this comment a little more for me?
chriswk
deleted the
fix/donotRemoveTokenCacheWhenUpstreamDoesNotRespond
branch
If upstream goes away, edge should remain durable and keep the features it has gotten for the tokens. Since we already keep the authentication status for the tokens we've seen this fix allows you to start edge, manage to talk to upstream once, then sever connection to upstream, but keep serving feature toggles for known tokens.
There's some superfluous version updates here, just to not have to merge 6 different PRs in addition to this one.