Cluster's spec.ControlPlaneEndpoint is marked optional but not a pointer type

[harveyxia]

The Cluster CRD's spec.ControlPlaneEndpoint is marked as optional and should be a pointer type (source code here).

This has implications for consumers of the API that are programmatically generating the object. The field is managed by the infrastructure provider controller and thus should not be touched by the user. However, given that the type is not a pointer, clients are forced to send some value for that field to the kube-apiserver. This can be worked around by first reading the object before issuing an update/patch and using Kubernetes' optimistic concurrency control (i.e. resourceVersion) to detect whether the client's local copy of the object is out of date. But it would be much simpler (and more conventional) to simply make the type a pointer type.

Note that this would be a breaking API change.

References:

• https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required

[killianmuldoon]

Thanks for raising this @harveyxia! - it's definitely a good discussion to have ahead of the next API revision. I don't have the full context for why ControlPlaneEndpoint is a non-pointer, and what the impact of changing it would be so I'll leave that to others.

One note is that though Cluster API is informed by the Kubernetes API conventions the project doesn't follow them strictly - we have our own guidelines for optional pointers here: https://github.com/kubernetes-sigs/cluster-api/blob/main/CONTRIBUTING.md#api-conventions . We assume there that optional types can be either pointers or concrete types.

Secondly, if I'm not wrong, fields marked as optional will end up with Go defaults applied when they're created. e.g. right after creating a Cluster object with no controlPlaneEndpoint I got:

spec:
  controlPlaneEndpoint:
    host: ""
    port: 0

Could you expand more on the issue of needing to send some value to the kubeapi server?

[harveyxia]

Thanks for the fast response!

We are building a controller (in Golang using controller-runtime) to programmatically manage Cluster API objects. Our code represents the objects as Go structs, which are then serialized before sending to the kube-apiserver. Because the ControlPlaneEndpoint is a value type, there is no way for our logic to avoid serializing it. This is problematic because we do not want to override the value set on this field by the infrastructure provider controller.

On the contrary, if it were a pointer type, we could simply omit it from the Go struct instance (or set it to nil), and not worry about overriding the value provided by the infrastructure controller.

As I mentioned, we can work around this problem with the read-modify-update pattern. That being said, it's more conventional to use a pointer value for optional fields, especially for objects like the Cluster whose spec is configured by multiple sources.

[fabriziopandini]

/kind api-change
/milestone next

[k8s-ci-robot]

@fabriziopandini: The provided milestone is not valid for this repository. Milestones in this repository: [Next, v1.1, v1.2]

Use /milestone clear to clear the milestone.

In response to this:

/kind api-change
/milestone next

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.

[sbueringer]

Sounds good to me (in the next apiVersion)

[harveyxia]

FYI this is also an issue for the AWSCluster object

[killianmuldoon]

/area api

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/triage accepted

[sbueringer]

Let's also take a look at CAPD when we get to implementing this issue. We probably should make similar changes in DockerCluster.

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[fabriziopandini]

/lifecycle frozen

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority important-longterm

[fabriziopandini]

Just waiting for when we cut the next API version
/triage accepted