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

add access token authentication #70

Merged

Conversation

bradkwadsworth-mw
Copy link
Contributor

Signed-off-by: Brad Wadsworth brad.wadsworth@mavenwave.com

Description of your changes

Allow use of temporary service account access tokens to be used for authentication.

Fixes #65

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Updated Configuration.md with instructions on how to test with access tokens.

@Upbound-CLA
Copy link

Upbound-CLA commented Dec 1, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @bradkwadsworth-mw,
Thank you for working on introducing token-based authentication support to provider-gcp.

My understanding is that the PR proposes to switch to token-based auth. by inspecting the referenced credential data (trying to parse the supplied credentials as JSON) and if this succeeds, use token authentication in the underlying Terraform (native) provider by configuring the access_token argument in the provider block. And if it fails, it configures the credentials argument of the provider block for the native provider.

I would prefer a more explicit API where we have a separate credential source (enum) constant for the token-based authentication like we do for provider-aws here. Then we could configure the access_token argument for the native provider configuration block in this switch when the credential source is AccessToken.

This would expose an explicit API for token-based authentication with, in my opinion, better UX. Because we are surfacing the API to the credential source (enums) level, it will be easier for users to follow what options are supported and to enable them to specify their intentions (regarding the auth. mechanism to use) in a more explicit manner. For instance, if the user of the ProviderConfig API would like to use token-based auth. but has a syntax error in the JSON credentials she supplies, then we would implicitly be switching to service principal credentials-based authentication, although the intent was different. What do you think?

@bradkwadsworth-mw
Copy link
Contributor Author

Hi @bradkwadsworth-mw, Thank you for working on introducing token-based authentication support to provider-gcp.

My understanding is that the PR proposes to switch to token-based auth. by inspecting the referenced credential data (trying to parse the supplied credentials as JSON) and if this succeeds, use token authentication in the underlying Terraform (native) provider by configuring the access_token argument in the provider block. And if it fails, it configures the credentials argument of the provider block for the native provider.

I would prefer a more explicit API where we have a separate credential source (enum) constant for the token-based authentication like we do for provider-aws here. Then we could configure the access_token argument for the native provider configuration block in this switch when the credential source is AccessToken.

This would expose an explicit API for token-based authentication with, in my opinion, better UX. Because we are surfacing the API to the credential source (enums) level, it will be easier for users to follow what options are supported and to enable them to specify their intentions (regarding the auth. mechanism to use) in a more explicit manner. For instance, if the user of the ProviderConfig API would like to use token-based auth. but has a syntax error in the JSON credentials she supplies, then we would implicitly be switching to service principal credentials-based authentication, although the intent was different. What do you think?

That sounds good to me as well. I'll work on that.

@ulucinar
Copy link
Collaborator

ulucinar commented Jan 2, 2023

Triggered to check the Secret provider authentication scheme:
/test-examples="examples/compute/address.yaml"

Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Hi @bradkwadsworth-mw,
Thank you very much for working on the new authentication scheme!
Left some nitpicks but I think the most important review comment is regarding the import statement. Because the actual authentication is performed by the underlying Terraform provider, which is a separate binary, do we really need the import statement?

internal/clients/gcp.go Outdated Show resolved Hide resolved
internal/clients/gcp.go Outdated Show resolved Hide resolved
internal/clients/gcp.go Outdated Show resolved Hide resolved
cmd/provider/main.go Show resolved Hide resolved
@ulucinar
Copy link
Collaborator

Triggered to check the Secret provider authentication scheme:
/test-examples="examples/compute/address.yaml"

@ulucinar ulucinar merged commit 38003fe into crossplane-contrib:main Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All Access Tokens for Authentication
4 participants