-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
ext_authz: allows multiple headers of same name in Denied response #8668
Conversation
Enhance the ext_authz filter to allow multiple `Set-Cookie` headers to be added by a `Denied` `Check` response. Previously, when the `Check` response contained multiple headers of the same name, only the last one would be applied in the http reponse. Fixes envoyproxy#8649 Signed-off-by: Peter Chen <pchen@pivotal.io> Signed-off-by: Ryan Richard <rrichard@pivotal.io>
Signed-off-by: Ryan Richard <rrichard@pivotal.io> Signed-off-by: Peter Chen <pchen@pivotal.io>
🤷♀️ nothing to rebuild. |
🤷♀️ nothing to rebuild. |
/azp run envoy-linux |
Azure Pipelines successfully started running 1 pipeline(s). |
Commenter does not have sufficient privileges for PR 8668 in repo envoyproxy/envoy |
@dio, we believe the test failures might be unrelated to our changeset. Could you re-run the test or give us some feedback about why these tests might be failing? |
@lizan We believe the test failures are unrelated. Could you take a look as well? |
@srwaggon @peterhaochen47 can you try to merge master? Thanks! |
Signed-off-by: Ryan Richard <rrichard@pivotal.io>
@dio Thanks for the advice 😄 That did it! This PR looks like it can be merged now with an approval! |
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.
Looks good, a request for comments.
- Added to ext_authz.cc at the request of the PR reviewer Signed-off-by: Ryan Richard <rrichard@pivotal.io>
@gsagula could you help to verify this as well? Thanks! |
Thanks for reviewing the PR. Is it ready to be accepted? Is there anything else that we can do to help it get accepted? I ask because it is blocking the initial release of the new istio-ecosystem authservice project, which needs to be able to set and delete multiple cookies. Thanks! |
@cfryanr seems like you need to merge master again. Sorry. |
/wait |
Signed-off-by: Ryan Richard <rrichard@pivotal.io>
Hi @dio. Thanks. I merged master into the fork. There were no merge conflicts. |
Description:
Enhance the ext_authz filter to allow multiple
Set-Cookie
headers to be added by a
Denied
Check
response.Previously, when the
Check
response contained multipleheaders of the same name, only the last one would be applied
in the http response. Please see full description of problem
in #8649.
Risk Level: Low
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A
Fixes #8649