Skip to content

Replace CloudScoped with ValidatingStrategy for credential chain filtering#1516

Open
hectorcast-db wants to merge 3 commits intomainfrom
restore-credentials-chain-interface
Open

Replace CloudScoped with ValidatingStrategy for credential chain filtering#1516
hectorcast-db wants to merge 3 commits intomainfrom
restore-credentials-chain-interface

Conversation

@hectorcast-db
Copy link
Contributor

@hectorcast-db hectorcast-db commented Mar 5, 2026

Changes

Replaces the CloudScoped interface (introduced in #1505 follow-up) with a more expressive ValidatingStrategy interface, and restores NewCredentialsChain to return CredentialsStrategy.

New interface

// ValidatingStrategy is an optional interface that a CredentialsStrategy can
// implement to declare upfront whether it is applicable for the current
// configuration.
type ValidatingStrategy interface {
    Validate(context.Context, *Config) error
}

// ErrInvalidCloud is returned by ValidatingStrategy.Validate when the
// configured host's cloud does not match the cloud required by the strategy.
var ErrInvalidCloud = errors.New("cloud not supported by this strategy")

Chain behavior

  • Auto-detect mode: if Validate returns any error, the strategy is skipped (logged at debug level). This covers both cloud mismatches and missing required fields.
  • Explicit auth_type: ErrInvalidCloud is ignored so users can force a cloud-specific strategy on any host (e.g. azure-cli on a GCP host). Any other validation error is propagated so misconfigured strategies fail fast with a clear message.

Per-strategy Validate methods

Each Azure and GCP strategy (AzureCliCredentials, AzureMsiCredentials, AzureClientSecretCredentials, AzureGithubOIDCCredentials, GoogleCredentials, GoogleDefaultCredentials) now implements Validate with:

  1. Field checks first — explicit messages about which required config fields are missing (e.g. "azure_client_id is required", "google_service_account is not set")
  2. Cloud check last — returns ErrInvalidCloud if the host's cloud doesn't match; placed last because it is the softer condition (overridable via explicit auth_type)

The original nil, nil guards in Configure are preserved as a safety net.

Tests

  • Updated TestCredentialsChain_CloudFiltering_* to use a validatingStrategy test helper implementing the new interface
  • All config package tests pass

Copy link
Contributor

@renaudhartert-db renaudhartert-db left a comment

Choose a reason for hiding this comment

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

It is weird to me that the requirements are not owned by the CredentialsStrategy as one will have to understand the internals of the strategy to be able to decide what requirements to apply. Also, every client (e.g. CLI) who build on top of this will have to re-implement the requirement mapping.

Could we think of an alternative design?

@hectorcast-db hectorcast-db force-pushed the restore-credentials-chain-interface branch from 15f8bb0 to d3d7ee5 Compare March 5, 2026 10:08
@hectorcast-db hectorcast-db changed the title Restore NewCredentialsChain interface and add NewCredentialsChainWithCloudRequirements Restore NewCredentialsChain interface and add CloudScoped Mar 5, 2026
@hectorcast-db hectorcast-db force-pushed the restore-credentials-chain-interface branch from d3d7ee5 to 0db3876 Compare March 5, 2026 10:11
…CloudRequirements

PR #1505 changed NewCredentialsChain to return *credentialsChain (unexported)
to enable WithCloudRequirements chaining. This broke the public API.

Restore the return type to CredentialsStrategy and introduce
NewCredentialsChainWithCloudRequirements for callers that need cloud-based
filtering. DefaultCredentials now uses the new constructor directly.
The WithCloudRequirements method is removed as it is no longer needed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Hector Castejon Diaz <hector.castejon@databricks.com>
@hectorcast-db hectorcast-db force-pushed the restore-credentials-chain-interface branch from 0db3876 to 231965e Compare March 5, 2026 10:25
if requiredCloud, ok := c.cloudRequirements[s.Name()]; ok {
if cfg.Environment().Cloud != requiredCloud {
logger.Debugf(ctx, "Skipping %q: not configured for %s", s.Name(), requiredCloud)
if cs, ok := s.(CloudScoped); ok {
Copy link
Contributor

@renaudhartert-db renaudhartert-db Mar 6, 2026

Choose a reason for hiding this comment

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

Do you foresee any reason to generalize this beyond cloud checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only if we decide to do a full rewrite and align the 3 SDKs. Then we may want to completely change the logic not use nil, nil / nil, error when it does not apply and use explicit functions like this one.

But that would be a big behavioral breaking change, so I don't think redoing this interface would be much of a big deal.

@github-actions
Copy link

github-actions bot commented Mar 6, 2026

If integration tests don't run automatically, an authorized user can run them manually by following the instructions below:

Trigger:
go/deco-tests-run/sdk-go

Inputs:

  • PR number: 1516
  • Commit SHA: 6101e70c464e0ea2932d741fc86ae17d049e3434

Checks will be approved automatically on success.

@hectorcast-db hectorcast-db changed the title Restore NewCredentialsChain interface and add CloudScoped Replace CloudScoped with ValidatingStrategy for credential chain filtering Mar 6, 2026
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