validations.agones.dev and mutations.agones.dev don't declare side effects

[jonbuckley33]

What happened: validations.agones.dev and mutations.agones.dev fail to declare whether they have any side effects, so dryRun requests fail.

What you expected to happen: validations.agones.dev and mutations.agones.dev should declare whether they have side effects so that dryRun requests can decide whether to include the webhooks.

Anything else we need to know?:

I've looked at the implementations of the webhooks, and it looks like neither have side effects, so I suggest sideEffects: None. If the maintainers can confirm this is true and is expected to hold for the foreseeable future, I'm happy to send a patch adding the yaml.

Environment:

• Agones version: 1.9.0

[markmandel]

Thanks for the report! Just so I am clear, what version of Kubernetes is this an issue on?

[jonbuckley33]

My pleasure! I'm running Kubernetes 1.16. I believe server-side dryrun support was enabled by default in Kubernetes 1.13

[markmandel]

Got it! And what benefit are we looking to gain here from getting dry-run functionality working? Is this a debugging tool, or something else?

Not trying to say we shouldn't make this change, just trying to work through the use case -- exploring all the applicability.

Also, is this causing Agones functionality to fail somehow on some platform? What functionality is it stopping not having this set to sideEffects: None? Do we need to test this somehow in CI?

[verb]

Thanks for looking into this. To add a little more context, dry-run requests are used by kubectl diff to detect whether production has diverged from objects in source configuration, so kubectl diff will fail for requests to resources with webhooks that haven't declared sideEffect to be None or NoneOnDryRun. We use kubectl diff to detect when objects in clusters have changed and should be updated.

[markmandel]

AAAH! Very interesting! I've not seen kubectl diff! That makes perfect sense.

To your original point:

I've looked at the implementations of the webhooks, and it looks like neither have side effects, so I suggest sideEffects: None.

I can't think of any webhooks that do anything other than validate or mutate the AdmissionReview data - so this sounds like it should be fine. A PR would definitely be appreciated!

I'm wondering if this work should get wrapped up with the Move API references from beta to v1 section of #1824 - or just done at the same time?

[jonbuckley33]

I'm wondering if this work should get wrapped up with the Move API references from beta to v1 section of #1824 - or just done at the same time?

Can you explain these options a bit more? Does the decision change how we fix the issue?

[markmandel]

With the next cycles release (1.11.0) we wanted to move the Webhooks definitions from admissionregistration.k8s.io/v1beta1 to admissionregistration.k8s.io/v1 - so just wondering if that all should happen in a single operation with this change.

I.e if you are in there messing with the ValidatingWebhookConfiguration and MutatingWebhookConfiguration - you might as well migrate us to v1 while you are in there 😄

[verb]

fwiw, for the specific case of setting sideEffects to None there's no difference between v1beta1 and v1, so there's no requirement to upgrade first to v1 like there might be if the value changed between the versions.

[markmandel]

Yeah, not saying the work is dependent, just saying it might be nice to do both pieces at once.

Don't have to though!

[jonbuckley33]

Thanks for the fix, Robert! :) (Sorry I didn't get to it before -- I got sidetracked with some other work.)