Merge pull request #695 from Vincent056/ocp-11228

OCPBUGS-11228: Check for duplicate variables in TailoredProfile

Author: rhmdnd

pkg/controller/tailoredprofile/tailoredprofile_controller.go

@@ -536,14 +536,30 @@ func (r *ReconcileTailoredProfile) getRulesFromSelections(tp *cmpv1alpha1.Tailor
 	}
 	return rules, nil
 }
-
 func (r *ReconcileTailoredProfile) getVariablesFromSelections(tp *cmpv1alpha1.TailoredProfile, pb *cmpv1alpha1.ProfileBundle) ([]*cmpv1alpha1.Variable, error) {
-	variableList := []*cmpv1alpha1.Variable{}
-	for _, setValues := range tp.Spec.SetValues {
+	// First pass: Build de-duplicated map of variables, keeping last occurrence when duplicates exist.
+	variables := make(map[string]string)
+
+	for i, setValue := range tp.Spec.SetValues {
+		if oldVal, seen := variables[setValue.Name]; seen {
+			// We found a duplicate. Warn that we will ignore the previous usage.
+			r.Eventf(tp, corev1.EventTypeWarning,
+				"DuplicatedSetValue",
+				"Variable '%s' appears multiple times in setValues. The operator will keep the last usage with value '%s' (position %d), ignoring the previous value '%s'. Specifying a variable multiple times using setValues will fail in a future release. Please remove duplicates from setValues as it introduces ambiguity.",
+				setValue.Name, setValue.Value, i, oldVal)
+		}
+		// Always use the last occurrence's value
+		variables[setValue.Name] = setValue.Value
+	}
+
+	// Second pass: Retrieve Variables from cluster and set their values
+	// Variables are processed in random order (map iteration)
+	variableList := make([]*cmpv1alpha1.Variable, 0, len(variables))
+	for varName, value := range variables {
+		// Grab the Variable from the cluster
 		variable := &cmpv1alpha1.Variable{}
-		varKey := types.NamespacedName{Name: setValues.Name, Namespace: tp.Namespace}
-		err := r.Client.Get(context.TODO(), varKey, variable)
-		if err != nil {
+		varKey := types.NamespacedName{Name: varName, Namespace: tp.Namespace}
+		if err := r.Client.Get(context.TODO(), varKey, variable); err != nil {
 			if kerrors.IsNotFound(err) {
 				return nil, common.NewNonRetriableCtrlError("fetching variable: %w", err)
 			}
@@ -557,8 +573,7 @@ func (r *ReconcileTailoredProfile) getVariablesFromSelections(tp *cmpv1alpha1.Ta
 		}
 
 		// try setting the variable, this also validates the value
-		err = variable.SetValue(setValues.Value)
-		if err != nil {
+		if err := variable.SetValue(value); err != nil {
 			return nil, common.NewNonRetriableCtrlError("setting variable: %s", err)
 		}
 

pkg/controller/tailoredprofile/tailoredprofile_controller_test.go

@@ -837,6 +837,94 @@ var _ = Describe("TailoredprofileController", func() {
 			})
 		})
 
+		Context("with duplicated setValues for the same variable", func() {
+			var tpName = "tailoring-duplicate-vars"
+
+			BeforeEach(func() {
+				// Create a TailoredProfile with duplicate entries for "var-1"
+				// Note "var-1" is owned by pb1 in your test setup if i < 5
+				tp := &compv1alpha1.TailoredProfile{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      tpName,
+						Namespace: namespace,
+					},
+					Spec: compv1alpha1.TailoredProfileSpec{
+						// No extends here, so it picks up the PB from the first rule/variable it finds
+						EnableRules: []compv1alpha1.RuleReferenceSpec{
+							{
+								Name:      "rule-1",
+								Rationale: "Ensure ownership detection picks pb1",
+							},
+						},
+						SetValues: []compv1alpha1.VariableValueSpec{
+							{
+								Name:  "var-1",
+								Value: "1111",
+							},
+							{
+								Name:  "var-1",
+								Value: "2222", // Duplicate #2
+							},
+							{
+								Name:  "var-2",
+								Value: "3333",
+							},
+							{
+								Name:  "var-1",
+								Value: "4444", // Duplicate #3 (final mention)
+							},
+						},
+					},
+				}
+
+				createErr := r.Client.Create(ctx, tp)
+				Expect(createErr).To(BeNil())
+			})
+
+			It("keeps the last mention of the duplicated variable and raises a warning", func() {
+				tpKey := types.NamespacedName{
+					Name:      tpName,
+					Namespace: namespace,
+				}
+				tpReq := reconcile.Request{}
+				tpReq.Name = tpName
+				tpReq.Namespace = namespace
+
+				By("Reconciling the TailoredProfile the first time (to set ownership)")
+				_, err := r.Reconcile(context.TODO(), tpReq)
+				Expect(err).To(BeNil())
+
+				By("Reconciling the TailoredProfile the second time (to process setValues)")
+				_, err = r.Reconcile(context.TODO(), tpReq)
+				Expect(err).To(BeNil())
+
+				By("Ensuring the TailoredProfile ends in the Ready state")
+				tp := &compv1alpha1.TailoredProfile{}
+				geterr := r.Client.Get(ctx, tpKey, tp)
+				Expect(geterr).To(BeNil())
+				Expect(tp.Status.State).To(Equal(compv1alpha1.TailoredProfileStateReady))
+				Expect(tp.Status.ErrorMessage).To(BeEmpty())
+
+				By("Ensuring the final configMap uses the last mention of var-1 (4444)")
+				cm := &corev1.ConfigMap{}
+				cmKey := types.NamespacedName{
+					Name:      tp.Status.OutputRef.Name,
+					Namespace: tp.Status.OutputRef.Namespace,
+				}
+				geterr = r.Client.Get(ctx, cmKey, cm)
+				Expect(geterr).To(BeNil())
+				data := cm.Data["tailoring.xml"]
+				// The final mention for var-1 must be "4444" in the rendered tailoring
+				Expect(data).To(ContainSubstring(`idref="var_1"`))
+				Expect(data).To(ContainSubstring(`4444`))
+				// Ensure that 2222 or 1111 never appear, verifying the earlier duplicates are discarded
+				Expect(data).NotTo(ContainSubstring("1111"))
+				Expect(data).NotTo(ContainSubstring("2222"))
+				Expect(data).To(ContainSubstring(`idref="var_2"`))
+				Expect(data).To(ContainSubstring(`3333`))
+			})
+		})
+
 		Context("with no rules nor variables", func() {
 			BeforeEach(func() {
 				tp := &compv1alpha1.TailoredProfile{

tests/e2e/framework/common.go

@@ -520,6 +520,42 @@ func (f *Framework) WaitForProfileDeprecatedWarning(t *testing.T, scanName strin
 	return nil
 }
 
+func (f *Framework) WaitForDuplicatedVariableWarning(t *testing.T, tpName string, variableName string) error {
+	polledTailoredProfile := &compv1alpha1.TailoredProfile{}
+
+	// Wait for profile deprecation warning event
+	err := wait.Poll(RetryInterval, Timeout, func() (bool, error) {
+		getErr := f.Client.Get(context.TODO(), types.NamespacedName{Name: tpName, Namespace: f.OperatorNamespace}, polledTailoredProfile)
+		if getErr != nil {
+			t.Log(getErr)
+			return false, nil
+		}
+
+		duplicateValuesEventList, getEventErr := f.KubeClient.CoreV1().Events(f.OperatorNamespace).List(context.TODO(), metav1.ListOptions{
+			FieldSelector: "reason=DuplicatedSetValue",
+		})
+		if getEventErr != nil {
+			t.Log(getEventErr)
+			return false, nil
+		}
+
+		re := regexp.MustCompile(fmt.Sprintf(".*%s.*", variableName))
+
+		for _, item := range duplicateValuesEventList.Items {
+			if item.InvolvedObject.Name == polledTailoredProfile.Name && re.MatchString(item.Message) {
+				t.Logf("Found TailoredProfile duplicated variable event: %s", item.Message)
+				return true, nil
+			}
+		}
+		return false, nil
+	})
+	if err != nil {
+		t.Fatalf("No TailoredProfile event for variable \"%s\" found", variableName)
+		return err
+	}
+	return nil
+}
+
 // waitForProfileBundleStatus will poll until the compliancescan that we're
 // lookingfor reaches a certain status, or until a timeout is reached.
 func (f *Framework) WaitForProfileBundleStatus(name string, status compv1alpha1.DataStreamStatusType) error {

tests/e2e/parallel/main_test.go

@@ -839,6 +839,48 @@ func TestScanTailoredProfileIsDeprecated(t *testing.T) {
 	}
 }
 
+func TestScanTailoredProfileHasDuplicateVariables(t *testing.T) {
+	t.Parallel()
+	f := framework.Global
+	pbName := framework.GetObjNameFromTest(t)
+	prefixName := func(profName, ruleBaseName string) string { return profName + "-" + ruleBaseName }
+	varName := prefixName(pbName, "var-openshift-audit-profile")
+	tpName := "test-tailored-profile-has-duplicate-variables"
+	tp := &compv1alpha1.TailoredProfile{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      tpName,
+			Namespace: f.OperatorNamespace,
+		},
+		Spec: compv1alpha1.TailoredProfileSpec{
+			Extends:     "ocp4-cis",
+			Title:       "TestScanTailoredProfileIsDuplicateVariables",
+			Description: "TestScanTailoredProfileIsDuplicateVariables",
+			SetValues: []compv1alpha1.VariableValueSpec{
+				{
+					Name:      varName,
+					Rationale: "Value to be set",
+					Value:     "WriteRequestBodies",
+				},
+				{
+					Name:      varName,
+					Rationale: "Value to be set",
+					Value:     "SomethingElse",
+				},
+			},
+		},
+	}
+	err := f.Client.Create(context.TODO(), tp, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Client.Delete(context.TODO(), tp)
+	// let's check if the profile is created and if event warning is being generated
+	if err = f.WaitForDuplicatedVariableWarning(t, tpName, varName); err != nil {
+		t.Fatal(err)
+	}
+
+}
+
 func TestScanProducesRemediations(t *testing.T) {
 	t.Parallel()
 	f := framework.Global