grpc: restrict status codes from control plane (gRFC A54) #5653
Conversation
| @@ -589,7 +590,11 @@ func (t *http2Client) getTrAuthData(ctx context.Context, audience string) (map[s | |||
| for _, c := range t.perRPCCreds { | |||
| data, err := c.GetRequestMetadata(ctx, audience) | |||
| if err != nil { | |||
| if _, ok := status.FromError(err); ok { | |||
| if st, ok := status.FromError(err); ok { | |||
There was a problem hiding this comment.
I'm a little concerned that we have to do this in multiple places. How can we guarantee that we haven't missed a place (in this PR), or that we don't miss one in the future when we are adding something.
There was a problem hiding this comment.
A54 specifies the places that should be covered, and this PR should cover all of them. I've also reached out to Eric about whether other plugins (e.g. compressors and encoders) should be considered.
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case csErr <- tc.csErr: | ||
| default: | ||
| } |
There was a problem hiding this comment.
We could instead move the whole setup (creating the stub server, starting it, and updating the manual resolver with a config selector) into here, and get rid of the channel in the funcConfigSelector. Doing that would make the subTests completely independent of each other, and would make it impossible for one to interfere in anyway with the other.. And these things run quite quickly, so I wouldn't be too worried about increase in run time. What do you think?
test/control_plane_status_test.go
Outdated
|
|
||
| pickerErr := make(chan error, 1) | ||
| balancer.Register(&lbBuilderWrapper{ | ||
| builder: balancer.Get("round_robin"), |
There was a problem hiding this comment.
Why do we need a real balancer inside this test balancer? Can't we use the stub balancer and override the UpdateClientConnState to return an error picker (with an error configured through the test table)?
And similar to the comment on the earlier test, could you please move all setup inside the t.Run() so that the subTests can be independent of each other.
There was a problem hiding this comment.
Why do we need a real balancer inside this test balancer? Can't we use the stub balancer and override the UpdateClientConnState to return an error picker (with an error configured through the test table)?
Yes, that's much simpler, thanks.
move all setup inside the t.Run()
Done; simplified.
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case errChan <- tc.credsErr: | ||
| default: | ||
| } |
There was a problem hiding this comment.
Same here, we could make the sub tests independent.
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case errChan <- tc.credsErr: | ||
| default: | ||
| } |
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case csErr <- tc.csErr: | ||
| default: | ||
| } |
test/control_plane_status_test.go
Outdated
|
|
||
| pickerErr := make(chan error, 1) | ||
| balancer.Register(&lbBuilderWrapper{ | ||
| builder: balancer.Get("round_robin"), |
There was a problem hiding this comment.
Why do we need a real balancer inside this test balancer? Can't we use the stub balancer and override the UpdateClientConnState to return an error picker (with an error configured through the test table)?
Yes, that's much simpler, thanks.
move all setup inside the t.Run()
Done; simplified.
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case errChan <- tc.credsErr: | ||
| default: | ||
| } |
test/control_plane_status_test.go
Outdated
| // In case the channel is full due to a previous iteration failure, | ||
| // do not block. | ||
| select { | ||
| case errChan <- tc.credsErr: | ||
| default: | ||
| } |
|
Ping @easwars |
RELEASE NOTES: