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

auth/credentials: add sentinel error for DetectDefault when no credentials are detected #11258

Open
karlkfi opened this issue Dec 10, 2024 · 0 comments
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@karlkfi
Copy link

karlkfi commented Dec 10, 2024

Motivation

In Config Sync, we have some code in one of our controllers that does different things depending on whether or not the K8s Deployment has configured authentication or not. For example, on AWS we still export Prometheus metrics, but disable Cloud Monitoring metrics.

To do this, we call credentials.DetectDefault and check for an error message that starts with credentials: could not find default credentials. This implies that credentials were not configured at all, so it can still surface other errors as configuration errors. https://github.com/googleapis/google-cloud-go/blob/auth/v0.12.1/auth/credentials/detect.go#L108

This case would be easier to detect and less fragile with a sentinel error.

Proposed solution

The credentials.DetectDefault function could return a new sentinel error, named something like CredentialsNotFoundError when all detection methods are skipped. This would allow Config Sync to check for the sentinel error by type with errors.Is, instead of by error string prefix.

Alternatives considered

To ensure that the code doesn't start failing in the future due to an error message text change, we tried to add tests to verify the credentials.DetectDefault error message, but found that while that's possible locally, there's no way to make credentials.DetectDefault produce the credentials: could not find default credentials. error when running in Prow on GKE/GCE, because metadata.OnGCE() will return true, and the allowOnGCECheck test variable is package local. This alternate feature has been requested before: #4920.

If we could force-disable the OnGCE check, by setting an env var, like GCE_METADATA_HOST="", that would also work for us, and help other use cases, like integration testing, but adding a sentinel error sounds easier and less risky.

Another potentially less risky option would be to add an option to DetectOptions to disable the OnGCE check. That way it wouldn't need to use an env var. Then we could set that in our tests and continue using the error string match.

@karlkfi karlkfi added the triage me I really want to be triaged. label Dec 10, 2024
@codyoss codyoss added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Dec 10, 2024
karlkfi added a commit to karlkfi/kpt-config-sync that referenced this issue Dec 10, 2024
- Migrate from the legacy golang.org/x/oauth2 to the new
  cloud.google.com/go/auth for GCP & federated auth.
- Remove the 5m auth token cache in the askpass sidecar.
  Use the 3m45s EarlyTokenRefresh default provided by
  auth.DefaultCredentials instead.
- Make asskpass, helm-sync, oci-sync, and reconciler-manager all use
  the same credential and token provider code for consistency.
- Add unit tests to confirm that IsCredentialsNotFoundError works.
  It's fragile because it depends on an error string instead of a
  sentinel error.
  googleapis/google-cloud-go#11258
  Unfortunately, the not found error is not reproducable or testable
  on GKE/GCE, so if the error text changes, the test may still pass
  in CI but fail when run locally.
  googleapis/google-cloud-go#4920
google-oss-prow bot pushed a commit to GoogleContainerTools/kpt-config-sync that referenced this issue Dec 10, 2024
- Migrate from the legacy golang.org/x/oauth2 to the new
  cloud.google.com/go/auth for GCP & federated auth.
- Remove the 5m auth token cache in the askpass sidecar.
  Use the 3m45s EarlyTokenRefresh default provided by
  auth.DefaultCredentials instead.
- Make asskpass, helm-sync, oci-sync, and reconciler-manager all use
  the same credential and token provider code for consistency.
- Add unit tests to confirm that IsCredentialsNotFoundError works.
  It's fragile because it depends on an error string instead of a
  sentinel error.
  googleapis/google-cloud-go#11258
  Unfortunately, the not found error is not reproducable or testable
  on GKE/GCE, so if the error text changes, the test may still pass
  in CI but fail when run locally.
  googleapis/google-cloud-go#4920
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants