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

Replace GRPC InvalidCode with equivalent code from Http #33867

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Replace GRPC InvalidCode with equivalent code from Http #33867

wants to merge 1 commit into from

Conversation

juanmolle
Copy link
Contributor

Commit Message: common: Replace GRPC InvalidCode with equivalent code from Http

Additional Description: In case of the grpc status code is not set or an invalid code, envoy will set it with a code mapped from http code.

Risk Level: Low
Testing: yes
Docs Changes: no
Release Notes: no
Platform Specific Features: n/a

@juanmolle
Replace GRPC InvalidCode with equivalent code from Http
c6c38e3 
Signed-off-by: Juan Manuel Ollé <jolle@mulesoft.com>
@juanmolle juanmolle mentioned this pull request Apr 30, 2024
Merged
@juanmolle
Copy link
Contributor Author

@PiotrSikora Do I Understand correctly your comment?

PiotrSikora
PiotrSikora reviewed Apr 30, 2024
View reviewed changes
source/common/http/utility.cc
enumToInt(local_reply_data.grpc_status_
? local_reply_data.grpc_status_.value()
: Grpc::Utility::httpToGrpcStatus(enumToInt(response_code)))));
response_headers->setGrpcStatus(std::to_string(enumToInt(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you should gate the setGrpcStatus() call on the presence of valid gRPC status.

If there is no valid gRPC status set, then grpc-status shouldn't be set at all... and I don't believe your current change does that.

Also, I didn't follow Envoy's development for last couple of years, so I'm not sure if this is sufficient change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understand, there is a mapping between http code and grpc equivalent code. legacy do the mapping in case not set. Just added invalidcode.

Lets see if a maintainer following this module closely think if here is the most suitable place to do it.

adisuissa
adisuissa reviewed Apr 30, 2024
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Left a few minor comments.
You'll also need to add a release note.
One high-level question: why is gRPC InvalidCode needs a special treatment?

/wait

test/common/http/utility_test.cc
@@ -1034,6 +1034,26 @@ TEST(HttpUtility, SendLocalGrpcReplyGrpcStatusAlreadyExists) {
Grpc::Status::WellKnownGrpcStatus::InvalidArgument, false});
}

TEST(HttpUtility, SendLocalGrpcReplyGrpcStatusInvalidCode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a one-line comment describing the test.

source/common/http/utility.cc
? local_reply_data.grpc_status_.value()
: Grpc::Utility::httpToGrpcStatus(enumToInt(response_code)))));
response_headers->setGrpcStatus(std::to_string(enumToInt(
local_reply_data.grpc_status_ && local_reply_data.grpc_status_.value() !=
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is user-facing and should be runtime-guarded.

@adisuissa
Copy link
Contributor

Assigning Yan as this is a core-change.
/assign @yanavlasov

@adisuissa
Copy link
Contributor

Also assigning Kuat as the PR relates to #33856.
/assign @kyessenov

@yanavlasov
Copy link
Contributor

/wait

@juanmolle
Copy link
Contributor Author

@adisuissa Thanks for your quick reply

One high-level question: why is gRPC InvalidCode needs a special treatment?

Sending -1 (InvalidCode) it is casted as unsigned big number (I'm using grpcurl). Does it has a special meaning for a client? Not sure.
The original case is from wasm plugin, C interface does not allow to be called by default with null, then by default if no grpc code is not available, the sdk use -1.
My original PR covers only the wasm case, where it is expected to send it back -1 in case grpc status is not set #33856, @PiotrSikora opinion is that a general case could be covered.
If this change has concerns, we can deprecated it and focus in the wasm specific change. Both changes has the same output in the case of wasm filter.

@juanmolle
Copy link
Contributor Author

Should I continue with this change? does it make sense?

@adisuissa
Copy link
Contributor

I'm not a gRPC expert (and might be misunderstanding something here) but if InvalidCode is a valid return code then why not return it in the general case?
If there is a good reason to override the return code, then I think this PR is good.

cc @kyessenov who also reviewed #33856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PiotrSikora PiotrSikora PiotrSikora left review comments

@adisuissa adisuissa adisuissa left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@yanavlasov yanavlasov

@kyessenov kyessenov

Labels
waiting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@juanmolle @adisuissa @yanavlasov @PiotrSikora @kyessenov