Fix the paradox description of the klusterlet condition (#290)

Signed-off-by: zhujian <jiazhu@redhat.com>

Signed-off-by: zhujian <jiazhu@redhat.com>

pkg/helpers/helpers.go

@@ -173,6 +173,17 @@ func UpdateKlusterletConditionFn(conds ...metav1.Condition) UpdateKlusterletStat
 	}
 }
 
+// TODO: just for upgrading, we need to remove this in 0.11.0
+func ReplaceKlusterletConditionFn(removeConditionType string, conds ...metav1.Condition) UpdateKlusterletStatusFunc {
+	return func(oldStatus *operatorapiv1.KlusterletStatus) error {
+		meta.RemoveStatusCondition(&oldStatus.Conditions, removeConditionType)
+		for _, cond := range conds {
+			meta.SetStatusCondition(&oldStatus.Conditions, cond)
+		}
+		return nil
+	}
+}
+
 func CleanUpStaticObject(
 	ctx context.Context,
 	client kubernetes.Interface,

pkg/helpers/helpers_test.go

@@ -161,6 +161,99 @@ func TestUpdateStatusCondition(t *testing.T) {
 	}
 }
 
+func TestReplaceKlusterletConditionFn(t *testing.T) {
+	cases := []struct {
+		name               string
+		startingConditions []metav1.Condition
+		newCondition       metav1.Condition
+		removeType         string
+		expectedUpdated    bool
+		expectedConditions []metav1.Condition
+	}{
+		{
+			name: "replace empty",
+			startingConditions: []metav1.Condition{
+				newCondition("two", "True", "my-reason", "my-message", nil),
+			},
+			newCondition:    newCondition("one", "True", "my-reason", "my-message", nil),
+			expectedUpdated: true,
+			expectedConditions: []metav1.Condition{
+				newCondition("two", "True", "my-reason", "my-message", nil),
+				newCondition("one", "True", "my-reason", "my-message", nil),
+			},
+		},
+		{
+			name: "replace an existing type",
+			startingConditions: []metav1.Condition{
+				newCondition("two", "True", "my-reason", "my-message", nil),
+			},
+			newCondition:    newCondition("one", "True", "my-reason", "my-message", nil),
+			removeType:      "two",
+			expectedUpdated: true,
+			expectedConditions: []metav1.Condition{
+				newCondition("one", "True", "my-reason", "my-message", nil),
+			},
+		},
+		{
+			name: "replace a non-existing type",
+			startingConditions: []metav1.Condition{
+				newCondition("two", "True", "my-reason", "my-message", nil),
+			},
+			newCondition:    newCondition("one", "True", "my-reason", "my-message", nil),
+			removeType:      "three",
+			expectedUpdated: true,
+			expectedConditions: []metav1.Condition{
+				newCondition("two", "True", "my-reason", "my-message", nil),
+				newCondition("one", "True", "my-reason", "my-message", nil),
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			fakeOperatorClient := opereatorfake.NewSimpleClientset(
+				&operatorapiv1.ClusterManager{
+					ObjectMeta: metav1.ObjectMeta{Name: "testmanagedcluster"},
+					Status: operatorapiv1.ClusterManagerStatus{
+						Conditions: c.startingConditions,
+					},
+				},
+				&operatorapiv1.Klusterlet{
+					ObjectMeta: metav1.ObjectMeta{Name: "testmanagedcluster"},
+					Status: operatorapiv1.KlusterletStatus{
+						Conditions: c.startingConditions,
+					},
+				},
+			)
+
+			klusterletstatus, updated, err := UpdateKlusterletStatus(
+				context.TODO(),
+				fakeOperatorClient.OperatorV1().Klusterlets(),
+				"testmanagedcluster",
+				ReplaceKlusterletConditionFn(c.removeType, c.newCondition),
+			)
+			if err != nil {
+				t.Errorf("unexpected err: %v", err)
+			}
+			if updated != c.expectedUpdated {
+				t.Errorf("expected %t, but %t", c.expectedUpdated, updated)
+			}
+
+			for i := range c.expectedConditions {
+				expected := c.expectedConditions[i]
+
+				klusterletactual := klusterletstatus.Conditions[i]
+				if expected.LastTransitionTime == (metav1.Time{}) {
+					klusterletactual.LastTransitionTime = metav1.Time{}
+				}
+				if !equality.Semantic.DeepEqual(expected, klusterletactual) {
+					t.Errorf(diff.ObjectDiff(expected, klusterletactual))
+				}
+			}
+		})
+	}
+}
+
 func newCondition(name, status, reason, message string, lastTransition *metav1.Time) metav1.Condition {
 	ret := metav1.Condition{
 		Type:    name,

pkg/operators/clustermanager/controllers/clustermanagercontroller/clustermanager_controller.go

@@ -270,7 +270,7 @@ func (n *clusterManagerController) sync(ctx context.Context, controllerContext f
 	conditions := &clusterManager.Status.Conditions
 
 	// If there are some invalid feature gates of registration, will output
-	// condition `InvalidRegistrationFeatureGates` in ClusterManager.
+	// condition `ValidRegistrationFeatureGates` False in ClusterManager.
 	if clusterManager.Spec.RegistrationConfiguration != nil {
 		featureGates, condition, err := n.checkFeatureGate(ctx, clusterManagerName,
 			clusterManager.Spec.RegistrationConfiguration.FeatureGates, "registration")
@@ -281,7 +281,7 @@ func (n *clusterManagerController) sync(ctx context.Context, controllerContext f
 		meta.SetStatusCondition(conditions, condition)
 	}
 	// If there are some invalid feature gates of work, will output
-	// condition `InvalidWorkFeatureGates` in ClusterManager.
+	// condition `ValidWorkFeatureGates` False in ClusterManager.
 	if clusterManager.Spec.WorkConfiguration != nil {
 		featureGates, condition, err := n.checkFeatureGate(ctx, clusterManagerName,
 			clusterManager.Spec.WorkConfiguration.FeatureGates, "work")

pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller.go

@@ -49,6 +49,7 @@ const (
 	appliedManifestWorkFinalizer = "cluster.open-cluster-management.io/applied-manifest-work-cleanup"
 
 	spokeRegistrationFeatureGatesInvalid = "InvalidRegistrationFeatureGates"
+	spokeRegistrationFeatureGatesValid   = "ValidRegistrationFeatureGates"
 )
 
 var (
@@ -239,22 +240,28 @@ func (n *klusterletController) sync(ctx context.Context, controllerContext facto
 		appliedManifestWorkClient: n.appliedManifestWorkClient,
 	}
 
-	// If there are some invalid feature gates of registration, will output condition `InvalidRegistrationFeatureGates` in Klusterlet.
+	// If there are some invalid feature gates of registration, will output condition `ValidRegistrationFeatureGates` False in Klusterlet.
 	if klusterlet.Spec.RegistrationConfiguration != nil && len(klusterlet.Spec.RegistrationConfiguration.FeatureGates) > 0 {
 		featureGateArgs, invalidFeatureGates := helpers.FeatureGatesArgs(
 			klusterlet.Spec.RegistrationConfiguration.FeatureGates, helpers.ComponentSpokeRegistrationKey)
 		if len(invalidFeatureGates) == 0 {
 			config.RegistrationFeatureGates = featureGateArgs
-			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.UpdateKlusterletConditionFn(metav1.Condition{
-				Type: spokeRegistrationFeatureGatesInvalid, Status: metav1.ConditionTrue, Reason: "FeatureGatesAllValid",
-				Message: "Registration feature gates of klusterlet are all valid",
-			}))
+			// TODO: after the 0.10.0 is released, change back to UpdateKlusterletConditionFn
+			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.ReplaceKlusterletConditionFn(
+				spokeRegistrationFeatureGatesInvalid,
+				metav1.Condition{
+					Type: spokeRegistrationFeatureGatesValid, Status: metav1.ConditionTrue, Reason: "FeatureGatesAllValid",
+					Message: "Registration feature gates of klusterlet are all valid",
+				}))
 		} else {
 			invalidFGErr := fmt.Errorf("there are some invalid feature gates of registration: %v ", invalidFeatureGates)
-			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.UpdateKlusterletConditionFn(metav1.Condition{
-				Type: spokeRegistrationFeatureGatesInvalid, Status: metav1.ConditionFalse, Reason: "InvalidFeatureGatesExisting",
-				Message: invalidFGErr.Error(),
-			}))
+			// TODO: after the 0.10.0 is released, change back to UpdateKlusterletConditionFn
+			_, _, _ = helpers.UpdateKlusterletStatus(ctx, n.klusterletClient, klusterletName, helpers.ReplaceKlusterletConditionFn(
+				spokeRegistrationFeatureGatesInvalid,
+				metav1.Condition{
+					Type: spokeRegistrationFeatureGatesValid, Status: metav1.ConditionFalse, Reason: "InvalidFeatureGatesExisting",
+					Message: invalidFGErr.Error(),
+				}))
 			return invalidFGErr
 		}
 	}

pkg/operators/klusterlet/controllers/klusterletcontroller/klusterlet_controller_test.go

@@ -516,7 +516,7 @@ func TestSyncDeploy(t *testing.T) {
 	testinghelper.AssertOnlyConditions(
 		t, operatorAction[3].(clienttesting.UpdateActionImpl).Object,
 		testinghelper.NamedCondition(klusterletApplied, "KlusterletApplied", metav1.ConditionTrue),
-		testinghelper.NamedCondition(spokeRegistrationFeatureGatesInvalid, "FeatureGatesAllValid", metav1.ConditionTrue))
+		testinghelper.NamedCondition(spokeRegistrationFeatureGatesValid, "FeatureGatesAllValid", metav1.ConditionTrue))
 }
 
 // TestSyncDeployHosted test deployment of klusterlet components in hosted mode
@@ -619,7 +619,7 @@ func TestSyncDeployHosted(t *testing.T) {
 
 	conditionReady := testinghelper.NamedCondition(klusterletReadyToApply, "KlusterletPrepared", metav1.ConditionTrue)
 	conditionApplied := testinghelper.NamedCondition(klusterletApplied, "KlusterletApplied", metav1.ConditionTrue)
-	conditionFeaturesValid := testinghelper.NamedCondition(spokeRegistrationFeatureGatesInvalid, "FeatureGatesAllValid", metav1.ConditionTrue)
+	conditionFeaturesValid := testinghelper.NamedCondition(spokeRegistrationFeatureGatesValid, "FeatureGatesAllValid", metav1.ConditionTrue)
 	testinghelper.AssertOnlyConditions(
 		t, operatorAction[1].(clienttesting.UpdateActionImpl).Object, conditionReady, conditionFeaturesValid)
 	testinghelper.AssertOnlyConditions(
@@ -936,7 +936,7 @@ func TestDeployOnKube111(t *testing.T) {
 	testinghelper.AssertOnlyConditions(
 		t, operatorAction[3].(clienttesting.UpdateActionImpl).Object,
 		testinghelper.NamedCondition(klusterletApplied, "KlusterletApplied", metav1.ConditionTrue),
-		testinghelper.NamedCondition(spokeRegistrationFeatureGatesInvalid, "FeatureGatesAllValid", metav1.ConditionTrue))
+		testinghelper.NamedCondition(spokeRegistrationFeatureGatesValid, "FeatureGatesAllValid", metav1.ConditionTrue))
 
 	// Delete the klusterlet
 	now := metav1.Now()