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

A27 update: "channel_creds" bootstrap field is required and supports "insecure" #204

Merged
merged 1 commit into from Sep 3, 2020
Merged

A27 update: "channel_creds" bootstrap field is required and supports "insecure" #204

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions A27-xds-global-load-balancing.md
View file
Open in desktop
Expand Up @@ -123,9 +123,8 @@ like this:
{
"server_uri": <string containing URI of xds server>,
// List of channel creds; client will stop at the first type it
// supports. This field is optional; if not specified, xDS will use
// the same channel creds as the backends, but with any associated
// call creds stripped off.
// supports. This field is required and must contain at least one
// channel creds type that the client supports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make "insecure" default instead of making a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would still be a breaking change. If someone is relying on getting, e.g., TlsCreds from the parent channel, that would still break if we change the default to "insecure".

I suspect no one will notice this change either way at this point, which is why I'm willing to do it (although we could certainly change our mind if someone objects in this PR). But I don't see that making "insecure" the default is any better.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "inherit from parent channel" is anyway going to break since that is going away. By making "insecure" default, at least the insecure case is not broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only way to currently get insecure creds is to inherit it from the parent channel, and that's only one of many possible types of creds that could be inherited from the parent channel. I don't see why insecure creds is special here.

In general, I don't think insecure creds is a very good default. This could lead to people accidentally having an insecure connection when they didn't expect it. I don't want to do that unless the user explicitly asks us to. This is the same reason why (e.g.) our CreateChannel() API doesn't default to insecure; it creates an insecure connection only if the application explicitly uses InsecureCreds.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @markdroth. Especially if the default behavior is changing, changing it to something that gives an error is preferable to something that will only fail later with a more confusing failure mode -- or, worse, leads to a security problem due to "insecure".

"channel_creds": [
{
"type": <string containing channel cred type>,
Expand All @@ -140,10 +139,10 @@ like this:
}
```

Initially, the only type of channel creds we support will be
`google_default`. In the future, we will add a general-purpose
mechanism for configuring arbitrary channel creds types with arbitrary
configuration.
Initially, the only types of channel creds we support will be
`google_default` and `insecure`. In the future, we will add a
general-purpose mechanism for configuring arbitrary channel creds types
with arbitrary configuration.

The `node` field will be populated with the [xDS `Node`
proto](https://github.com/envoyproxy/data-plane-api/blob/1adb5d54abb0e28ca409254d26fad1cf5535239b/envoy/api/v2/core/base.proto#L85).
Expand Down