fix using KNative with ArgoCD (don't set `ownerReferences` for webhooks)

[thesuperzapper]

Background

In 2021 KNative made it so that the ValidatingWebhookConfiguration and MutatingWebhookConfiguration resources were "owned" by the Namespace which KNative is installed into (kanative-serving by default):

• set webhooks ownerReferences to namespace pkg#2098.

This was intended to ensure that users did not leave the webhooks when uninstalling (via deleting the Namespace) and break KNative when they re-installed (because the webhooks are cluster resources, and their backend would not exist and so would fail to validate anything).

Kubernetes has the concept of ownerReferences to indicate the relationships between resources. If an ownerReference sets blockOwnerDeletion, kubernetes will clean up these "child" resources before/after the "parent" resources is deleted (before: foreground delete, after: background delete).

For example, a Pod owned by a ReplicaSet might have the following ownerReferences:

apiVersion: v1
kind: Pod
metadata:
  name: xxxxxx
  namespace: xxxxxx
  ownerReferences:
    - apiVersion: apps/v1
      blockOwnerDeletion: true
      controller: true
      kind: ReplicaSet
      name: xxxxxx-759d8cb89
      uid: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

Whats the problem?

This breaks ArgoCD, a very widely used GitOps system for Kubernetes.

Specifically, ArgoCD will never remove a resource that has ownerReferences set, so the issue we were trying to prevent actually happens 100% of the time when deploying KNative with ArgoCD.

Here are some related upstream issues:

• Resources with custom OwnerReference not deleted when removed from Helm Chart argoproj/argo-cd#4764
• Ignore Owner References argoproj/argo-cd#11972
• Respect controller flag in owner references argoproj/argo-cd#12210

What's the solution?

I propose we make two changes:

1. Add a config which will disable the behavior of setting the ownerReferences:
   • For example, an environment variable like KNATIVE_DISABLE_WEBHOOK_OWNER which can be set on all controller pods.
2. When we do set ownerReferences, we should NOT be setting controller=true:
   • This is much less likely to break downstream projects (obviously the Namespace is not the controlling resource). It also allows KNative to work with the upstream patch for ArgoCD that ignores non-controller ownerReferences.

Where is the relevant code?

The code which sets the ownerReferences lives in the knative-pkg libraries:

• webhook/resourcesemantics/validation/reconcile_config.go
• webhook/configmaps/configmaps.go
• webhook/resourcesemantics/defaulting/defaulting.go

[thesuperzapper]

@dprotaso @skonto @creydr thanks for you work on KNative! I wanted to highlight this important issue as it causes KNative to break when deployed with ArgoCD because the webhooks are not removed on uninstall.

[dprotaso]

Hey @thesuperzapper I wanted to confirm something

Specifically, ArgoCD will never remove a resource that has ownerReferences set, so the issue we were trying to prevent actually happens 100% of the time when deploying KNative with ArgoCD.

Shouldn't ArgoCD delete the namespace (since it has no owner reference) and then this should propagate and then the webhooks should eventually be deleted?

[thesuperzapper]

Hey @thesuperzapper I wanted to confirm something

Specifically, ArgoCD will never remove a resource that has ownerReferences set, so the issue we were trying to prevent actually happens 100% of the time when deploying KNative with ArgoCD.

Shouldn't ArgoCD delete the namespace (since it has no owner reference) and then this should propagate and then the webhooks should eventually be deleted?

@dprotaso there are a few issues with that:

1. Lots of Argo CD users provision namespaces in a separate application (so deleting the kNative app wouldn't delete it, hence the deadlock)
2. Even if the application did contain the namespace, I think Argo CD would try and delete non-namespace resources first, still leading to a deadlock.

[dprotaso]

Unsure if pkg/3095 closes this issue - since there's no end-user knob to disable the owner references

[dprotaso]

Lots of Argo CD users provision namespaces in a separate application (so deleting the kNative app wouldn't delete it, hence the deadlock)

Interesting is this documented anywhere? or is there some guide that recommends this?

Even if the application did contain the namespace, I think Argo CD would try and delete non-namespace resources first, still leading to a deadlock.

So you said before argo wouldn't delete resources with owner references - so does it error out or skip the resource?

[thesuperzapper]

Lots of Argo CD users provision namespaces in a separate application (so deleting the kNative app wouldn't delete it, hence the deadlock)

Interesting is this documented anywhere? or is there some guide that recommends this?

This is a common pattern, especially in app-of-apps deployments.

Either way, ArgoCD is the most widest used gitOps tool in the world, so if we want people to use kNative, we need to be flexible with it.

Even if the application did contain the namespace, I think Argo CD would try and delete non-namespace resources first, still leading to a deadlock.

So you said before argo wouldn't delete resources with owner references - so does it error out or skip the resource?

@dprotaso think you might be slightly misunderstanding.

What is happening is that ArgoCD will never delete the ValidatingWebhookConfiguration / MutatingWebhookConfiguration resources (because they have the owner reference), however, it will delete the other resources, including the back-end services for the webhooks.

This means that because the webhooks have a failurePolicy: Fail, when that the back end is deleted, any operations which match the webhook selectors are now blocked (because the backend is down).

The easiest way to reproduce this is simply create an ArgoCD app for KNative that has the resources-finalizer.argocd.argoproj.io cascading deletion finalizer, sync it, then try delete it.

This cascading delete will deadlock 100% of the time in the way described.

[thesuperzapper]

@dprotaso I have made a follow-up PR in knative/pgk that adds an environment variable to disable the Namespace owner reference from being set in KNative Serving:

• Add WEBHOOK_DISABLE_NAMESPACE_OWNERSHIP env-var pkg#3103

It adds an environment variable called WEBHOOK_DISABLE_NAMESPACE_OWNERSHIP which sets the DisableNamespaceOwnership config that was added by @jonathan-innis in:

• 🧹 Give users the ability to disable namespace ownership of webhook configurations pkg#3095

Next Steps

I think we should also backport it to 1.14 and 1.15 because otherwise its nearly impossible to deploy KNative Serving with ArgoCD. We might also want to add a basic reference to this in the docs (or hope people stumble across this issue).

That means we will need to cherry-pick the two PRs into the release-1.14 and release-1.15 of knative/pgk, as we can't just update the versions used to master.

We need to update the version of knative/pkg used in the following places:

• knative/serving
• knative-extensions/net-istio
• knative-extensions/net-certmanager (needed for 1.14, but this repo has been archived?)
• ? there might be more places we make webhooks