From 34617c6a9263d22ef77dbd3a7e4350b6f63c84a5 Mon Sep 17 00:00:00 2001 From: Vincent Shen Date: Wed, 8 Nov 2023 04:26:15 -0800 Subject: [PATCH] Add warnings field to TailoredProfile CRD --- ...pliance.openshift.io_tailoredprofiles.yaml | 2 + .../v1alpha1/tailoredprofile_types.go | 14 ++- .../profilebundle/profilebundle_controller.go | 12 ++- .../tailoredprofile_controller.go | 102 +++++++++++++----- tests/e2e/parallel/main_test.go | 75 ++++++++++++- 5 files changed, 161 insertions(+), 44 deletions(-) diff --git a/config/crd/bases/compliance.openshift.io_tailoredprofiles.yaml b/config/crd/bases/compliance.openshift.io_tailoredprofiles.yaml index 0763b2897..8c3e87cb7 100644 --- a/config/crd/bases/compliance.openshift.io_tailoredprofiles.yaml +++ b/config/crd/bases/compliance.openshift.io_tailoredprofiles.yaml @@ -156,6 +156,8 @@ spec: state: description: The current state of the tailored profile type: string + warnings: + type: string type: object type: object served: true diff --git a/pkg/apis/compliance/v1alpha1/tailoredprofile_types.go b/pkg/apis/compliance/v1alpha1/tailoredprofile_types.go index fe1e0508d..27971b917 100644 --- a/pkg/apis/compliance/v1alpha1/tailoredprofile_types.go +++ b/pkg/apis/compliance/v1alpha1/tailoredprofile_types.go @@ -7,19 +7,16 @@ import ( // FIXME: move name/rationale to a common struct with an interface? // DisableOutdatedReferenceValidation a label is used to disable validation of outdated references -const DisableOutdatedReferenceValidation = "compliance.openshift.io/outdated-reference-validation-disable" +const DisableOutdatedReferenceValidation = "compliance.openshift.io/disable-outdated-reference-validation" -// PurneOutdatedReferencesAnnotationKey is the annotation key used to indicate that the outdated references of rules or variables should be pruned -const PurneOutdatedReferencesAnnotationKey = "compliance.openshift.io/prune-outdated-references" +// PruneOutdatedReferencesAnnotationKey is the annotation key used to indicate that the outdated references of rules or variables should be pruned +const PruneOutdatedReferencesAnnotationKey = "compliance.openshift.io/prune-outdated-references" // RuleLastCheckTypeChangedAnnotationKey is the annotation key used to indicate that the rule check type has changed, store its previous check type const RuleLastCheckTypeChangedAnnotationKey = "compliance.openshift.io/rule-last-check-type" -// VariableDeprecatedAnnotationKey is the annotation key used to indicate that the variable is deprecated -const VariableDeprecatedAnnotationKey = "compliance.openshift.io/variable-deprecated" - -// DeprecatedRuleAnnotationKey is the annotation key used to indicate that the rule is deprecated -const DeprecatedRuleAnnotationKey = "compliance.openshift.io/deprecated-rule" +// DeprecatedAnnotationKey is the annotation key used to indicate that the variable or rule is deprecated +const DeprecatedAnnotationKey = "compliance.openshift.io/deprecated" // RuleReferenceSpec specifies a rule to be selected/deselected, as well as the reason why type RuleReferenceSpec struct { @@ -89,6 +86,7 @@ type TailoredProfileStatus struct { // The current state of the tailored profile State TailoredProfileState `json:"state,omitempty"` ErrorMessage string `json:"errorMessage,omitempty"` + Warnings string `json:"warnings,omitempty"` } // OutputRef is a reference to the object created from the tailored profile diff --git a/pkg/controller/profilebundle/profilebundle_controller.go b/pkg/controller/profilebundle/profilebundle_controller.go index b389215aa..be14e8576 100644 --- a/pkg/controller/profilebundle/profilebundle_controller.go +++ b/pkg/controller/profilebundle/profilebundle_controller.go @@ -580,14 +580,18 @@ func workloadNeedsUpdate(image string, depl *appsv1.Deployment) bool { return true } + isSameContentImage := false + isSaneProfileparserImage := false + for _, container := range initContainers { if container.Name == "content-container" { // we need an update if the image reference doesn't match. - return image != container.Image + isSameContentImage = container.Image == image + } + if container.Name == "profileparser" { + isSaneProfileparserImage = utils.GetComponentImage(utils.OPERATOR) == container.Image } } - // If we didn't find the container we were looking for. There's something funky going on - // and we should try to update anyway - return true + return !(isSameContentImage && isSaneProfileparserImage) } diff --git a/pkg/controller/tailoredprofile/tailoredprofile_controller.go b/pkg/controller/tailoredprofile/tailoredprofile_controller.go index f05510633..cf5cc039b 100644 --- a/pkg/controller/tailoredprofile/tailoredprofile_controller.go +++ b/pkg/controller/tailoredprofile/tailoredprofile_controller.go @@ -208,12 +208,13 @@ func (r *ReconcileTailoredProfile) Reconcile(ctx context.Context, request reconc if _, ok := ann[cmpv1alpha1.DisableOutdatedReferenceValidation]; ok { reqLogger.Info("Reference validation is disabled, skipping validation") } else if isValidationRequired(instance) { - pureOutdated := false - if _, ok := ann[cmpv1alpha1.PurneOutdatedReferencesAnnotationKey]; ok { - pureOutdated = true + reqLogger.Info("Validating TailoredProfile") + pruneOutdated := false + if _, ok := ann[cmpv1alpha1.PruneOutdatedReferencesAnnotationKey]; ok { + pruneOutdated = true } // remove any deprecated variables or rules from the tailored profile - doContinue, err := r.handleDeprecation(instance, reqLogger, pureOutdated) + doContinue, variableNeedtoBeDeprecatedList, ruleNeedToBeDeprecatedList, err := r.handleDeprecation(instance, reqLogger, pruneOutdated) if err != nil { return reconcile.Result{}, err } @@ -221,13 +222,23 @@ func (r *ReconcileTailoredProfile) Reconcile(ctx context.Context, request reconc return reconcile.Result{}, nil } - doContinue, err = r.handleMigration(instance, reqLogger, pureOutdated) + doContinue, ruleNeedToBeMigratedList, err := r.handleRulePruning(instance, reqLogger, pruneOutdated) if err != nil { return reconcile.Result{}, err } if !doContinue { return reconcile.Result{}, nil } + + warningMsg := generateWarningMessage(variableNeedtoBeDeprecatedList, ruleNeedToBeDeprecatedList, ruleNeedToBeMigratedList) + // check if warning message matches the previous warning message + // if it does, we don't need to update the tp, if not we need to update it with the new warning message + if warningMsg != instance.Status.Warnings { + tpCopy := instance.DeepCopy() + tpCopy.Status.Warnings = warningMsg + r.Client.Status().Update(context.TODO(), tpCopy) + } + } rules, ruleErr := r.getRulesFromSelections(instance, pb) @@ -265,24 +276,49 @@ func (r *ReconcileTailoredProfile) Reconcile(ctx context.Context, request reconc return r.ensureOutputObject(instance, tpcm, reqLogger) } -// handleMigration check if there are any migrated rules in the TailoredProfile -// and we will handle the migration of the tailored profile accordingly -func (r *ReconcileTailoredProfile) handleMigration( - v1alphaTp *cmpv1alpha1.TailoredProfile, logger logr.Logger, pruneOudated bool) (bool, error) { +// generateWarningMessage generates a warning message for the user +// based on the list of deprecated variables and rules that are detected +// as well as the list of migrated rules that are detected that are not +// migrated yet +func generateWarningMessage(variableNeedtoBeDeprecatedList []string, ruleNeedToBeDeprecatedList []string, ruleNeedToBeMigratedList []string) string { + var warningMessage string + if len(variableNeedtoBeDeprecatedList) > 0 { + warningMessage = fmt.Sprintf("The following variables are deprecated and need to be removed from the TailoredProfile: %s\n", strings.Join(variableNeedtoBeDeprecatedList, ",")) + } + if len(ruleNeedToBeDeprecatedList) > 0 { + if warningMessage != "" { + warningMessage = fmt.Sprintf("%sThe following rules are deprecated and need to be removed from the TailoredProfile: %s\n", warningMessage, strings.Join(ruleNeedToBeDeprecatedList, ",")) + } else { + warningMessage = fmt.Sprintf("The following rules are deprecated and need to be removed from the TailoredProfile: %s\n", strings.Join(ruleNeedToBeDeprecatedList, ",")) + } + } + if len(ruleNeedToBeMigratedList) > 0 { + if warningMessage != "" { + warningMessage = fmt.Sprintf("%sThe following rules are migrated and need to be migrated or removed from the TailoredProfile: %s\n", warningMessage, strings.Join(ruleNeedToBeMigratedList, ",")) + } else { + warningMessage = fmt.Sprintf("The following rules are migrated and need to be migrated or removed from the TailoredProfile: %s\n", strings.Join(ruleNeedToBeMigratedList, ",")) + } + } + return warningMessage +} +// handleRulePruning check if there are any migrated rules in the TailoredProfile +// and we will handle the migration of the tailored profile accordingly +func (r *ReconcileTailoredProfile) handleRulePruning( + v1alphaTp *cmpv1alpha1.TailoredProfile, logger logr.Logger, pruneOudated bool) (doContinue bool, ruleNeedToBeMigratedList []string, err error) { + doContinue = true // Get the list of KubeletConfig rules that are migrated with checkType change migratedRules, err := r.getMigratedRules(v1alphaTp, logger) if err != nil { - return false, err + return false, nil, err } if len(migratedRules) == 0 { - return true, nil + return true, nil, nil } profileType := utils.GetScanType(v1alphaTp.GetAnnotations()) - doContinue := true v1alphaTpCP := v1alphaTp.DeepCopy() // check if there are any disabled rules that are migrated @@ -295,10 +331,11 @@ func (r *ReconcileTailoredProfile) handleMigration( if pruneOudated { doContinue = false logger.Info("Removing migrated rule from disableRules", "rule", rule.Name) - r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Removing migrated rule: %s from disableRules", rule.Name) + r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Removing migrated rule: %s from disableRules, it has been changed from %s to %s", rule.Name, checkType, profileType) } else { logger.Info("Migrated rule detected in disableRules", "rule", rule.Name) - r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Migrated rule detected: %s Please migrate it or remove it from the TailoredProfile", rule.Name) + r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "%s type changed from %s to %s. Please migrate it or remove it from the TailoredProfile", rule.Name, checkType, profileType) + ruleNeedToBeMigratedList = append(ruleNeedToBeMigratedList, rule.Name) newRules = append(newRules, *rule) } continue @@ -319,10 +356,11 @@ func (r *ReconcileTailoredProfile) handleMigration( if pruneOudated { doContinue = false logger.Info("Removing migrated rule from enableRules", "rule", rule.Name) - r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Removing migrated rule %s from enableRules", rule.Name) + r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Removing migrated rule %s from enableRules, it has been changed from %s to %s", rule.Name, checkType, profileType) } else { logger.Info("Migrated rule detected in enableRules", "rule", rule.Name) - r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "Migrated rule detected: %s Please migrate it or remove it from the TailoredProfile", rule.Name) + r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileMigratedRule", "%s type changed from %s to %s. Please migrate it or remove it from the TailoredProfile", rule.Name, checkType, profileType) + ruleNeedToBeMigratedList = append(ruleNeedToBeMigratedList, rule.Name) newRules = append(newRules, *rule) } continue @@ -346,35 +384,34 @@ func (r *ReconcileTailoredProfile) handleMigration( if !doContinue { err = r.Client.Update(context.TODO(), v1alphaTpCP) - logger.Info("Updating TailoredProfile with migrated rules removed") + logger.Info("Updating TailoredProfile after handling migration") if err != nil { - return false, err + return false, nil, err } } - return doContinue, nil + return doContinue, ruleNeedToBeMigratedList, nil } func isValidationRequired(tp *cmpv1alpha1.TailoredProfile) bool { if tp.Spec.Extends != "" { return tp.Spec.DisableRules != nil || tp.Spec.EnableRules != nil || tp.Spec.ManualRules != nil || tp.Spec.SetValues != nil } - return tp.Spec.EnableRules != nil || tp.Spec.ManualRules != nil + return tp.Spec.EnableRules != nil || tp.Spec.ManualRules != nil || tp.Spec.SetValues != nil } func (r *ReconcileTailoredProfile) handleDeprecation( - v1alphaTp *cmpv1alpha1.TailoredProfile, logger logr.Logger, pruneOudated bool) (bool, error) { - + v1alphaTp *cmpv1alpha1.TailoredProfile, logger logr.Logger, pruneOudated bool) (doContinue bool, variableNeedtoBeDeprecatedList []string, ruleNeedToBeDeprecatedList []string, err error) { + doContinue = true deprecatedRules, err := r.getDeprecatedRules(v1alphaTp, logger) if err != nil { - return false, err + return false, nil, nil, err } deprecatedVariables, err := r.getDeprecatedVariables(v1alphaTp, logger) if err != nil { - return false, err + return false, nil, nil, err } - doContinue := true v1alphaTpCP := v1alphaTp.DeepCopy() // remove any deprecated variables from the tailored profile @@ -390,6 +427,7 @@ func (r *ReconcileTailoredProfile) handleDeprecation( } else { logger.Info("Deprecated variable detected", "variable", variables.Name) r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileDeprecatedSetting", "Deprecated variable detected: %s Please remove it from the TailoredProfile", variables.Name) + variableNeedtoBeDeprecatedList = append(variableNeedtoBeDeprecatedList, variables.Name) newVariables = append(newVariables, *variables) } continue @@ -415,6 +453,7 @@ func (r *ReconcileTailoredProfile) handleDeprecation( } else { logger.Info("Deprecated rule detected in enableRules", "rule", rule.Name) r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileDeprecatedSetting", "Deprecated rule detected: %s Please remove it from the TailoredProfile", rule.Name) + ruleNeedToBeDeprecatedList = append(ruleNeedToBeDeprecatedList, rule.Name) newRules = append(newRules, *rule) } continue @@ -439,6 +478,7 @@ func (r *ReconcileTailoredProfile) handleDeprecation( } else { logger.Info("Deprecated rule detected in disableRules", "rule", rule.Name) r.Eventf(v1alphaTp, corev1.EventTypeWarning, "TailoredProfileDeprecatedSetting", "Deprecated rule detected: %s Please remove it from the TailoredProfile", rule.Name) + ruleNeedToBeDeprecatedList = append(ruleNeedToBeDeprecatedList, rule.Name) newRules = append(newRules, *rule) } continue @@ -466,10 +506,10 @@ func (r *ReconcileTailoredProfile) handleDeprecation( err = r.Client.Update(context.TODO(), v1alphaTpCP) logger.Info("Updating TailoredProfile with deprecated rules and variables removed") if err != nil { - return false, err + return false, nil, nil, err } } - return doContinue, nil + return doContinue, variableNeedtoBeDeprecatedList, ruleNeedToBeDeprecatedList, nil } // getMigratedRules get list of rules and check if it has RuleLastCheckTypeChangedAnnotationKey annotation @@ -518,7 +558,7 @@ func (r *ReconcileTailoredProfile) getDeprecatedVariables(tp *cmpv1alpha1.Tailor for vi := range variableList.Items { variable := &variableList.Items[vi] if variable.Annotations != nil { - if _, ok := variable.Annotations[cmpv1alpha1.VariableDeprecatedAnnotationKey]; ok { + if _, ok := variable.Annotations[cmpv1alpha1.DeprecatedAnnotationKey]; ok { deprecatedVariables[variable.GetName()] = "" } } @@ -542,7 +582,7 @@ func (r *ReconcileTailoredProfile) getDeprecatedRules(tp *cmpv1alpha1.TailoredPr for ri := range ruleList.Items { rule := &ruleList.Items[ri] if rule.Annotations != nil { - if _, ok := rule.Annotations[cmpv1alpha1.DeprecatedRuleAnnotationKey]; ok { + if _, ok := rule.Annotations[cmpv1alpha1.DeprecatedAnnotationKey]; ok { deprecatedRules[rule.GetName()] = "" } } @@ -858,6 +898,10 @@ func assertValidRuleTypes(rules map[string]*cmpv1alpha1.Rule) error { if rule.CheckType == cmpv1alpha1.CheckTypeNone { continue } + // check if the rule is a migrated rule and if it is, we should not check the type + if _, ok := rule.Annotations[cmpv1alpha1.RuleLastCheckTypeChangedAnnotationKey]; ok { + continue + } // Initialize expected check type if expectedCheckType == "" { expectedCheckType = rule.CheckType diff --git a/tests/e2e/parallel/main_test.go b/tests/e2e/parallel/main_test.go index 294ef29b6..317ee6e20 100644 --- a/tests/e2e/parallel/main_test.go +++ b/tests/e2e/parallel/main_test.go @@ -2163,6 +2163,7 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { moderateProfileName = "moderate" tpMixName = "many-migrated-mix-tp" tpSingleName = "migrated-single-tp" + tpSingleNoPruneName = "migrated-single-no-prune-tp" ) var ( baselineImage = fmt.Sprintf("%s:%s", brokenContentImagePath, "kubelet_default") @@ -2219,7 +2220,7 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { Name: tpMixName, Namespace: f.OperatorNamespace, Annotations: map[string]string{ - compv1alpha1.PurneOutdatedReferencesAnnotationKey: "", + compv1alpha1.PruneOutdatedReferencesAnnotationKey: "", }, }, Spec: compv1alpha1.TailoredProfileSpec{ @@ -2243,12 +2244,29 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { Name: tpSingleName, Namespace: f.OperatorNamespace, Annotations: map[string]string{ - compv1alpha1.PurneOutdatedReferencesAnnotationKey: "", + compv1alpha1.PruneOutdatedReferencesAnnotationKey: "", }, }, Spec: compv1alpha1.TailoredProfileSpec{ Title: "TestForManyRules", Description: "TestForManyRules", + EnableRules: []compv1alpha1.RuleReferenceSpec{ + { + Name: changeTypeRuleName, + Rationale: "this rule should be removed from the profile", + }, + }, + }, + } + + tpSingleNoPrune := &compv1alpha1.TailoredProfile{ + ObjectMeta: metav1.ObjectMeta{ + Name: tpSingleNoPruneName, + Namespace: f.OperatorNamespace, + }, + Spec: compv1alpha1.TailoredProfileSpec{ + Title: "TestForNoPrune", + Description: "TestForNoPrune", EnableRules: []compv1alpha1.RuleReferenceSpec{ { Name: changeTypeRuleName, @@ -2305,6 +2323,27 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { t.Fatalf("Expected the tailored profile to have rule: %s", changeTypeRuleName) } + // tpSingleNoPrune test + createTPErr = f.Client.Create(context.TODO(), tpSingleNoPrune, nil) + if createTPErr != nil { + t.Fatal(createTPErr) + } + defer f.Client.Delete(context.TODO(), tpSingleNoPrune) + // check the status of the TP to make sure it has no errors + err = f.WaitForTailoredProfileStatus(f.OperatorNamespace, tpSingleNoPruneName, compv1alpha1.TailoredProfileStateReady) + if err != nil { + t.Fatal(err) + } + + hasRule, err = f.EnableRuleExistInTailoredProfile(f.OperatorNamespace, tpSingleNoPruneName, changeTypeRuleName) + if err != nil { + t.Fatal(err) + } + + if !hasRule { + t.Fatalf("Expected the tailored profile to have rule: %s", changeTypeRuleName) + } + // update the image with a new hash modPb := origPb.DeepCopy() if err := f.Client.Get(context.TODO(), types.NamespacedName{Namespace: modPb.Namespace, Name: modPb.Name}, modPb); err != nil { @@ -2351,7 +2390,7 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { } // check that the tp has been updated with the removed rule singleTP - err = f.WaitForTailoredProfileStatus(f.OperatorNamespace, tpSingleName, compv1alpha1.TailoredProfileStateReady) + err = f.WaitForTailoredProfileStatus(f.OperatorNamespace, tpSingleName, compv1alpha1.TailoredProfileStateError) if err != nil { t.Fatal(err) } @@ -2360,10 +2399,40 @@ func TestScanSettingBindingTailoringManyEnablingRulePass(t *testing.T) { if err != nil { t.Fatal(err) } + if hasRule { + t.Fatalf("Expected the tailored profile not to have rule: %s", changeTypeRuleName) + } + + // check that the no prune tp still has the rule + err = f.WaitForTailoredProfileStatus(f.OperatorNamespace, tpSingleNoPruneName, compv1alpha1.TailoredProfileStateReady) + if err != nil { + t.Fatal(err) + } + + hasRule, err = f.EnableRuleExistInTailoredProfile(f.OperatorNamespace, tpSingleNoPruneName, changeTypeRuleName) + if err != nil { + t.Fatal(err) + } if !hasRule { t.Fatalf("Expected the tailored profile to have rule: %s", changeTypeRuleName) } + // check that we have a warning message in the tailored profile + tpSingleNoPruneFetched := &compv1alpha1.TailoredProfile{} + key := types.NamespacedName{Namespace: f.OperatorNamespace, Name: tpSingleNoPruneName} + if err := f.Client.Get(context.Background(), key, tpSingleNoPruneFetched); err != nil { + t.Fatal(err) + } + + if len(tpSingleNoPruneFetched.Status.Warnings) == 0 { + t.Fatalf("Expected the tailored profile to have a warning message but got none") + } + + // check that the warning message is about the rule + if !strings.Contains(tpSingleNoPruneFetched.Status.Warnings, changeTypeRule) { + t.Fatalf("Expected the tailored profile to have a warning message about migrated rule: %s but got: %s", changeTypeRule, tpSingleNoPruneFetched.Status.Warnings) + } + } func TestScanSettingBindingUsesDefaultScanSetting(t *testing.T) {