Checksum label is missing when prune is false

[stealthybox]

When prune is true, child objects of Kustomizations have these labels:

labels:
  kustomize.toolkit.fluxcd.io/checksum: 21ff4959f5c2052e702115c585ea8cf694c67613
  kustomize.toolkit.fluxcd.io/name: flux-system
  kustomize.toolkit.fluxcd.io/namespace: flux-system

When you set prune to false, the checksum label is dropped:

labels:
  kustomize.toolkit.fluxcd.io/name: flux-system
  kustomize.toolkit.fluxcd.io/namespace: flux-system

Even if the user does not have garbage collection enabled, it is still very useful to have these checksum labels because you can use them to identify stale resources.
This would for example be useful in a flux tree view or a web interface like the Flux UI that shows which objects are managed and which are not.

I don't see any reason not to include these checksums.
It should be a relatively safe change for existing Kustomizations when kustomize-controller is updated, but it will cause every object to be patched with new labels.

[stefanprodan]

We used to have the checksum labels no matter if GC was enabled and that caused many issues for our users, controllers like Kafka, MySQL and others would delete the stateful sets every time the label changes, we also spam with alerts since all objects are changed.

[stealthybox]

Fair enough, although that sounds like the behavior with prune is suboptimal in those cases.
Would a matching annotation do any harm?
Could we modify the GC to filter annotations and take a small performance hit?

[stealthybox]

An issue with the above suggestion is that the GC gets slower linearly as garbage grows, but maybe it's worth the tradeoff.

Is the issue with the Kafka/MySQL controller that the labels are being copied down to the child STS objects from a parent CR? (and the labels aren't stable)
I assume using annotations would fix that since they are designed to be a RW extension point for API's.

[stefanprodan]

Could we modify the GC to filter annotations and take a small performance hit?

I don’t think you can query based on annotations and fetching all objects of a kind could be expensive and due to rate limiting, on some distributions it will result in timeouts.

[stealthybox]

If the label selector is only the kustomization Name + Namespace, we can still query more efficiently, but it would include all of the garbage.

[simongottschlag]

Is the biggest problem here that some controllers doesn't handle updates of labels well?

What if we kept the default behavior to include checksums and made it possible to disable per resource using an annotation like kustomize.toolkit.fluxcd.io/checksum-disabled: "true"?

[stealthybox]

Noted in conversation with Stefan, a concern with merging #348 with the current state of kustomize-controller is that it will cause Events to be generated whenever the kustomize build output changes.

The exact desired Object Tracking behavior and constraints are not super clear to me yet.

1. I know the kustomize-controller's interaction with the Kubernetes API needs to be performant.
2. Users of the Kustomization Status fields and related Events should also have a good experience.

a.
Currently, when Prune is enabled and the checksum changes, every child object of the Kustomization will be updated with new labels.
This can sometimes cause undesireable reconcile behavior in other 3rd-party controllers.
If the checksum changes, I also expect an Event to be created as record of the Kustomization update.

Annotations are intended to be arbitrary extensions, so adding them when the labels are already changing in the Prune Enabled case seems inconsequential to me.

b.
Currently, when Prune is disabled, the checksum is not included in any objects, only the static Flux Kustoimzation labels are, so no updates to other objects can occur unless they actually change.
Objects that don't change will never be reconciled by downstream controllers.

Adding annotations when Prune Disabled is less likely to cause undesirable downstream reconciles, but there is additional overhead to consider now that every child object of the Kustomization must be updated, which currently doesn't happen.

Using annotations but omitting labels prevents clients interested in stale objects from using label-selectors like kustomize-controller can.
Clients must do filtering client-side which is probably fine for most use-cases, unless people are implementing their own Prune/Garbage-Collector.

c.
I'm not fully understanding the relation to kubectl diff and Drift Correction.
Drift Correction seems possible to bolt-on before the objects are reconciled into the Cluster, regardless of what we do with the labels.
If we are trying to filter Events on Kustomizations only when the checksum changes, that already seems possible and adding the checksum annotation to downstream objects shouldn't affect that.
It seems like we should already be able to filter Intended-Update Events from normal reconciles by detecting when the checksum changes.

The only consequence I can think of:
if we introduce this behavioral change to kustomize-controller, teams who have Prune disabled will have a one-time change calculated, when they upgrade kustomize-controller and the downstream annotations are added.
This behavior is already true when you enable Prune on an existing Kustomization.

---

If we could clarify together what sort of behavioral constraints we want from kustomize-controller's Object Tracking, we'll come up with the right solution.

I'm almost inclined to suggest that we just use Prune-labels (not annotations) all the time and include a boolean annotation or spec field so that people who really need to opt-out of Object Tracking checksums can do so.

[jonathan-innis]

@stealthybox Are we able to control these Kubernetes events in some way or are these just a construct of updating the object in general and can't be avoided? I'm assuming that this is a construct of Kubernetes but just clarifying.

[stefanprodan]

For users that have GC disabled this is what currently happens:

• user: creates a Kubernetes Deployment, Kubernetes Service and a Flux Kustomization in Git
• kustomize-controller: labels the Deployment and Service and applies them on the cluster
• kustomize-controller: issues event with deployment created, service created
• user: updates the Kubernetes Deployment in Git
• kustomize-controller: issues event with deployment changed

If we merge Jonathan's PR:

• user: updates the Kubernetes Deployment in Git
• kustomize-controller: annotates the Deployment and Service and applies them on the cluster
• kustomize-controller: issues event with deployment changed, service changed

As you can see with that PR we'll be sending "bogus" events, the service didn't changed in Git nor in cluster, but kubectl apply reports a change since the annotation that we generate did changed.

To resolve this issue, instead of relying on kubectl apply output, we should run kubectl diff first, parse the output, remove all objects where only the checksum annotation changed, and use this list for the events. Once we do that, we can drop the checksum label, and use the annotation for GC as well. This would be a major improvement for those that have GC enabled, as they will receive events only for the objects where there is an actual drift.

[stealthybox]

Hopefully cache-hits in the apply for all of the objects from the diff will make adding the diff unnoticeable.

It would be really ideal if the kubectl output had structured diff information already.
I wonder if using kubectl as a library would make this more possible.

[phillebaba]

So one thing I found when testing out kubectl diff in one of my clusters is that it failed. The reason it failed is because I have OPA Gatekeeper mutating webhook n the cluster. It returns an error message.

Error from server (BadRequest): admission webhook "mutation.gatekeeper.sh" does not support dry run

The reason this occurs is because the mutating webhook has set sideEffects: Unknown which will auto-reject dry run commands.
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#mutatingwebhook-v1-admissionregistration-k8s-io

We need to document that flux depends on dry run working and not being limited by a webhook. I will try to get this solved in the gatekeeper project, but I am unsure if there are other projects which may cause flux to stop working.

[stefanprodan]

kustomize-controller uses dry run when spec.validation is set, the diff comes after the validation step, so Gatekeeper would block Flux no matter if we add the diff step or not.

[phillebaba]

Yes but validation is an optional setting that is disabled by default? This feature would run dry run for every apply.

[stefanprodan]

Validation is enforced at bootstrap, so anyone running bootstrap via the CLI or Terraform will run into this issue. I can see how doesn't apply to Azure... Guess we'll have to make diff optional too but then we are back where we're started, without diff you get events spam and so on.

[stefanprodan]

@phillebaba I'm for dropping validation: none in v1beta2, so then it will no longer matter if we use diff, it will always fail.

[phillebaba]

I think responsibility should fall on the end user to make this work. I dont normally use the diff or dry run commands which is why I have totally missed this. I think we should move forward with having dry run as a required feature and document this requirement as I bet it will become more obvious when v1beta2 is released.

I created an issue in the gatekeeper repository, and my guess is that kyverno does not have this problem, but I will have a look just to be sure.

[stefanprodan]

Every kyverno release is being e2e tested with every Flux release :) See fluxcd/flux2-multi-tenancy#30

[stefanprodan]

@phillebaba you may want to review #362 and express your concerns there, maybe in that PR we should run the diff only if validation is enable so that Flux doesn't crash when used with OPA Gatekeeper?

[stefanprodan]

@phillebaba I looked it up and Kyverno uses sideEffects: NoneOnDryRun, a server side dry run or diff would trigger it:

$ k apply --dry-run=client -f fails-policy.yaml 
kustomization.kustomize.toolkit.fluxcd.io/podinfo2 created (dry run)

$ k apply --dry-run=server -f fails-policy.yaml 
$ k diff -f fails-policy.yaml 
Error from server: admission webhook "validate.kyverno.svc" denied the request: 

resource Kustomization/apps/podinfo was blocked due to the following policies

flux-multi-tenancy:
  serviceAccountName: 'validation error: .spec.serviceAccountName is required. Rule
    serviceAccountName failed at path /spec/serviceAccountName/'

[phillebaba]

This has now been fixed in Gatekeeper so should not be any issues when the next version is released. open-policy-agent/gatekeeper/pull/1360

[stefanprodan]

We have in #379 an implementation that uses diff, but this falls short on numerous cases (see #379 (comment)) given this situation I'm inclined on closing this issue as wontfix.

[kingdonb]

I'm going to go ahead and close the issue. Thanks for the discussion.