-
Notifications
You must be signed in to change notification settings - Fork 5.3k
listener: filter chain unified matchers #18871
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
Closed
kyessenov
wants to merge
27
commits into
envoyproxy:main
from
kyessenov:extensible_filter_chain_match
Closed
Changes from all commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
6a37223
[wip] Add filter chain match predicate order
kyessenov 8aa165f
spelling
kyessenov cfdb93a
review
kyessenov a13ed92
review
kyessenov 621fcbf
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 2228fef
review
kyessenov b610420
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov f45bc06
review
kyessenov bada313
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 3f8f8e4
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 937f7a8
review
kyessenov 9fd0f34
review
kyessenov 595eb18
add move note
kyessenov 7d7909a
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 9a56f14
merge fix
kyessenov 262d084
merge
kyessenov 76e5040
more review
kyessenov 3b92ec5
typo
kyessenov 03fbfc6
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov ddeeaf9
update
kyessenov be4636d
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 72b978e
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov e981df6
review
kyessenov a9b056a
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 987a2fd
Merge remote-tracking branch 'upstream/main' into extensible_filter_c…
kyessenov 0185225
try validation
kyessenov d1c8f75
verify example
kyessenov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -262,6 +262,9 @@ message FilterChain { | |||||
| // [#not-implemented-hide:] The unique name (or empty) by which this filter chain is known. If no | ||||||
| // name is provided, Envoy will allocate an internal UUID for the filter chain. If the filter | ||||||
| // chain is to be dynamically updated or removed via FCDS a unique name must be provided. | ||||||
| // Note: :ref:`filter_chain_matcher | ||||||
|
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.
Suggested change
|
||||||
| // <envoy_v3_api_field_config.listener.v3.Listener.filter_chain_matcher>` | ||||||
| // requires that filter chains are uniquely named. | ||||||
| string name = 7; | ||||||
|
|
||||||
| // [#not-implemented-hide:] The configuration to specify whether the filter chain will be built on-demand. | ||||||
|
|
||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This doesn't need to block the PR, but if we wanted to really prove out how usable the API is, I think we could do it without implementing it in Envoy. Snow has a golang implementation of the generic matcher. Istio already has an extensive test suite for filter chain matching, based on a golang implementation of the current FCM logic. So if we were able to implement the new matcher in Istio (which presumably we need to do anyways after this merges), we could get full test coverage as well before committing to the Envoy implementation.
All of that may be harder than just doing the implementation though, just throwing out the idea.
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.
Generic matcher does not have custom match implemented in either envoy or golang, so it's not really realistic at this stage.
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 was referring to envoyproxy/go-control-plane#511. I guess would need to add all the custom matchers though