Skip to content

match_delegate: Add runtime control#28937

Closed
tyxia wants to merge 5 commits into
envoyproxy:mainfrom
tyxia:runtimeflag
Closed

match_delegate: Add runtime control#28937
tyxia wants to merge 5 commits into
envoyproxy:mainfrom
tyxia:runtimeflag

Conversation

@tyxia
Copy link
Copy Markdown
Member

@tyxia tyxia commented Aug 9, 2023

Add runtime flag for match_delegate. When flag is set to false, this filter becomes a passthrough filter, the things wrapped under it(i.e. delegate filter) will be skipped as well

Signed-off-by: tyxia <tyxia@google.com>
@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #28937 was opened by tyxia.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #28937 was opened by tyxia.

see: more, trace.

tyxia added 4 commits August 9, 2023 23:09
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Aug 10, 2023

/retest

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Aug 10, 2023

/assign @htuch @rshriram @yanavlasov

@tyxia tyxia marked this pull request as ready for review August 10, 2023 13:14
if (!has_match_tree_) {
// If no match tree is set or it is disabled by runtime guard, interpret as a skip.
if (!has_match_tree_ ||
!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.enable_extension_with_matcher")) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These feature flags aren't designed as permanent config knobs, and I feel this capability should exist generally. Also, you might want to do the runtime override on a per match filter basis.

I'd suggest having an API in which we add runtime_key (search for other users in Envoy API) to allow the runtime key to be specific. That way you can have individual filter control.

As discussed the other day, adding a disabling filter might be more general, since this is a useful feature in general for filters, the idea that we might want to with runtime turn on/off a specific filter in the filter chain. Such filters could be wrapper with a RuntimeDisabledFilter filter. This is more an optional nicety, I'm good with just adding runtime_key to the config.

CC @envoyproxy/api-shepherds

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recent work (#25927) added the disabled knob for an http filter. Is it sufficient, or is a runtime key also required for this use-case?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: @htuch , I thought we just need a temporary runtime flag. I agree that runtime key will be a permanent knob and enables per-filter granularity and other flexibilities. Will update.

Re @adisuissa: We need runtime control here. That disabled knob still needs the config updates, IIUC.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does require a config update.
If you need a RuntimeFeatureFlag field, consider adding it next to the disabled field.

@yanavlasov
Copy link
Copy Markdown
Contributor

/wait

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Aug 23, 2023

Just FYI, we decided that this runtime control is not immediately needed for us at this moment. But it is still good to have it. I will revisit this PR later

@github-actions
Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot added the stale stalebot believes this issue/PR has not been touched recently label Sep 22, 2023
@github-actions
Copy link
Copy Markdown

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions Bot closed this Sep 30, 2023
@tyxia tyxia deleted the runtimeflag branch December 26, 2024 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants