Cluster-state drift detection #643
Comments
hiddeco
added
enhancement
|
Thanks for so smart implementation of this "so desired" feature ;-)
So helm-controller, for each reconciled
If any drift is detected -> a new Helm upgrade procedure is performed using the referred Helm chart. Did I get it right? If so, some few additional questions:
Thanks in advance. |
We look at if the object was created or changed during the dry-run apply. If it changed, we look at the comparison of "old" (the resource as present on the cluster) and "new" (the resource after the resource as defined in the release has been dry-run applied) to provide a diff in the logs. But the "created" or "changed" is already sufficient to determine if we should trigger a new release, as it means the resource in the cluster has either been deleted or mutated.
This is correct.
This is correct, as hooks are not part of the manifest blob in a Helm release storage object, but stored separately.
This is correct. |
|
Hi, I've read the prometheus example around drift-detection. If I understand well, PrometheusRules are excluded from drift-detection. So, when someone deletes/modifies one of the mix-in rules, the change is not reverted by flux ? |
|
I tried this out for a little bit and it seems to be working, correctly recreated a deployment I deleted 👍️ |
This is correct, as PrometheusRule/monitoring/kube-prometheus-stack-etcd diff:
.metadata.annotations.prometheus-operator-validated
+true
PrometheusRule/monitoring/kube-prometheus-stack-general.rules diff:
.metadata.annotations.prometheus-operator-validated
+true
...We are looking into options to e.g. define a YAML path to ignore to circumvent this issue, rather than ignoring the resources in full. |
Sounds really promising. Thank you. |
in preparation of fluxcd/helm-controller#643 we raise the reconcile times, as the reconciliation might now really do some work, finally!
in preparation of fluxcd/helm-controller#643 we raise the reconcile times, as the reconciliation might now really do some work, finally!
in preparation of fluxcd/helm-controller#643 we raise the reconcile times, as the reconciliation might now really do some work, finally!
in preparation of fluxcd/helm-controller#643 we raise the reconcile times, as the reconciliation might now really do some work, finally!
in preparation of fluxcd/helm-controller#643 we raise the reconcile times, as the reconciliation might now really do some work, finally!
|
Thanks for this feature! @hiddeco We have two questions regarding drift detection:
|
This is indeed planned, but not expected to land before end Q2 / sometime in Q3.
Can you please create a separate issue for this? |
Is there an issue or something I could follow? |
|
@benarnold42 I think this is the same issue in principle ( |
|
What does this error mean?
|
hiddeco
changed the title
|
Have updated the issue to reflect the changes since the |
|
Is it possible to set the default drift detection to be |
|
I think a better solution than controller global defaults is to make use of a policy agent which enforces certain default configurations through validation, an example of this are Kyverno policies (https://kyverno.io/docs/writing-policies/). This ensures you stick to a pure declarative model which can easily be reapplied elsewhere while functioning the same. The reason the feature was previously available as a global configuration option was due to it being experimental, and us having questions around how we wanted to introduce it in the API. |
|
Alright, I understand the reasoning. Thanks for the feedback. |
I don't really understand this, isn't this way less declarative than just specifying a global default? We'd also be interested in enabling this feature by default without having to touch every HelmRelease and definitely without having to write random mutating hooks in random policy agents. Especially since a global default is much more portable than "you have to install another component you didn't want and depending on what policy agent you decide on you have to define this value differently. Oh, and don't forget that this doesn't apply to already existing resources, so, yeah, should've thought about this earlier" |
|
Thanks for this cool feature :) One question: I see that in both the debug logs and in the events, we print something like: I would be interested in what are the actual changes, additions and removals before changing the mode from "warn" to "enabled". Is there any way to do so? Thanks! |
|
If you set In the new year, we are planning to add a command to |
|
If flux detects some kind of drift, e.g. I delete a resource, does flux execute the hooks? A bit of testing gave me the impression that the hooks won't be executed, in particular https://github.com/teutonet/teutonet-helm-charts/blob/main/charts/base-cluster/templates/flux/_create-authentication-key-secret-job.yaml Seeing the secrets, or the helm history, no
Could this be implemented? We rely on hook for various dynamic things in our helm charts |
|
Btw, the current implementation, with So creating a flux-global flag or just enabling this by default should be done. |
Before helm-controller had no drift detection capability so it couldn't recreate missing resources. |
|
I used |
|
@Legion2 to achieve the same thing with Flux 2.2 do |
Nice, but still, a global flag would be amazing, because I see no other good way of setting this globally. And setting this on all of my dozens of HelmReleases is quite annoying and error prone |
|
+1 for global setting, this would be amazing, otherwize this is unusable |
|
HelmReleases are applied on the cluster by Flux Kustomizations, adding a patch to enable drift detection for all ca be done with: apiVersion: kustomize.toolkit.fluxcd.io/v1
kind: Kustomization
spec:
patches:
- patch: |
apiVersion: helm.toolkit.fluxcd.io/v2beta2
kind: HelmRelease
metadata:
name: all
spec:
driftDetection:
mode: enabled
target:
kind: HelmRelease |
Mh, that would be an annoying but semi workable solution, although I'm not happy with that |
|
The drift detection is not enabled by default because it introduces risk. It would be a goal of the v2beta2 -> v2 update to make this safe enough to enable by default. Enabling this globally introduces hazards that you should be monitoring for, and mitigating. The hazards are not uncommon, they occur in highly popular charts like kube-prometheus-stack and other common places, like anything that uses an HPA. https://fluxcd.io/flux/components/helm/helmreleases/#drift-detection which gets a reference from: https://fluxcd.io/flux/installation/configuration/helm-drift-detection/ covers the hazard, how to monitor for it, and the mitigation. This is important, users who enable drift detection may have to read it all and take action to prevent Flux from wasting a lot of resources dry-cycling and upgrading forever. Not every configuration will be affected, but when we introduce global configuration it brings the risk that some combination of defaults we have not considered is not good, and in this case, we've advised many users to do When we can condense all that into a single action that Flux can take on the user's behalf, then I think it will be possible to do global enable again. Ideally it's not even a feature flag, we just turn it on, since this is what users expect from the definition of GitOps. But as long as the users expectations may be violated by the hazards, certain HelmReleases are probably going to need some careful handling by an admin, and I would expect the global enable to generate more confused support inquiries than it resolves. We understand the current state isn't optimal, that's why it's v2beta2 and not yet v2. Thanks for your patience as we make progress on this. |
We can’t make this safe as it’s not in our control. There are lots of charts out there which mutate the state from jobs running as Helm hooks, not to mention HPA/VPA. People need to opt-in and set ignore rules. |
|
Why then ArgoCD manages to enable this globally somehow and they are just fine with that? Every global chart out there has some instructions to make it work with Argo properly (the same thing with exclude rules) and you for some reason encourage people to waste their time building circumventions for poor design... |
Because ArgoCD doesn't really support Helm Charts, they only use it for simple templating and don't do hooks or API access. 😥 While flux does it correctly, allowing API calls, hooks, ... 🥳 Which is also why ArgoCD and flux can't really be compared 🤷 |
|
They do hooks, in a subtly incompatible way that will only bite you when you're getting deeply invested in Helm charts, you won't even notice it's a problem at first: https://blog.teamhephy.info/blog/posts/tutorials/argocd-sentry-apps.html It has been the story for years, ArgoCD maintainers recommend Flux if you need "perfect helm installations" argoproj/argo-cd#4288 (comment) This is one of the leading reasons why create Flamingo
I think the jury is out, there is some mechanism we can add to make it safe, but letting users opt in to the behavior and read the manual is a fine solution IMO. It may be better if the mechanism comes along that lets us short-circuit failures so we don't spin out of control. (Or the appropriate controls may have already made it in, I don't have a test that I can point at that says there is still any problem here...) I agree that it's not safe to enable globally, if I haven't made that clear, and I won't recommend it because of the danger. This is because we use Helm SDK from upstream, and that's why you won't find this report under ArgoCD anywhere. |
📣 Announcement
We are excited to announce a new (long requested) feature in the helm-controller - drift detection! This is now available in
>=v0.37.0, and can be enabled by configuring aHelmReleasewith.spec.driftDetection.modeset toenabled.To enable drift detection without correction, set
.modetowarn.🔎 What is drift detection?
Drift detection allows you to detect any unintentional changes to a resource in your Kubernetes cluster that may have occurred outside of your Helm release process. The feature uses the same approach as kustomize-controller to detect drift by performing a dry-run Server Side Apply of the rendered manifests of a release. When drift is detected, the controller will emit an Event and recreate and/or patch the Kubernetes resources.
💥 Current limitations
--log-levelis set todebug. In the Kubernetes Event, only the creation or change of a Kubernetes resource is reported with a brief summary of the changes.flux diffcommand available (yet) to manually inspect or detect drift.📚 References
☎️ Request for Feedback
Please note that this feature is still in its early stages and lacks certain UX features. However, we encourage you to try it out and provide feedback on your experience with it, including any issues you encounter or suggestions for improvements.
Thank you for your help in making the controller even more powerful and reliable!
The text was updated successfully, but these errors were encountered: