Issue with Pre-Upgrade Hook in Ziti Router Chart When Using ArgoCD

[jan94]

Hi @qrkourier,

I am in the process of deploying the Ziti router chart via ArgoCD and have encountered an issue with the pre-upgrade hook in the router chart. This issue renders the chart unusable with ArgoCD because ArgoCD cannot distinguish between the Helm hooks pre-install and pre-upgrade (as detailed in this issue and the ArgoCD documentation).

Problem:
ArgoCD initiates the pre-upgrade hook—which depends on the PVC created by the chart—even during the initial deployment. At this stage, no Persistent Volume Claim (PVC) has been created yet, causing the pre-upgrade hook to fail and preventing the deployment from succeeding.

Proposal:
To resolve this issue, I propose adding an option in the chart's values file to omit the identity migration pre-upgrade hook. This hook is only necessary when migration is actually required.

Benefit:

• Allows successful deployment of the Ziti router chart via ArgoCD.

BR
Jan

[qrkourier]

orthogonal to issue #272

[qrkourier]

@jan94,

What if, instead of a surgical toggle for specific resources, we update the docs to emphasize that Helm hooks are not applicable when not deploying the rendered manifest with helm?

For example:

GitOps

You can use Helm to render the manifest and deploy it later with your preferred GitOps toolchain, e.g., Helm => Kustomize => ArgoCD => K8S.

Importantly, life cycle hooks, e.g., pre-upgrade jobs, are not applicable unless you are deploying with Helm. You must render the templates without hooks and supply any missing values or resources when deploying another way, e.g., a GitOps agent.

helm template --no-hooks

---

We could have a GitOps section like this in each chart's README.md.gotmpl, primarily the controller and router, and note any specific considerations for managing Helm releases for that particular chart, e.g., migrating the router's identity.

[jan94]

@qrkourier,
while your proposed solution addresses the problem, it doesn't fully leverage the capabilities of ApplicationSets and Generators (including CustomGenerators) in ArgoCD. Therefore from my point of view, 'pre-rendering' isn't a viable option in this context.

In my scenario, I'm using a list generator in ArgoCD to create Ziti networks. This setup allows me to add an element to a YAML list, which then automatically spins up a new Ziti network within the Kubernetes cluster.

Adopting your solution would require moving all the logic currently managed by ArgoCD's ApplicationSets and Generators into an upstream pipeline. Having this extra upstream pipeline, only to avoid the execution of a helm hook, does not seem justified to me.

Additionally other projects like karpenter and hpe-storage seem to also either implement a toggle or even remove the hook - argoproj/argo-cd#4331

Probably we can move all the chart resources into one yaml file, while separating the resources with document separators (---). This way, one if clause surrounding these resources in one yaml file would be enough. Does that seem less surgical to you ? 😉

[jan94]

But I also think that it would still be beneficial to document the reason why this 'toggle' is there and definitely set the default in a way that the helm hook gets executed.

So in case someone just wants to get the chart deployed and does not care about GitOps practices with tools like ArgoCD, the migration, which the hook is intended for, will take place without requiring the user to change any values.

This would then lead to the chart behaving the same way as before, while giving more experienced users the option to use the chart in more advanced scenarios also :)

[qrkourier]

@jan94 Thanks for clarifying that ArgoCD users are not necessarily running helm template. I take it ArgoCD is consuming the chart directly, so an input value is necessary for immediate compatibility with ArgoCD.

Yes, I prefer a single, broad bool that has the same effect as helm template --no-hooks, e.g. in values.yaml, noHelmHooks: false.

Thank you for solving this problem!

[jan94]

So If I understand you correctly, you want me to move all the resources which should be omitted into one yaml file sth. like pre-upgrade-hook.yaml and in there there should be an if clause checking for .Values.noHelmHooks ? :)

[jan94]

@jan94 Thanks for clarifying that ArgoCD users are not necessarily running helm template. I take it ArgoCD is consuming the chart directly, so an input value is necessary for immediate compatibility with ArgoCD.

Yes, I prefer a single, broad bool that has the same effect as helm template --no-hooks, e.g. in values.yaml, noHelmHooks: false.

Thank you for solving this problem!

I was of the impression that you had already worked with ArgoCD. If I had known that, I could have explained better/more about Argo. :)

You're welcome, as always. :)

[qrkourier]

Hrrm. 🤔

Would it be better to consolidate those templates than to continue with the template file naming convention? It seems simple enough to wrap each resource with a bool condition keying upon noHelmHooks. As a maintainer or contributor, since there's only one "toggle" value for Helm hooks, it's easy to find all occurrences and trace the implications of rendering without hooks. We don't need to move those hook-related resources into a single template file like helm-hooks.yml, do we?

I haven't surveyed how other projects have addressed this issue. If we can use an elegant pattern, that's preferable, even if it means moving around the template files.

[qrkourier]

If you define any Argo CD hooks, all Helm hooks will be ignored.

Argo CD cannot know if it is running a first-time "install" or an "upgrade" - every operation is a "sync'. This means that, by default, apps that have pre-install and pre-upgrade will have those hooks run at the same time.

The Argo CD application controller periodically compares Git state against the live state, running the helm template command to generate the helm manifests. Because the random value is regenerated every time the comparison is made, any application which makes use of the randAlphaNum function will always be in an OutOfSync state. This can be mitigated by explicitly setting a value in the values.yaml or using argocd app set command to overide the value such that the value is stable between each comparison.
Tips from ref:

from ref: https://argo-cd.readthedocs.io/en/stable/user-guide/helm/#helm-hooks

[jan94]

https://argo-cd.readthedocs.io/en/stable/user-guide/helm/#helm-hooks -> yes that is exactly what I was referring to when opening the PR :)

Argo CD cannot know if it is running a first-time "install" or an "upgrade" - every operation is a "sync'. This means that, by default, apps that have pre-install and pre-upgrade will have those hooks run at the same time.

This is basically the issue one has, when working with ArgoCD and helm charts that have pre-install or pre-upgrade hooks.

If you define any Argo CD hooks, all Helm hooks will be ignored.

That seems to work on an "per-resource" level. In theory it is possibly to set an ArgoCD hook on the pre-upgrade job, but the only hook feasible from ArgoCD would be the post-sync one - which is also not the right time to execute that hook in this case of the router-chart.

[jan94]

Hrrm. 🤔

Would it be better to consolidate those templates than to continue with the template file naming convention? It seems simple enough to wrap each resource with a bool condition keying upon noHelmHooks. As a maintainer or contributor, since there's only one "toggle" value for Helm hooks, it's easy to find all occurrences and trace the implications of rendering without hooks. We don't need to move those hook-related resources into a single template file like helm-hooks.yml, do we?

I haven't surveyed how other projects have addressed this issue. If we can use an elegant pattern, that's preferable, even if it means moving around the template files.

I agree, it is not necessary to merge all related resources into one file - I just thought that would be sth. that you prefer, because you meant that the bool seems 'surgical' ;)

I'm am not aware of any more elegant pattern for this use-case.

[jan94]

@qrkourier,
I've implemented the changes we discussed. Looking forward to your feedback!

[qrkourier]

I forgot to bump the router chart version, and apparently, that check doesn't run on fork PRs for some reason.

#296

[qrkourier]

released in https://github.com/openziti/helm-charts/releases/tag/ziti-router-1.1.7