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

xds: Remove usages of grpc.WithInsecure(). #4028

Merged
merged 2 commits into from
Nov 12, 2020

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Nov 11, 2020

No description provided.

@menghanl
Copy link
Contributor

What's the purpose of this change? I thought the main reason the insecure.New() is for the xds creds to set as the fallback?
And are we going to change the other non-xds code the same? If we are going to change everything, can we just change how grpc.WithInsecure() works?

@easwars
Copy link
Contributor Author

easwars commented Nov 12, 2020

What's the purpose of this change? I thought the main reason the insecure.New() is for the xds creds to set as the fallback?
And are we going to change the other non-xds code the same? If we are going to change everything, can we just change how grpc.WithInsecure() works?

We could change how grpc.WithInsecure() works, but I thought the plan was to deprecate it.

@menghanl
Copy link
Contributor

We can deprecate WithInsecure() and change its implementation in the same PR. We could probably also simplify grpc.Dial a bit at the same time.

And, is there any specific reason that you sent this change only to update xds code? Do you need this for xds security? From the changes, I don't see how they are different from the non-xds usage of WithInsecure().

@easwars
Copy link
Contributor Author

easwars commented Nov 12, 2020

And, is there any specific reason that you sent this change only to update xds code? Do you need this for xds security? From the changes, I don't see how they are different from the non-xds usage of WithInsecure().

This was easier (and I just did this while I was blocked on something else) and I didn't want to get into the big change at this point. I thought changing usages separately and incrementally and then doing the deprecation and required simplification to grpc.Dial might be easier.

@easwars
Copy link
Contributor Author

easwars commented Nov 12, 2020

Do you need this for xds security? From the changes, I don't see how they are different from the non-xds usage of WithInsecure().

Not required for security. As I mentioned in the other comment, I just did this when I was blocked on something else, and thought it would be better to do this incrementally than one giant change.

@easwars
Copy link
Contributor Author

easwars commented Nov 12, 2020

Rebased after #4015

@easwars easwars merged commit 22dba5e into grpc:master Nov 12, 2020
@easwars easwars deleted the xds_insecure_creds branch November 12, 2020 20:13
davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants