A provider cannot tell whether the user wants a Machine to have no taints, or whether the user does not care

[dlipovetsky]

/kind bug

What steps did you take and what happened:
I wanted to create a Machine and have the provider apply no taints to it. My provider uses kubeadm, which applies a default set of taints, unless the user specifies a set of taints, or indicates "no taints" by setting the field (a slice) to its zero value.

What did you expect to happen:
I expect to be able to express all of the following in the Spec.Taints field:
(a) Don't care; the provider can apply default taints.
(b) Apply no taints.
(c) Apply one or more taints I specify.

In the current API, I can't express all of these. My provider can interpret an empty slice to mean either (a) or (b), but not both.

Anything else you would like to add:
If the Spec.Taints field were a pointer to a slice, it would be possible to express both (a) and (b) by attaching different meanings to nil vs. empty slice. [Update: It is possible to express (a), (b), and (c) using the current field. However, both a nil and an empty slice are omitted when the Taints field is marshalled. ]

The desire to apply no taints means that the Spec.Taints field is authoritative. @derekwaynecarr suggested that the field should be additive, not authoritative. Being additive allows automation running concurrently with the provider to set its own set of taints. On the other hand, being authoritative is the only way to say that a Machine should have no taints.

I believe kubeadm has a similar issue with its own API. I filed kubernetes/kubeadm#1358.

[dlipovetsky]

If taints are immutable and are applied once on initialization, as discusssed #651, then I think the authoritative/additive dilemma goes away: taints are authoritative, but only applied once. In that case, I think it would be useful to be able to express both "Don't care" and "Apply no taints."

[dlipovetsky]

I should add that it is possible to set the Spec.Taints field an empty slice or to nil; this information is only lost when the Machine struct is marshalled, as the field will be omitted in both cases.

This means that, when the Machine struct is unmarshalled, the Spec.Taints will be set to nil, even if originally it had been an empty slice.

[dlipovetsky]

Looks like this issue has come up before. Here's what the API conventions say:

In most cases, optional fields should also have the omitempty struct tag (the omitempty option specifies that the field should be omitted from the json encoding if the field has an empty value). However, If you want to have different logic for an optional field which is not provided vs. provided with empty values, do not use omitempty (e.g. kubernetes/kubernetes#34641).

[davidewatson]

Related to #552

[timothysc]

I think you can easily work-around via the k8s api as a post-step.

[dlipovetsky]

The same issue in the kubeadm types is being resolved by removing the omitempty tag from the Taints field in the v1beta2 types.

I'll file a PR (that can be put on hold until after v1alpha1)

[vincepri]

/area api

[dlipovetsky]

The kubeadm bootstrap provider (https://github.com/kubernetes-sigs/cluster-api-bootstrap-provider-kubeadm/) is using the kuebadm v1beta2 types, so it should be free from this problem.

Other bootstrap providers will need to be aware of this issue. Something to keep in mind for the future set of CAPI conformance tests.

Time to close this issue?

[detiber]

+1 to closing