Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add filter state IP matcher #37637

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add filter state IP matcher #37637

wants to merge 13 commits into from

Conversation

antoniovleonti
Copy link
Contributor

Commit Message: Add filter state IP matcher
Additional Description:

This cl adds a matcher to the generic filter state matcher which attempts to coerce the specified filter state object into an IP, then matches it against a configured CIDR range. If the IP is in the range, the matcher returns true, otherwise false.

Risk Level: low
Testing: unit tested
Docs Changes: none
Release Notes: changelog updated
Platform Specific Features: none

@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37637 was opened by antoniovleonti.

see: more, trace.

@antoniovleonti
Copy link
Contributor Author

/assign @markdroth

@antoniovleonti
Copy link
Contributor Author

/retest

@antoniovleonti
add filter state IP matcher
1cd6e9e 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
add filter state IP matcher
5c7943d 
Signed-off-by: antoniovleonti <[email protected]>
wbpcode
wbpcode reviewed Dec 16, 2024
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution. And a comment to API to start this work.

api/envoy/type/matcher/v3/filter_state.proto Outdated Show resolved Hide resolved
@antoniovleonti
Copy link
Contributor Author

If you have any idea what is causing this error

WARNING: undefined label: 'envoy_api_msg_type.matcher.filterstatematcher'

Please let me know :). It's from the changelog but I used the same pattern from other matchers: https://github.com/search?q=repo%3Aenvoyproxy%2Fenvoy%20envoy_api_msg_type.matcher&type=code

@antoniovleonti antoniovleonti mentioned this pull request Dec 17, 2024
Closed
@wbpcode
Copy link
Member

wbpcode commented Dec 18, 2024

If you have any idea what is causing this error


WARNING: undefined label: 'envoy_api_msg_type.matcher.filterstatematcher'

Please let me know :). It's from the changelog but I used the same pattern from other matchers: https://github.com/search?q=repo%3Aenvoyproxy%2Fenvoy%20envoy_api_msg_type.matcher&type=code

ｉthink you missed the version in that label?

@antoniovleonti
add version to label
0987937 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
add address.proto
726a571 
Signed-off-by: antoniovleonti <[email protected]>
@antoniovleonti
Copy link
Contributor Author

/retest

wbpcode
wbpcode reviewed Dec 25, 2024
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

Thanks for the update and sorry the delay. I add some comments to the source code. But I think it's good overall. And the api is fine now.

/lgtm api

envoy/network/address.h Outdated Show resolved Hide resolved
source/common/common/matchers.cc Outdated Show resolved Hide resolved
source/common/common/filter_state_object_matchers.h Outdated Show resolved Hide resolved
source/common/common/filter_state_object_matchers.h Show resolved Hide resolved
@wbpcode
Copy link
Member

wbpcode commented Dec 25, 2024

/wait

@wbpcode
Copy link
Member

wbpcode commented Dec 25, 2024 •
edited
Loading

And merry Christmas!!! 🎄

And please ping me at slack directly if you are back and want a quick review. Sometimes I will fall into a lazy status and won't check the github notifications. 😴

@antoniovleonti
address most comments
af91f3d 
Signed-off-by: antoniovleonti <[email protected]>
wbpcode
wbpcode previously approved these changes Jan 4, 2025
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks. Could you check the CI? Thanks.

@antoniovleonti
add protodoc title
24b3c8d 
Signed-off-by: antoniovleonti <[email protected]>
@wbpcode
Copy link
Member

wbpcode commented Jan 7, 2025

You may also need add the new proto to the docs/root/api-v3/types/types.rst

@wbpcode wbpcode added the waiting label Jan 7, 2025
@antoniovleonti
add proto to docs/root/...
5e1836a 
Signed-off-by: antoniovleonti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth left review comments

@wbpcode wbpcode wbpcode left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@wbpcode wbpcode

@markdroth markdroth

Labels
api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@antoniovleonti @wbpcode @markdroth