Optional struct literal fields in the API do not allow defaulting them via a Webhook

[alvaroaleman]

Currently we have a set of optional struct literal fields in our API, e.G: (This is an incomplete list):

• cluster-api/pkg/apis/cluster/v1alpha1/cluster_types.go

  Lines 52 to 53 in 19551f1

  	// +optional
  	ProviderConfig ProviderConfig `json:"providerConfig,omitempty"`

• cluster-api/pkg/apis/cluster/v1alpha1/machinedeployment_types.go

  Lines 42 to 43 in 19551f1

  	// +optional
  	Strategy MachineDeploymentStrategy `json:"strategy,omitempty"`

• cluster-api/pkg/apis/cluster/v1alpha1/machine_types.go

  Lines 68 to 69 in 19551f1

  	// +optional
  	Versions MachineVersionInfo `json:"versions,omitempty"`

These are impossible to default via a MutatingAdmissionWebhook, because the only supported patch type currently is Jsonpatch. A jsonpatch is generated by marshalling an object original and an object current, then calculating the json patch for the diff. However the marshaled representation the Webhook that knows the types will always contain an empty object field ({}) for the optional property when it is a literal, even when omitempty is set. Consequently, the resulting Jsonpatch will not try to add the main object but only its children. This patch will then get rejected by the APIServer with an error like doc is missing path: "/spec/strategy/typebecause it doesn't know anything about the main object (here: /spec/strategy).

[roberthbailey]

@pwittrock / @erictune - can you confirm that making these fields pointers is the intended approach with CRDs? I think we followed the convention from [pod] Deployments for making spec.Strategy a literal, and it seems unfortunate to have different conventions for CRD-based types and native types.

[erictune]

I wasn't aware of this specific issue. I see this is a problem.
@caesarxuchao @droot Were you aware of this issue with webhooks not being able to override non-pointer "optional" fields.

The reason in that I and others have said to use pointers for optional is so that controllers, validation, and webhooks can distinguish between intentionally set to zero vs unset. I think some API implementors thought that they could skip the pointer if they could not think of a valid reason for setting to an empty value. But, it seems that is causing a problem.

[erictune]

As I read it, this is not a CRD vs core types issue, right? If I wanted to write a webhook that mutated deployment strategy, then I would have the same problem, right?

[erictune]

For this specific case, are you trying to set to a fixed default value? If so, does defaulting in the openAPI fix the problem?

[alvaroaleman]

@erictune

As I read it, this is not a CRD vs core types issue, right? If I wanted to write a webhook that mutated deployment strategy, then I would have the same problem, right?

Not sure, it may work as the apiserver knows that appv1.Deployment has a .Spec.Strategy field. We would have to test that.

For this specific case, are you trying to set to a fixed default value? If so, does defaulting in the openAPI fix the problem?

Well, not exactly, we want to set spec.Strategy.Type to rolling and if spec.Strategy.Type == "rolling", we want to default spec.Strategy.RollingUpdate.MaxSurge and spec.Strategy.RollingUpdate.MaxUnavailable to 1/0, respectively.

The docs on crd openapi tell me the field defaults [...] can not be set, is that outdated?

[droot]

Were you aware of this issue with webhooks not being able to override non-pointer "optional" fields.

@mengqiy might know the answer to this one since he is more familiar with the webhook machinery.

[mengqiy]

As I read it, this is not a CRD vs core types issue, right?

This will happen to both CRD and core types. This is a pointer vs non-pointer issue, aka round-tripping issue in k8s.

If I wanted to write a webhook that mutated deployment strategy, then I would have the same problem, right?

Not always.
If the mutating webhook reacts on CREATE operation, then YES. The reason is that an empty non-pointer filed will get a zero value after round-tripping, i.e. bytes(in admission review) -> go struct (webhook modifying it) -> bytes (calculating json patch). The empty non-pointer field is treated as ALREADY existing when calculating, but NOT existing when applying the patch.
One fix is using pointer. One Google internal team has seen this issue before, and we have suggested them to use pointer. It seems that works for them so far.

If the mutating webhook reacts on UPDATE operation, then NO. Because calculating and applying patch both see the field as ALREADY exists.

[justinsb]

Is this a bug in the way we are calculating JSON patches?

Perhaps another way of asking the question: if we constructed the JSON patch using a different mechanism, is it possible to express the object initialization and would apiserver correctly apply the patch?

[alvaroaleman]

Perhaps another way of asking the question: if we constructed the JSON patch using a different mechanism, is it possible to express the object initialization and would apiserver correctly apply the patch?

Interesting question. I think its not, because the json patch has to look different depending on if the user passed in .spec.strategy.type: "" or if .spec.strategy was completely unset, this is something we can currently not distinguish.

The only way to make that happen would be to marshal the raw request object into a map[string]interface{} but this would defeat the initial goal of using the webhook to default and/or validate something.

Ideally, we would have a different patch type that is less sensitive to these subtile difference or the option to have the webhook return the whole object instead of a patch. Unfortunately, json patch is the only supported way of altering an object via a MutatingAdmissionWebhook

[roberthbailey]

/assign @alvaroaleman

[mengqiy]

kubernetes-sigs/controller-runtime#256 should be able to help with the issue here.
It will be available in controller-runtime v0.2.0 release.

[alvaroaleman]

That approach is indeed a lot more elegant. I'd vote for just closing this issue then. If someone disagrees, just re-open it.

/close

[k8s-ci-robot]

@alvaroaleman: Closing this issue.

In response to this:

That approach is indeed a lot more elegant. I'd vote for just closing this issue then. If someone disagrees, just re-open it.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.