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

Set the field ignore_case to true when use header match #35520

Merged
merged 6 commits into from Oct 15, 2021
Merged

Set the field ignore_case to true when use header match #35520

merged 6 commits into from Oct 15, 2021

Conversation

devincd
Copy link
Member

@devincd devincd commented Oct 8, 2021

Signed-off-by: devincd 505259926@qq.com

Please provide a description of this PR:

Fix the issue: #35220

To help us figure out who should review this PR, please put an X in all the areas that this PR affects.

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

Please check any characteristics that apply to this pull request.

  • Does not have any user-facing changes. This may include CLI changes, API changes, behavior changes, performance improvements, etc.

@devincd
set ignore_case to true when use header match
4c554bf 
Signed-off-by: devincd <505259926@qq.com>
@devincd devincd requested a review from a team as a code owner October 8, 2021 07:50
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Oct 8, 2021
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 8, 2021
@devincd devincd added the release-notes-none Indicates a PR that does not require release notes. label Oct 8, 2021
@devincd
Copy link
Member Author

devincd commented Oct 8, 2021

/retest

@devincd
fix test failed
8eedd5e 
Signed-off-by: devincd <505259926@qq.com>
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 8, 2021
@devincd
execute make gen command
1ad71a2 
Signed-off-by: devincd <505259926@qq.com>
@devincd
Copy link
Member Author

devincd commented Oct 8, 2021

/retest

howardjohn
howardjohn reviewed Oct 8, 2021
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 make sure we consider backwards compatibility. Even if this is a good change it is a behavioral change. At the very least it needs a release note.

This only impacts ext authz which is an experimental feature right?

@devincd
Copy link
Member Author

devincd commented Oct 8, 2021

Please make sure we consider backwards compatibility. Even if this is a good change it is a behavioral change. At the very least it needs a release note.

This only impacts ext authz which is an experimental feature right?

yes，this is only impacts ext authz and I will add a release note.

yangminzhu
yangminzhu reviewed Oct 8, 2021
View reviewed changes
Copy link
Contributor

@yangminzhu yangminzhu left a comment

Choose a reason for hiding this comment

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

yes, please add a release note for this change.

This should be backward compatible as the HTTP header name is considered case insensitive in the first place.

@devincd
add releasenotes
8047e0e 
Signed-off-by: devincd <505259926@qq.com>
@devincd devincd requested review from a team as code owners October 9, 2021 01:52
@devincd devincd removed the release-notes-none Indicates a PR that does not require release notes. label Oct 9, 2021
@devincd
fix release notes
59eef48 
Signed-off-by: devincd <505259926@qq.com>
@devincd
Copy link
Member Author

devincd commented Oct 11, 2021

@howardjohn @yangminzhu Please take a look, thinks.

yangminzhu
yangminzhu reviewed Oct 12, 2021
View reviewed changes
releasenotes/notes/35220.yaml Outdated
- 35220
releaseNotes:
- |
**Added** envoyExtAuthzHttp does **case-insensitive** when matches on headers.
Copy link
Contributor

Choose a reason for hiding this comment

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

**Fixed** the EnvoyExternalAuthorizationHttpProvider to match HTTP headers in a case-insensitive way.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed.

@devincd
change the notes
37fff6e 
Signed-off-by: devincd <505259926@qq.com>
@jacob-delgado
Copy link
Contributor

LGTM, but I'll wait for John's feedback

@devincd
Copy link
Member Author

devincd commented Oct 15, 2021

/retest

@istio-testing istio-testing merged commit aa475b9 into istio:master Oct 15, 2021
@devincd devincd deleted the set_ignore_case_true branch October 15, 2021 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

@yangminzhu yangminzhu yangminzhu approved these changes

Assignees

@yangminzhu yangminzhu

Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@devincd @jacob-delgado @howardjohn @yangminzhu @istio-testing