[cache-filter] Add disabled option#25916
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/retest |
|
Retrying Azure Pipelines: |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
Left a high-level comment about API.
| // Config specific to the cache storage implementation. | ||
| // [#extension-category: envoy.http.cache] | ||
| google.protobuf.Any typed_config = 1 [(validate.rules).any = {required: true}]; | ||
| oneof cache_implementation { |
There was a problem hiding this comment.
Unfortunately this breaks backwards compatibility.
Annotation will be needed here.
There was a problem hiding this comment.
Good catch, I always forget about Golang ruining oneof for everyone else.
Changed to just making it a separate field, which is more forward-looking anyway (in that this way is compatible with merge-inheritable config).
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/retest |
|
Retrying Azure Pipelines: |
|
/lgtm api |
| google.protobuf.Any typed_config = 1 [(validate.rules).any = {required: true}]; | ||
| google.protobuf.Any typed_config = 1; | ||
|
|
||
| // When true, the cache filter is a no-op filter. |
There was a problem hiding this comment.
The comment should describe the use-cases where this is true (e.g., override by route and enable it).
There was a problem hiding this comment.
Again, doing that would probably be more confusing than helpful at this point because overriding by route isn't actually implemented yet. Describing things you can't actually do in the documentation seems worse than nothing.
At this version, the use for supporting disabling (until route overrides are available) is allowing ECDS to turn the filter off.
Which is also why the route-level generic disable isn't useful for this case - you can't toggle that with ECDS, you'd have to update some larger entity (the listener?).
The generic disable also doesn't allow for providing a disabled config at the base level and overriding it to enabled per-route (once per-route overrides are implemented).
There was a problem hiding this comment.
Suggestion was to add the ECDS use case to the doc even though it's a less-likely one, and the unimplemented one as a [#comment:]; done.
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/lgtm api |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks.
LGTM modulo small doc nit.
cc'ing other code-owner for review.
@toddmgreer @jmarantz @penguingao @mpwarres @capoferro
/assign-from @envoyproxy/senior-maintainers
| // When true, the cache filter is a no-op filter. | ||
| // | ||
| // Possible use-cases for this include: | ||
| // - Turning a filter on and off with [ECDS](https://www.envoyproxy.io/docs/envoy/latest/api-v3/service/extension/v3/config_discovery.proto). |
There was a problem hiding this comment.
nit: use :ref:... to take advantage of docs linking.
|
@envoyproxy/senior-maintainers assignee is @alyssawilk |
Signed-off-by: Raven Black <ravenblack@dropbox.com>
|
/lgtm api |
|
Does it have a config example please? I'm struggling with enabling per-route caching filter. |
It still doesn't have any per-route support, this change was just a first small step towards that. |
6 participants
Commit Message: [cache-filter] Add disabled option
Additional Description: Adds the ability to have cache filter present but disabled. This makes it so a cache filter can more easily be configured with ECDS, and is a good precursor to making the configuration "inheritable". Addresses part of #25819.
This model is forward-compatible with the idea of inheritable/overrideable config, and the implementation is backward-compatible with existing config.
Risk Level: Low - cache filter is WIP, and change should be no-op for existing configs.
Testing: Added unit test coverage.
Docs Changes: Automatic from proto.
Release Notes: n/a
Platform Specific Features: n/a