search

kubernetes-sigs / cluster-api

Home for Cluster API, a subproject of sig-cluster-lifecycle
https://cluster-api.sigs.k8s.io
Apache License 2.0
3.44k stars 1.26k forks source link

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

Open harveyxia opened 2 years ago

harveyxia commented 2 years ago

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:

killianmuldoon commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

/kind api-change /milestone next

k8s-ci-robot commented 2 years ago

@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](https://github.com/kubernetes-sigs/cluster-api/issues/6416#issuecomment-1103771455): >/kind api-change >/milestone next Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
sbueringer commented 2 years ago

Sounds good to me (in the next apiVersion)

harveyxia commented 2 years ago

FYI this is also an issue for the AWSCluster object

killianmuldoon commented 1 year ago

/area api

k8s-triage-robot commented 1 year ago

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:

You can:

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

/lifecycle stale

fabriziopandini commented 1 year ago

/triage accepted

sbueringer commented 1 year ago

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 commented 1 year ago

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:

You can:

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

/lifecycle rotten

fabriziopandini commented 1 year ago

/lifecycle frozen

k8s-triage-robot commented 5 months ago

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

You can:

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

/remove-triage accepted

fabriziopandini commented 2 months ago

/priority important-longterm

fabriziopandini commented 2 months ago

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