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

feat(internaloption): add AllowNonDefaultServiceAccount internaloption #1127

Merged
merged 3 commits into from Aug 23, 2021

Conversation

mohanli-ml
Copy link
Contributor

Add an internal option AllowNonDefaultServiceAccount for cloud services to bypass the check that credential must be retrieved from the metadata server and must use the default service account. In this way, non-default service account can be used for DirectPath.

@mohanli-ml mohanli-ml requested review from yoshi-approver and a team as code owners July 23, 2021 04:51
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jul 23, 2021
@codyoss
Copy link
Member

codyoss commented Jul 26, 2021

@mohanli-ml Is there an AIP or design doc for this? I lack the context for this change.

@broady
Copy link
Contributor

broady commented Aug 4, 2021

@mohanli-ml Following up on Cody's comment... Is this something we can enable across all APIs? Why do we need an option for it?

@mohanli-ml
Copy link
Contributor Author

Sorry for the late response. This option is proposed to solve the conflict that DirectPath only support the default service account, while currently in GCP non-default service account is largely used. We:

  1. collect some use cases in go/directpath-file-credentials;
  2. have the proposal in go/directpath-gcp-authentication;
  3. have the rollout plan in go/directpath-file-credentials-rollout.

This option should not be enabled before the service owners talk to the MDB group directpath-security-reviews and get their approval. See details in https://docs.google.com/document/d/1DgKCBcmz3J0OjbpO0BZBrwQIm3LrDBs8t9AS67NG0z4/edit#heading=h.qkiqy0p84mi8.

@@ -138,7 +138,7 @@ func dial(ctx context.Context, insecure bool, o *internal.DialSettings) (*grpc.C
// * The endpoint is a host:port (or dns:///host:port).
// * Credentials are obtained via GCE metadata server, using the default
// service account.
if o.EnableDirectPath && checkDirectPathEndPoint(endpoint) && isTokenSourceDirectPathCompatible(creds.TokenSource) && metadata.OnGCE() {
if o.EnableDirectPath && checkDirectPathEndPoint(endpoint) && isTokenSourceDirectPathCompatible(creds.TokenSource, o) && metadata.OnGCE() {
Copy link
Contributor

@broady broady Aug 18, 2021

Choose a reason for hiding this comment

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

&& (o.AllowNonDefaultServiceAccount || isTokenSourceDirectPathCompatible(creds.TokenSource))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry it seems this comment will change the functionality. For example if creds.TokenSource is nil, then isTokenSourceDirectPathCompatible(creds.TokenSource, o) will be false, but (o.AllowNonDefaultServiceAccount || isTokenSourceDirectPathCompatible(creds.TokenSource)) could be true if o.AllowNonDefaultServiceAccount is true.

@broady
Copy link
Contributor

broady commented Aug 18, 2021

LGTM, just a nit

@broady broady added the automerge Merge the pull request once unit tests and other checks pass. label Aug 23, 2021
@gcf-merge-on-green gcf-merge-on-green bot merged commit d713001 into googleapis:master Aug 23, 2021
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants