chart changes to fix race condition during helm install

[rajatjindal]

When creating SpinAppExecutor, it fails as the webhook service is not available yet (because spin-operator deployment pods needs to be running, which needs certificate volume, which depends on Certificate resource, which is a post-install hook for helm).

the fix is to create SpinAppExecutor as post-install hook (with weight: "3" to ensure it runs after certificate resource is created).

The fix is

• to pull cert-manager helm chart as a pre-requisite instead of dependency of spin-operator chart
• remove post-install helm hooks from cert-issuer and webhook-cert
• install spin-app-executor as post-install hook (once webhook service is ready)
• install spin-app-executor in default namespace (as our examples refer to default namespace)

I also added a smoke test for this. but it needs a publically available image for the operator (which we publish on ttl.sh as part of PR). we just need to find a way to have helm-install-smoke-test have a dependency on the action that publish ephermal docker image for PR.

[github-actions]

This PR now has an image available for testing:

  ttl.sh/spoopy-operator-pr-:24h

[vdice]

LGTM, thank you @rajatjindal!

[rajatjindal]

I was attempting to write a smoke test for this and realized it may not be the complete fix yet. I will be spending some more time on this today.

[endocrimes]

Needs a rebase (and probably the github workflow updating to point to go1.22)

[rajatjindal]

Needs a rebase (and probably the github workflow updating to point to go1.22)

done

[vdice]

Tested locally and timing issues appear to be resolved but --wait is essential on the helm install command... see associated comment...

[vdice]

We either need to add these same annotations to the canonical source (https://github.com/spinkube/spin-operator/blob/main/config/samples/shim-executor.yaml) or perhaps remove this logic and maintain a separate, chart-specific version. (Otherwise, these changes are lost on the next chart (re-)generation by helmify.)

[vdice]

It seems like if I don't add --wait to the helm install command, I get the timing error:

helm upgrade --install \
		-n spin-operator \
		--create-namespace \
		--set controllerManager.manager.image.repository=vdice/spin-operator \
		--set controllerManager.manager.image.tag=latest \
		spin-operator charts/spin-operator
Release "spin-operator" does not exist. Installing it now.
Error: failed post-install: warning: Hook post-install spin-operator/templates/containerd-shim-spin-executor.yaml failed: 1 error occurred:
	* Internal error occurred: failed calling webhook "mspinappexecutor.kb.io": failed to call webhook: Post "https://spin-operator-webhook-service.spin-operator.svc:443/mutate-core-spinoperator-dev-v1-spinappexecutor?timeout=10s": no endpoints available for service "spin-operator-webhook-service"


make: *** [helm-install] Error 1

Do we ensure docs/CI/etc all use --wait to avoid this error? Or do we look at not including this CR in the helm chart and rather documenting how it needs to be installed in any namespace that users wish to run SpinApps? (Well, we should doc that behavior anyways... looks like this is being tracked by #55)

[vdice]

Re: install->uninstall->install, currently the SpinAppExecutor CR is stuck terminating when deleted (currently tracked in #7 (comment)) so the subsequent install fails with:

Release "spin-operator" does not exist. Installing it now.
Error: failed post-install: warning: Hook post-install spin-operator/templates/containerd-shim-spin-executor.yaml failed: 1 error occurred:
	* object is being deleted: spinappexecutors.core.spinoperator.dev "containerd-shim-spin" already exists

I guess this is a heads up for now? i.e. should resolve once this CR is reliably removed...

[rajatjindal]

yeah, --wait is indeed required if we want the post-install hook of setting up CR succeed. My suggestion would be to add --wait and also make it clear using doc that CR is required in all namespaces where we wish to install spinapps.

one other way could be to make available a helm chart variable "spin-app-namespaces", we can then iterate over that list and install CR automatically during helm release installation. wdyt?

[rajatjindal]

We either need to add these same annotations to the canonical source (https://github.com/spinkube/spin-operator/blob/main/config/samples/shim-executor.yaml) or perhaps remove this logic and maintain a separate, chart-specific version. (Otherwise, these changes are lost on the next chart (re-)generation by helmify.)

👍 thank you for catching this. Made the change to the canonical source.

[vdice]

one other way could be to make available a helm chart variable "spin-app-namespaces", we can then iterate over that list and install CR automatically during helm release installation. wdyt?

That's a creative idea to explore in the future!

[vdice]

Oh, it looks like docs should be updated in spinkube/documentation eg https://github.com/spinkube/documentation/blob/main/content/en/docs/spin-operator/tutorials/deploy-on-azure-kubernetes-service.md

[rajatjindal]

fixes #74 and #20

[vdice]

one other way could be to make available a helm chart variable "spin-app-namespaces", we can then iterate over that list and install CR automatically during helm release installation. wdyt?

That's a creative idea to explore in the future!