add extension point for custom StringMatcher, and lua implementation #32586
Conversation
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
ggreenway
requested review from
zuercher,
mattklein123,
alyssawilk,
wbpcode and
lizan
as code owners
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
Note: I pulled one commit into a separate PR (#32587) to make it easier to review. |
| // Use an extension as the matcher type. | ||
| // [#extension-category: envoy.matching.string] | ||
| // [#comment: This uses the xds type instead of the envoy type to avoid a circular dependency in the build system.] | ||
| xds.core.v3.TypedExtensionConfig extension = 8; |
There was a problem hiding this comment.
Why do we need this new extension point in StringMatcher? Why not use the new-style unified matcher API instead, since that is already fully pluggable?
https://github.com/cncf/xds/blob/main/xds/type/matcher/v3/matcher.proto
There was a problem hiding this comment.
Because StringMatcher is used in many places where the new matchers are not supported, and adding this here gets support in all those places.
I'm not very familiar with the new matcher API, but I looked at it briefly and it seems to always have actions associated with a match; I'm not exactly sure how to apply that in some cases where StringMatcher is used where it just needs a boolean response.
One specific case I want to use this extension matcher is in SAN validation in the tls transport socket.
There was a problem hiding this comment.
Yeah, I think I'm fine conceptually here with this being an extension type for the reasons Greg gives.
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
repokitteh-read-only
bot
added
the
deps
|
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
@wbpcode this is a really good point. When we mirror types into the xds package namespace, we then have the challenge of forcing canonical evolution of those types to take place there and need a systematic way to bring them back into the legacy types for cases where features like this matter. This is actually pretty messy today as this example shows. I think for this reason, I'd discourage this pattern in the future - regardless of the repository where a proto lives, we should stick to ODR and not replicate across package namespaces, at least if that type has some material amount of usage in the API already. |
| enable_reuse_port_default_(true), stats_flush_in_progress_(false) {} | ||
| enable_reuse_port_default_(true), stats_flush_in_progress_(false) { | ||
|
|
||
| InjectableSingleton<ThreadLocal::SlotAllocator>::clear(); |
There was a problem hiding this comment.
This is needed to make the multi-envoy integration test pass. Without this, the 2nd server ASSERTs because it's trying to set a non-null singleton.
I think this means that extensions that need this singleton will not work in certain envoy-mobile setups where there are multiple engines running.
As best I can tell, this is the same situation we're in for a regex engine, which is also managed by a singleton (for identical reasons: it's REALLY ugly to pass all these bits in to every place where a StringMatcherImpl is constructed; I tried, and it touched too much code).
There was a problem hiding this comment.
OK, maybe leave a comment on this? It's unclear to me which things are chosen for initialization here and why other than trial-and-error.
There was a problem hiding this comment.
Agreed; adding a comment
There was a problem hiding this comment.
Thanks, I can see the issue from this. I think it might be OK to selectively allow this violation of singleton safety, but I'd like to just quickly check with @mattklein123 (who did a lot of the singleton work) and @yanavlasov (who has coverage on how we create Google internal environments for Envoy binaries) to verify. Good to merge if we can agree on this.
There was a problem hiding this comment.
Yep, still waiting on input/signoff.
There was a problem hiding this comment.
Friendly ping @mattklein123 @yanavlasov.
There was a problem hiding this comment.
It would be hard to tell what impact it will have internally. I think the code is contained enough that I'm not too concerned about patching it internally.
My main concern here is that a bad precedent with regex engine is getting reinforced here. If a factory does not provide the context that an extension needs, people will continue plumbing missing objects through a singleton.
I think it may be ok to accept this as a temporary solution. However I think the matcher factory needs to be fixed to have at least the base (or server) context passed to it. I suspect it is a major pain.
This should also be a requirement that all new factories have context available.
There was a problem hiding this comment.
I agree with @yanavlasov. This is OK for now but it's not great and we should try to track cleaning it up as tech debt. In a perfect world it would be nice for use of this to fail in the multi-envoy case vs. just not work silently but I'm not sure if that is easily possible.
There was a problem hiding this comment.
I agree it's not ideal. Before posting the PR, I started trying to pass the context through everywhere and the change was HUGE and I hadn't yet solved the most awkward case, which is that the TLS cert validator gets constructed from an SDS secret-update callback, and we'd need the context there.
Filed #32792 to track.
Signed-off-by: Greg Greenway <[email protected]>
Signed-off-by: Greg Greenway <[email protected]>
|
/retest |
Signed-off-by: Greg Greenway <[email protected]>
|
/retest |
1 similar comment
|
/retest |
repokitteh-read-only
bot
removed
the
deps
Commit Message: Add an extension point for custom StringMatcher implementations, with a Lua extension
Additional Description:
Risk Level: Low
Testing: New tests
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]