Skip to content

Conversation

@nazq
Copy link

@nazq nazq commented Nov 5, 2025

Summary

Fixes an issue where GoogleCloudStorageBuilder would fail to build when Application Default Credentials (ADC) exist in an unsupported format, even when explicit credentials are provided via with_service_account_path(), with_service_account_key(), or with_credentials().

Problem

Previously, GoogleCloudStorageBuilder::build() unconditionally attempted to read ADC files and would fail immediately if the ADC format was unsupported (e.g., external_account_authorized_user from Workload Identity Federation with external identity providers). This prevented users from using explicit credentials in environments where ADC was configured with newer credential types.

Solution

This PR makes ADC reading conditional:

  • When explicit credentials are provided: ADC reading errors are ignored, allowing the builder to use the explicit credentials
  • When no explicit credentials exist: ADC errors are propagated normally, preserving error visibility for users relying on ADC

The credential precedence remains unchanged: explicit credentials > ADC > instance metadata

Changes

  • Modified src/gcp/builder.rs:495-503 to conditionally handle ADC reading errors based on whether explicit credentials were provided
  • Added 4 comprehensive tests covering all credential paths

Testing

All tests pass (113 passed, 0 failed):

  • ✅ Explicit service account path ignores invalid ADC
  • ✅ Explicit service account key ignores invalid ADC
  • ✅ Custom credentials provider ignores invalid ADC
  • ✅ ADC errors still propagate when no explicit credentials provided

Impact

This change enables users in enterprise environments with Workload Identity Federation to use explicit credentials without being blocked by unsupported ADC formats, while maintaining backward compatibility and error visibility for ADC-only users.

Previously, GoogleCloudStorageBuilder unconditionally attempted to read
Application Default Credentials (ADC), causing build failures when ADC
files existed in unsupported formats (e.g., external_account_authorized_user),
even when explicit credentials were provided via with_service_account_path(),
with_service_account_key(), or with_credentials().

This change makes ADC reading conditional:
- When explicit credentials are provided, ADC reading errors are ignored
- When no explicit credentials exist, ADC errors are propagated normally

This preserves error visibility for users relying on ADC while allowing
users with explicit credentials to work regardless of ADC state.

The credential precedence remains: explicit credentials > ADC > instance metadata

Tests added:
- Verify explicit service account path ignores invalid ADC
- Verify explicit service account key ignores invalid ADC
- Verify custom credentials provider ignores invalid ADC
- Verify ADC errors still propagate when no explicit credentials provided
} else {
// We have explicit credentials, so ignore ADC errors
ApplicationDefaultCredentials::read(self.application_credentials_path.as_deref())
.unwrap_or(None)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have credentials, then why do we attempt to call ApplicationDefaultCredentials::read at all? Can we not just skip it in this case?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! You're absolutely right - there's no need to attempt reading ADC at all when explicit credentials are already provided.

I've updated the code to skip the ADC read entirely in that case, changing:

} else {
    ApplicationDefaultCredentials::read(self.application_credentials_path.as_deref())
        .unwrap_or(None)
}

to:

} else {
    // Explicit credentials provided, skip ADC reading entirely
    None
}

This is cleaner, more efficient (no unnecessary file I/O)

Per reviewer feedback, avoid unnecessary file I/O by returning None
directly instead of attempting to read ADC and discarding errors.
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