Orphan fields after kubectl replacements

[stefanprodan]

Fields that are removed from a manifest, then applied with kubectl, then applied by the controller end up without a manager and are left in spec.

How to reproduce

1. Add an ingress in Git and reconcile it with flux:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test
  namespace: default
  labels:
    app: test
spec:
  ingressClassName: internal
  rules:
    - host: host1.internal
      http:
        paths:
          - backend:
              service:
                name: podinfo
                port:
                  name: http
            path: /
            pathType: Prefix
  tls:
    - hosts:
        - tls.internal
      secretName: test

2 . Add a host and remove tls and apply it with kubectl:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: test
  namespace: default
  labels:
    app: test
spec:
  ingressClassName: internal
  rules:
    - host: host1.internal
      http:
        paths:
          - backend:
              service:
                name: podinfo
                port:
                  name: http
            path: /
            pathType: Prefix
    - host: host2.internal
      http:
        paths:
          - backend:
              service:
                name: podinfo
                port:
                  name: http
            path: /
            pathType: Prefix

3. Commit the change to Git and reconcile with Flux.
4. The tls field is left on the cluster instead of being removed:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  creationTimestamp: "2022-02-02T08:32:46Z"
  generation: 2
  labels:
    app: test
    kustomize.toolkit.fluxcd.io/name: webapp
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  managedFields:
  - apiVersion: networking.k8s.io/v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:labels:
          f:app: {}
          f:kustomize.toolkit.fluxcd.io/name: {}
          f:kustomize.toolkit.fluxcd.io/namespace: {}
      f:spec:
        f:ingressClassName: {}
        f:rules: {}
    manager: kustomize-controller
    operation: Apply
    time: "2022-02-02T08:33:29Z"
  name: test
  namespace: default
  resourceVersion: "129440684"
  uid: 5f77374d-c308-45a5-88af-4e0c832fd228
spec:
  ingressClassName: internal
  rules:
  - host: host1.internal
    http:
      paths:
      - backend:
          service:
            name: podinfo
            port:
              name: http
        path: /
        pathType: Prefix
  - host: host2.internal
    http:
      paths:
      - backend:
          service:
            name: podinfo
            port:
              name: http
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - tls.internal
    secretName: test
status:
  loadBalancer: {}

[somtochiama]

I couldn't reproduce this bug in on a cluster running Kubernetes version 1.21.1

Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.1", GitCommit:"5e58841cce77d4bc13713ad2b91fa0d961e69192", GitTreeState:"clean", BuildDate:"2021-05-21T23:01:33Z", GoVersion:"go1.16.4", Compiler:"gc", Platform:"linux/amd64"}

But it was present in 1.20.1

Server Version: version.Info{Major:"1", Minor:"20", GitVersion:"v1.20.7", GitCommit:"132a687512d7fb058d0f5890f07d4121b3f0a2e2", GitTreeState:"clean", BuildDate:"2021-05-27T23:27:49Z", GoVersion:"go1.15.12", Compiler:"gc", Platform:"linux/amd64"}

I will continue to test with other versions

[kingdonb]

My repro was on K8s v1.23.2 – there's a critical part missing from the repro instructions,

3. Commit the change to Git and reconcile with Flux.

This must take place before the next Kustomization sync. If the Kustomization syncs first, after kubectl applies the change adding a rule for a host, and before the git change arrives through Kustomization controller, the repro will not be effective because the window for orphaning the resource has passed once the kubectl manager is replaced by Flux. (The tls section must be removed and the GitRepository reconciled before the next Kustomize controller reconciling interval.)

Also, I don't think you should remove the tls in kubectl apply to get this reproduced. The idea is that adding a hosts entry makes kubectl an owner of spec, and Kustomization will remove that ownership. If the commit that removes the ownership of spec also removes the tls section, then Flux will not recognize that tls should be removed from the resource in-cluster, and the tls section will be orphaned without an owner. (If you remove it with kubectl, I don't think you will observe the issue.)

There is a similar issue in any resource with optional fields. We discovered this issue through Clusters CRD, which has an optional section users and removing users is not possible once the field is orphaned, without re-adding the section first.

[somtochiama]

Hey @kingdonb ,
I published an image for kustomize-controller that uses the branch of fluxcd/pkg where this issue is resolved.
I have tested it but if it also be great if you can too.
Image: somma/kustomize-controller:rc-f7e83795a4f

[kingdonb]

Thanks @somtochiama – is there any way you can point me at the branch where I can build this from?

My test machines are arm64 linux now, so the single arch image gives me an exec format error.

[kingdonb]

I was able to get image: somma/kustomize-controller:rc-f7e83795a4f running on my x86 cluster,

It looks like you have done it, 🎉 I don't see the issue reproduced anymore on your rc @somtochiama

Have a great weekend!

[seh]

I'm not sure whether my situation matches what this issue intended to address, but we've suffered a regression in how Flux cooperates with other managers.

We have a manifest for a ConfigMap that looks like this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: gotk-sync-overrides
  namespace: flux-system
  annotations:
    kustomize.toolkit.fluxcd.io/prune: disabled

Flux reconciles this manifest in a Kustomization named "flux-system."

Using kubectl patch or even kubectl edit, we come along and change this ConfigMap, adding fields under a new top-level "data" field:

kubectl --namespace=flux-system patch configmap gotk-sync-overrides --patch='
data:
  APPS_GIT_REPOSITORY_TAG: flux/production'

That yields a ConfgMap like the following, per kubectl --namespace=flux-system get configmap gotk-sync-overrides --show-managed-fields --output=yaml:

apiVersion: v1
data:
  APPS_GIT_REPOSITORY_TAG: flux/production
kind: ConfigMap
metadata:
  annotations:
    kustomize.toolkit.fluxcd.io/prune: disabled
  creationTimestamp: "2021-07-19T20:22:38Z"
  labels:
    kustomize.toolkit.fluxcd.io/name: flux-system
    kustomize.toolkit.fluxcd.io/namespace: flux-system
  managedFields:
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          f:kustomize.toolkit.fluxcd.io/prune: {}
        f:labels:
          f:kustomize.toolkit.fluxcd.io/name: {}
          f:kustomize.toolkit.fluxcd.io/namespace: {}
    manager: kustomize-controller
    operation: Apply
    time: "2022-02-07T19:23:59Z"
  - apiVersion: v1
    fieldsType: FieldsV1
    fieldsV1:
      f:data:
        .: {}
        f:APPS_GIT_REPOSITORY_TAG: {}
    manager: kubectl-patch
    operation: Update
    time: "2022-02-07T19:33:00Z"
  name: gotk-sync-overrides
  namespace: flux-system
  resourceVersion: "1092750058"
  selfLink: /api/v1/namespaces/flux-system/configmaps/gotk-sync-overrides
  uid: f20404d5-1a69-4f35-8683-8b6581684a00

Shortly after we patch the ConfigMap, Flux now comes along and removes that top-level "data" field. It didn't used to do this before version 0.26.2.

[kingdonb]

Thanks for the report @seh

I tried the process that you described, and it clearly removes the new data field.

I tried the same thing with a server-side apply instead of a patch, and I got the same results. I'm going back to the previous version just to confirm this is definitely a regression, but this looks like a regression to me. We have had a lot of edge cases to cover in order to resolve these issues patching, so that users can still remove optional sections which were present in manifests applied by older versions of Flux that pre-dated server side apply.

Unfortunately I'm not surprised to hear that wasn't the final weird edge case. 😞

I think the maintainers will want to track this information in a separate issue.

[seh]

I see some effort to remove changes made by field managers named "kubectl" in kustomization-controller here. In my case in #558 (comment), the manager name was "kubectl-patch," so I think I'd be clear of that interference.

[More reading ....]

Oh, we use these names as a prefix, so "kubect" is indeed a prefix of "kubectl-patch."

[seh]

If I include the command-line flag --field-manager=seh-kubectl-patch with kubectl patch, I can fool Flux into ignoring this field manager. I don’t think that’s the intended behavior. Predicating this on the field manager name prefix will leave changes from too many clients alone while punishing this one client undeservedly.

[stefanprodan]

We have to remove all fields added by other managers as objects managed by Flux shouldn’t be altered outside Git. Right now we only target kubectl-*, but we need to expend it to all the others managers regardless of their name. We also need to allow users to define an allowlist for in-cluster managers that Flux should play along with.

[seh]

This is change in behavior, but I don't know if it's a change in your intentions for how Flux should work. I thought that the recent move to using SSA was intended to allow Flux to work with other managers more easily and properly: namely, that Flux would enforce that what it reads from manifests gets applied (as Flux owns all the fields in the manifests), and that it would leave fields owned by other managers alone.

Your proposal to require defining a tolerable set of other managers would accommodate known, popular managers like kube-controller-manager, but will require users to learn the manager names of and configure Flux to tolerate other controllers that they will use. I anticipate this is going to cause a support burden and frustrate users.

That said, I respect your command of the project. I'd like to understand how to proceed here. Please entertain a few questions:

• Will Flux still handle annotations such as "kustomize.toolkit.fluxcd.io/reconcile" and "kustomize.toolkit.fluxcd.io/prune"?
  Those would be added by other field managers such as kubectl.
• Do you recommend that we not include our "gotk-sync-overrides" ConfigMap manifest in sources that Flux reads via a Kustomization?
  Clearly, we intend to modify this ConfigMap via kubectl and other clients.
• Would you consider allowing ConfigMaps referenced by post-build substitution to be optional?
  That is, we'd be able to indicate to Flux that if the referenced ConfigMap doesn't exist, then just treat any fields we'd read from it as absent or empty? This would involve adding a field to SubstituteReference such as "Optional" with a default value of false.

[stefanprodan]

@seh I think you should decide on a manager name (other than kubectl-*) for all operations performed outside Git onto Flux managed objects, this can be achieved with kubectl apply --server-side --field-manager=my-manager.

The main challenge we are facing now, is being able to undo manual changes made with kubectl apply as most people expect this feature from a GitOps operator. Another challenge is to be able to remove fields from Git even if those were applied at some point with kubectl. Given that kubectl conflicts with itself and removing fields is no longer possible we were forced into this solution to take over all kubectl-* managers. Hopefully when things will be sorted upstream, we can find a better solution and allow kubectl edits to fields not specified in Git.

[stefanprodan]

Your proposal to require defining a tolerable set of other managers would accommodate known, popular managers like kube-controller-manager, but will require users to lean the manager names of and configure Flux to tolerate other controllers that they will use. I anticipate this is going to cause a support burden and frustrate users.

It's really a struggle to find the perfect solution, on one hand users expect for Flux to override and undo changes made outside Git, on the other hand, Flux needs to tolerate other controllers such as HPA or Flagger, so they can perform their function even if it means altering objects defined in Git. Given that Kubernetes records the manager name and the operation type (Update or Apply), I'm inclined towards an allowlist for Update managers (those that use client-side apply) and allow any server-side apply manager to do whatever. If it's server-side then it's a controller running in-cluster so Flux shouldn't undo the fields added by those, if it's client-side then it's some tool that is bypassing Git, so cluster admin could explicitly allow it.

[seh]

I think that's a fine compromise for now. It's inconvenient to need to use the --field-manager flag with kubectl, and other client tools might not allow customizing such a name, but it's at least predictable, while still allowing Flux to cooperate nicely with other controllers.

It sounds like we shouldn't count on "any field manager with a name that doesn't start with 'kubectl' is tolerated" being true for long, and should instead expect that we'll need to tell Flux which manager names (or name prefixes?) it should tolerate.

[seh]

I decided to remove the aforementioned ConfigMap from Flux's coverage, so that we create it imperatively when first bootstrapping Flux into our clusters (we have a shell program that arranges for this). This leaves us with the liability that someone might delete this ConfigMap accidentally, which will cause Flux to fail on its next reconciliation attempt.

Have we ever considered—as I suggested earlier—making ConfigMaps and Secrets referenced for post-build substitution optional?

[stefanprodan]

@seh please open an issue for the optional substitutions.

[seh]

please open an issue for the optional substitutions.

Please see #565.