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: add allowNonDefaultServiceAccount option for DirectPath #1433

Merged
merged 3 commits into from Aug 5, 2021

Conversation

mohanli-ml
Copy link
Contributor

@mohanli-ml mohanli-ml commented Jul 23, 2021

Add an option allowNonDefaultServiceAccount for cloud services to bypass the check that credential must be retrieved from the metadata server, which implicitly requires using the default service account. In this way, non-default service account can be used for DirectPath.

@mohanli-ml mohanli-ml requested review from as code owners Jul 23, 2021
@google-cla google-cla bot added the cla: yes label Jul 23, 2021
Copy link
Contributor

@vam-google vam-google left a comment

LGTM, with a minor comment

if (allowNonDefaultServiceAccount != null) {
return allowNonDefaultServiceAccount;
}
return credentials instanceof ComputeEngineCredentials;
Copy link
Contributor

@vam-google vam-google Aug 3, 2021

Choose a reason for hiding this comment

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

Do we need this? I see there is already the isOnComputeEngine() method, so ideally we want to keep compute-engine specific check in one place. Also, this method (isNonDefaultServiceAccountAllowed()) is used only in conjunction with isOnComputeEngine() on line 324 of this file).

Copy link
Contributor Author

@mohanli-ml mohanli-ml Aug 3, 2021

Choose a reason for hiding this comment

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

Sorry the name might be misleading:

  1. isOnComputeEngine() checks the environment of the VM as DirectPath is allowed on Compute Engine/GKE but not App Engine.

  2. isNonDefaultServiceAccountAllowed() checks which service account the VM is using to access DirectPath. credential is instance of ComputeEngineCredentials guarantees that the VM is using the default service account (because the credential is retrieved from the metadata server), but it can not guarantee the environment (i.e., ComputeEngineCredentials can also be used on App Engine).

Based on the above understanding I think the two checks are different and should be kept.

Copy link
Contributor

@vam-google vam-google Aug 3, 2021

Choose a reason for hiding this comment

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

ok

@mohanli-ml
Copy link
Contributor Author

@mohanli-ml mohanli-ml commented Aug 5, 2021

I updated the code. If the option is not set, or set to false, the code behavior should be the same as currently it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants