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

cli: Allow Application Default Credentials discovery for GCP #1207

Merged
merged 2 commits into from
Nov 26, 2024

Conversation

xytis
Copy link
Contributor

@xytis xytis commented Oct 13, 2024

When deploying tusd under GCP, it is better to use integrated Application Default Credentials discovery mechanism, that exists under most google SDK's.

This change simply allows not providing any string as credential file, and then client library defaults to trying ADC as per https://cloud.google.com/docs/authentication/application-default-credentials

If this proposal is acceptable, I could update the readme as well.

@Acconut
Copy link
Member

Acconut commented Oct 22, 2024

The idea looks good to me, thank you for bringing this up.

What error does the SDK and thus tusd now return if no GCS_SERVICE_ACCOUNT_FILE was given and ADC failed as well? We must ensure that this still gives a descriptive error message.

I could update the readme as well.

That would be great!

@Acconut Acconut changed the title fix: allow default credentials to work for GCP cli: Allow Application Default Credentials discovery for GCP Oct 22, 2024
@xytis
Copy link
Contributor Author

xytis commented Oct 22, 2024

Default error is okey-ish:

2024/10/22 07:10:01.750054 Attempting to use default GCS credentials because no GCS_SERVICE_ACCOUNT_FILE env var was provided
2024/10/22 07:10:01.802682 Unable to create Google Cloud Storage service: dialing: google: could not find default credentials. See https://cloud.google.com/docs/authentication/external/set-up-adc for more information

@Acconut
Copy link
Member

Acconut commented Oct 22, 2024

Yes, that seems acceptable, especially if our documentation covers authentication.

@xytis xytis force-pushed the main branch 2 times, most recently from c70aba1 to 0135b7a Compare October 22, 2024 08:21
@xytis
Copy link
Contributor Author

xytis commented Oct 22, 2024

Ok, updated the docs, plus I removed the warning message (since it now is explained in the docs).

cmd/tusd/cli/composer.go Outdated Show resolved Hide resolved
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@Acconut Acconut merged commit b83b859 into tus:main Nov 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants