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.

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

grpc: restrict status codes from control plane (gRFC A54) #5653

Merged
merged 3 commits into from Oct 4, 2022

Conversation

dfawley
Copy link
Contributor

@dfawley dfawley commented Sep 13, 2022

RELEASE NOTES:

  • grpc: restrict status codes from control plane (gRFC A54)

@dfawley dfawley added the Type: Behavior Change Behavior changes not categorized as bugs label Sep 13, 2022
@dfawley dfawley added this to the 1.50 Release milestone Sep 13, 2022
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@easwars easwars assigned dfawley and unassigned easwars Sep 15, 2022
@dfawley dfawley assigned easwars and unassigned dfawley Sep 20, 2022
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

The code change look fine. Have a few comments about the tests.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case csErr <- tc.csErr:
default:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks!


pickerErr := make(chan error, 1)
balancer.Register(&lbBuilderWrapper{
builder: balancer.Get("round_robin"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case errChan <- tc.credsErr:
default:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, we could make the sub tests independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case errChan <- tc.credsErr:
default:
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@easwars easwars assigned dfawley and unassigned easwars Sep 27, 2022
Copy link
Contributor Author

@dfawley dfawley 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 the review; comments addressed.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case csErr <- tc.csErr:
default:
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks!


pickerErr := make(chan error, 1)
balancer.Register(&lbBuilderWrapper{
builder: balancer.Get("round_robin"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case errChan <- tc.credsErr:
default:
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// In case the channel is full due to a previous iteration failure,
// do not block.
select {
case errChan <- tc.credsErr:
default:
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dfawley dfawley assigned easwars and unassigned dfawley Sep 28, 2022
@dfawley
Copy link
Contributor Author

dfawley commented Oct 4, 2022

Ping @easwars

easwars
easwars approved these changes Oct 4, 2022
@easwars easwars assigned dfawley and unassigned easwars Oct 4, 2022
@dfawley dfawley modified the milestones: 1.50 Release, 1.51 Release Oct 4, 2022
@dfawley dfawley merged commit 12db695 into grpc:master Oct 4, 2022
11 checks passed
@dfawley dfawley deleted the restrict_code branch October 4, 2022 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Behavior Change Behavior changes not categorized as bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants