-
Notifications
You must be signed in to change notification settings - Fork 4k
xds: Add composite filter #12646
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
base: master
Are you sure you want to change the base?
xds: Add composite filter #12646
Changes from 1 commit
603d4fe
cd6c611
64e7c1c
10c8d1e
0648217
1d4d18e
4d2f442
af9a9ce
be05900
dc297fe
96c1fb1
3d98fb9
8d137d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -230,7 +230,7 @@ static FilterChain parseFilterChain( | |
| // FilterChain contains L4 filters, so we ensure it contains only HCM. | ||
| if (proto.getFiltersCount() != 1) { | ||
| throw new ResourceInvalidException("FilterChain " + filterChainName | ||
| + " should contain exact one HttpConnectionManager filter"); | ||
| + " should contain exactly one HttpConnectionManager filter"); | ||
|
AgraVator marked this conversation as resolved.
|
||
| } | ||
| io.envoyproxy.envoy.config.listener.v3.Filter l4Filter = proto.getFiltersList().get(0); | ||
| if (!l4Filter.hasTypedConfig()) { | ||
|
|
@@ -513,6 +513,7 @@ static io.grpc.xds.HttpConnectionManager parseHttpConnectionManager( | |
| } | ||
|
|
||
| // Parse http filters. | ||
| // todo: AgraVator are we changing the assumption here ?? | ||
| if (proto.getHttpFiltersList().isEmpty()) { | ||
| throw new ResourceInvalidException("Missing HttpFilter in HttpConnectionManager."); | ||
| } | ||
|
Comment on lines
516
to
518
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping this the same, assuming we will still be getting Filters here and not all through ExtensionWithMatcherPerRoute |
||
|
|
@@ -582,6 +583,7 @@ private static boolean isTerminalFilter(Filter.FilterConfig filterConfig) { | |
| static StructOrError<Filter.FilterConfig> parseHttpFilter( | ||
| io.envoyproxy.envoy.extensions.filters.network.http_connection_manager.v3.HttpFilter | ||
| httpFilter, FilterRegistry filterRegistry, boolean isForClient) { | ||
| // todo: AgraVator do we need to change anything here for composite filter ?? | ||
| String filterName = httpFilter.getName(); | ||
| boolean isOptional = httpFilter.getIsOptional(); | ||
| if (!httpFilter.hasTypedConfig()) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,26 @@ public static Matchers.StringMatcher parseStringMatcher( | |
| "Unknown StringMatcher match pattern: " + proto.getMatchPatternCase()); | ||
| } | ||
| } | ||
|
|
||
| /** Translate StringMatcher xDS proto to internal StringMatcher. */ | ||
| public static Matchers.StringMatcher parseStringMatcher( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The stringMatcher parsing logic should not be in this PR. It will be covered by A106. You then need to just modify add the |
||
| com.github.xds.type.matcher.v3.StringMatcher proto) { | ||
| switch (proto.getMatchPatternCase()) { | ||
| case EXACT: | ||
| return Matchers.StringMatcher.forExact(proto.getExact(), proto.getIgnoreCase()); | ||
| case PREFIX: | ||
| return Matchers.StringMatcher.forPrefix(proto.getPrefix(), proto.getIgnoreCase()); | ||
| case SUFFIX: | ||
| return Matchers.StringMatcher.forSuffix(proto.getSuffix(), proto.getIgnoreCase()); | ||
| case SAFE_REGEX: | ||
| return Matchers.StringMatcher.forSafeRegEx( | ||
| Pattern.compile(proto.getSafeRegex().getRegex())); | ||
| case CONTAINS: | ||
| return Matchers.StringMatcher.forContains(proto.getContains()); | ||
| case MATCHPATTERN_NOT_SET: | ||
| default: | ||
| throw new IllegalArgumentException( | ||
| "Unknown StringMatcher match pattern: " + proto.getMatchPatternCase()); | ||
| } | ||
| } | ||
| } | ||
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.
go/java-practices/null#collection
Is there a semantic difference between a null
filterMetadataand emptyfilterMetadata?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.
In the current implementation, a null filterMetadata represents the absence of metadata (as per the proto not having it set), while an empty map would represent present but empty metadata. I followed the existing pattern in the codebase for optional maps. However, I agree that using non-null empty collections is generally preferred in Java to avoid null checks. We can consider a refactor to use empty maps consistently in a separate cleanup PR to keep this PR focused on the core feature ?