-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ext_proc: Add per-route configuration #17691
Conversation
Today "disabled" and the processing mode are the two things supported because that is what is currently supported in ext_proc. Signed-off-by: Gregory Brail <gregbrail@google.com>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Gregory Brail <gregbrail@google.com>
Signed-off-by: Gregory Brail <gregbrail@google.com>
Ping @markdroth? |
test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
Outdated
Show resolved
Hide resolved
Simplify tests using more helpers. Signed-off-by: Gregory Brail <gregbrail@google.com>
Signed-off-by: Gregory Brail <gregbrail@google.com>
thanks for update, lgtm |
Signed-off-by: Gregory Brail <gregbrail@google.com>
api/envoy/extensions/filters/http/ext_proc/v3alpha/ext_proc.proto
Outdated
Show resolved
Hide resolved
Signed-off-by: Gregory Brail <gregbrail@google.com>
/lgtm api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks good just one question
/wait
// Set up configuration on the virtual host and on the route and see that | ||
// the two are merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how the other per route configurations work? I may be misremembering but I thought most they wouldn't merge, but pick the most specific one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this confusing myself. I copied the language in the .proto from the ext_authz proto, which says, "If disabled is specified in multiple per-filter-configs, the most specific one will be used."
I think that this is the right logic for this. In the future, we'll implement overrides for request and response properties, and in that case I think that we might want to try and merge them if that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So for the two properties that are supported today, the behavior is indeed to pick the most specific one -- but we might choose something different for the properties that aren't supported yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the most specific one is your choice, then sounds like using this method is most simple for you (Sorry, I missed that from previous review)
envoy/source/common/http/utility.h
Lines 495 to 501 in 17b24c7
/** | |
* The non template implementation of resolveMostSpecificPerFilterConfig. see | |
* resolveMostSpecificPerFilterConfig for docs. | |
*/ | |
const Router::RouteSpecificFilterConfig* | |
resolveMostSpecificPerFilterConfigGeneric(const std::string& filter_name, | |
const Router::RouteConstSharedPtr& route); |
Also, the properties ExtProcOverrides overrides
is named as overrides
, so it doesn't sounds like extendable for future you want to merge them also.
ExtAuthz using the merge logic, but it is mixed also.
Only the context_extensions
has the merge logic, Other fields is the most specific one logic.
Maybe we can follow the ExtAuthz, don't name the field with override
name, or we design something else to explicitly specific override or merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After chatting a bit with other maintainers I think this is fine, as the behavior appears to be documented.
Signed-off-by: Gregory Brail <gregbrail@google.com>
Signed-off-by: Gregory Brail <gregbrail@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay here, this seems good to me.
Today "disabled" and the processing mode are the two things supported because that is what is currently supported in ext_proc. Signed-off-by: Gregory Brail <gregbrail@google.com> Signed-off-by: Greg Brail <gbrail@users.noreply.github.com>
Commit Message: ext_proc: Add per-route configuration for processing mode and "disabled" flag.
Additional Description:
Risk Level: Low. Not triggered unless configured.
Testing: New unit and integration tests added.
Docs Changes:
Release Notes:
Platform Specific Features: