Add rule type change detection in profile parse #401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Vincent056
force-pushed
the
type_change
branch
2 times, most recently
from
7cb8d66 to
dcd2c5d
Compare
|
Testing profiles Expected Behavior: |
|
/hold for test |
Vincent056
force-pushed
the
type_change
branch
2 times, most recently
from
ff78432 to
66a2bc6
Compare
|
Didn't get expected result with commit 66a2bc6 |
Vincent056
force-pushed
the
type_change
branch
from
66a2bc6 to
339a7ac
Compare
I think you need to rerun the updated PB so the rule with get updated with correct annotation, it will be like a operator upgrade @xiaojiey |
Just updated the e2e, I think it will cover some of the upgrade cases. |
Vincent056
force-pushed
the
type_change
branch
from
339a7ac to
c4b5e89
Compare
|
/retest |
Vincent056
force-pushed
the
type_change
branch
2 times, most recently
from
e8d273a to
8b7d445
Compare
yuumasato
reviewed
Sep 20, 2023
There was a problem hiding this comment.
@Vincent056 These are the comments I have so far.
I'll continue reviewing tomorrow.
tests/e2e/parallel/main_test.go
Outdated
Comment on lines
215
to
218
| modPb.Spec.ContentImage = modifiedImage | ||
| if err := f.Client.Update(context.TODO(), modPb); err != nil { | ||
| t.Fatalf("failed to update ProfileBundle %s: %s", modPb.Name, err) | ||
| } |
There was a problem hiding this comment.
Is there a difference between the following?
- deep copying
modPb, creating it and updating.Spec.ContentImage - deep copying
modPb, updating the.Spec.ContentImageand creating it
There was a problem hiding this comment.
so what we are doing here is first to make a copy of original profile bundle cr instance and then fetch profile bundle CR, and change the content image of the original image, in the end we update the CR.
There was a problem hiding this comment.
since the pb is already there, we are not creating a new one, but to make change to the existing one
Vincent056
force-pushed
the
type_change
branch
2 times, most recently
from
82bf9bb to
26c74b3
Compare
rhmdnd
reviewed
Sep 27, 2023
rhmdnd
reviewed
Sep 28, 2023
rhmdnd
reviewed
Sep 28, 2023
rhmdnd
reviewed
Sep 28, 2023
rhmdnd
reviewed
Sep 28, 2023
tests/e2e/parallel/main_test.go
Outdated
| @@ -2431,6 +2570,9 @@ func TestManualRulesTailoredProfile(t *testing.T) { | |||
| ObjectMeta: metav1.ObjectMeta{ | |||
| Name: suiteName, | |||
| Namespace: f.OperatorNamespace, | |||
| Labels: map[string]string{ | |||
| compv1alpha1.OutdatedReferenceValidationDisable: "true", | |||
There was a problem hiding this comment.
How come we need to disable validation here?
There was a problem hiding this comment.
that's a good catch, I will remove it. It was left from the last approach, where we manually mark migrated rules, now we will not have these issues.
There was a problem hiding this comment.
This change is marked as as outdated, but still seems to be there at:
https://github.com/ComplianceAsCode/compliance-operator/pull/401/files#diff-a5334e69e88b595448bc22c081212911d6b0f04c2e91752ac2ef1ef7cb68c9edR2664
Is is expected to be there?
rhmdnd
reviewed
Sep 28, 2023
Vincent056
force-pushed
the
type_change
branch
from
54a4159 to
a55b8f3
Compare
rhmdnd
reviewed
Nov 15, 2023
rhmdnd
reviewed
Nov 15, 2023
Adding more test results:
|
Vincent056
force-pushed
the
type_change
branch
from
a55b8f3 to
34617c6
Compare
Vincent056
force-pushed
the
type_change
branch
from
34617c6 to
178535d
Compare
yuumasato
reviewed
Nov 22, 2023
My editor automatically picked up these changes when I was making some modifications locally. Pulling them in a separate commit.
We were using tpSingleNoPrune as a variable in the test, but the actual tailored profile was using multiple rules. This commit updates the variable to reflect that.
rhmdnd
reviewed
Nov 22, 2023
| } else if isValidationRequired(instance) { | ||
| reqLogger.Info("Validating TailoredProfile") | ||
| pruneOutdated := false | ||
| if _, ok := ann[cmpv1alpha1.PruneOutdatedReferencesAnnotationKey]; ok { |
There was a problem hiding this comment.
This only checks if the annotation exists, but in our tests we're setting this to true at times. Which should we be using?
Setting it to true feels more explicit to me, with the absence or false value being the default.
There was a problem hiding this comment.
Here I think I'd follow the same pattern done for rescan= annotation.
There was a problem hiding this comment.
I think this makes sense here
This migration/feature adds new annotations that help users manage their
TailoredProfiles. For example, they can set an annotation that
automatically prune outdated rule references from the TailoredProfile.
Previously, we were only checking the presence of these annotations on
the TailoredProfile. This meant users should invoke the behavior with an
empty string (""), or a truthy value ("true"), or even a falsy ("false)
value.
This commit explicitly checks the value of the annotation and makes sure
it's set to true before invoking the behavior.
We can improve the readability of the test, especially since rule change from Platfrom to Node type, by having dedicated assertions for checking that attribute. This commit uses those assertions in the end-to-end test so it's easier for readers to know when that type change happens.
We have a pretty good understanding of how to migrate rules from one type (Node) to another (Platform), but initially we included some thoughts on how to handle deprecated rules and variables. However, up to this point, we don't have a way to flag deprecated rules or variables in the content, which is what's needed to annotate them accordingly. Since this code is involving a separate case (deprecation), and the current migration e2e tests pass without it, let's remove it so we can come up with a dedicated approach for it later. This cuts down on the overall patch size, making it more digestable for reviewers, especially since it wasn't fully tested.
|
@yuumasato and I walked through the code and found a few things to adjust, but we can step through them together next week, too. I just wanted to get the updates we discussed proposed before the break. Each is its own commit, which we can evaluate and consider incorporating if we all agree they're addressing legitimate concerns. Overall, we cleaned up some of e2e test to include some additional assertions, added strict checks on the annotation, and reduced the overall patch size by removing untested code for deprecating variables and rules. |
This code was unused and untested, and we don't have a consistent, or agreed upon way for deprecating rules and variables in the content. Let's remove this for now so that we don't make the patch more complicated. We can always come back to this functionality later.
yuumasato
reviewed
Nov 23, 2023
|
/retest |
Vincent056
commented
Nov 28, 2023
| @@ -2439,3 +2439,25 @@ func (f *Framework) AssertScanDoesNotContainCheck(scanName, checkName, namespace | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Thanks for adding this, I think those make sense
Improve warning message on rule type change, explain to user what they might need to do on those rules
|
/label qe-approved |
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhmdnd, Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 8cd35fa
into
ComplianceAsCode:master
12 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We went through migration cases for the new KubeletConfig changes. We decided to handle the TailoredProfile migration on the operator instead.
This fixes future rule checkType change migration issues as well as rule/variables deprecation issues.
To spot deprecated variables and rules, we'll use annotations derived from the content:
compliance.openshift.io/deprecatedDuring the profileParser upgrade, we'll detect rule TypeChanges by comparing existing rule types against the new updates. If there's a change, the annotation
compliance.openshift.io/rule-last-check-typewill be added to the rule.We will issues event and show warning in the TailoredProfile Status, if there are any deprecated or migrated rules/variables in the TailoredProfile, and user can also choose to set following annotation on the tailoredProfile:
compliance.openshift.io/prune-outdated-references, so that the tailoredProfile controller will removal of all deprecated rules and variables along with migrated rules references from that specific tailoredProfile object.To bypass migration and deprecation validation on specific TailoredProfile object, add the tag:
compliance.openshift.io/disable-outdated-reference-validation.Testing Profiles:
Check comment for testing profiles