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/bootstrap: add plugin system for credentials specified in bootstrap file #5136

Merged
merged 26 commits into from Feb 1, 2022

Conversation

anicr7
Copy link
Contributor

@anicr7 anicr7 commented Jan 15, 2022

Currently the credentials used for communication with the xDS management
server is restricted to "google_default" and "insecure". There is no way
for users to inject custom credentials, even though the bootstrap can be
used to pass in different values.

This PR adds support for a new credentials package which can be used to
register supported credentials along with a name that can be recognized
by the bootstrap.

The xDS credential package also registers the currently supported
"google_default" and "insecure" options to maintain the status quo. To keep the
interface of the xDS Credentials builder clean, the insecure credentials now
satisfies the gRPC credentials bundle interface.

RELEASE NOTES:

  • xds/bootstrap: add plugin system for credentials specified in bootstrap file

xds/credentials/credentials.go Outdated Show resolved Hide resolved
xds/credentials/credentials.go Outdated Show resolved Hide resolved
xds/credentials/credentials.go Outdated Show resolved Hide resolved
xds/credentials/credentials.go Outdated Show resolved Hide resolved
xds/credentials/credentials.go Outdated Show resolved Hide resolved
xds/credentials/credentials.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned menghanl, easwars and anicr7 and unassigned dfawley Jan 27, 2022
@dfawley
Copy link
Member

dfawley commented Jan 27, 2022

@anicr7 anicr7 removed their assignment Jan 31, 2022
credentials/google/google.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@dfawley dfawley assigned anicr7 and unassigned menghanl and easwars Jan 31, 2022
@anicr7 anicr7 removed their assignment Jan 31, 2022
credentials/google/google.go Outdated Show resolved Hide resolved
credentials/google/insecure/insecure.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap.go Show resolved Hide resolved
xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@anicr7 anicr7 removed their assignment Jan 31, 2022
credentials/insecure/insecure.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@dfawley dfawley changed the title Support xDS credentials plugin xds/bootstrap: add plugin system for credentials specified in bootstrap file Jan 31, 2022
@dfawley dfawley added this to the 1.45 Release milestone Jan 31, 2022
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
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.

Mostly nits.

credentials/insecure/insecure.go Show resolved Hide resolved
xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap.go Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/internal/xdsclient/bootstrap/bootstrap_test.go Outdated Show resolved Hide resolved
xds/bootstrap/bootstrap.go Outdated Show resolved Hide resolved
@easwars
Copy link
Contributor

easwars commented Feb 1, 2022

@anicr7 : Also, please don't mark conversations as resolved. We generally let the original commenter do that when they are satisfied. Thanks.

@easwars easwars assigned anicr7 and unassigned dfawley Feb 1, 2022
xds/credentials/*.go. Also add the relevant builders in the google creds
package.
1. New bootstrap package that users can use.
2. Move insecure creds builder to google/creds.
3. Move registration to google/ and google/insecure.
credential/insecure instead.

Don't use google/insecure in bootstrap.go as it leads to cyclic
dependencies.
…undle() to return new transport credentials again
}

// NewWithMode returns a new insecure Bundle. The mode is ignored.
func (insecureBundle) NewWithMode(_ string) (credentials.Bundle, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: since this method takes no other parameters, you could skip the _.

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

// JSON config.
type insecureCredsBuilder struct{}

func (i *insecureCredsBuilder) Build(_ json.RawMessage) (credentials.Bundle, error) {
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 and in other places. The _ can be skipped since it is the only parameter to the method.

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

Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

LGTM

@dfawley dfawley merged commit c7f7d3a into grpc:master Feb 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants