From 8bb26c42ce5213260a82fe9b288c786c1dedbffb Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Wed, 13 Nov 2024 11:16:40 +0100 Subject: [PATCH] Add v1beta2 conditions to ClusterClass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- api/v1beta1/clusterclass_types.go | 25 ++- api/v1beta1/condition_consts.go | 4 + .../clusterclass/clusterclass_controller.go | 141 ++++++++++------ .../clusterclass_controller_status.go | 107 ++++++++++++ .../clusterclass_controller_status_test.go | 157 ++++++++++++++++++ .../clusterclass_controller_test.go | 123 ++++---------- 6 files changed, 416 insertions(+), 141 deletions(-) create mode 100644 internal/controllers/clusterclass/clusterclass_controller_status.go create mode 100644 internal/controllers/clusterclass/clusterclass_controller_status_test.go diff --git a/api/v1beta1/clusterclass_types.go b/api/v1beta1/clusterclass_types.go index ba4ed2513cf9..1f7984fd00f1 100644 --- a/api/v1beta1/clusterclass_types.go +++ b/api/v1beta1/clusterclass_types.go @@ -28,17 +28,40 @@ import ( // ClusterClassKind represents the Kind of ClusterClass. const ClusterClassKind = "ClusterClass" -// Conditions that will be used for the ClusterClass object in v1Beta2 API version. +// ClusterClass VariablesReady condition and corresponding reasons that will be used in v1Beta2 API version. const ( // ClusterClassVariablesReadyV1Beta2Condition is true if the ClusterClass variables, including both inline and external // variables, have been successfully reconciled and thus ready to be used to default and validate variables on Clusters using // this ClusterClass. ClusterClassVariablesReadyV1Beta2Condition = "VariablesReady" + // ClusterClassVariablesReadyV1Beta2Reason surfaces that the variables are ready. + ClusterClassVariablesReadyV1Beta2Reason = "VariablesReady" + + // ClusterClassVariablesReadyVariableDiscoveryFailedV1Beta2Reason surfaces that variable discovery failed. + ClusterClassVariablesReadyVariableDiscoveryFailedV1Beta2Reason = "VariableDiscoveryFailed" +) + +// ClusterClass RefVersionsUpToDate condition and corresponding reasons that will be used in v1Beta2 API version. +const ( // ClusterClassRefVersionsUpToDateV1Beta2Condition documents if the references in the ClusterClass are // up-to-date (i.e. they are using the latest apiVersion of the current Cluster API contract from // the corresponding CRD). ClusterClassRefVersionsUpToDateV1Beta2Condition = "RefVersionsUpToDate" + + // ClusterClassRefVersionsUpToDateV1Beta2Reason surfaces that the references in the ClusterClass are + // up-to-date (i.e. they are using the latest apiVersion of the current Cluster API contract from + // the corresponding CRD). + ClusterClassRefVersionsUpToDateV1Beta2Reason = "RefVersionsUpToDate" + + // ClusterClassRefVersionsNotUpToDateV1Beta2Reason surfaces that the references in the ClusterClass are not + // up-to-date (i.e. they are not using the latest apiVersion of the current Cluster API contract from + // the corresponding CRD). + ClusterClassRefVersionsNotUpToDateV1Beta2Reason = "RefVersionsNotUpToDate" + + // ClusterClassRefVersionsUpToDateInternalErrorV1Beta2Reason surfaces that an unexpected error occurred when validating + // if the references are up-to-date. + ClusterClassRefVersionsUpToDateInternalErrorV1Beta2Reason = InternalErrorV1Beta2Reason ) // +kubebuilder:object:root=true diff --git a/api/v1beta1/condition_consts.go b/api/v1beta1/condition_consts.go index 19fe608b71d1..b2797402e8e6 100644 --- a/api/v1beta1/condition_consts.go +++ b/api/v1beta1/condition_consts.go @@ -353,4 +353,8 @@ const ( // up-to-date (i.e. they are not using the latest apiVersion of the current Cluster API contract from // the corresponding CRD). ClusterClassOutdatedRefVersionsReason = "OutdatedRefVersions" + + // ClusterClassRefVersionsUpToDateInternalErrorReason (Severity=Warning) surfaces that an unexpected error occurred when validating + // if the references are up-to-date. + ClusterClassRefVersionsUpToDateInternalErrorReason = "InternalError" ) diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index da3c2d40ed80..84c26875e630 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/internal/controllers/clusterclass/clusterclass_controller.go @@ -47,7 +47,7 @@ import ( "sigs.k8s.io/cluster-api/feature" runtimeclient "sigs.k8s.io/cluster-api/internal/runtime/client" "sigs.k8s.io/cluster-api/internal/topology/variables" - "sigs.k8s.io/cluster-api/util/conditions" + "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conversion" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/paused" @@ -94,7 +94,7 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt return nil } -func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) { +func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (retres ctrl.Result, reterr error) { clusterClass := &clusterv1.ClusterClass{} if err := r.Client.Get(ctx, req.NamespacedName, clusterClass); err != nil { if apierrors.IsNotFound(err) { @@ -112,14 +112,30 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re return ctrl.Result{}, nil } + s := &scope{ + clusterClass: clusterClass, + } + patchHelper, err := patch.NewHelper(clusterClass, r.Client) if err != nil { return ctrl.Result{}, err } defer func() { + updateStatus(ctx, s) + + patchOpts := []patch.Option{ + patch.WithOwnedConditions{Conditions: []clusterv1.ConditionType{ + clusterv1.ClusterClassRefVersionsUpToDateCondition, + clusterv1.ClusterClassVariablesReconciledCondition, + }}, + patch.WithOwnedV1Beta2Conditions{Conditions: []string{ + clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + clusterv1.ClusterClassVariablesReadyV1Beta2Condition, + }}, + } + // Patch ObservedGeneration only if the reconciliation completed successfully - patchOpts := []patch.Option{} if reterr == nil { patchOpts = append(patchOpts, patch.WithStatusObservedGeneration{}) } @@ -127,25 +143,63 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re reterr = kerrors.NewAggregate([]error{reterr, err}) return } + + if reterr != nil { + retres = ctrl.Result{} + } }() - return ctrl.Result{}, r.reconcile(ctx, clusterClass) + + reconcileNormal := []clusterClassReconcileFunc{ + r.reconcileExternalReferences, + r.reconcileVariables, + } + return doReconcile(ctx, reconcileNormal, s) } -func (r *Reconciler) reconcile(ctx context.Context, clusterClass *clusterv1.ClusterClass) error { - if err := r.reconcileVariables(ctx, clusterClass); err != nil { - return err +type clusterClassReconcileFunc func(context.Context, *scope) (ctrl.Result, error) + +func doReconcile(ctx context.Context, phases []clusterClassReconcileFunc, s *scope) (ctrl.Result, error) { + res := ctrl.Result{} + errs := []error{} + for _, phase := range phases { + // Call the inner reconciliation methods. + phaseResult, err := phase(ctx, s) + if err != nil { + errs = append(errs, err) + } + if len(errs) > 0 { + continue + } + res = util.LowestNonZeroResult(res, phaseResult) } - outdatedRefs, err := r.reconcileExternalReferences(ctx, clusterClass) - if err != nil { - return err + + if len(errs) > 0 { + return ctrl.Result{}, kerrors.NewAggregate(errs) } - reconcileConditions(clusterClass, outdatedRefs) + return res, nil +} - return nil +// scope holds the different objects that are read and used during the reconcile. +type scope struct { + // clusterClass is the ClusterClass object being reconciled. + // It is set at the beginning of the reconcile function. + clusterClass *clusterv1.ClusterClass + + reconcileExternalReferencesError error + outdatedExternalReferences []outdatedRef + + variableDiscoveryError error +} + +type outdatedRef struct { + Outdated *corev1.ObjectReference + UpToDate *corev1.ObjectReference } -func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterClass *clusterv1.ClusterClass) (map[*corev1.ObjectReference]*corev1.ObjectReference, error) { +func (r *Reconciler) reconcileExternalReferences(ctx context.Context, s *scope) (ctrl.Result, error) { + clusterClass := s.clusterClass + // Collect all the reference from the ClusterClass to templates. refs := []*corev1.ObjectReference{} @@ -185,7 +239,7 @@ func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterCla // or MachinePool classes. errs := []error{} reconciledRefs := sets.Set[string]{} - outdatedRefs := map[*corev1.ObjectReference]*corev1.ObjectReference{} + outdatedRefs := []outdatedRef{} for i := range refs { ref := refs[i] uniqueKey := uniqueObjectRefKey(ref) @@ -212,16 +266,25 @@ func (r *Reconciler) reconcileExternalReferences(ctx context.Context, clusterCla errs = append(errs, err) } if ref.GroupVersionKind().Version != updatedRef.GroupVersionKind().Version { - outdatedRefs[ref] = updatedRef + outdatedRefs = append(outdatedRefs, outdatedRef{ + Outdated: ref, + UpToDate: updatedRef, + }) } } if len(errs) > 0 { - return nil, kerrors.NewAggregate(errs) + err := kerrors.NewAggregate(errs) + s.reconcileExternalReferencesError = err + return ctrl.Result{}, err } - return outdatedRefs, nil + + s.outdatedExternalReferences = outdatedRefs + return ctrl.Result{}, nil } -func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clusterv1.ClusterClass) error { +func (r *Reconciler) reconcileVariables(ctx context.Context, s *scope) (ctrl.Result, error) { + clusterClass := s.clusterClass + errs := []error{} allVariableDefinitions := map[string]*clusterv1.ClusterClassStatusVariable{} // Add inline variable definitions to the ClusterClass status. @@ -270,10 +333,10 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust } } if len(errs) > 0 { + err := kerrors.NewAggregate(errs) + s.variableDiscoveryError = errors.Wrapf(err, "VariableDiscovery failed") // 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) + return ctrl.Result{}, errors.Wrapf(err, "failed to discover variables for ClusterClass %s", clusterClass.Name) } statusVarList := []clusterv1.ClusterClassStatusVariable{} @@ -297,37 +360,11 @@ func (r *Reconciler) reconcileVariables(ctx context.Context, clusterClass *clust 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) + s.variableDiscoveryError = errors.Wrapf(err, "VariableDiscovery failed") + return ctrl.Result{}, errors.Wrapf(err, "failed to discover variables for ClusterClass %s", clusterClass.Name) } - conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) - return nil -} - -func reconcileConditions(clusterClass *clusterv1.ClusterClass, outdatedRefs map[*corev1.ObjectReference]*corev1.ObjectReference) { - if len(outdatedRefs) > 0 { - var msg []string - for currentRef, updatedRef := range outdatedRefs { - msg = append(msg, fmt.Sprintf("Ref %q should be %q", refString(currentRef), refString(updatedRef))) - } - conditions.Set( - clusterClass, - conditions.FalseCondition( - clusterv1.ClusterClassRefVersionsUpToDateCondition, - clusterv1.ClusterClassOutdatedRefVersionsReason, - clusterv1.ConditionSeverityWarning, - strings.Join(msg, ", "), - ), - ) - return - } - - conditions.Set( - clusterClass, - conditions.TrueCondition(clusterv1.ClusterClassRefVersionsUpToDateCondition), - ) + return ctrl.Result{}, nil } func addNewStatusVariable(variable clusterv1.ClusterClassVariable, from string) *clusterv1.ClusterClassStatusVariable { @@ -373,7 +410,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste obj, err := external.Get(ctx, r.Client, ref, clusterClass.Namespace) if err != nil { if apierrors.IsNotFound(errors.Cause(err)) { - return errors.Wrapf(err, "Could not find external object for the ClusterClass. 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 ClusterClass. refGroupVersionKind: %s, refName: %s", ref.GroupVersionKind(), ref.Name) } @@ -384,7 +421,7 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, clusterClass *cluste return err } - // Set external object ControllerReference to the ClusterClass. + // Set external object owner reference to the ClusterClass. if err := controllerutil.SetOwnerReference(clusterClass, obj, r.Client.Scheme()); err != nil { return errors.Wrapf(err, "failed to set ClusterClass owner reference for %s %s", obj.GetKind(), klog.KObj(obj)) } diff --git a/internal/controllers/clusterclass/clusterclass_controller_status.go b/internal/controllers/clusterclass/clusterclass_controller_status.go new file mode 100644 index 000000000000..b779029a2f95 --- /dev/null +++ b/internal/controllers/clusterclass/clusterclass_controller_status.go @@ -0,0 +1,107 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package clusterclass + +import ( + "context" + "fmt" + "strings" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/util/conditions" + v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" +) + +func updateStatus(ctx context.Context, s *scope) { + setRefVersionsUpToDateCondition(ctx, s.clusterClass, s.outdatedExternalReferences, s.reconcileExternalReferencesError) + setVariablesReconciledCondition(ctx, s.clusterClass, s.variableDiscoveryError) +} + +func setRefVersionsUpToDateCondition(_ context.Context, clusterClass *clusterv1.ClusterClass, outdatedRefs []outdatedRef, reconcileExternalReferencesError error) { + if reconcileExternalReferencesError != nil { + conditions.MarkUnknown(clusterClass, + clusterv1.ClusterClassRefVersionsUpToDateCondition, + clusterv1.ClusterClassRefVersionsUpToDateInternalErrorReason, + "Please check controller logs for errors", + ) + v1beta2conditions.Set(clusterClass, metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.ClusterClassRefVersionsUpToDateInternalErrorV1Beta2Reason, + Message: "Please check controller logs for errors", + }) + return + } + + if len(outdatedRefs) > 0 { + var msg []string + for _, outdatedRef := range outdatedRefs { + msg = append(msg, fmt.Sprintf("Ref %q should be %q", refString(outdatedRef.Outdated), refString(outdatedRef.UpToDate))) + } + conditions.Set(clusterClass, + conditions.FalseCondition( + clusterv1.ClusterClassRefVersionsUpToDateCondition, + clusterv1.ClusterClassOutdatedRefVersionsReason, + clusterv1.ConditionSeverityWarning, + strings.Join(msg, ", "), + ), + ) + v1beta2conditions.Set(clusterClass, metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.ClusterClassRefVersionsNotUpToDateV1Beta2Reason, + Message: strings.Join(msg, ", "), + }) + return + } + + conditions.Set(clusterClass, + conditions.TrueCondition(clusterv1.ClusterClassRefVersionsUpToDateCondition), + ) + v1beta2conditions.Set(clusterClass, metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Reason, + }) +} + +func setVariablesReconciledCondition(_ context.Context, clusterClass *clusterv1.ClusterClass, variableDiscoveryError error) { + if variableDiscoveryError != nil { + conditions.MarkFalse(clusterClass, + clusterv1.ClusterClassVariablesReconciledCondition, + clusterv1.VariableDiscoveryFailedReason, + clusterv1.ConditionSeverityError, + variableDiscoveryError.Error(), + ) + v1beta2conditions.Set(clusterClass, metav1.Condition{ + Type: clusterv1.ClusterClassVariablesReadyV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.ClusterClassVariablesReadyVariableDiscoveryFailedV1Beta2Reason, + Message: variableDiscoveryError.Error(), + }) + return + } + + conditions.MarkTrue(clusterClass, clusterv1.ClusterClassVariablesReconciledCondition) + v1beta2conditions.Set(clusterClass, metav1.Condition{ + Type: clusterv1.ClusterClassVariablesReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterClassVariablesReadyV1Beta2Reason, + }) +} diff --git a/internal/controllers/clusterclass/clusterclass_controller_status_test.go b/internal/controllers/clusterclass/clusterclass_controller_status_test.go new file mode 100644 index 000000000000..fbfd2d847163 --- /dev/null +++ b/internal/controllers/clusterclass/clusterclass_controller_status_test.go @@ -0,0 +1,157 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package clusterclass + +import ( + "testing" + + . "github.com/onsi/gomega" + "github.com/pkg/errors" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + v1beta2conditions "sigs.k8s.io/cluster-api/util/conditions/v1beta2" +) + +func TestSetRefVersionsUpToDateCondition(t *testing.T) { + testCases := []struct { + name string + outdatedExternalReferences []outdatedRef + reconcileExternalReferencesError error + expectCondition metav1.Condition + }{ + { + name: "error occurred", + reconcileExternalReferencesError: errors.New("failed to set ClusterClass owner reference for KubeadmControlPlaneTemplate test-kcp"), + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionUnknown, + Reason: clusterv1.ClusterClassRefVersionsUpToDateInternalErrorV1Beta2Reason, + Message: "Please check controller logs for errors", + }, + }, + { + name: "some refs are outdated", + outdatedExternalReferences: []outdatedRef{ + { + Outdated: &corev1.ObjectReference{ + APIVersion: "controlplane.cluster.x-k8s.io/v1beta1", + Kind: "KubeadmControlPlaneTemplate", + Name: "test-kcp", + Namespace: metav1.NamespaceDefault, + }, + UpToDate: &corev1.ObjectReference{ + APIVersion: "controlplane.cluster.x-k8s.io/v1beta2", + Kind: "KubeadmControlPlaneTemplate", + Name: "test-kcp", + Namespace: metav1.NamespaceDefault, + }, + }, + { + Outdated: &corev1.ObjectReference{ + APIVersion: "infrastructure.cluster.x-k8s.io/v1beta1", + Kind: "DockerMachineTemplate", + Name: "test-dmt", + Namespace: metav1.NamespaceDefault, + }, + UpToDate: &corev1.ObjectReference{ + APIVersion: "infrastructure.cluster.x-k8s.io/v1beta2", + Kind: "DockerMachineTemplate", + Name: "test-dmt", + Namespace: metav1.NamespaceDefault, + }, + }, + }, + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.ClusterClassRefVersionsNotUpToDateV1Beta2Reason, + Message: "Ref \"controlplane.cluster.x-k8s.io/v1beta1, Kind=KubeadmControlPlaneTemplate default/test-kcp\" should be " + + "\"controlplane.cluster.x-k8s.io/v1beta2, Kind=KubeadmControlPlaneTemplate default/test-kcp\", " + + "Ref \"infrastructure.cluster.x-k8s.io/v1beta1, Kind=DockerMachineTemplate default/test-dmt\" should be " + + "\"infrastructure.cluster.x-k8s.io/v1beta2, Kind=DockerMachineTemplate default/test-dmt\"", + }, + }, + { + name: "all refs are up-to-date", + outdatedExternalReferences: []outdatedRef{}, + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Reason, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + cc := &clusterv1.ClusterClass{} + + setRefVersionsUpToDateCondition(ctx, cc, tc.outdatedExternalReferences, tc.reconcileExternalReferencesError) + + condition := v1beta2conditions.Get(cc, clusterv1.ClusterClassRefVersionsUpToDateV1Beta2Condition) + g.Expect(condition).ToNot(BeNil()) + g.Expect(*condition).To(v1beta2conditions.MatchCondition(tc.expectCondition, v1beta2conditions.IgnoreLastTransitionTime(true))) + }) + } +} + +func TestSetVariablesReconciledCondition(t *testing.T) { + testCases := []struct { + name string + variableDiscoveryError error + expectCondition metav1.Condition + }{ + { + name: "error occurred", + variableDiscoveryError: errors.New("VariableDiscovery failed: patch1.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type: Unsupported value: \"invalidType\": " + + "supported values: \"array\", \"boolean\", \"integer\", \"number\", \"object\", \"string\""), + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterClassVariablesReadyV1Beta2Condition, + Status: metav1.ConditionFalse, + Reason: clusterv1.ClusterClassVariablesReadyVariableDiscoveryFailedV1Beta2Reason, + Message: "VariableDiscovery failed: patch1.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type: Unsupported value: \"invalidType\": " + + "supported values: \"array\", \"boolean\", \"integer\", \"number\", \"object\", \"string\"", + }, + }, + { + name: "variable reconcile succeeded", + expectCondition: metav1.Condition{ + Type: clusterv1.ClusterClassVariablesReadyV1Beta2Condition, + Status: metav1.ConditionTrue, + Reason: clusterv1.ClusterClassVariablesReadyV1Beta2Reason, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + cc := &clusterv1.ClusterClass{} + + setVariablesReconciledCondition(ctx, cc, tc.variableDiscoveryError) + + condition := v1beta2conditions.Get(cc, clusterv1.ClusterClassVariablesReadyV1Beta2Condition) + g.Expect(condition).ToNot(BeNil()) + g.Expect(*condition).To(v1beta2conditions.MatchCondition(tc.expectCondition, v1beta2conditions.IgnoreLastTransitionTime(true))) + }) + } +} diff --git a/internal/controllers/clusterclass/clusterclass_controller_test.go b/internal/controllers/clusterclass/clusterclass_controller_test.go index 2c000c092771..1df78cd48845 100644 --- a/internal/controllers/clusterclass/clusterclass_controller_test.go +++ b/internal/controllers/clusterclass/clusterclass_controller_test.go @@ -461,12 +461,12 @@ func TestReconciler_reconcileVariables(t *testing.T) { }..., ) tests := []struct { - name string - clusterClass *clusterv1.ClusterClass - want []clusterv1.ClusterClassStatusVariable - patchResponse *runtimehooksv1.DiscoverVariablesResponse - wantConditions clusterv1.Conditions - wantErrMessage string + name string + clusterClass *clusterv1.ClusterClass + want []clusterv1.ClusterClassStatusVariable + patchResponse *runtimehooksv1.DiscoverVariablesResponse + wantVariableDiscoveryErrorMessage string + wantErrMessage string }{ { name: "Reconcile inline variables to ClusterClass status", @@ -524,12 +524,6 @@ func TestReconciler_reconcileVariables(t *testing.T) { }, }, }, - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionTrue, - }, - }, }, { name: "Reconcile inline and external variables to ClusterClass status", @@ -742,12 +736,6 @@ func TestReconciler_reconcileVariables(t *testing.T) { }, }, }, - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionTrue, - }, - }, }, { name: "Error if reconciling inline and external variables to ClusterClass status with conflicts", @@ -779,15 +767,7 @@ func TestReconciler_reconcileVariables(t *testing.T) { }, wantErrMessage: "failed to discover variables for ClusterClass class1: " + "the following variables have conflicting schemas: cpu", - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionFalse, - Severity: clusterv1.ConditionSeverityError, - Reason: clusterv1.VariableDiscoveryFailedReason, - Message: "VariableDiscovery failed: the following variables have conflicting schemas: cpu", - }, - }, + wantVariableDiscoveryErrorMessage: "VariableDiscovery failed: the following variables have conflicting schemas: cpu", }, { name: "Reconcile external variables to ClusterClass status", @@ -957,12 +937,6 @@ func TestReconciler_reconcileVariables(t *testing.T) { }, }, }, - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionTrue, - }, - }, }, { name: "Error if external patch defines a variable with same name multiple times", @@ -1002,15 +976,7 @@ func TestReconciler_reconcileVariables(t *testing.T) { wantErrMessage: "failed to discover variables for ClusterClass class1: " + "patch1.variables[cpu].name: Invalid value: \"cpu\": variable name must be unique. " + "Variable with name \"cpu\" is defined more than once", - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionFalse, - Severity: clusterv1.ConditionSeverityError, - Reason: clusterv1.VariableDiscoveryFailedReason, - Message: "VariableDiscovery failed: patch1.variables[cpu].name: Invalid value: \"cpu\": variable name must be unique. Variable with name \"cpu\" is defined more than once", - }, - }, + wantVariableDiscoveryErrorMessage: "VariableDiscovery failed: patch1.variables[cpu].name: Invalid value: \"cpu\": variable name must be unique. Variable with name \"cpu\" is defined more than once", }, { name: "Error if external patch returns an invalid variable (OpenAPI schema)", @@ -1054,16 +1020,8 @@ func TestReconciler_reconcileVariables(t *testing.T) { wantErrMessage: "failed to discover variables for ClusterClass class1: " + "patch1.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type: Unsupported value: \"invalidType\": " + "supported values: \"array\", \"boolean\", \"integer\", \"number\", \"object\", \"string\"", - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionFalse, - Severity: clusterv1.ConditionSeverityError, - Reason: clusterv1.VariableDiscoveryFailedReason, - Message: "VariableDiscovery failed: patch1.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type: Unsupported value: \"invalidType\": " + - "supported values: \"array\", \"boolean\", \"integer\", \"number\", \"object\", \"string\"", - }, - }, + wantVariableDiscoveryErrorMessage: "VariableDiscovery failed: patch1.variables[httpProxy].schema.openAPIV3Schema.properties[noProxy].type: Unsupported value: \"invalidType\": " + + "supported values: \"array\", \"boolean\", \"integer\", \"number\", \"object\", \"string\"", }, { name: "Error if external patch returns an invalid variable (CEL)", @@ -1123,21 +1081,13 @@ func TestReconciler_reconcileVariables(t *testing.T) { "messageExpression compilation failed: ERROR: :1:55: Syntax error: mismatched input 'does' expecting \n " + "| 'Expected integer greater or equal to 1, got ' + this does not compile\n " + "| ......................................................^]", - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionFalse, - Severity: clusterv1.ConditionSeverityError, - Reason: clusterv1.VariableDiscoveryFailedReason, - Message: "VariableDiscovery failed: [patch1.variables[cpu].schema.openAPIV3Schema.properties[nestedField].default: Invalid value: \"integer\": failed rule: self >= 1, " + - "patch1.variables[anotherCPU].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression: Invalid value: " + - "apiextensions.ValidationRule{Rule:\"self >= 1\", Message:\"\", MessageExpression:\"'Expected integer greater or equal to 1, got ' + this does not compile\", " + - "Reason:(*apiextensions.FieldValueErrorReason)(nil), FieldPath:\"\", OptionalOldSelf:(*bool)(nil)}: " + - "messageExpression compilation failed: ERROR: :1:55: Syntax error: mismatched input 'does' expecting \n " + - "| 'Expected integer greater or equal to 1, got ' + this does not compile\n " + - "| ......................................................^]", - }, - }, + wantVariableDiscoveryErrorMessage: "VariableDiscovery failed: [patch1.variables[cpu].schema.openAPIV3Schema.properties[nestedField].default: Invalid value: \"integer\": failed rule: self >= 1, " + + "patch1.variables[anotherCPU].schema.openAPIV3Schema.x-kubernetes-validations[0].messageExpression: Invalid value: " + + "apiextensions.ValidationRule{Rule:\"self >= 1\", Message:\"\", MessageExpression:\"'Expected integer greater or equal to 1, got ' + this does not compile\", " + + "Reason:(*apiextensions.FieldValueErrorReason)(nil), FieldPath:\"\", OptionalOldSelf:(*bool)(nil)}: " + + "messageExpression compilation failed: ERROR: :1:55: Syntax error: mismatched input 'does' expecting \n " + + "| 'Expected integer greater or equal to 1, got ' + this does not compile\n " + + "| ......................................................^]", }, { name: "Error if external patch returns an invalid variable (CEL: using opts that are not available with the current compatibility version)", @@ -1182,22 +1132,14 @@ func TestReconciler_reconcileVariables(t *testing.T) { "ERROR: :1:16: undeclared reference to 'family' (in container '')\n" + " | ip(self).family() == 6\n" + " | ...............^", - wantConditions: clusterv1.Conditions{ - { - Type: clusterv1.ClusterClassVariablesReconciledCondition, - Status: corev1.ConditionFalse, - Severity: clusterv1.ConditionSeverityError, - Reason: clusterv1.VariableDiscoveryFailedReason, - Message: "VariableDiscovery failed: patch1.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].rule: Invalid value: " + - "apiextensions.ValidationRule{Rule:\"ip(self).family() == 6\", Message:\"\", MessageExpression:\"\", Reason:(*apiextensions.FieldValueErrorReason)(nil), FieldPath:\"\", OptionalOldSelf:(*bool)(nil)}: compilation failed: " + - "ERROR: :1:3: undeclared reference to 'ip' (in container '')\n" + - " | ip(self).family() == 6\n" + - " | ..^\n" + - "ERROR: :1:16: undeclared reference to 'family' (in container '')\n" + - " | ip(self).family() == 6\n" + - " | ...............^", - }, - }, + wantVariableDiscoveryErrorMessage: "VariableDiscovery failed: patch1.variables[someIP].schema.openAPIV3Schema.x-kubernetes-validations[0].rule: Invalid value: " + + "apiextensions.ValidationRule{Rule:\"ip(self).family() == 6\", Message:\"\", MessageExpression:\"\", Reason:(*apiextensions.FieldValueErrorReason)(nil), FieldPath:\"\", OptionalOldSelf:(*bool)(nil)}: compilation failed: " + + "ERROR: :1:3: undeclared reference to 'ip' (in container '')\n" + + " | ip(self).family() == 6\n" + + " | ..^\n" + + "ERROR: :1:16: undeclared reference to 'family' (in container '')\n" + + " | ip(self).family() == 6\n" + + " | ...............^", }, } for _, tt := range tests { @@ -1226,13 +1168,18 @@ func TestReconciler_reconcileVariables(t *testing.T) { g.Expect(utilversion.DefaultComponentGlobalsRegistry.Register(utilversion.DefaultKubeComponent, v, nil)).To(Succeed()) } - err := r.reconcileVariables(ctx, tt.clusterClass) + s := &scope{ + clusterClass: tt.clusterClass, + } + _, err := r.reconcileVariables(ctx, s) - // Cleanup condition timestamps for easier comparison. - for i := range tt.clusterClass.Status.Conditions { - tt.clusterClass.Status.Conditions[i].LastTransitionTime = metav1.Time{} + if tt.wantVariableDiscoveryErrorMessage != "" { + g.Expect(s.variableDiscoveryError).To(HaveOccurred()) + g.Expect(s.variableDiscoveryError.Error()).To(Equal(tt.wantVariableDiscoveryErrorMessage)) + } else { + g.Expect(s.variableDiscoveryError).ToNot(HaveOccurred()) } - g.Expect(tt.wantConditions).To(Equal(tt.clusterClass.Status.Conditions)) + if tt.wantErrMessage != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(Equal(tt.wantErrMessage))