canonical / admission-webhook-operator

Admission Webhook Operator
Apache License 2.0
1 stars 4 forks source link

`MutatingWebhook` namespace selector missing, giving it wider scope than intended #15

Closed ca-scribner closed 2 years ago

ca-scribner commented 2 years ago

Upstream Kubeflow applies a namespace selector in their manifests of:

  namespaceSelector:
    matchLabels:
      app.kubernetes.io/part-of: kubeflow-profile

which restricts the admission-webhook to apply only to namespaces which have been created by kubeflow-profiles for users. Currently, we have this webhook to apply to all pods in all namespaces. I think this configuration is why if we break our admission-webook workload (eg: delete the deployment, but do not delete the mutatingwebhook that uses the deployment), it blocks all pods on the cluster from deploying.

We can adopt upstream's configuration by changing our charm's mutatingwebhookconfiguration from:

"mutatingWebhookConfigurations": [
    {
        "name": "admission-webhook",
        "webhooks": [
            {
                "name": "admission-webhook.kubeflow.org",
                "failurePolicy": "Fail",
                "clientConfig": {
                    "caBundle": ca_bundle,
                    "service": {
                        "name": hookenv.service_name(),
                        "namespace": model,
                        "path": "/apply-poddefault",
                        "port": 4443,
                    },
                },
                "objectSelector": {
                    "matchExpressions": [
                        {
                            "key": "juju-app",
                            "operator": "NotIn",
                            "values": ["admission-webhook"],
                        },
                        {
                            "key": "app.kubernetes.io/name",
                            "operator": "NotIn",
                            "values": ["admission-webhook"],
                        },
                        {
                            "key": "juju-operator",
                            "operator": "NotIn",
                            "values": ["admission-webhook"],
                        },
                        {
                            "key": "operator.juju.is/name",
                            "operator": "NotIn",
                            "values": ["admission-webhook"],
                        },
                    ]
                },
                "rules": [
                    {
                        "apiGroups": [""],
                        "apiVersions": ["v1"],
                        "operations": ["CREATE"],
                        "resources": ["pods"],
                    }
                ],
            },
        ],
    }
],

to:

"mutatingWebhookConfigurations": [
    {
        "name": "admission-webhook",
        "webhooks": [
            {
                "name": "admission-webhook.kubeflow.org",
                "failurePolicy": "Fail",
                "clientConfig": {
                    "caBundle": ca_bundle,
                    "service": {
                        "name": hookenv.service_name(),
                        "namespace": model,
                        "path": "/apply-poddefault",
                        "port": 4443,
                    },
                },
                "namespaceSelector": {
                    "matchLabels": {
                        "app.kubernetes.io/part-of": "kubeflow-profile",
                    },
                },
                "rules": [
                    {
                        "apiGroups": [""],
                        "apiVersions": ["v1"],
                        "operations": ["CREATE"],
                        "resources": ["pods"],
                    }
                ],
            },
        ],
    }
],

(remove objectSelector, add namespaceSelector). Strictly speaking, the objectSelector could stay, but I believe it is unnecessary with the namespaceSelector added.

To test this:

exec a shell in the above pod (kubectl exec -it testpod -- bash) and confirm that /var/run/secrets/kubeflow/pipelines/token exists

Something like the procedure above should be added as an integration test so we confirm PodDefaults actually work correctly

ca-scribner commented 2 years ago

An interesting additional test could be creating a pod in an unlabeled namespace (eg: one that shouldn't be in-scope for the mutating webhook) to make sure the poddefault is not applied