Support for managed pulling from private ECRs

[Caomoji]

This PR adds the possibility to set additional references to ECRs as sources in the configuration file.

This change allows to take advantage of the logic already in place to rotate ECR tokens and authenticate to private registries, but for pulling images from private ECRs in a context of multi ECR replication. The same AWS credentials as for the target ECR are used.

Main changes:

• Updated the configuration to add the field privateRegistries in source
• Added a new attribute to the imagePullSecretProviders containing the registries' clients
• Added a function to produce a dockerconfig in a JSON format from a registry client to merge with the authfile passed to Skopeo
• Updated GetImagePullSecrets to include dockerconfigs from private registries to the image pull secrets from pods

Notes:

• imagePullSecrets from hooked Pods still have priority over the authentication via these default private registries
• Changes do not impact the Helm chart and are compatible with previous version
• Source private registries cannot authenticate with different credentials from the ones used by the target registry passed as environment variables

[Caomoji]

Hello @estahn,
Sorry for bothering, but we would like to ask if you are possibly interested in this PR, and if we could expect to have it eventually merged or if it's not a move you would like for this project.
I'm open to any change suggestions.
Thank you in advance 🙏

[estahn]

Hello @estahn,

Sorry for bothering, but we would like to ask if you are possibly interested in this PR, and if we could expect to have it eventually merged or if it's not a move you would like for this project.

I'm open to any change suggestions.

Thank you in advance 🙏

PRs are much appreciated. Let me take a look at it today and come back to you shortly.

[codecov]

Codecov Report

Patch coverage: 16.20% and project coverage change: -32.66 ⚠️

Comparison is base (934d2d9) 64.48% compared to head (6767619) 31.82%.

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #394       +/-   ##
===========================================
- Coverage   64.48%   31.82%   -32.66%     
===========================================
  Files           6        9        +3     
  Lines         366      839      +473     
===========================================
+ Hits          236      267       +31     
- Misses        107      546      +439     
- Partials       23       26        +3     

Impacted Files	Coverage Δ
pkg/config/config.go	0.00% <0.00%> (ø)
pkg/registry/gar.go	0.00% <0.00%> (ø)
pkg/secrets/dummy.go	80.00% <0.00%> (-20.00%)	⬇️
pkg/types/types.go	68.96% <0.00%> (ø)
pkg/registry/ecr.go	5.06% <11.29%> (ø)
pkg/registry/client.go	37.93% <37.93%> (ø)
pkg/secrets/kubernetes.go	58.46% <62.50%> (+0.76%)	⬆️
pkg/webhook/image_swapper.go	62.06% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[estahn]

@Caomoji I prepared this initially to allow for registries requiring authentication (https://github.com/estahn/k8s-image-swapper/blame/bcc56b57ac91c1e8312f4e558283c68684a38678/.k8s-image-swapper.yml#L34-L37).

I prefer not to distinguish between private & public registries, but rather have registries requiring additional configuration. I had requests to support Azure Image Registries and I plan at looking at this soon. It would be good to have this already incorporated:

My suggestion is to adjust this as follows:

source:
  registries:
    - type: aws
      aws:
        accountId: 123456789
        region: ap-southeast-2
    - type: aws
      aws:
        accountId: 234567890
        region: us-east-1
    - type: dockerio
      dockerio:
        username: 
        password: 
        secretRef: 

Thoughts?

[Caomoji]

@Caomoji I prepared this initially to allow for registries requiring authentication (bcc56b5/.k8s-image-swapper.yml#L34-L37 (blame)).

I prefer not to distinguish between private & public registries, but rather have registries requiring additional configuration. I had requests to support Azure Image Registries and I plan at looking at this soon. It would be good to have this already incorporated:

My suggestion is to adjust this as follows:

source:
  registries:
    - type: aws
      aws:
        accountId: 123456789
        region: ap-southeast-2
    - type: aws
      aws:
        accountId: 234567890
        region: us-east-1
    - type: dockerio
      dockerio:
        username: 
        password: 
        secretRef: 

Thoughts?

I fully agree with your point of view, I updated the config to look like your example, as well as some methods to make registry client creation a bit cleaner. I also updated the target registry for consistency.
Adding other types of registry clients and implementing them should fit nicely now I think, please let me know you thoughts.

[Caomoji]

I rebased on main and adapted some things:

• Integrated changes with the new ECROptions field in AWS config
• Removed function SetRepositoryTags specific to ECR to rather integrate tags & options as part of the Client creation
• Made function newECRClient internal to restrict its use and rather emphasis on the more generic NewClient

@estahn please let me know if you have any suggestions.

[Caomoji]

Hello @estahn,
Do you think we could go on with this PR?
I'm available for any change suggestions, please let me know 🙏

[estahn]

Hello @estahn, Do you think we could go on with this PR? I'm available for any change suggestions, please let me know 🙏

@Caomoji Yes, sure thing. I was on vacation and had some other things to take care of. I'm looking at it again now.

I just merged GCP support. I remember there was some overlap with this PR. Can you double check if there is a conflict or if everything still works?

[Caomoji]

@estahn Thank you for your response, no worries.

The two PRs were quite conflicting indeed, both trying to bring genericity to registry handling but in a different way. I resolved the conflicts by trying to take the best of the two, while taking into account the core changes of this PR (managing sources registries as well).
I especially tried to make ECR and GAR registry clients look the same in their respective implementation, I modified some attributes from GAR client to look like ECR and rationalized common stuff.

[estahn]

@estahn Thank you for your response, no worries.

The two PRs were quite conflicting indeed, both trying to bring genericity to registry handling but in a different way. I resolved the conflicts by trying to take the best of the two, while taking into account the core changes of this PR (managing sources registries as well). I especially tried to make ECR and GAR registry clients look the same in their respective implementation, I modified some attributes from GAR client to look like ECR and rationalized common stuff.

Awesome. Will check it out tonight. Did you have a chance to run it locally to test it?

[Caomoji]

Awesome. Will check it out tonight. Did you have a chance to run it locally to test it?

Thanks! I've not tested it yet, I'll try to do so when I have some time.