fix(embed): preserve legacy grpc-gateway error field via internal proto #21143
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fuweid The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 18 files with indirect coverage changes @@ Coverage Diff @@
## main #21143 +/- ##
==========================================
- Coverage 68.40% 68.38% -0.03%
==========================================
Files 429 429
Lines 35242 35251 +9
==========================================
- Hits 24109 24107 -2
- Misses 9737 9752 +15
+ Partials 1396 1392 -4 Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
grpc-gateway v2.0 dropped the legacy "error" JSON field, breaking clients relying on it (grpc-ecosystem/grpc-gateway#1098, etcd-io#1242). Add internalpb.GRPCGatewayError and rewrite Status responses to preserve backward-compatible error payloads. Deprecation plan: keep "error" alongside "message" in v3.7 with documentation warning; remove "error" in v3.8 once clients migrate. Signed-off-by: Wei Fu <fuweid89@gmail.com>
|
@fuweid: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
We should avoid making any breaking changes for API. |
Agree, as long as grpc-gateway can provide hook for us to update response status. 😂 |
We should ask them nicely. |
|
Pls see #21100 (comment) Let's try to request grpc-gateway to add the error back |
| // The "error" field is deprecated in v3.7 and will be removed in v3.8; use | ||
| // the standard gRPC Status fields ("code", "message", "details") instead. |
There was a problem hiding this comment.
IF we plan to remove it in 3.8, then we don't need to resolve the issue at all.
The key for now is that we should formally document/standardize the error response format, either on grpc-gateway side or etcd side. Once finalized, we shouldn't change it.
There was a problem hiding this comment.
Personally, I think we can just document this going forward, since the HTTP status code is correct and the error field is duplicated with message. If we all agree not to resolve it, that should be sufficient.
There was a problem hiding this comment.
I think we can just document this going forward
If we don't change anything, it means we depend on whatever grpc-gateway provides. We should link to grpc-gateway's doc if any.
4 participants
grpc-gateway v2.0 dropped the legacy "error" JSON field, breaking clients relying on it (grpc-ecosystem/grpc-gateway#1098, grpc-ecosystem/grpc-gateway#1242). Add internalpb.GRPCGatewayError and rewrite Status responses to preserve backward-compatible error payloads.
Deprecation plan: keep "error" alongside "message" in v3.7 with documentation warning; remove "error" in v3.8 once clients migrate.
Fixes: #21100
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.