✨ Add ClusterClass variable and patch types

[sbueringer]

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:
This PR adds the patch and variable types as discussed in: #5212

It intentionally does not contain any webhook or controller code changes.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[schrej]

I see that there is a naming scheme, but maybe consider calling it TopologyVariable as that makes it easier to understand what it actually is.

[sbueringer]

I could change it, the problem is getting consensus again on the struct names. Same applies to the other suggestions.

I see (at least 3 options):

1. keep the current names
2. your suggestions
3. just drop the Topology and Class suffixes entirely

(I would prefer option 3)

[sbueringer]

@schrej @enxebre @vincepri @fabriziopandini @CecileRobertMichon @MarcelMue WDYT?

[fabriziopandini]

I'm personally for keeping current names because those name were discussed at length in the proposal/in the google doc predating the proposal and they are nicely consistent with the suffix used for all the Topology/Class types.

[sbueringer]

I'm also absolutely fine with the current implementation. And as you said we already discussed it on the proposal and I would also really like to avoid loosing too much time by waiting for consensus again.

[sbueringer]

Further discussed in Slack and Zoom.

Result is the following:

spec:
  variables: # []ClusterClassVariable
  - name: clusterIdentityName
    required: true
    schema: # VariableSchema
      openAPIV3Schema: # JSONSchemaProps
        type: string
        default: cluster-identity
  patches: # []ClusterClassPatch
  - name: clusterName
    definitions: # []PatchDefinition
    - selector: # PatchSelector
        apiVersion: controlplane.cluster.x-k8s.io/v1beta1
        kind: KubeadmControlPlaneTemplate
        matchResources: # PatchSelectorMatch
          machineDeploymentClass: # PatchSelectorMatchMachineDeploymentClass
            names:
            - default-worker
      jsonPatches: # []JSONPatchDefinition
      - op: replace
        path: "/spec/template/spec/kubeadmConfigSpec/clusterConfiguration/controllerManager/extraArgs/cluster-name"
        valueFrom: # JSONPatchDefinitionValue
          variable: builtin.cluster.name

spec:
  topology:
    variables: # []ClusterVariable
    - name: clusterIdentityName
      value: cluster-identity

Reasoning (re-stating some of Fabrizio's points for completeness):

• We should strive for names that mean something.
• Topology/ClusterClass Kinds that already exist and have been released before this proposal are ok. And they cannot be changed anymore without going through the full deprecation cycle.
• Essentially we have two kinds of new structs in this PR:
  • Structs which are specific to the API type where they are used
    • These structs have been prefixed: ClusterClassVariable, ClusterClassPatch and ClusterVariable
  • Structs which are generic and could be reused in other types in the future (at least in theory)
    • In those case we optimized for semantic, understandability, not too-long names. Those are:
      • VariableSchema, JSONSchemaProps
      • PatchDefinition, PatchSelector, PatchSelectorMatch, PatchSelectorMatchMachineDeploymentClass
      • JSONPatchDefinition, JSONPatchDefinitionValue

Note: We don't want to establish any hard rules here. That's just the reasoning for the current case/PR.

[fabriziopandini]

+1 to the proposed solution

[schrej]

Any reason not to drop the Definition so we have JSONPatch and JSONPatchValue? The objects themselves are a definition of something, so it feels redundant.
The rest looks good.

[sbueringer]

We could, but I'm not sure we should. The valueFrom part does not exist in the usual definitions of what a JSON patch is supposed to look like. So it's maybe good to signal with JSONPatchDefinition (related to the other PatchDefinition we have) that it's not just a regular JSON patch.

But I would be fine with both.

@fabriziopandini @vincepri WDYT?

[fabriziopandini]

Ok to drop definition from JSONPatchDefinition and JSONPatchDefinitionValue

[vincepri]

/hold

for review

[fabriziopandini]

Overall lgtm pending discussion on fuzzers for JSON fields and type names

[killianmuldoon]

/lgtm

[vincepri]

/hold cancel

[neolit123]

Current (suffix): VariableTopology, VariablesClass, VariableDefinitionClass, VariableDefinitionSchemaClass, PatchClass
Proposed (prefix): TopologyVariable, ClassVariables, ClassVariableDefinition, ClassVariableDefinitionSchema, ClassPatch

the common way is to have the type at the end - e.g. "StorageClass"
https://kubernetes.io/docs/concepts/storage/storage-classes/

[sbueringer]

Current (suffix): VariableTopology, VariablesClass, VariableDefinitionClass, VariableDefinitionSchemaClass, PatchClass
Proposed (prefix): TopologyVariable, ClassVariables, ClassVariableDefinition, ClassVariableDefinitionSchema, ClassPatch

the common way is to have the type at the end - e.g. "StorageClass" https://kubernetes.io/docs/concepts/storage/storage-classes/

I agree for the top-level API type names, if something is a Template or a Class. But it seems to me that non-top-level non-generic structs in the core API are prefixed (sometimes by the top-level type, sometimes by the "context"):

The Kubernetes core APIs follow the prefix pattern, a few examples: PodSpec, PodStatus, PodTemplateSpec, NodeCondition, EndpointSubset, EndPointAddress, EndpointPort, DownwardAPIVolumeSource, CSIVolumeSource, ...

[neolit123]

i might be confusing what Class means here, but i agree with the k8s guide:

The Kubernetes core APIs follow the prefix pattern, a few examples: PodSpec, PodStatus, PodTemplateSpec

Pod is the object and Spec, Status are the types (or object specifics) which are more commonly suffixes.

having Class as an object is a bit confusing in general.

[sbueringer]

i might be confusing what Class means here, but i agree with the k8s guide:

The Kubernetes core APIs follow the prefix pattern, a few examples: PodSpec, PodStatus, PodTemplateSpec

Pod is the object and Spec, Status are the types (or object specifics) which are more commonly suffixes.

having Class as an object is a bit confusing in general.

Yes, there are also a lot more not only Status and Spec (full list of core structs)

[sbueringer]

Thx everyone for the discussion. I summarized the struct renaming in: #5418 (comment) (and adjusted the implementation and proposal accordingly)

Additionally we dropped the ".variables.definitions[].{name,required,schema}" layer. Initially the idea was to use it to make variables extensible so we can later on consume variables from somewhere else. This was inconsistent with ".patches[].definitions[].{selector,jsonPatches}" (e.g. patches was an array, variables was not). We've kept definitions in Patches as we want to be able to have multiple {selector,jsonPatches} in a patch.

We would now (probably) solve consuming external variables and patches by adding something like variablesFrom, patchesFrom.

PTAL @schrej @fabriziopandini @vincepri @neolit123

[fabriziopandini]

great job!
/lgtm

[sbueringer]

s/JSONPatchDefinition/JSONPatch/ + squashed

[fabriziopandini]

/lgtm

[schrej]

lgtm

[sbueringer]

/assign @vincepri

[vincepri]

Could we use an actual random number?

[sbueringer]

Yeah, we could

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment