feat: implement AssignImage mutator

[davis-haba]

Signed-off-by: davis-haba davishaba@google.com

Implements the new AssignImage mutator, which is designed for setting docker image strings. The mutator allows setting the domain, path, and tag separately. #2381

An image string is parsed into its components using mostly the same logic used by docker, the difference being we do not impose any defaults (e.g. we do not assume any empty domain should be interpreted as "docker.io", or that an empty tag is "latest").

To ensure an AssignImage cannot define a non convergent mutator, we perform validation on the 3 component fields. Each component is validated separately using similar validation used by docker. This prevents users from putting tokens we use to split the image string into components where they do not belong. For example, without validation, setting the tag to /abc/def on the string nginx:latest would change it to nginx/abc/def on the first round, but since no tag is parsed out of this string, the next round would produce nginx/abc/def/abc/def and so on.

In addition to validating each component individually, we also require that the path is not set to a "domain-like" value if domain is not set. Since docker domains can have / and ., it would otherwise be possible to set a path like abc.io/repo/app to modify an image string like nginx:latest, which would cause abc.io to be parsed as part of the domain on the second round of mutation, resulting in non convergence.

[codecov-commenter]

Codecov Report

Base: 53.92% // Head: 53.51% // Decreases project coverage by -0.42% ⚠️

Coverage data is based on head (d2caf9f) compared to base (73611cc).
Patch coverage: 45.80% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2429      +/-   ##
==========================================
- Coverage   53.92%   53.51%   -0.42%     
==========================================
  Files         116      120       +4     
  Lines       10287    10635     +348     
==========================================
+ Hits         5547     5691     +144     
- Misses       4314     4512     +198     
- Partials      426      432       +6     

Flag	Coverage Δ
unittests	53.51% <45.80%> (-0.42%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pis/mutations/unversioned/zz_generated.deepcopy.go	1.50% <0.00%> (-0.42%)	⬇️
...mutation/mutators/assignmeta/assignmeta_mutator.go	29.80% <0.00%> (ø)
pkg/mutation/mutators/conversion.go	50.00% <0.00%> (-16.67%)	⬇️
pkg/mutation/schema/node.go	91.06% <ø> (ø)
pkg/mutation/types/mutator.go	33.33% <ø> (ø)
pkg/webhook/policy.go	38.73% <0.00%> (-1.41%)	⬇️
pkg/mutation/mutators/core/mutator.go	36.66% <36.66%> (ø)
...tation/mutators/assignimage/assignimage_mutator.go	38.26% <38.26%> (ø)
.../mutation/mutators/modifyset/modify_set_mutator.go	50.33% <42.85%> (+0.33%)	⬆️
pkg/readiness/ready_tracker.go	68.84% <44.73%> (-0.99%)	⬇️
... and 7 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tahirraza]

Have not gone thru all changes yet.
Just calling out we are covering scenarios where we can

• replace with any arbitrary image repository
• add image repository (with default one) - in cases if only image was defined without repo path

[acpana]

this LGTM! Just some small questions. There's also a chance to use require for testing if you want to; that's just a non blocking suggestion!

[maxsmythe]

Getting very close. Just the error testing and hardening the validation of the mutator parameters a bit more.

[tahirraza]

@maxsmythe and @davis-haba any updates on this change?
I'm thinking I can achieve something like this based on the changes proposed?

apiVersion: mutations.gatekeeper.sh/v1alpha1
kind: AssignImage
metadata:
  name: defaultimageregistry
spec:
  applyTo:
    - groups: [ "" ]
      kinds: [ "Pod" ]
      versions: [ "v1" ]
  location: "spec.containers[name:*].image"
  parameters:
    assignDomain: "1234567.dkr.ecr.us-east-1.amazonaws.com"
  match:
    source: "All"
    scope: Cluster # (Cluster, Namespaced)
    kinds:
      - apiGroups: [ "*" ]
        kinds: [ "Pod" ]
    excludedNamespaces: ["excl-ns-1"]

And if I do this, will it work for cases where domain is defined and not defined? I'm happy to test if you can provide guidance.

[maxsmythe]

@tahirraza the PR is still being worked on, but has been dormant for a bit due to the holidays.

The PR should work exactly as you describe! I think it should be functional as-is (most of the back-and-forth is about safety and ensuring convergence IIRC). If you have the cycles and want to run a test build of the code, let us know how it goes!

[tahirraza]

Okay. I'll take a jab at it.

[maxsmythe]

Couple of testing and wording nits, but basically LGTM (will LGTM after nit fix)

[maxsmythe]

LGTM after nit fixes!

[davis-haba]

@ritazh @sozercan PTAL

[sozercan]

Thank you! LGTM

[ritazh]

LGTM