envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.03k stars 4.82k forks source link

runtime: reject runtime variables with the runtime guard naming if they're not known flags #20212

Open alyssawilk opened 2 years ago

alyssawilk commented 2 years ago

We runtime guard behavioral changes (default true) but if folks override the default behavior they (previously) have no warning when the code paths are removed and their override becomes a no-op. As of https://github.com/envoyproxy/envoy/pull/19880 we will ENVOY_BUG for unknown flags which is better than nothing, but I propose we outright reject configs with unknown flags, in case folks aren't paying attention to their logs.

alyssawilk commented 2 years ago

cc @krajshiva

alyssawilk commented 2 years ago

cc @htuch any concerns on this w.r.t. API stability? On the one hand, this means that Envoy A may run with RTDS envoy.reloadable_feature.foo and Envoy B built 3 months later will reject that config. On the other hand, the runtime feature guards are explicitly transient features, and I think rejecting config if you're setting one that no longer exists it's far better to reject the config, than silently revert the behavioral change you are trying to set because the code has been removed.

htuch commented 2 years ago

I agree the config NACK semantics make sense here. I'd like to check with @fuqianggao that this won't cause us any trouble (or maybe provide a heads up that we need to adapt to this style). The main place things like this can become a problem is when the xDS server binary has RTDS baked in and you need to carefully coordinate rollouts/rollbacks.

fuqianggao commented 2 years ago

This would be problematic for control plane that supports multiple Envoy versions. "this means that Envoy A may run with RTDS envoy.reloadable_feature.foo and Envoy B built 3 months later will reject that config." -> this will cause issue for Envoy B, unless control plane knows Envoy B doesn't support envoy.reloadable_feature.foo and exclude it from RTDS, which needs minor versioning support to be in place.

alyssawilk commented 2 years ago

Right, but folks really shouldn't be setting any of those guards without notifying us, which pushes back the deprecation window. I can leave them as envoy bugs rather than fatal but that's going to lead to unexpected data plane changes which IMO is significantly more dangerous. Can we just explicitly comment somewhere that those specific runtime guards are ephemeral and not subject to the 1y policy?

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.