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

refactor authz HeaderMatcher #42655

Merged
merged 14 commits into from
Aug 17, 2023
Merged

refactor authz HeaderMatcher #42655

merged 14 commits into from
Aug 17, 2023

Conversation

wulianglongrd
Copy link
Member

@wulianglongrd wulianglongrd commented Jan 4, 2023 •
edited
Loading

Please provide a description of this PR:

This PR made some refactoring about authz HeaderMatcher.

  1. Use HeaderMatcher_StringMatch instead of deprecated HeaderMatcher_PrefixMatch, HeaderMatcher_SuffixMatch, HeaderMatcher_ExactMatch and HeaderMatcher_SafeRegexMatch.
  2. Remove some redundant code for building StringMatcher.
  3. Use the more readable trim func, e.g. use strings.TrimPrefix(v, "*") instead of v[1:].
  4. Update the test cases.

@wulianglongrd wulianglongrd requested a review from a team as a code owner January 4, 2023 11:27
@istio-policy-bot istio-policy-bot added area/networking area/security release-notes-none Indicates a PR that does not require release notes. labels Jan 4, 2023
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Jan 4, 2023
@istio-testing
Copy link
Collaborator

Hi @wulianglongrd. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

costinm
costinm reviewed Jan 4, 2023
View reviewed changes
Copy link
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

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

Looks good, but please confirm it is not a behavior change.

@wulianglongrd wulianglongrd requested review from howardjohn and costinm and removed request for howardjohn and costinm January 5, 2023 16:24
@wulianglongrd
Copy link
Member Author

ping @liminw

liminw
liminw reviewed Jan 7, 2023
View reviewed changes
pilot/pkg/security/authz/matcher/header_test.go Outdated Show resolved Hide resolved
pilot/pkg/security/authz/matcher/header_test.go Show resolved Hide resolved
pilot/pkg/security/authz/matcher/header_test.go Outdated Show resolved Hide resolved
pilot/pkg/security/authz/matcher/header_test.go Outdated Show resolved Hide resolved
pilot/pkg/security/authz/matcher/header_test.go Outdated Show resolved Hide resolved
pilot/pkg/security/authz/matcher/header_test.go Outdated Show resolved Hide resolved
howardjohn
howardjohn requested changes Jan 7, 2023
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

please address my open comment

@wulianglongrd
Copy link
Member Author

@howardjohn done

@wulianglongrd
Copy link
Member Author

please address my open comment

hello @howardjohn, all open comments have been marked as resolved. Did I forget something?

@howardjohn
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jan 9, 2023
@wulianglongrd wulianglongrd changed the title refactor authz HeaderMatcher [WIP] refactor authz HeaderMatcher Jan 10, 2023
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 10, 2023
@wulianglongrd
Copy link
Member Author

TestGRPC/gRPC-svc/gRPC-svc-tls failed because the grpc xds verified the type of HeaderMatcher. Currently grpc xds does not support HeaderMatcher_StringMatch implementation of HeaderMatcher (see there and this issue). Any suggestions?

@wulianglongrd wulianglongrd changed the title [WIP] refactor authz HeaderMatcher refactor authz HeaderMatcher Jan 16, 2023
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 16, 2023
@wulianglongrd wulianglongrd reopened this Aug 10, 2023
@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Aug 10, 2023
@wulianglongrd wulianglongrd removed the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Aug 10, 2023
@wulianglongrd wulianglongrd reopened this Aug 10, 2023
@wulianglongrd
Copy link
Member Author

Reopen because grpc-go already supports it.
grpc/grpc-go#6226

howardjohn
howardjohn requested changes Aug 17, 2023
View reviewed changes
pilot/pkg/security/authz/matcher/header.go Outdated
Prefix: v[:len(v)-1],
},
},
StringMatch: StringMatcherPrefix(strings.TrimSuffix(v, "*"), true),
Copy link
Member

Choose a reason for hiding this comment

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

please keep the old behavior v[:len(v)-1]. This changes the behavior of foo** or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

done, thanks.

howardjohn
howardjohn approved these changes Aug 17, 2023
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

thanks!

@howardjohn
Copy link
Member

/retest

@istio-testing istio-testing merged commit 1449cf7 into istio:master Aug 17, 2023
@wulianglongrd wulianglongrd deleted the wll-remove-deprecated branch August 18, 2023 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costinm costinm costinm left review comments

@ymesika ymesika ymesika left review comments

@howardjohn howardjohn howardjohn approved these changes

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@liminw liminw liminw approved these changes

Assignees

@liminw liminw

Labels
area/networking area/security ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. release-notes-none Indicates a PR that does not require release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@wulianglongrd @istio-testing @howardjohn @costinm @hzxuzhonghu @liminw @ymesika @istio-policy-bot