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.55k stars 1.3k forks source link

Current v1beta1 API Types do not define list-type annotation #6504

Open chrischdi opened 2 years ago

chrischdi commented 2 years ago

What steps did you take and what happened:

While implementing #6462 I stumbled across the warning that we do not define list-type for slices in our v1beta1 structs:

API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassPatch,Definitions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Patches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,ClusterClassSpec,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Enum
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,JSONSchemaProps,Required
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineDeploymentVariables,Overrides
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckClass,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckSpec,UnhealthyConditions
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,MachineHealthCheckStatus,Targets
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,NetworkRanges,CIDRBlocks
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchDefinition,JSONPatches
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,PatchSelectorMatchMachineDeploymentClass,Names
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,Topology,Variables
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersClass,MachineDeployments
API rule violation: list_type_missing,sigs.k8s.io/cluster-api/api/v1beta1,WorkersTopology,MachineDeployments 

This should be the same also for occurencies in v1alpha3 and v1alpha4.

What did you expect to happen:

API rule violation warning does not occur

Anything else you would like to add:

Adding the list-type helps to improve support for server-side apply and allows to define the merge-strategy for server-side apply.

Especially when continuing at #6320 results in making use of server-side apply (as currently proposed) works out adding the list-type to the defintions help improving support here.

Setting the list-type may be considered as a breaking change and may require a new api version.

Environment:

/kind bug /area api

sbueringer commented 2 years ago

Sounds good to me, I'm generally +1 to follow the best practice to define the list type.

I personally wouldn't consider this a breaking change. I think depending on how we set it, it could make the merge behavior of kubectl more precise, but I think that's rather a bugfix.

fabriziopandini commented 2 years ago

+1 this goes in the direction of @JoelSpeed suggestions about API conventions /milestone v1.2

JoelSpeed commented 2 years ago

+1

Today, because lists are atomic by default, clients will have to observe the entire list, make their updates, and patch the entire list. By adding these annotations, we won't break the existing usage as they will still allow sending the entire list.

Do we know if any of our controllers are already attempting to use server side apply? The only break I could think of is if any controllers are already using SSA, they will go from owning the list to owning all the items in the list. Others would have to use force to override that. If there is already a controller doing this, I would expect this to have already been a problem though.

This assumes of course that the manager name differs, which I know we haven't resolved yet.

In all though, it should allow the controllers to simplify their logic once they do make the move to SSA

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 /lifecycle frozen

fabriziopandini commented 1 year ago

/remove-kind bug /kind api-change

k8s-triage-robot commented 9 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

sbueringer commented 9 months ago

/triage accepted

fabriziopandini commented 6 months ago

/priority important-longterm