mutation "location" part of applyTo

[jdef]

Describe the solution you'd like I want to apply the same mutation to objects w/ slightly different structure. Without writing duplicate mutation specs.

spec:
  match:
    scope: Namespaced
    kinds: [ { apiGroups: "", kinds: [ Pod ] }, { apiGroups: "apps", kinds: [ Deployment, ReplicaSet ] } ]
  applyTo:
  - groups: [ "" ]
    kinds: [ Pod ]
    versions: v1
    location: "spec.containers.spec[name:injected*].securityContext"
  - groups: [ apps ]
    kinds: [ Deployment, ReplicaSet ]
    versions: [ v1 ]
    location: "spec.template.spec.containers.spec[name:injected*].securityContext"
  parameters:
    assign:
      value:
        readOnlyRootFilesystem: true

[maxsmythe]

I like this. It's not a trivial change, programming-wise, but I think it makes a lot of sense.

@mmirecki @fedepaol @shomron @sozercan @ritazh thoughts?

[maxsmythe]

The one problem with this is that it becomes less clear how pathTests would work.

[mmirecki]

Would we create multiple mutators out of one Assign? (I like the current simplicity of a 1-1 model) Could we create a mutation having multiple locations on the same resource? This could make tracking which mutation is applied on a resource somewhat difficult (aka: #1140), especially if more than one location is modified for a kind.

It has some potential for being abused for grouping unrelated items, for example:

  applyTo:
  - groups: [ "" ]
    kinds: [ Pod ]
    versions: v1
    location: "spec.containers[name:*].livenessProbe.failureThreshold"
  - groups: [ apps ]
    kinds: [ Pod ]
    versions: [ v1 ]
    location: "spec.containers[name:*].resources.limits.cpu"
  - groups: [ apps ]
    kinds: [ Deployment ]
    versions: [ v1 ]
    location: "spec.replicas"
  - groups: [ apps ]
    kinds: [ Deployment ]
    versions: [ v1 ]
    location: "spec.template.spec.containers[name:*].terminationGracePeriodSeconds:"
  parameters:
    assign:
      value: 10

[jdef]

OK, well, if we can't move location into applyTo for reasons, then what is the benefit of applyTo at all? Objects can already be selected by spec.match. moving location into applyTo seems to further justify the existence of applyTo because it would let you surgically modify values for various objects across even when their structures differ. Otherwise, if the goal is 1:1 between selector:mutation then having both spec.match and applyTo do the same job seems to add redundancy for no benefit. I'm also still getting my head into mutations, so maybe I'm missing something obvious. If so, please enlighten me :)

It has some potential for being abused for grouping unrelated items...

I can also see this being framed as a "feature" vs. "abuse".

[maxsmythe]

The main reason for applyTo is that it binds a mutator to specific GVK(s). This is required (though that requirement is not explicitly enforced in the current release, as we are still building mutation) because each mutation implicitly defines a bit of an object's schema.

If I have 2 mutators, one that sets:

spec.containers = 7

And another one that sets:

spec.containers[name: Foo] = {<<some container>>}

then the two different mutators have different views for the type of spec.containers (integer vs. list). Which one is right?

For Pods, the second one is right, and we should ignore the first one.

What if we had some new resources called MaximumContainersPerPod though? The first mutator might be referring to that.

We have applyTo to solve for collision cases like this, which is why it's required.

It is true that applyTo and match.kinds overlap a lot. applyTo is more strict in that it doesn't allow wildcards, which makes which kinds the schema is being bound to explicit. match.kinds is still valuable for the AssignMetadata mutator, however. Because all objects share the same metadata schema, there is no need for schema binding here and wildcards are useful.

We decided to keep vestigial code (that still works) on ApplyTo for the sake of uniformity. We only need to maintain one set of matcher logic and users don't need to memorize schema differences (even though they'll never use match.kinds in the Assign mutator because it adds ~zero value).

[jdef]

@maxsmythe thanks for the detailed explanation.

The one problem with this is that it becomes less clear how pathTests would work...

Can you elaborate on this feedback? I think I'm still missing some piece of the puzzle.

[maxsmythe]

For sure. PathTests are testing conditions at certain parts of the path depth (e.g. spec.containers should exist, but spec.containers[name: my-sidecar] must not exist. This is useful for things like using mutation to set a default value.

If you have multiple paths, maybe we could figure out which path test belongs to which path by figuring out which one is a prefix of the other, but that might fail if there is overlap (e.g. all of these have a spec field).

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.