And that the call must not have side effects.

+1, what about something also indicating that a value of DryRun false doesn't guarantee that the object will be persisted, so any side effects should have a reconciliation mechanism, or is that redundant because it's already somewhere in the webhook documentation?

It should be documented, but feel free to state it again here if you think it makes it clearer.

Okay, added this

We don't want to pass on the different sorts of dry run calls?

I don't think we'd want to get more granular than including/excluding an entire phase of admission. That would mean a particular webhook call would only need to know if the request was dryRun or not. Does that seem sufficient?

I'm not sure, I'm not the one arguing that dryRun should be an array of strings...

I think this was discussed on the wg-apply slack channel, and the conclusion was that giving the webhooks the array of strings would be exposing a lot of complexity to them, which they don't necessarily need. Because we don't want to allow the level of granularity to dry run with only some of the admission webhooks, anyway.

Is it really safe to make this optional? Do we need to make a completely new version?

it has to be optional in v1beta1, because it's a new field, and has to default to false, since we can't assume current webhooks handle dry run correctly.

the current plan (in service of pushing all admission webhooks to support dry run) is described in https://github.com/kubernetes/community/blob/master/keps/sig-api-machinery/0015-dry-run.md#admission-controllers:

If dry-run is requested on a non-supported webhook, the request will be completely rejected, as a 400: Bad Request. This field will be defaulted to true and deprecated in v1, and completely removed in v2. All webhooks registered with v2 will be assumed to support dry run.

oops, my comment was actually around the dryRunnable bool on the webhook object. similar reasoning applies to this field though (optional additive field in an existing API version), with the exception of changing defaults and removing it in a future version

Yes, of course if we add it to the existing object it has to be optional.

The comment I'm working on leaving on the other API object will explain my position.

are you proposing to only add dryRun to a v1 (or v1beta2, etc) AdmissionReview object, require webhooks to use the v1 format to indicate support for dry run, and treat all that use the v1beta1 format as not supporting dry run?

upsides:

• simpler (though implicit) signal that the webhook knows about dryrun
• no default false in v1beta1, default true in v1, remove dryRunnable in v2 path required

downsides:

• harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)
• would require rebuilding existing webhooks that have no side effects (and so can support dry run mode without changes) to support a new version

harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)

Why?

Also, if a cluster admin wants to use dry run but some of their webhooks don't support it, they could manually add dryRunnable: true to the webhooks themself if they are willing to accept the risk in doing so

Yeah, this is a foot-gun :)

harder for webhooks to support pre-1.12 and 1.12+ kube (they'd need distinct webhook registration manifests)

Why?

They'd need to request v1beta1 AdmissionReview objects pre-1.12, and v1 AdmissionReview objects in 1.12+. I actually thought we already had the requested AdmissionReview version in the webhook registration object, but it turns out we don't.

Thinking about that a little more, we'll likely want a list of AdmissionReview versions the webhook can accept. Each apiserver, when sending an AdmissionReview object, would find the first version in the webhook's supported list it is able to send. That would let 1.12 and pre-1.12 servers both talk to the same webhook, as long as it supported both v1 and v1beta1 AdmissionReview objects.

if a cluster admin wants to use dry run but some of their webhooks don't support it, they could manually add dryRunnable: true to the webhooks themself if they are willing to accept the risk in doing so

Yeah, this is a foot-gun :)

I don't think it's catastrophic... admission already has to tolerate and recover from being called in cases where the object isn't actually persisted to storage (because of a conflict, etc). telling the admission plugin this is a dry-run operation is largely an optimization that lets it skip eventually reconciling away a side-effect from a non-persisted request.

They'd need to request v1beta1 AdmissionReview ...

Webhooks don't request anything, they get what we send them. I am confused about what you mean.

apiserver can try with the new version and retry with an old version if that fails.

So I understand that it's much more expedient to put this here, but it's IMO the wrong way to do this.

I think this should be solved by revving the admissionreview API version, and checking at runtime if the webhook understands it.

If we put this here, then:

• administrators will be tempted to flip the bit without checking whether the webhook actually supports it (how can they even check?)
• administrators have to flip the bit in every configuration that references the webhook.

Did we decide that the expediency outweighs these considerations?

I think it was done this way more out of consistency with how we add fields to other API types than a concern about expediency.

It doesn't look like we give webhooks a way to indicate what version of AdmissionReview they want today. As soon as we support more than one version, we need that information. I'd be on board with adding that, only adding a dryRun indicator to a new rev of AdmissionReview, and requiring webhooks to request a version of AdmissionReview that supports dryRun.

@jennybuckley's point about this requiring rebuilding admission plugins that currently don't have side effects is a good one. All the conversations we had around this are coming back to me now.

It doesn't look like we give webhooks a way to indicate what version of AdmissionReview they want today.

We don't need to know this. We can try a call with the new version, if it fails, retry with the old version. We don't need a place for that in the configuration.

@jennybuckley's point about this requiring rebuilding admission plugins that currently don't have side effects is a good one.

That's still an argument from expediency.

I'm not arguing we shouldn't do the expedient thing, I just want to know that we actually considered doing the correct thing and are making a good trade off. Given that the correct thing is apparently non-obvious, maybe it is better to just do it this way :/

same question here... requiring a positive match on an enum value we understand that means dry run is safe seems better

did validation get added for this field?

worth adding a TODO here (or in a "v1 graduation criteria" list if we have one) to revisit/remove this default and possibly make the field required when promoting to v1, so we don't forget