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

man8pr commented 7 months ago

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 commented 7 months ago

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 :warning:
...a/org/eclipse/edc/gcp/common/GcpConfiguration.java	0.00%	3 Missing :warning:
...connector/provision/gcp/GcsResourceDefinition.java	60.00%	2 Missing :warning:
...connector/provision/gcp/GcsProvisionExtension.java	0.00%	1 Missing :warning:

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

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

man8pr commented 7 months ago

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.

man8pr commented 7 months ago

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

eclipse-edc / Technology-Gcp