Make grpc_http1_reverse_bridge return HTTP 200. #31047
Conversation
|
@hq6 please fix DCO per these instructions https://github.com/envoyproxy/envoy/pull/31047/checks?check_run_id=19010034687 |
|
/wait |
hq6
force-pushed
the
hqin/fix_http_status_in_grpc_bridge
branch
from
adb8dfd
to
1085d79
Compare
Done. |
| @@ -179,6 +179,9 @@ Http::FilterHeadersStatus Filter::encodeHeaders(Http::ResponseHeaderMap& headers | |||
| // We can only insert trailers at the end of data, so keep track of this value | |||
| // until then. | |||
| grpc_status_ = grpcStatusFromHeaders(headers); | |||
|
|
|||
| // gRPC clients expect that the HTTP status will always be 200. | |||
There was a problem hiding this comment.
I think you should do it only when it is a gRPC response. For regular HTTP responses the status should not be modified.
Also as this is a user visible change it will require a runtime flag protection.
There was a problem hiding this comment.
My understanding is that if this filter is active (enabled_ is only set to true if the request headers indicate it's a gRPC request), then the response is a gRPC response. Thus, my code change only applies when it is a gRPC response.
Could you link me to an example PR or docs for enabling runtime flag protection?
There was a problem hiding this comment.
Runtime flags are defined here: https://github.com/envoyproxy/envoy/blob/main/source/common/runtime/runtime_features.cc
|
/wait |
hq6
force-pushed
the
hqin/fix_http_status_in_grpc_bridge
branch
from
1085d79
to
9d9b5a5
Compare
hq6
force-pushed
the
hqin/fix_http_status_in_grpc_bridge
branch
from
9d9b5a5
to
91924a2
Compare
|
@yanavlasov @zuercher I have rebased this PR and added runtime protection as requested. Are there additional changes you'd like to see in this PR? |
hq6
force-pushed
the
hqin/fix_http_status_in_grpc_bridge
branch
from
a21aaf6
to
5ce350b
Compare
|
@yanavlasov @zuercher Are there additional changes you'd like to see in this PR? |
|
/retest |
|
@zuercher PTAL |
|
@hq6 if you resolve the merge conflict this should be good to land |
gRPC clients expect that request status will be communicated using the gRPC status header, and that HTTP status for responses should always be 200 for properly behaving servers. This commit updates the grpc_http1_reverse_bridge to follow this behavior. Signed-off-by: Henry Qin <[email protected]>
hq6
force-pushed
the
hqin/fix_http_status_in_grpc_bridge
branch
from
5ce350b
to
fc40208
Compare
|
@hq6 if you have a reviewer preferably dont force-push |
Oops I saw this message too late. Sorry I'm used to following a rebase workflow and didn't realize it was causing review dismissal. |
|
apologies @hq6 i set this to automerge but it flaked and now there is another conflict - would you mind updating again |
Signed-off-by: Henry Qin <[email protected]>
Signed-off-by: Henry Qin <[email protected]>
Head branch was pushed to by a user without write access
This commit removes the runtime flag introduced in envoyproxy#31047 since it has been 6 months since that change. Signed-off-by: Henry Qin <[email protected]>
This commit removes the runtime flag introduced in envoyproxy#31047 since it has been 6 months since that change. Signed-off-by: Henry Qin <[email protected]>
…p_status This commit removes the runtime flag introduced in #31047 since it has been 6 months since that change. Signed-off-by: Henry Qin <[email protected]>
…p_status This commit removes the runtime flag introduced in envoyproxy#31047 since it has been 6 months since that change. Signed-off-by: Henry Qin <[email protected]> Signed-off-by: Martin Duke <[email protected]>
…p_status This commit removes the runtime flag introduced in envoyproxy#31047 since it has been 6 months since that change. Signed-off-by: Henry Qin <[email protected]> Signed-off-by: asingh-g <[email protected]>
gRPC clients expect that request status will be communicated using the gRPC status header, and that HTTP status for responses should always be 200 for properly behaving servers.
This commit updates the grpc_http1_reverse_bridge to follow this behavior.
Testing: Tested manually with grpcurl.