`client.lifecycle.config.k8s.io/mutation: ignore` seems broken with 1.21

[ioanrogers]

We use apply-time-mutation to copy the oauth secret from an IAPIdentityAwareProxyClient into a Secret so that it can be referenced by a GCPBackendPolicy. As that secret value is plain text, the apply-time-mutation target writes the value to .stringData. We have a client.lifecycle.config.k8s.io/mutation=ignore annotation to prevent Config Sync from trying to set the Secret back to the pre-mutated state from git.

That has worked well for a while, but upon upgrading to Config Sync 1.21 sync fails with KNV2009: failed to apply Secret, grafana/oauth-client-secret: failed to mutate "grafana_oauth-client-secret__Secret" with "ApplyTimeMutator": failed to read field ($.stringData.clientSecret)

This is only affecting Secrets where we are writing to the write-only .stringData. Other uses of mutate then ignore sill work, e.g. copying the oauth client id from the IAPIdentityAwareProxyClient directly to the GCPBackendPolicy.

[sdowell]

Could you please provide clear steps to reproduce and a nomos bugreport? That will help us diagnose the issue.

This PR had some changes to ignore-mutation which were first released with 1.21. @Camila-B can you take a look?

[ioanrogers]

Sorry, I meant to add a reproducer in the original report.

---
kind: Pod
apiVersion: v1
metadata:
  name: test
  namespace: mutation-test
spec:
  containers:
    - name: nginx
      image: docker.io/nginx:latest
      ports:
        - name: http
          containerPort: 80
---
apiVersion: v1
kind: Secret
metadata:
  name: test
  namespace: mutation-test
  annotations:
    client.lifecycle.config.k8s.io/mutation: ignore
    config.kubernetes.io/apply-time-mutation: |
      - sourceRef:
          kind: Pod
          name: test
          namespace: mutation-test
        sourcePath: $.status.podIP
        targetPath: $.stringData.foo
stringData:
  foo: VALUE

This results in

Error:   KNV2009: failed to apply Secret, mutation-test/test: failed to mutate "mutation-test_test__Secret" with "ApplyTimeMutator": failed to read field ($.stringData.foo) from target object (v1/namespaces/mutation-test/Secret/test): expected 1 match, but found 0)

but the secret is updated:

$ kubectl -n mutation-test get secret test -o yaml | yq .data.foo | base64 -d
10.251.0.155

Here's the nomos bugreport.
bug_report_1747154040.zip

[Camila-B]

Thanks! I'll take a look at it

[ioanrogers]

Thanks. I just updated the description to clarify it.

[janetkuo]

apply-time-mutation is a kpt feature, but it's not officially supported in Config Sync. Config Sync uses kpt, so that explains why it worked before 1.21. Supporting apply-time-mutation in Config Sync would be a feature request.

[ioanrogers]

The apply-time-mutation seems to be working, it's the ignoring the mutation afterwards that isn't. client.lifecycle.config.k8s.io/mutation: ignore is documented which I assume makes it supported. However, I have no idea know how closely related the two features are or how they interact.

If this is definitely a feature request then it would be low priority for us given that we only use it for IAPIdentityAwareProxyClient and the underlying API for that is deprecated.

[sdowell]

apply-time-mutation is a feature of cli-utils, which is a library used by both kpt and Config Sync. We have not tested/validated the compatibility of apply-time-mutation with Config Sync, so we do not guarantee that it will work with Config Sync.

If it worked in conjunction with client.lifecycle.config.k8s.io/mutation before, that was incidental and not something we formally validate/validated.

In 1.21 we fixed a known issue with the client.lifecycle.config.k8s.io/mutation annotation where it didn't properly ignore mutations under several circumstances. It seems this is the cause for the change in behavior you observed with apply-time-mutation.

---

The reason the behavior changed in your apply-time-mutation use case is the following:

The object is declared in source as:

apiVersion: v1
kind: Secret
metadata:
  name: test
  namespace: mutation-test
  annotations:
    client.lifecycle.config.k8s.io/mutation: ignore
    config.kubernetes.io/apply-time-mutation: |
      - sourceRef:
          kind: Pod
          name: test
          namespace: mutation-test
        sourcePath: $.status.podIP
        targetPath: $.stringData.foo
stringData:
  foo: VALUE

And when applied to the cluster will look like:

apiVersion: v1
kind: Secret
metadata:
  name: test
  namespace: mutation-test
  annotations:
    client.lifecycle.config.k8s.io/mutation: ignore
    config.kubernetes.io/apply-time-mutation: |
      - sourceRef:
          kind: Pod
          name: test
          namespace: mutation-test
        sourcePath: $.status.podIP
        targetPath: $.stringData.foo
data:
  foo: VALUE

Note how stringData moves to data (the upstream docs warn about how this doesn't play nicely with server-side apply).

In the aforementioned PR, we fixed the known issue with ignore-mutation by reading the live object(s) from the cluster and passing them into the applier.

When re-applying the Secret object, the apply-time-mutation annotation references the stringData field but the stringData field is not present (because it was moved to data). This is the reason that the applier runs into the error that was observed here:

W0513 21:16:24.106358       1 updater.go:201] Applier failed: KNV2009: failed to apply Secret, mutation-test/test: failed to mutate "mutation-test_test__Secret" with "ApplyTimeMutator": failed to read field ($.stringData.foo) from target object (v1/namespaces/mutation-test/Secret/test): expected 1 match, but found 0)

---

I wouldn't suggest using apply-time-mutation since it's not validated with Config Sync, but as a workaround you could try using an idempotent resource that looks the same before/after apply (e.g. ConfigMap).

[roland-otta]

i am willing in contributing to add support for the apply-time-mutation as we also have many use cases for it in case the project team is interested in adding the support to config sync.

see also #1724

[ioanrogers]

I should note that I got originally got the idea for doing it this way from GoogleCloudPlatform/k8s-config-connector#716 (comment)
I realise no one in that thread is mentioning Config Sync but I'm betting Config Sync + Config Connector are often used together. One GKE addon seems to recommending this feature and another is saying not to use it.

[mikebz]

Thank you for referencing the actual problem you are solving and the dynamic IP addresses have often been cited. I assume a dynamic reference to the field has been explored. What were the reasons against that?

The configuration cited is probably best "hydrated" before things go into git:

metadata:
  name: {{ .Release.Name }}-ir
  namespace: {{ .Release.Namespace }}
  annotations:
    "networking.gke.io/managed-certificates": "{{ .Release.Name }}-cert"

Putting more of the hydration on the cluster leads to harder debugging and predictability. We generally have been recommending all the hydration to happen "off the cluster"

https://cloud.google.com/kubernetes-engine/enterprise/config-sync/docs/concepts/gitops-best-practices#create-wet-repo

[ioanrogers]

Thanks for looking. Dynamic references are not an option for the particular combo of resources in the issue description. We do hydrate at build time before pushing to the repo Config Sync is watching, but AFAICT that would require 2 deploys (deploy the CC resource; copy the value in to git somehow; deploy consuming resource)

Following on from #1668 (comment) we've now removed the IAPIdentityAwareProxyClient in favour of the new automatic Google IAP creds in our GCPBackendPolicy. So we no longer need this particular apply-time-mutation.