⚠️ Adjust CC & Cluster controller to block on variable conflicts, deprecate definitionFrom (kubernetes-sigs#10841)

* Deprecate definitionFrom, adjust CC & Cluster controller to block on
conflicts

* fix review findings

Author: sbueringer

api/v1beta1/cluster_types.go

@@ -103,6 +103,8 @@ type Topology struct {
 	// patches. They must comply to the corresponding
 	// VariableClasses defined in the ClusterClass.
 	// +optional
+	// +listType=map
+	// +listMapKey=name
 	Variables []ClusterVariable `json:"variables,omitempty"`
 }
 
@@ -319,10 +321,10 @@ type ClusterVariable struct {
 	// Name of the variable.
 	Name string `json:"name"`
 
-	// DefinitionFrom specifies where the definition of this Variable is from. DefinitionFrom is `inline` when the
-	// definition is from the ClusterClass `.spec.variables` or the name of a patch defined in the ClusterClass
-	// `.spec.patches` where the patch is external and provides external variables.
-	// This field is mandatory if the variable has `DefinitionsConflict: true` in ClusterClass `status.variables[]`
+	// DefinitionFrom specifies where the definition of this Variable is from.
+	//
+	// Deprecated: This field is deprecated, must not be set anymore and is going to be removed in the next apiVersion.
+	//
 	// +optional
 	DefinitionFrom string `json:"definitionFrom,omitempty"`
 
@@ -340,20 +342,26 @@ type ClusterVariable struct {
 type ControlPlaneVariables struct {
 	// Overrides can be used to override Cluster level variables.
 	// +optional
+	// +listType=map
+	// +listMapKey=name
 	Overrides []ClusterVariable `json:"overrides,omitempty"`
 }
 
 // MachineDeploymentVariables can be used to provide variables for a specific MachineDeployment.
 type MachineDeploymentVariables struct {
 	// Overrides can be used to override Cluster level variables.
 	// +optional
+	// +listType=map
+	// +listMapKey=name
 	Overrides []ClusterVariable `json:"overrides,omitempty"`
 }
 
 // MachinePoolVariables can be used to provide variables for a specific MachinePool.
 type MachinePoolVariables struct {
 	// Overrides can be used to override Cluster level variables.
 	// +optional
+	// +listType=map
+	// +listMapKey=name
 	Overrides []ClusterVariable `json:"overrides,omitempty"`
 }
 

api/v1beta1/zz_generated.openapi.go

@@ -96,6 +96,7 @@ status:
                 description: "comma-separated list of machine or domain names excluded from using the proxy."
       - name: http-proxy
         # definitionsConflict is true if there are non-equal definitions for a variable.
+        # Note: This conflict has to be resolved, until then corresponding Clusters are not reconciled.
         definitionsConflict: true
         definitions:
           - from: inline
@@ -121,13 +122,15 @@ Variables that are defined by an external DiscoverVariables hook will have the n
 Variables that are defined in the ClusterClass `.spec.variables` will have `inline` as the value of `from`.
 Note: `inline` is a reserved name for patches. It cannot be used as the name of an external patch to avoid conflicts.
 
-If all variables that share a name have equivalent schemas the variable definitions are not in conflict. These variables can
-be set without providing `definitionFrom` value - [see below](#setting-values-for-variables-in-the-cluster). The CAPI components will
-consider variable definitions to be equivalent when they share a name and their schema is exactly equal.
+If all variables that share a name have equivalent schemas the variable definitions are not in conflict. The CAPI components will
+consider variable definitions to be equivalent when they share a name and their schema is exactly equal. If variables are in conflict
+the `VariablesReconciled` will be set to false and the conflict has to be resolved. While there are variable conflicts, corresponding
+Clusters will **not be reconciled**.
+
+Note: We enforce that variable conflicts have to be resolved by ClusterClass authors, so that defining Cluster topology is as simply as possible for end users.
 
 ### Setting values for variables in the Cluster
-Setting variables that are defined with external variable definitions requires attention to be paid to variable definition conflicts, as exposed in the ClusterClass status. 
-Variable values are set in Cluster `.spec.topology.variables`.
+Variables that are defined with external variable definitions can be set like regular variables in Cluster `.spec.topology.variables`.
 
 ```yaml
 apiVersion: cluster.x-k8s.io/v1beta1
@@ -136,18 +139,10 @@ kind: Cluster
 spec:
     topology:
       variables:
-        # `definitionFrom` is not needed as this variable does not have conflicting definitions.
         - name: no-proxy
           value: "internal.domain.com"
-        # variables with the same name but different definitions require values for each individual schema.
         - name: http-proxy
-          definitionFrom: inline
           value: http://proxy.example2.com:1234
-        - name: http-proxy
-          definitionFrom: lbImageRepository
-          value:
-            host: proxy.example2.com
-            port: 1234
 ```
 
 ## Using one or multiple external patch extensions

config/crd/bases/cluster.x-k8s.io_clusters.yaml

@@ -284,6 +284,21 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 		return statusVarList[i].Name < statusVarList[j].Name
 	})
 	clusterClass.Status.Variables = statusVarList
+
+	variablesWithConflict := []string{}
+	for _, v := range clusterClass.Status.Variables {
+		if v.DefinitionsConflict {
+			variablesWithConflict = append(variablesWithConflict, v.Name)
+		}
+	}
+
+	if len(variablesWithConflict) > 0 {
+		err := fmt.Errorf("the following variables have conflicting schemas: %s", strings.Join(variablesWithConflict, ","))
+		conditions.MarkFalse(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition, clusterv1.VariableDiscoveryFailedReason, clusterv1.ConditionSeverityError,
+			"VariableDiscovery failed: %s", err)
+		return errors.Wrapf(err, "failed to discover variables for ClusterClass %s", clusterClass.Name)
+	}
+
 	conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition)
 	return nil
 }