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

grpc: implement WithInsecure() using the insecure package #4718

Merged
merged 4 commits into from
Nov 9, 2021
Merged

grpc: implement WithInsecure() using the insecure package #4718

merged 4 commits into from
Nov 9, 2021

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Aug 31, 2021 •
edited
Loading

We have an insecure transport credentials implementation in the insecure package. This PR changes the implementation of WithInsecure() to use that instead of setting a different field inside of dialOptions.

RELEASE NOTES:

  • Implement grpc.WithInsecure using insecure.NewCredentials * Mark grpc.WithInsecure as deprecated * Make it mandatory for credentials.Bundle to return non-nil TransportCredentials

@easwars easwars added the Type: Internal Cleanup Refactors, etc label Aug 31, 2021
@easwars easwars added this to the 1.41 Release milestone Aug 31, 2021
@easwars
Copy link
Contributor Author

easwars commented Aug 31, 2021

Fixes #4716.

@easwars easwars closed this Sep 8, 2021
@easwars easwars reopened this Nov 9, 2021
@easwars easwars assigned dfawley and unassigned menghanl Nov 9, 2021
@easwars
Copy link
Contributor Author

easwars commented Nov 9, 2021

Now, that we have the URL parsing and other changes merged, I've brought this PR back to life.

@easwars easwars modified the milestones: 1.41 Release, 1.43 release Nov 9, 2021
dfawley
dfawley requested changes Nov 9, 2021
View reviewed changes
Copy link
Member

@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.

🧟

dialoptions.go Outdated
@@ -303,7 +303,7 @@ func WithReturnConnectionError() DialOption {
// set.
Copy link
Member

Choose a reason for hiding this comment

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

Can we spiff up this docstring, please? Indicate it selects the insecure.NewCredentials() and that it is equivalent to doing that. Also clarify that it is incompatible with WithCredsBundle and per-RPC creds that RequireTransportSecurity. Should we call it deprecated and recommend the more "pure" form?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified things in this docstring. Marked it as deprecated.
Also clarified the same thing on insecure.NewCredentials().

Copy link

Choose a reason for hiding this comment

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

(got here via staticcheck deprecation failures.)

Q: insecure is still declared experimental. Is that still the case or is it safe to take in use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The insecure package is marked experimental. When we add new APIs, it is standard practice to mark them experimental as it gives us the flexibility to change things going forward based on user feedback etc. Once we are confident about the API, we usually remove the experimental tag.

insecure.NewCredentials is the way going forward.

clientconn.go Outdated
if transportCreds == nil {
transportCreds = cc.dopts.copts.CredsBundle.TransportCredentials()
}
if transportCreds != nil && transportCreds.Info().SecurityProtocol == "insecure" {
Copy link
Member

Choose a reason for hiding this comment

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

transportCreds != nil == true

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 is transportCreds != nil not sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was pointing out that it is unnecessary.

I suppose the bundle could have a nil TransportCredentials() in it. But then we should call that an error, too (on its own), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that was exactly the case for which I added the nil check.

Hmm, since we don't allow not configuring a transport credentials, I think it is valid to error out when we have a bundle with nil transport creds. But the Bundle interface does not document that it should not contain a nil transport credentials.

Do you think this will be a behavior change that can break people or you think it is better to enforce it?

Copy link
Member

Choose a reason for hiding this comment

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

Nobody should be taking advantage of that loophole. Bundles are experimental, as well. Let's update the documentation to say transport credentials are required (and may be insecure.NewCredentials() if needed), and enforce it here as well.

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. Added the constraint, related documentation and test.

test/insecure_creds_test.go Outdated
desc: "send PerRPCCredentials via CallOptions",
perRPCCredsViaDialOptions: false,
perRPCCredsViaCallOptions: true,
func (s) TestInsecureCredsWith_PerRPCCredentials_AsCallOption(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Should the first underscore be before With?

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 :)

test/insecure_creds_test.go Outdated
const wantErr = "transport: cannot send secure credentials on an insecure connection"
c := testpb.NewTestServiceClient(cc)
if _, err = c.EmptyCall(ctx, &testpb.Empty{}, copts...); err == nil || !strings.Contains(err.Error(), wantErr) {
t.Fatalf("InsecureCredsWithPerRPCCredentials/send_PerRPCCredentials_via_CallOptions = %v; want %s", err, wantErr)
Copy link
Member

Choose a reason for hiding this comment

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

This error is no longer accurate. Also I'm not sure why the name of the test is in the error message in the first place. :)

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 Nov 9, 2021
@easwars easwars assigned dfawley and unassigned easwars Nov 9, 2021
@dfawley dfawley assigned easwars and unassigned dfawley Nov 9, 2021
@easwars easwars assigned dfawley and unassigned easwars Nov 9, 2021
dfawley
dfawley approved these changes Nov 9, 2021
View reviewed changes
credentials/credentials.go Outdated
@@ -178,8 +178,14 @@ type TransportCredentials interface {
//
// This API is experimental.
type Bundle interface {
// TransportCredentials returns the transport credentials from the Bundle.
//
// Implementations must return non-nil transport credentials.
Copy link
Member

Choose a reason for hiding this comment

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

Mention insecure credentials here?

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.

credentials/credentials.go
TransportCredentials() TransportCredentials

// PerRPCCredentials returns the per-RPC credentials from the Bundle.
Copy link
Member

Choose a reason for hiding this comment

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

"May be nil if per-RPC credentials are not needed"?

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 menghanl and easwars and unassigned dfawley Nov 9, 2021
@easwars easwars merged commit dd76741 into grpc:master Nov 9, 2021
@easwars easwars deleted the merge_insecures branch November 9, 2021 23:42
@frzifus frzifus mentioned this pull request Dec 16, 2021
Merged
@adambabik adambabik mentioned this pull request Dec 20, 2021
Merged
@easwars easwars mentioned this pull request Jan 20, 2022
Closed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mythi mythi mythi left review comments

@dfawley dfawley dfawley approved these changes

@menghanl menghanl Awaiting requested review from menghanl

Assignees

@easwars easwars

Labels
Type: Internal Cleanup Refactors, etc
Projects
None yet
Milestone
1.43 Release
Development

Successfully merging this pull request may close these issues.
4 participants
@easwars @mythi @dfawley @menghanl