docs: correct api/envoy/service/auth/v2 docs#6211
Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom Mar 25, 2019
LukeShu:lukeshu/ext-authz-grpc-docs
Merged
docs: correct api/envoy/service/auth/v2 docs#6211mattklein123 merged 3 commits intoenvoyproxy:masterfrom LukeShu:lukeshu/ext-authz-grpc-docs
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
LukeShu:lukeshu/ext-authz-grpc-docs
Conversation
Update some documentation comments in api/envoy/service/auth/v2/*.proto to more accurately describe the *current* behavior (without making any judgment on whether that behavior is "correct" or desirable). Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Contributor
Author
|
v2: Add DCO |
LukeShu
commented
Mar 9, 2019
| message CheckResponse { | ||
| // Status `OK` allows the request. Any other status indicates the request should be denied. | ||
| // Status `OK` allows the request. Status `UNKNOWN` causes Envoy to abort. Any other status | ||
| // indicates the request should be denied. |
junr03
reviewed
Mar 11, 2019
| // appears in the first line of the HTTP request. No decoding is performed. | ||
| // This field is always empty, and exists for compatibility reasons. The HTTP URL query is | ||
| // included in `path` field. | ||
| string query = 7; |
Member
There was a problem hiding this comment.
can we enforce empty here? Is it worth it?
Contributor
Author
There was a problem hiding this comment.
What do you mean "enforce empty"? Note that Envoy is what populates this, not the AuthService talking to Envoy. I don't believe that code outside of Envoy ever needs to create this.
| // The network protocol used with the request, such as | ||
| // "http/1.1", "spdy/3", "h2", "h2c" | ||
| // The network protocol used with the request, as enumerated by | ||
| // `source/common/http/headers.h:ProtocolStrings`, such as "HTTP/1.0", "HTTP/1.1", or |
Contributor
Author
There was a problem hiding this comment.
What should that URL look like? AFAICT, there's not a documentation page on ProtocolStrings. Should it link to the source on GitHub?
| // Intended for gRPC and Network Authorization servers `only`. | ||
| message CheckResponse { | ||
| // Status `OK` allows the request. Any other status indicates the request should be denied. | ||
| // Status `OK` allows the request. Status `UNKNOWN` causes Envoy to abort. Any other status |
Contributor
Author
There was a problem hiding this comment.
"abort" means "exit with a stack trace", which I guess is probably a bug (#6210).
| string query = 7; | ||
|
|
||
| // The HTTP URL fragment, excluding leading `#`. No URL decoding is performed. | ||
| // This field is always empty, and exists for compatibility reasons. The URL fragment is |
Member
|
@LukeShu just replied to your comments. I will approve after update. |
Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
Contributor
Author
|
@junr03 I just added a commit. I think I got the link syntax correct, but please verify that. |
mattklein123
requested changes
Mar 23, 2019
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks, one small request.
/wait
… link Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
alyssawilk
pushed a commit
that referenced
this pull request
May 7, 2019
…6821) Description: PR #6211 updated the documentation of CheckResponse.status to reflect Envoy's actual behavior at the time. Later, PR #6505 changed that behavior to be in-line with the pre-6211 docs. So, revert that part of PR #6211. Risk Level: Low Testing: None Docs Changes: Inline in API protos Release Notes: none Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description: Update some documentation comments in
api/envoy/service/auth/v2/*.prototo more accurately describe the current behavior (without making any judgment on whether that behavior is "correct" or desirable).Risk Level: Low
Testing: None
Docs Changes: Inline in API protos
Release Notes: None?
Attn: @saumoh, who I believe to be the original author.