Skip to content

Conversation

@davidcollom
Copy link
Member

@davidcollom davidcollom commented Jun 30, 2025

This implements the go-containerregistry AuthN Libraries to support fetching and updating credentials from ServiceAccount ImagePullSecrets along with additional kubernetes.io/dockerconfigjson secrets.

There are significant breaking changes here:

  • Removal of the Fallback client, as the true fallback should be OCI going forward
  • Removal of Self-Hosted, this is an ongoing effort to bring things under the OCI Client
    • This is in turn from the lack of support from go-containerregistries, not supporting the original docker/distribution API.
  • Replacing IsHost with Factories for each client, this ensures a clear break from what a "Client" implements vs how we detect "What" Client we need to fetch Tag information.

A few things outstanding:

  • Customization for each hostname, I.E: CA Certs or Timeouts etc.
  • Full testing of the following clients:
    • GCP / GAR
    • ACR
    • ECR
  • Handling for the HelmValues and safe migration of existing credentials that are no longer directly passed in the helmchart

And some more, that I've likely forgotten 🙈

Resolves #322

@davidcollom davidcollom requested a review from a team July 17, 2025 13:45
@davidcollom davidcollom added the enhancement New feature or request label Jul 17, 2025
@davidcollom davidcollom changed the base branch from main to release-v1.0 November 27, 2025 11:05
@davidcollom davidcollom requested a review from Copilot December 9, 2025 09:55
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a major refactoring to support Kubernetes-native authentication via ServiceAccount ImagePullSecrets and go-containerregistry's AuthN libraries. The changes replace the previous host-based client selection with a factory pattern and introduce keychain-based credential management.

Key changes:

  • Introduces a new keychains package for managing authentication via Pod/ServiceAccount credentials
  • Refactors client architecture from host-based to factory-based pattern
  • Removes self-hosted and fallback clients in favor of OCI as the default
  • Updates Helm charts to support kubernetes.io/dockerconfigjson secret format

Reviewed changes

Copilot reviewed 80 out of 81 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
pkg/keychains/* New package implementing Manager interface for Pod, ServiceAccount, and Manual credential modes
pkg/client/clientmanager.go New client manager with factory-based client instantiation and keychain resolution
pkg/client/*/factory.go Factory implementations for each registry client type
pkg/api/types.go Updated ImageClient interface, added ImageClientFactory interface
pkg/controller/pod_controller.go Updated to use ClientManager instead of Client
cmd/app/options.go Added keychain flags and deprecated old auth flags
deploy/charts/version-checker/* Updated Helm templates for new secret format and imagePullSecrets support
Dockerfile Minor build optimization

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

-}}
{{- $_ := set $auths $registry $entry }}
{{- else }}
{{- fail (printf "dockerconfigjson entry missing required fields: %#v" .) }}
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The fail call here includes the entire pullSecrets entry in the error message using %#v, which will print sensitive fields like password or token to Helm output and any associated CI/CD logs. An attacker or unintended party with access to deployment logs could recover registry credentials if a misconfigured entry triggers this branch. To fix this, avoid logging secret values (only mention which field is missing or the entry index) and remove or redact sensitive fields from the formatted error message.

Suggested change
{{- fail (printf "dockerconfigjson entry missing required fields: %#v" .) }}
{{- $missing := list }}
{{- if not $registry }}{{- $missing = append $missing "registry" }}{{- end }}
{{- if not $username }}{{- $missing = append $missing "username" }}{{- end }}
{{- if not $secret }}{{- $missing = append $missing (ternary "token or password" "password" (ne $token "")) }}{{- end }}
{{- fail (printf "dockerconfigjson entry for registry '%s' is missing required field(s): %s" (default "<unknown>" $registry) (join $missing ", ")) }}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Auto discovery of credentials using image pull secrets for private repos

2 participants