-
Notifications
You must be signed in to change notification settings - Fork 5.3k
ext_authz: added ability to detect partial request body data #6583
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
Merged
mattklein123
merged 20 commits into
envoyproxy:master
from
brectanus-sigsci:buffered_authz_partial_flag
Apr 24, 2019
Merged
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
7236a7a
add partial_body field to attribute_context
brectanus-sigsci 7cfd880
update changelog
brectanus-sigsci fbeba65
fixed ref in changelog
brectanus-sigsci 944ff2b
added a constant for the new metadata header
brectanus-sigsci 12e3e10
changed from a new field to adding a metadata header to indicate part…
brectanus-sigsci d69fceb
document the metadata header being added to the authorization request…
brectanus-sigsci 9a2f45b
update release notes
brectanus-sigsci 950e7cf
fix formatting
brectanus-sigsci 6e196c2
make sure that a client supplied EnvoyAuthPartialBody header is not used
brectanus-sigsci cebcc3c
fixed changelog order
brectanus-sigsci 2b3b7dd
Merge branch 'master' into buffered_authz_partial_flag
brectanus-sigsci 20a3ce9
reorder changelog entry again after merge destroyed alpha order, lol
brectanus-sigsci 31a2079
Merge branch 'master' into buffered_authz_partial_flag
brectanus-sigsci 6f1f781
change header values from `0|1` to `false|true`
brectanus-sigsci 568b9c7
fix formatting
brectanus-sigsci 9d3255a
Merge branch 'master' into buffered_authz_partial_flag
brectanus-sigsci d2f0bc4
synced changelog with master to fix docs test issues
brectanus-sigsci 3893170
Merge branch 'master' into buffered_authz_partial_flag
brectanus-sigsci b1ce1f6
fix conflict in changelog
brectanus-sigsci 7507afe
Merge branch 'master' into buffered_authz_partial_flag
brectanus-sigsci 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
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
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.
We may also want to check that this header does not exist when the request is not a partial message. WDYT?
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.
Not exactly sure what you mean here. Only send the header if the body was actually truncated? Or only send the header if the
allow_partial_messageoption is set...The metadata header is currently only there if
max_request_bytesis set. In order to detect if this feature is available, I needed to send it with a bool (0|1) value (vs only sending the header if the body is partial). This way, in the gRPC service, you can use the logic "if the header is there, then it is coming from an envoy with this feature (v1.11+) and the value indicates partial or not. In this case the work is done for you, otherwise this is an older envoy and you need to detect this on your own using C-L header or other means."Now, if you mean only write the header if the config option
allow_partial_message: true, then yeah, that could be done, but I did not want to add another parameter toCheckRequestUtils::setHttpRequestto expose a check for this. I did not think it was worth it as it meant it was harder to determine from the gRPC service if the new partial flag feature was available. That is, if the header is not sent whenallow_partial_message: false, then the gRPC service cannot tell if it is envoy v1.10 or the partial feature is disabled in v1.11.If you think that the metadata header may get in people's way when
allow_partial_message: false, then I can expose the config option as a parameter toCheckRequestUtils::setHttpRequest. Unless maybe you see another way to get access to the config option without it as a parameter?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.
What happens if the client request contains
x-envoy-auth-partial-body: 1header? Not sure if we should trust the client.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.
Good point. I'll skip that header during the copy.