@kubernetes/sig-api-machinery-bugs

@billpratt: Reiterating the mentions to trigger a notification:
@kubernetes/sig-api-machinery-bugs

looks like this is the istio bug where this came from - istio/istio#5810

cc @sttts @hzxuzhonghu thoughts please

I am looking into it.

/priority important-soon
/milestone v1.11

@k8s-mirror-api-machinery-bugs please feel free to change the priority and/or the milestone

I have a doubt: why enable AlwaysPullImages while inject Istio sidecar with 'IfNotPresent' image pull policy?

That make no sense.

cc @deads2k @sttts

Would putting AlwaysPullImages after mutating webhook fix the issue? In 1.9, it comes after. See https://kubernetes.io/docs/reference/access-authn-authz/admission-controllers/#is-there-a-recommended-set-of-admission-controllers-to-use

Mutating webhooks are run at the end for the reason to be able to tweak everything other stages of the system have done. We cannot arbitrarily move some plugin adhoc somewhere else. This also breaks compatibility for people relying on the old order.

I can see though that some plugins make more sense at the very end. I wonder whether we need some very limited ordering feature (please not priorities) in the webhook API, maybe a binary flag.

Mutating webhooks are run at the end for the reason to be able to tweak everything other stages of the system have done. We cannot arbitrarily move some plugin adhoc somewhere else.

Correct.

I can see though that some plugins make more sense at the very end.

Everyone wants to be at the very end. Istio is likely to run afoul of PSP as well and changing an ordering for PSP will eliminate some power for mutating admission plugins in their ability to reconfigure objects.

It seems more like istio may need to understand the world it's playing in than trying to twiddle the world around istio (of which there are likely to be many and various use-cases).

@kubernetes/sig-api-machinery-misc @liggitt @smarterclayton @lavalamp

Webhooks run after built-in plugins. There is no way to determine the relative ordering. Eventually, we will have to solve that, I think by giving built-ins their own configuration objects (and possibly even making them webhooks).

(Clarification: @sttts and @deads2k did not mean to say that mutating webhooks run after validating webhooks: that's not true, validating webhooks run last.)

The general rule @bgrant0607 has been arguing for is that webhooks should not modify user intent. "User intent" == fields that are already set. There are (at least) two problems with this: you can't tell the difference between a default and user intent. And existing admission plugins (like AlwaysPull) flagrantly violate this rule. But I do think overall life would be better if we could make this the rule.

In this specific case, clearly you cannot enable both AlwaysPull and the Istio sidecar at the same time, no matter what order they are going to be executed in. They have contradictory desires. IMO AlwaysPull is a pretty bad plugin; it should be removed. People who want that behavior can make a webhook enforcing it, which can be more intelligent (e.g., make an exception for images known to be loaded on the node, like Istio's side car).

@liggitt had some thoughts about a two pass approach that he was going to share.

In this specific case, clearly you cannot enable both AlwaysPull and the Istio sidecar at the same time, no matter what order they are going to be executed in.

It may be the case (and I don't know for sure), that the istio plugin didn't express a desire and got a default. However, alwaypullimages is just one case of many. Something like PSP is more likely to be problematic.

[MILESTONENOTIFIER] Milestone Issue Needs Approval

@billpratt @kubernetes/sig-api-machinery-misc

Action required: This issue must have the status/approved-for-milestone label applied by a SIG maintainer. If the label is not applied within 2 days, the issue will be moved out of the v1.11 milestone.

/cc @yliaog @jennybuckley

In this specific case, clearly you cannot enable both AlwaysPull and the Istio sidecar at the same time, no matter what order they are going to be executed in

I also had a hard time understanding if Istio cared about the pull policy of the container it injects, or if it was just trying to mimic the server-side defaulting behavior.

• If Istio does care and requires PullIfNotPresent, then yes, it cannot coexist with AlwaysPull.
• If it doesn't care, then this is a specific case of two components having opinions about different fields in the same part of an object, which seems useful/important to solve.

IMO AlwaysPull is a pretty bad plugin; it should be removed.

It is currently the only way to ensure imagePullSecrets at the pod level are limited to providing access to images for that pod. A node/container-runtime-level alternative would be welcome, but is not forthcoming.

In any case, assuming Istio doesn't actually care about the pull policy, this issue isn't specific to AlwaysPull. An identical scenario would be encountered with an admission plugin that set or required runAsUser to be a specific value on all containers in the pod.

Solutions that come to mind:

• Manual ordering of plugins relative to each other. This is very likely to result in partial orders, and requires plugins be extremely granular (even to the point of breaking up a single coherent plugin that touches a couple parts of an object, just so those individual operations can be painstakingly placed in the global order)
• Automatic ordering of plugins based on the parts of the object they touch. For example, admission that adds pod containers would get run before plugins that modify subfields of containers. This doesn't seem feasible to express, and runs into the same granularity issues as manual ordering.
• Multiple mutating admission passes. In the case where a mutating admission plugin makes structural additions (e.g. adding a new container), we could rerun the mutating phase to give all plugins visibility to the new substruct, and the opportunity to enforce/default/reject fields on that substruct.

Multiple mutating admission passes. In the case where a mutating admission plugin makes structural additions (e.g. adding a new container), we could rerun the mutating phase to give all plugins visibility to the new substruct, and the opportunity to enforce/default/reject fields on that substruct.

to address #67050 i would support this. as a webhook dev, i want to do the minimum possible and inherit the controls the system puts in place. In my own request I don't want to have to re-implement the logic from previous admission plugins in my webhook. To this end, I want my mutating webhook to run first so as to be as close to the user's initial request as possible. This way I can inject my changes (update securityContext, add containers, volumes, etc) without having to know what the name of a service account's token is. This strategy leads to:

1. Much more readable code
2. More secure webhooks
3. Better reuse of webhook code

As an example, in order to inject the kerberos sidecar in our webhook we had to generate the following patches in OPA:

krb_sidecar_patch = [
    {
        "op": "replace",
        "path": "/spec/serviceAccountName",
        "value": "run-as-user"
    },
    {
        "op": "replace",
        "path": "/spec/imagePullSecrets/0/name",
        "value": user_data["sa-dockercfg"]
    },
    {
        "op": "replace",
        "path": "/spec/volumes/0/name",
        "value": user_data["sa-token"]
    },
    {
        "op": "replace",
        "path": "/spec/volumes/0/secret/secretName",
        "value": user_data["sa-token"]
    },
    {
        "op": "replace",
        "path": "/metadata/annotations/openshift.io~1scc",
        "value": sprintf("run-as-%s",[kerb_sidecar_data.uid])
    },
    {
        "op": "add",
        "path": "/spec/securityContext/runAsUser",
        "value": kerb_sidecar_data.uidnumber
    },
    {
        "op": "replace",
        "path": "/spec/containers/0/securityContext/runAsUser",
        "value": kerb_sidecar_data.uidnumber
    },
    {
        "op": "replace",
        "path": "/spec/containers/0/volumeMounts/0/name",
        "value": user_data["sa-token"]
    },
    {
        "op": "add",
        "path": "/spec/containers/1",
        "value": {
            "resources": null,
            "image": "mlbiam/krbsidecar",
            "name": "sidecar",
            "env": [
                {
                    "name": "USER_PRINCIPAL",
                    "value": kerb_sidecar_data.userPrincipalName
                },
                {
                    "name": "KRB5_CONFIG",
                    "value": "/etc/krb-conf/krb5.conf"
                }
            ],
            "volumeMounts": [
                {
                    "name": "krb-kt",
                    "mountPath": "/krb5"
                },
                {
                    "name": "krb-conf",
                    "mountPath": "/etc/krb-conf"
                },
                {
                    "name": "dshm",
                    "mountPath": "/dev/shm"
                }
            ],
            "imagePullPolicy": "Always",
            "terminationMessagePolicy": "File",
            "terminationMessagePath": "/dev/termination-log"
        }
    },
    {
        "op": "add",
        "path": "/spec/containers/0/env",
        "value": [
            {
                "name": "KRB5_CONFIG",
                "value": "/etc/krb-conf/krb5.conf"
            }
        ]
    },
    {
        "op": "add",
        "path": "/spec/containers/0/volumeMounts/1",
        "value": {
            "name": "krb-conf",
            "mountPath": "/etc/krb-conf"
        }
    },
    {
        "op": "add",
        "path": "/spec/containers/0/volumeMounts/2",
        "value": {
            "name": "dshm",
            "mountPath": "/dev/shm"
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/1",
        "value": {
            "name": "krb-kt",
            "hostPath": {
                "path": sprintf("/home/%s/%s/keytab",[kerb_sidecar_data.domain,kerb_sidecar_data.uid]),
                "type": "Directory"
            }
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/2",
        "value": {
            "name": "krb-conf",
            "configMap": {
                "name": "krb5-config"
            }
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/3",
        "value": {
            "name": "dshm",
            "emptyDir": {
                "medium": "Memory"
            }
        }
    },
    {
        "op": "replace",
        "path": "/spec/serviceAccount",
        "value": "run-as-user"
    },
    {
        "op": "add",
        "path": "/metadata/annotations/com.tremolosecurity.openshift.krb5_sidecar.added",
        "value": "krb5_sidecar"
    }
]

where as if the webhook could be run at the beginning of the chain my code would be:

krb_sidecar_patch = [
    {
        "op": "replace",
        "path": "/spec/serviceAccountName",
        "value": "run-as-user"
    },
    
    {
        "op": "add",
        "path": "/spec/securityContext/runAsUser",
        "value": kerb_sidecar_data.uidnumber
    },
    
    {
        "op": "add",
        "path": "/spec/containers/1",
        "value": {
            "resources": null,
            "image": "mlbiam/krbsidecar",
            "name": "sidecar",
            "env": [
                {
                    "name": "USER_PRINCIPAL",
                    "value": kerb_sidecar_data.userPrincipalName
                },
                {
                    "name": "KRB5_CONFIG",
                    "value": "/etc/krb-conf/krb5.conf"
                }
            ],
            "volumeMounts": [
                {
                    "name": "krb-kt",
                    "mountPath": "/krb5"
                },
                {
                    "name": "krb-conf",
                    "mountPath": "/etc/krb-conf"
                },
                {
                    "name": "dshm",
                    "mountPath": "/dev/shm"
                }
            ],
            "imagePullPolicy": "Always",
            "terminationMessagePolicy": "File",
            "terminationMessagePath": "/dev/termination-log"
        }
    },
    {
        "op": "add",
        "path": "/spec/containers/0/env",
        "value": [
            {
                "name": "KRB5_CONFIG",
                "value": "/etc/krb-conf/krb5.conf"
            }
        ]
    },
    {
        "op": "add",
        "path": "/spec/containers/0/volumeMounts/1",
        "value": {
            "name": "krb-conf",
            "mountPath": "/etc/krb-conf"
        }
    },
    {
        "op": "add",
        "path": "/spec/containers/0/volumeMounts/2",
        "value": {
            "name": "dshm",
            "mountPath": "/dev/shm"
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/1",
        "value": {
            "name": "krb-kt",
            "hostPath": {
                "path": sprintf("/home/%s/%s/keytab",[kerb_sidecar_data.domain,kerb_sidecar_data.uid]),
                "type": "Directory"
            }
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/2",
        "value": {
            "name": "krb-conf",
            "configMap": {
                "name": "krb5-config"
            }
        }
    },
    {
        "op": "add",
        "path": "/spec/volumes/3",
        "value": {
            "name": "dshm",
            "emptyDir": {
                "medium": "Memory"
            }
        }
    },
    {
        "op": "add",
        "path": "/metadata/annotations/com.tremolosecurity.openshift.krb5_sidecar.added",
        "value": "krb5_sidecar"
    }
]

The second block of code eliminates updates to individual container's service accounts and is portable between k8s distros (since its no longer coded to openshift's annotations). It also eliminates the need for me to lookup secret names for specific service accounts leading to fewer possible chances to leak information.

Followup to my comment above (#64333 (comment)):

As part of apply, we've developed the concept of "field sets"; we will be using this to track which fields the user deliberately set (and implicitly, which were defaulted). We probably will use this to indicate in the object which fields each mutating webhook set--we haven't written that part yet.

Next we have a choice:

1. We could reject a webhook's proposed patch if it modifies a field set by the user (or prior webhook?).
2. We could accept the patch and give the webhook ownership of the conflicting field.
3. We could accept the patch and leave the conflicting field owned by the user.

(1) doesn't solve things, it just gives a slightly better error message ("webhook X and Y conflict; talk to your cluster administrator")
(2) means the next time user applies, they'll be warned about the conflict and will have to either modify their config or "force" to override (assuming the field is mutable). This is annoying if the user is a CI/CD system that doesn't force without intervention--we'd prefer to have failed the first time rather than check in a change which then can't re-applied.
(3) means the user doesn't have to force, but will silently override the field's value (if it's mutable).

One example that came up in the discussion was a hypothetical webhook that took an image name:tag and resolved it to a hash, and put that in the image field (of e.g. a deployment's pod template), and didn't re-resolve on an update unless the user changes the image name:tag. Since on an update, webhooks will see the object post-apply, only (3) above allows the user to make a change without forcing every time. I'm not sure how important this use case is.

Anyway if folks have use cases for choices 1 or 2 above, now would be a great time to give us that feedback.

cc @apelisse @jennybuckley

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

/remove-lifecycle stale

@mbohlool @liggitt @kubernetes/sig-api-machinery-misc I see this as an area that needs to be resolved before admission webhooks can move to GA.

To me, it seems as though there are two types of mutating WebHooks:

1. Changing functionality (e.g. Istio adding a sidecar)
2. Conformance (e.g. AlwaysPullImage)

The Conformance WebHooks are typically tied to a Validation WebHook that then validates the object adhere's to policy.

Could the structure of validation AdmissionsWebhooks be modified to contain a reference to a "remediation" MutatingWebHook that that gets executed on failure, and then revalidated. If it still fails, then the object is rejected. This allows for real mutation hooks to be executed before validation (as is currently implemented) but differentiation MHW that only serve to ensure functionality of a Validation WebHook.

Another idea is as follows:
When a Mutating WebHook changes an object, consider it a new submission and require it to go back through all webhooks. This requires an assumption that MWH are idempotent.

Concerns: Would require logic to ensure that objects aren't getting caught in a modification loop with a bad acting/conflicting webhooks.

Pros: It would ensure that every MutatingWebhook has been applied to the object that gets passed to the validation webhooks.

I came across this issue when attempting to use the k8s api from a container injected via a mutating webhook. It failed because the service account token volume mount doesn't exist. I think the reason for this is that the service account admission controller runs before the MAWH.

@runyontr mentioned the idea of dividing MAWH into two categories, how about early and late? Early MAWHs run early and get access to the request as posted by the user. Late MAWHs run late, and see the results of most of the other plugins.

For my usecase, I would use an early MAWH to add another container into the podspec, this would then go through the serviceaccount plugin where both containers would have their service account token volume mounts added.

It seems there are other tools that solve this problem, and we might be able to leverage their implementations

Systemd

https://www.freedesktop.org/software/systemd/man/systemd.unit.html
Add two fields to the WebHook definitions and reference objects that it should run before/after

Before []corev1.ObjectReference
After   []corev1.ObjectReference

chkconfig

https://linux.die.net/man/8/chkconfig

RunLevel might be already captured with the two types of WebHooks (Mutating and Validation) but we may want to split them out and have different RunLevels for each type. Within a type, we could then specify the run position

# What RunLevel to be in.  Valid 0-6
Level  int
# What Position within the RunLevel to be in. Valid >0
Position int

We haven't thought of a good solution yet, where "good" is some combination of:

• solves the problem
• doesn't make the system insanely hard to configure or understand
  • in particular, it mustn't make it impossible to share webhook configurations, i.e., to other people / clusters
• doesn't make the system insanely non-performant

Requiring/permitting a graph (as in systemd) would:

• make the system hard to understand (imagine debugging your webhook flow in this system)
• make it difficult or impossible to share a webhook configuration (it might now reference a bunch of webhooks that aren't installed, and/or fail to reference some which are).
• doesn't solve webhooks which make transitive changes (A will change B will change C will change A)

"Run levels" are easier to understand/explain/predict but still have the latter two problems.

Our current working plan is basically to run every mutating webhook a second time, if a webhook that ran after it changed something. (max 2 runs)

There may be some factors I've forgotten to mention; @liggitt and @mbohlool have convinced me it's a hard problem.

Note that today you can explicitly control the order of webhooks by changing the webhook configuration name and/or combining them into the same configuration object.

fixed by #78080 in 1.15