Merge pull request #8076 from killianmuldoon/pr-exvars-validate-on-status

🌱 Default and Validate Cluster variables based on ClusterClass status

Author: k8s-ci-robot

api/v1beta1/condition_consts.go

@@ -55,6 +55,19 @@ const (
 
 // ANCHOR_END: CommonConditions
 
+// Conditions and condition Reasons for the ClusterClass object.
+const (
+	// ClusterClassVariablesReconciledCondition reports if the ClusterClass variables, including both inline and external
+	// variables, have been successfully reconciled.
+	// This signals that the ClusterClass is ready to be used to default and validate variables on Clusters using
+	// this ClusterClass.
+	ClusterClassVariablesReconciledCondition ConditionType = "VariablesReconciled"
+
+	// VariableDiscoveryFailedReason (Severity=Error) documents a ClusterClass with VariableDiscovery extensions that
+	// failed.
+	VariableDiscoveryFailedReason = "VariableDiscoveryFailed"
+)
+
 // Conditions and condition Reasons for the Cluster object.
 
 const (

cmd/clusterctl/client/cluster/assets/topology-test/new-clusterclass-and-cluster.yaml

@@ -10,6 +10,14 @@ metadata:
   name: my-cluster-class
   namespace: default
 spec:
+  variables:
+    - name: imageRepository
+      required: true
+      schema:
+        openAPIV3Schema:
+          type: string
+          default: "registry.k8s.io"
+          example: "registry.k8s.io"
   controlPlane:
     ref:
       apiVersion: controlplane.cluster.x-k8s.io/v1beta1

cmd/clusterctl/client/cluster/topology.go

@@ -39,8 +39,10 @@ import (
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/client/cluster/internal/dryrun"
+	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme"
 	logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log"
 	"sigs.k8s.io/cluster-api/feature"
+	clusterclasscontroller "sigs.k8s.io/cluster-api/internal/controllers/clusterclass"
 	clustertopologycontroller "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster"
 	"sigs.k8s.io/cluster-api/internal/webhooks"
 	"sigs.k8s.io/cluster-api/util/contract"
@@ -134,6 +136,7 @@ func (t *topologyClient) Plan(in *TopologyPlanInput) (*TopologyPlanOutput, error
 	if err := t.prepareInput(ctx, in, c); err != nil {
 		return nil, errors.Wrap(err, "failed preparing input")
 	}
+
 	// Run defaulting and validation on core CAPI objects - Cluster and ClusterClasses.
 	// This mimics the defaulting and validation webhooks that will run on the objects during a real execution.
 	// Running defaulting and validation on these objects helps to improve the UX of using the plan operation.
@@ -397,18 +400,28 @@ func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in
 		return errors.Wrap(err, "failed to run defaulting and validation on ClusterClasses")
 	}
 
-	// From the inputs gather all the objects that are not Clusters.
+	// From the inputs gather all the objects that are not Clusters or ClusterClasses.
 	// These objects will be used when initializing a dryrun client to use in the webhooks.
-	// We want to keep ClusterClasses in the webhook client. This is because validation of
-	// Cluster objects might need access to ClusterClass objects that are in the input.
 	filteredObjs = filterObjects(
 		in.Objs,
 		clusterv1.GroupVersion.WithKind("Cluster"),
+		clusterv1.GroupVersion.WithKind("ClusterClass"),
 	)
+
 	objs = []client.Object{}
 	for _, o := range filteredObjs {
 		objs = append(objs, o)
 	}
+	// Reconcile the ClusterClasses and add the reconciled version of them to the webhook client.
+	// This is required as validation of Cluster objects might need access to ClusterClass objects that are in the input.
+	// Cluster variable defaulting and validation relies on the ClusterClass `.status.variables` which is added
+	// during ClusterClass reconciliation.
+	reconciledClusterClasses, err := t.reconcileClusterClasses(ctx, in.Objs, apiReader)
+	if err != nil {
+		return errors.Wrapf(err, "failed to reconcile ClusterClasses for defaulting and validating")
+	}
+	objs = append(objs, reconciledClusterClasses...)
+
 	webhookClient = dryrun.NewClient(apiReader, objs)
 
 	// Run defaulting and validation on Clusters.
@@ -427,6 +440,108 @@ func (t *topologyClient) runDefaultAndValidationWebhooks(ctx context.Context, in
 	return nil
 }
 
+func (t *topologyClient) reconcileClusterClasses(ctx context.Context, inputObjects []*unstructured.Unstructured, apiReader client.Reader) ([]client.Object, error) {
+	reconciliationObjects := []client.Object{}
+	// From the inputs gather all the objects that are not ClusterClasses.
+	// These objects will be used when initializing a dryrun client to use in the reconciler.
+	for _, o := range filterObjects(inputObjects, clusterv1.GroupVersion.WithKind("ClusterClass")) {
+		reconciliationObjects = append(reconciliationObjects, o)
+	}
+	// Add mock CRDs of all the provider objects in the input to the list used when initializing the client.
+	// Adding these CRDs makes sure that UpdateReferenceAPIContract calls in the reconciler can work.
+	for _, o := range t.generateCRDs(inputObjects) {
+		reconciliationObjects = append(reconciliationObjects, o)
+	}
+
+	// Create a list of all ClusterClasses, including those in the dry run input and those in the management Cluster
+	// API Server.
+	allClusterClasses := []client.Object{}
+	ccList := &clusterv1.ClusterClassList{}
+	// If an APIReader is available add the ClusterClasses from the management cluster
+	if apiReader != nil {
+		if err := apiReader.List(ctx, ccList); err != nil {
+			return nil, errors.Wrap(err, "failed to find ClusterClasses to default and validate Clusters")
+		}
+		for i := range ccList.Items {
+			allClusterClasses = append(allClusterClasses, &ccList.Items[i])
+		}
+	}
+
+	// Add ClusterClasses from the input
+	inClusterClasses := getClusterClasses(inputObjects)
+	cc := clusterv1.ClusterClass{}
+	for _, class := range inClusterClasses {
+		if err := scheme.Scheme.Convert(class, &cc, ctx); err != nil {
+			return nil, errors.Wrapf(err, "failed to convert object %s/%s to ClusterClass", class.GetNamespace(), class.GetName())
+		}
+		allClusterClasses = append(allClusterClasses, &cc)
+	}
+
+	// Each ClusterClass should be reconciled in order to ensure variables are correctly added to `status.variables`.
+	// This is required as Clusters are validated based of variable definitions in the ClusterClass `.status.variables`.
+	reconciledClusterClasses := []client.Object{}
+	for _, class := range allClusterClasses {
+		reconciledClusterClass, err := reconcileClusterClass(apiReader, class, reconciliationObjects)
+		if err != nil {
+			return nil, errors.Wrapf(err, "ClusterClass %s could not be reconciled for dry run", class.GetName())
+		}
+		reconciledClusterClasses = append(reconciledClusterClasses, reconciledClusterClass)
+	}
+
+	// Remove the ClusterClasses from the input objects and replace them with the reconciled version.
+	for i, obj := range inputObjects {
+		if obj.GroupVersionKind() == clusterv1.GroupVersion.WithKind("ClusterClass") {
+			// remove the clusterclass from the list of reconciled clusterclasses if it was not in the input.
+			inputObjects = append(inputObjects[:i], inputObjects[i+1:]...)
+		}
+	}
+	for _, class := range reconciledClusterClasses {
+		obj := &unstructured.Unstructured{}
+		if err := localScheme.Convert(class, obj, nil); err != nil {
+			return nil, errors.Wrapf(err, "failed to convert %s to object", obj.GetKind())
+		}
+		inputObjects = append(inputObjects, obj)
+	}
+
+	// Return a list of successfully reconciled ClusterClasses.
+	return reconciledClusterClasses, nil
+}
+
+func reconcileClusterClass(apiReader client.Reader, class client.Object, reconciliationObjects []client.Object) (*unstructured.Unstructured, error) {
+	targetClusterClass := client.ObjectKey{Namespace: class.GetNamespace(), Name: class.GetName()}
+	reconciliationObjects = append(reconciliationObjects, class)
+
+	// Create a reconcilerClient that has access to all of the necessary templates to complete a successful reconcile
+	// of the ClusterClass.
+	reconcilerClient := dryrun.NewClient(apiReader, reconciliationObjects)
+
+	clusterClassReconciler := &clusterclasscontroller.Reconciler{
+		Client:                    reconcilerClient,
+		APIReader:                 reconcilerClient,
+		UnstructuredCachingClient: reconcilerClient,
+	}
+
+	if _, err := clusterClassReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: targetClusterClass}); err != nil {
+		return nil, errors.Wrap(err, "failed to dry run the ClusterClass controller")
+	}
+
+	// Pull the reconciled ClusterClass using the reconcilerClient, and return the version with the updated status.
+	reconciledClusterClass := &clusterv1.ClusterClass{}
+	if err := reconcilerClient.Get(ctx, targetClusterClass, reconciledClusterClass); err != nil {
+		return nil, fmt.Errorf("could not retrieve ClusterClass")
+	}
+
+	obj := &unstructured.Unstructured{}
+	// Converted the defaulted and validated object back into unstructured.
+	// Note: This step also makes sure that modified object is updated into the
+	// original unstructured object.
+	if err := localScheme.Convert(reconciledClusterClass, obj, nil); err != nil {
+		return nil, errors.Wrapf(err, "failed to convert %s to object", obj.GetKind())
+	}
+
+	return obj, nil
+}
+
 func (t *topologyClient) defaultAndValidateObjs(ctx context.Context, objs []*unstructured.Unstructured, o client.Object, defaulter crwebhook.CustomDefaulter, validator crwebhook.CustomValidator, apiReader client.Reader) error {
 	for _, obj := range objs {
 		// The defaulter and validator need a typed object. Convert the unstructured obj to a typed object.

internal/controllers/clusterclass/clusterclass_controller.go

@@ -131,6 +131,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
 }
 
 func (r *Reconciler) reconcile(ctx context.Context, clusterClass *clusterv1.ClusterClass) (ctrl.Result, error) {
+	if err := r.reconcileVariables(ctx, clusterClass); err != nil {
+		return ctrl.Result{}, err
+	}
+	outdatedRefs, err := r.reconcileExternalReferences(ctx, clusterClass)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
+
+	reconcileConditions(clusterClass, outdatedRefs)
+
+	return ctrl.Result{}, nil
+}
+
+func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterClass *clusterv1.ClusterClass) (map[*corev1.ObjectReference]*corev1.ObjectReference, error) {
 	// Collect all the reference from the ClusterClass to templates.
 	refs := []*corev1.ObjectReference{}
 
@@ -192,28 +206,19 @@ func (r *Reconciler) reconcile(ctx context.Context, clusterClass *clusterv1.Clus
 		}
 	}
 	if len(errs) > 0 {
-		return ctrl.Result{}, kerrors.NewAggregate(errs)
-	}
-
-	// Reconcile variables
-	if err := r.reconcileVariables(ctx, clusterClass); err != nil {
-		return ctrl.Result{}, err
+		return nil, kerrors.NewAggregate(errs)
 	}
-
-	reconcileConditions(clusterClass, outdatedRefs)
-
-	return ctrl.Result{}, nil
+	return outdatedRefs, nil
 }
 
 func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error {
 	errs := []error{}
 	uniqueVars := map[string]bool{}
-
+	allVars := []clusterv1.ClusterClassStatusVariable{}
 	// Add inline variable definitions to the ClusterClass status.
-	clusterClass.Status.Variables = []clusterv1.ClusterClassStatusVariable{}
 	for _, variable := range clusterClass.Spec.Variables {
 		uniqueVars[variable.Name] = true
-		clusterClass.Status.Variables = append(clusterClass.Status.Variables, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
+		allVars = append(allVars, statusVariableFromClusterClassVariable(variable, clusterv1.VariableDefinitionFromInline))
 	}
 
 	// If RuntimeSDK is enabled call the DiscoverVariables hook for all associated Runtime Extensions and add the variables
@@ -244,14 +249,19 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust
 						continue
 					}
 					uniqueVars[variable.Name] = true
-					clusterClass.Status.Variables = append(clusterClass.Status.Variables, statusVariableFromClusterClassVariable(variable, patch.Name))
+					allVars = append(allVars, statusVariableFromClusterClassVariable(variable, patch.Name))
 				}
 			}
 		}
 	}
 	if len(errs) > 0 {
+		// TODO: Decide whether to remove old variables if discovery fails.
+		conditions.MarkFalse(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition, clusterv1.VariableDiscoveryFailedReason, clusterv1.ConditionSeverityError,
+			"VariableDiscovery failed: %s", kerrors.NewAggregate(errs))
 		return errors.Wrapf(kerrors.NewAggregate(errs), "failed to discover variables for ClusterClass %s", clusterClass.Name)
 	}
+	clusterClass.Status.Variables = allVars
+	conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition)
 	return nil
 }
 func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) {
@@ -302,9 +312,9 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
 	obj, err := external.Get(ctx, r.UnstructuredCachingClient, ref, clusterClass.Namespace)
 	if err != nil {
 		if apierrors.IsNotFound(errors.Cause(err)) {
-			return errors.Wrapf(err, "Could not find external object for the cluster class. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name)
+			return errors.Wrapf(err, "Could not find external object for the ClusterClass. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name)
 		}
-		return errors.Wrapf(err, "failed to get the external object for the cluster class. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name)
+		return errors.Wrapf(err, "failed to get the external object for the ClusterClass. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name)
 	}
 
 	// If referenced object is paused, return early.
@@ -321,7 +331,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste
 
 	// Set external object ControllerReference to the ClusterClass.
 	if err := controllerutil.SetOwnerReference(clusterClass, obj, r.Client.Scheme()); err != nil {
-		return errors.Wrapf(err, "failed to set cluster class owner reference for %s", tlog.KObj{Obj: obj})
+		return errors.Wrapf(err, "failed to set ClusterClass owner reference for %s", tlog.KObj{Obj: obj})
 	}
 
 	// Patch the external object.