impl(auth): add GDCH credential builder

[alvarowolfx]

Towards #5584

[alvarowolfx]

better docs and examples are going to be added in coming PRs, when making mod gdch and gdch feature public.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.76%. Comparing base (6fd4739) to head (7ce84ac).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5596   +/-   ##
=======================================
  Coverage   97.76%   97.76%           
=======================================
  Files         220      220           
  Lines       50961    50961           
=======================================
+ Hits        49823    49824    +1     
+ Misses       1138     1137    -1     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gemini-code-assist]

Code Review

This pull request introduces a Builder for GdchServiceAccountCredentials in the auth crate, enabling configurable retry, backoff, and throttling policies. It also adds comprehensive tests for JSON validation, token caching, and retry logic. The reviewer recommended adding explicit validation for the credential type, removing an unused variable in tests, and correcting a typo in a comment.

I am having trouble creating individual review comments. Click here to see my feedback.

src/auth/src/credentials/gdch.rs (281-286)

While the GdchServiceAccountKey struct will fail to deserialize if mandatory fields are missing, it is good practice to explicitly validate that the type field in the provided JSON matches the expected gdch_service_account value. This ensures that a standard service account key is not accidentally passed to this builder, providing a clearer error message.

        if key.cred_type != "gdch_service_account" {
            return Err(crate::build_errors::Error::not_supported(format!(
                "unsupported credential type: {}. Expected 'gdch_service_account'",
                key.cred_type
            )));
        }

        if key.format_version != "1" {
            return Err(crate::build_errors::Error::not_supported(format!(
                "unsupported format_version: {}. Expected '1'",
                key.format_version
            )));
        }

References

1. Demand Explosive Correctness: Never swallow errors or ignore Result types. Fail loudly and explicitly when appropriate.

src/auth/src/credentials/gdch.rs (438-440)

The variable key is no longer used in this test after refactoring to use key_json with the new Builder. It should be removed to keep the test code clean.

src/auth/src/credentials/gdch.rs (491)

Typo in comment: "should on be called once" should be "should only be called once".

                .times(1) // should only be called once

References

1. Avoid excessive blank lines; use line breaks only to signal context shifts. (General quality and clarity in comments). ^(link)

[suzmue]

My comments here since commenting is down:

Question: Why do we not need to do verification on the cred_type? We are always looking for a specific value there, so I was curious about why it isn't checked like the format_version.

The test that has no format version says "missing format_version and other required fields". The code path that is tested for missing the format_version appears to be a special case. If the other fields are required, a separate test to verify those fail would be nice.

The descriptive test names are great for the retry policy!

minor naming nit: the retry test names read like they’re asserting GDCH’s default retry semantics, but the tests are actually verifying that retry settings provided through the builder are applied to the token-fetch path. Maybe consider renaming to something more explicit, like gdch_applies_retry_settings_for_transient_failures

[alvarowolfx]

thanks @suzmue

Why do we not need to do verification on the cred_type? We are always looking for a specific value there, so I was curious about why it isn't checked like the format_version

We don't check credential types in the credential itself, mainly because the happy path for customer is to use via ADC, and ADC routes to the right implementation via the credential type. And if they want to use an specific type, they can use it directly, in which the type itself is not that important, only the required fields. But GDCH is not being used on ADC flows, so maybe make sense to check the type. I added the check.

The test that has no format version says "missing format_version and other required fields". The code path that is tested for missing the format_version appears to be a special case. If the other fields are required, a separate test to verify those fail would be nice.

I split the test cases. Also at a given point we are just testing the behavior of serde_json::from_value::<GdchServiceAccountKey>, which fails if a non optional field is missing, so is hard to draw a line on how far should we go here.

Also changes the name of the tests around retry policy.

[coryan]

Can we parse this into a more meaningful type during initialization?

[coryan]

Oh, I see, we validate when we call build*()... hmm, weird, maybe, fine if we do the same everywhere else.

[alvarowolfx]

yeah, I just followed the pattern from other builders, where the Value is validated on build