-
Notifications
You must be signed in to change notification settings - Fork 119
feat: add allowNonDefaultServiceAccount option for DirectPath #1433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with a minor comment
if (allowNonDefaultServiceAccount != null) { | ||
return allowNonDefaultServiceAccount; | ||
} | ||
return credentials instanceof ComputeEngineCredentials; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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:
-
isOnComputeEngine()
checks the environment of the VM as DirectPath is allowed on Compute Engine/GKE but not App Engine. -
isNonDefaultServiceAccountAllowed()
checks which service account the VM is using to access DirectPath.credential
is instance ofComputeEngineCredentials
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
8135915
to
41c56d8
Compare
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. |
🤖 I have created a release \*beep\* \*boop\* --- ## [2.1.0](https://www.github.com/googleapis/gax-java/compare/v2.0.0...v2.1.0) (2021-08-11) ### Features * add allowNonDefaultServiceAccount option for DirectPath ([#1433](https://www.github.com/googleapis/gax-java/issues/1433)) ([209b494](https://www.github.com/googleapis/gax-java/commit/209b4944feba1c62be2c9de4545e3b01a806b738)) ### Bug Fixes * fix httpjson executor ([#1448](https://www.github.com/googleapis/gax-java/issues/1448)) ([8f48b70](https://www.github.com/googleapis/gax-java/commit/8f48b7027b95e8e75872d1f9dac537ea697d0acc)) * make closeAsync don't interrupt running thread ([#1446](https://www.github.com/googleapis/gax-java/issues/1446)) ([7c6c298](https://www.github.com/googleapis/gax-java/commit/7c6c29824487346d444730388ea6967408692696)) ### Dependencies * update dependency com.google.api:api-common to v2.0.1 ([#1452](https://www.github.com/googleapis/gax-java/issues/1452)) ([a52f16f](https://www.github.com/googleapis/gax-java/commit/a52f16f6cef8340357acb374ff31c8a6f248403c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.