feat: GCS provisioner using ADC or existing service account for access tokens #111

man8pr · 2024-01-22T21:59:37Z

What this PR changes/adds

IAM interface allows to use ADC or existing service account to generate access tokens, instead of creating and deleting new service accounts for this purpose.
GCS provisioner adapted to make use of the new functions provided by IAM.

Why it does that

The change avoids delays in the propagation of permissions to newly created service accounts.

Note

When using Application Default Credentials (i.e. when a service account is not specified), avoid user credentials and select the appropriate service account, http://cloud.google.com/docs/authentication/external/set-up-adc-on-cloud

Linked Issue(s)

Closes #110 #118

codecov-commenter · 2024-01-22T22:02:11Z

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (b31f543) 63.52% compared to head (7a4f527) 60.06%.

Files	Patch %	Lines
...n/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java	45.16%	13 Missing and 4 partials ⚠️
...a/org/eclipse/edc/gcp/common/GcpConfiguration.java	0.00%	3 Missing ⚠️
...connector/provision/gcp/GcsResourceDefinition.java	60.00%	2 Missing ⚠️
...connector/provision/gcp/GcsProvisionExtension.java	0.00%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
- Coverage   63.52%   60.06%   -3.46%     
==========================================
  Files          24       24              
  Lines         595      576      -19     
  Branches       30       30              
==========================================
- Hits          378      346      -32     
- Misses        205      217      +12     
- Partials       12       13       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java

ndr-brt

I think the scope of this PR is pretty vague, will we need both getServiceAccount and getOrCreateServiceAccount?
I agree with @paullatzelsperger that ApplicationDefaultCredentials scope is also misleading... you say that you introduced for testing reasons, but in fact it is part of the public API of the IamServiceImpl.

my suggestion would be to rephrase this PR as it is seen from the perspective of the caller (the provisioner, according to the description), that will help defining clearer interfaces and setting up the correct testing boundaries

man8pr · 2024-01-23T13:33:34Z

I think the scope of this PR is pretty vague, will we need both getServiceAccount and getOrCreateServiceAccount? I agree with @paullatzelsperger that ApplicationDefaultCredentials scope is also misleading... you say that you introduced for testing reasons, but in fact it is part of the public API of the IamServiceImpl.

my suggestion would be to rephrase this PR as it is seen from the perspective of the caller (the provisioner, according to the description), that will help defining clearer interfaces and setting up the correct testing boundaries

Agreed and I will create an interface for ApplicationDefaultCredentials, and then an inner class for implementing it.
Regarding the scope, I am currently keeping both the getServiceAccount and getOrCreateServiceAccount, but plan to send another PR immediately after this one to remove the getOrCreateServiceAccount, because the provisioner will only use the former.

- IamServiceImpl.Builder now single-field - Added AccessTokenProvider interface for adding app default credential provider and related test

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java

extensions/common/gcp/gcp-core/src/test/java/org/eclipse/edc/gcp/iam/IamServiceImplTest.java

ndr-brt

as I said I see no end-to-end endeavor, is not clear who will use these new methods and how, they are dead code currently.

please start from the provisioner point of view, and, if that will become too broad to be coded in a single PR, please provide an explanation on how this will result at the end.

man8pr · 2024-01-25T14:25:27Z

as I said I see no end-to-end endeavor, is not clear who will use these new methods and how, they are dead code currently.

please start from the provisioner point of view, and, if that will become too broad to be coded in a single PR, please provide an explanation on how this will result at the end.

SG I will update the PR and add the Provisioner perspective with an additional commit

GCS provisioner uses either application default credentials or existing service account to generate the access token for the sink

- GCS provisioner finds service account name for access token from (first valid in sequence): - transfer request configuration - connector configuration from GcpConfiguration - application default credentials - refactor serviceAccount to serviceAccountName for GcsResourceDefinition

google-api-services-iam v2-rev20240108-2.0.0 license is apache 2.0 https://mvnrepository.com/artifact/com.google.apis/google-api-services-iam/v2-rev20240108-2.0.0

ndr-brt

just some nits, all good for the rest

...sion/provision-gcs/src/main/java/org/eclipse/edc/connector/provision/gcp/GcsProvisioner.java

ndr-brt · 2024-01-29T08:17:16Z

...sion/provision-gcs/src/main/java/org/eclipse/edc/connector/provision/gcp/GcsProvisioner.java

@@ -89,28 +103,23 @@ public CompletableFuture<StatusResult<ProvisionResponse>> provision(
        }
    }

+    private String getServiceAccountName(GcsResourceDefinition resourceDefinition) {
+        if (resourceDefinition.getServiceAccountName() != null) {
+            // TODO verify service account name from resource definition before returning.


is this TODO supposed to be solved in this PR or is there an issue related to it?

This TODO requires more internal sync, there is currently no issue related but I would target this in another PR

ndr-brt · 2024-01-29T08:24:18Z

...sion/provision-gcs/src/main/java/org/eclipse/edc/connector/provision/gcp/GcsProvisioner.java

+            GcpAccessToken token = null;
+
+            var serviceAccountName = getServiceAccountName(resourceDefinition);
+            if (serviceAccountName != null && !serviceAccountName.isEmpty()) {


I think serviceAccountName can never be null (please look at GcpConfiguration).

unrelated: I think that's not a good approach to use empty string as default value, it would be better to leave null for not-configured settings, so a simple not-null check will already permit to say if that configuration exists or not.

Updated GcpConfiguration, not returning null now

ndr-brt · 2024-01-29T08:24:52Z

...sion/provision-gcs/src/main/java/org/eclipse/edc/connector/provision/gcp/GcsProvisioner.java

-        var uniqueServiceAccountDescription = generateUniqueServiceAccountDescription(processId, buckedName);
-        return iamService.getOrCreateServiceAccount(serviceAccountName, uniqueServiceAccountDescription);
-    }
-
    @NotNull
    private String sanitizeServiceAccountName(String processId) {


unused (same for other 2 methods in this class)

…moved / inlined functions

ndr-brt

LGTM, please open an issue related to that TODO so it won't be forgotten

feat: IAM service account impersonation and ADC access token

7bb71d6

man8pr added the feature_request New feature request, awaiting triage label Jan 22, 2024

man8pr added the enhancement New feature or request label Jan 22, 2024

man8pr requested review from paullatzelsperger and ndr-brt January 23, 2024 06:38

refactor: use type inference

8954ab7

paullatzelsperger reviewed Jan 23, 2024

View reviewed changes

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java Outdated Show resolved Hide resolved

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java Outdated Show resolved Hide resolved

refactor: variable name

8231c79

ndr-brt requested changes Jan 23, 2024

View reviewed changes

refactor: single-field builder for IAM Service

01b978d

- IamServiceImpl.Builder now single-field - Added AccessTokenProvider interface for adding app default credential provider and related test

github-advanced-security bot found potential problems Jan 24, 2024

View reviewed changes

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java Fixed Show fixed Hide fixed

chore: update dependencies

b4e07f5

man8pr requested review from paullatzelsperger and ndr-brt January 24, 2024 14:29

refactor: added @OverRide annotation

294fd23

paullatzelsperger reviewed Jan 25, 2024

View reviewed changes

extensions/common/gcp/gcp-core/src/main/java/org/eclipse/edc/gcp/iam/IamServiceImpl.java Outdated Show resolved Hide resolved

paullatzelsperger reviewed Jan 25, 2024

View reviewed changes

extensions/common/gcp/gcp-core/src/test/java/org/eclipse/edc/gcp/iam/IamServiceImplTest.java Outdated Show resolved Hide resolved

extensions/common/gcp/gcp-core/src/test/java/org/eclipse/edc/gcp/iam/IamServiceImplTest.java Outdated Show resolved Hide resolved

man8pr added 2 commits January 25, 2024 14:04

refactor: IAM service unit test

8579b32

refactor: IAM service impl. ADC token provider with record

33a1ba5

man8pr requested a review from paullatzelsperger January 25, 2024 13:22

ndr-brt reviewed Jan 25, 2024

View reviewed changes

feat: GCS provisioner using existing credentials

127d84a

GCS provisioner uses either application default credentials or existing service account to generate the access token for the sink

man8pr changed the title ~~feat: IAM service account impersonation and ADC access token~~ feat: GCS provisioner using ADC or existing service account for access tokens Jan 25, 2024

man8pr requested a review from ndr-brt January 25, 2024 16:13

paullatzelsperger approved these changes Jan 26, 2024

View reviewed changes

man8pr added 2 commits January 29, 2024 08:34

chore: updated dependencies

a338eec

google-api-services-iam v2-rev20240108-2.0.0 license is apache 2.0 https://mvnrepository.com/artifact/com.google.apis/google-api-services-iam/v2-rev20240108-2.0.0

man8pr requested a review from paullatzelsperger January 29, 2024 08:17

ndr-brt reviewed Jan 29, 2024

View reviewed changes

refactor: GCP config returns null if no parameter, GCS provisioner re…

7a4f527

…moved / inlined functions

man8pr requested a review from ndr-brt January 29, 2024 09:26

ndr-brt approved these changes Jan 29, 2024

View reviewed changes

ndr-brt merged commit f469278 into eclipse-edc:main Jan 29, 2024
14 checks passed

man8pr mentioned this pull request Jan 29, 2024

GCS provisioner uses ADC or existing service account to generate access tokens #118

Closed

man8pr deleted the feature/110_iam_existing_sa_adc_access_token branch March 8, 2024 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: GCS provisioner using ADC or existing service account for access tokens #111

feat: GCS provisioner using ADC or existing service account for access tokens #111

man8pr commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

ndr-brt left a comment •

edited

Loading

man8pr commented Jan 23, 2024

ndr-brt left a comment

man8pr commented Jan 25, 2024

ndr-brt left a comment

ndr-brt Jan 29, 2024

man8pr Jan 29, 2024

ndr-brt Jan 29, 2024

man8pr Jan 29, 2024

ndr-brt Jan 29, 2024

man8pr Jan 29, 2024

ndr-brt left a comment

feat: GCS provisioner using ADC or existing service account for access tokens #111

feat: GCS provisioner using ADC or existing service account for access tokens #111

Conversation

man8pr commented Jan 22, 2024 • edited Loading

What this PR changes/adds

Why it does that

Note

Linked Issue(s)

codecov-commenter commented Jan 22, 2024 • edited Loading

Codecov Report

ndr-brt left a comment • edited Loading

Choose a reason for hiding this comment

man8pr commented Jan 23, 2024

ndr-brt left a comment

Choose a reason for hiding this comment

man8pr commented Jan 25, 2024

ndr-brt left a comment

Choose a reason for hiding this comment

ndr-brt Jan 29, 2024

Choose a reason for hiding this comment

man8pr Jan 29, 2024

Choose a reason for hiding this comment

ndr-brt Jan 29, 2024

Choose a reason for hiding this comment

man8pr Jan 29, 2024

Choose a reason for hiding this comment

ndr-brt Jan 29, 2024

Choose a reason for hiding this comment

man8pr Jan 29, 2024

Choose a reason for hiding this comment

ndr-brt left a comment

Choose a reason for hiding this comment

man8pr commented Jan 22, 2024 •

edited

Loading

codecov-commenter commented Jan 22, 2024 •

edited

Loading

ndr-brt left a comment •

edited

Loading