Add support for autodiscovering default credentials

[markallanson]

This change reversed one made in PR #228 that broke automatic credential discovery and instead replaced it with a "No Credentials" implementation.

Autodiscovery is a better default, and matches the behaviour documented by the properties gcs.credentials.path and gcs.credentials.json which state

If not provided, the connector will try to detect the credentials
automatically

Closes #291

[markallanson]

Fixed spotless linting errors.

[markallanson]

Are there any tips for debugging these test failures? All the tests pass locally, so need to try and figure out the problem in the build system.

[markallanson]

Pushed a rebased copy of the changes on top of latest main.

[ivanyu]

@markallanson thank you for this PR. It would be great to have the forth mode of "no credentials", which is useful for e.g. working through some proxies, fake-gcs-server, etc. Would you consider adding it?
We have something similar in another repo, which could be used as a reference: link 1, link 2, link 3

[markallanson]

@markallanson thank you for this PR. It would be great to have the forth mode of "no credentials", which is useful for e.g. working through some proxies, fake-gcs-server, etc. Would you consider adding it? We have something similar in another repo, which could be used as a reference: link 1, link 2, link 3

I am open to this. Will take a look at the links and implement something the same.

[markallanson]

This PR has been updated to introduce a new property, gcs.credentials.default which matches that
supported by the TieredStorageManager for configuring autodiscovery of default credentials as suggested by @ivanyu.

When this property is set to false, or omitted entirely the NoCredentials option will be
used, but when it is defined and set to true it will autodiscover credentials.

[markallanson]

I have revised the code to avoid the PMD warning regarding inline constants.

[markallanson]

I'll tackle the remaining PMD violations on Monday - they appear to be mostly in code I didn't change as part of this PR, but am happy to fix them.

[jeqo]

Thanks @markallanson! Took a first look at the PR. Overall I agree with the changes and new configs.

Few chores to fix issues on CI:

• Run ./gradlew :spotlessApply and reformat as needed
• Run ./gradlew :pmdTest before pushing to validate changes.
• Avoid using import static of constants, instead use GcsSinkConfig.<CONSTANT> on methods to reduce number of imports.
• Consider creating a new test class for credentials e.g. GcsSinkConfigCredentialsTest -- and move the ~6 tests related to credentials there.

[markallanson]

@jeqo Addressed all your comments I believe. Thanks for the feedback and helping us get this through, it's very appreciated!

[markallanson]

@jeqo It looks like the default credentials test is failing. Without being able to see any detailed information about the failure I have to assume this is because there's no google credentials in the gitlab runner's environment so the google cloud sdk is throwing a missing credentials error (throw new IOException(CLOUDSDK_MISSING_CREDENTIALS);).

In order to avoid going around in circles, can you please let me know which of the two options you'd prefer I use:

1. Switch back to using a mock, or
2. Try and configure some google cloud sdk credentials in the gitlab workflow runner

[jeqo]

@markallanson you're right, it's failing because no default credentials were found.
I was thinking on creating default credentials if they don't exist or passing it via env variable on GH actions, but it could get messy.
Let's switch back to the mock approach then, I'm afraid. And let's add some comment on why the mock is required on the test case.

[markallanson]

@markallanson you're right, it's failing because no default credentials were found. I was thinking on creating default credentials if they don't exist or passing it via env variable on GH actions, but it could get messy. Let's switch back to the mock approach then, I'm afraid. And let's add some comment on why the mock is required on the test case.

Ok done - converted back to using mocks.

[jeqo]

LGMT, thanks @markallanson !