CMP-3974: Propagate custom labels and annotations from Rules to ComplianceCheckResults#1091
CMP-3974: Propagate custom labels and annotations from Rules to ComplianceCheckResults#1091Vincent056 wants to merge 11 commits intoComplianceAsCode:masterfrom
Conversation
|
@Vincent056: This pull request references CMP-3974 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the feature to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Vincent056 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Vincent056
force-pushed
the
custom-metadata-propagation
branch
2 times, most recently
from
ca3e70c to
5c24f2b
Compare
|
🤖 To deploy this PR, run the following command: |
Vincent056
force-pushed
the
custom-metadata-propagation
branch
from
5c24f2b to
9d53f27
Compare
|
🤖 To deploy this PR, run the following command: |
|
🤖 To deploy this PR, run the following command: |
|
🤖 To deploy this PR, run the following command: |
|
🤖 To deploy this PR, run the following command: |
Vincent056
force-pushed
the
custom-metadata-propagation
branch
from
e27e8a5 to
f0893f6
Compare
|
🤖 To deploy this PR, run the following command: |
|
🤖 To deploy this PR, run the following command: |
|
tested on AWS OCP 4.21 and it works.
|
|
e2e failures seems unrelated, check if it will fix with #1093 |
rhmdnd
left a comment
rhmdnd
left a comment
There was a problem hiding this comment.
Only a handful of comments, but otherwise this is looking great.
Thanks for adding all the test coverage. It makes reviews the reviews a lot easier.
|
|
||
| foundRule.Annotations = updatedRule.Annotations | ||
| // if the check type has changed, add an annotation to the rule | ||
| // to indicate that the rule needs to be checked in TailoredProfile validation |
There was a problem hiding this comment.
This comment is still applicable, just further down on line 116, right?
There was a problem hiding this comment.
correct, remove it from here to give more clarity
tests/e2e/parallel/main_test.go
Outdated
| } | ||
| defer f.Client.Delete(context.TODO(), ssb) | ||
|
|
||
| err = f.WaitForSuiteScansStatus(testNamespace, ssbName, compv1alpha1.PhaseDone, compv1alpha1.ResultNotApplicable) |
There was a problem hiding this comment.
I wonder if we can update WaitForSuiteScansStatus to accept multiple result types so we can reduce the number of conditionals.
| authors: | ||
| - Vincent056 | ||
| reviewers: | ||
| - TBD |
| reviewers: | ||
| - TBD | ||
| approvers: | ||
| - TBD |
| var operatorManagedPrefixes = []string{ | ||
| "compliance.openshift.io/", | ||
| "complianceoperator.openshift.io/", | ||
| "complianceascode.io/", |
There was a problem hiding this comment.
What about kubernetes-specific prefixes? Are those something we want to preemptively filter, too?
I'm thinking about things like kubernetes.io and k8s.io.
There was a problem hiding this comment.
tests/e2e/parallel/main_test.go
Outdated
| t.Fatalf("Failed to get ComplianceCheckResult: %v", err) | ||
| } | ||
|
|
||
| if checkResult.Labels["business-unit"] != "payments" { |
There was a problem hiding this comment.
We'd then condense these into a loop over the customLabels and customAnnotations. If we need to add to or update the customLabels, we only need to do it in one place.
tests/e2e/parallel/main_test.go
Outdated
| t.Errorf("operator-managed status label should not be overridden, got %q", checkResult.Labels[compv1alpha1.ComplianceCheckResultStatusLabel]) | ||
| } | ||
|
|
||
| t.Log("Test completed successfully. Custom metadata propagation verified:") |
There was a problem hiding this comment.
I wonder how much we want to log successful cases since Go kind of does this already with it's native test runner.
tests/e2e/parallel/main_test.go
Outdated
| t.Errorf("operator-managed scan label should not be overridden, got %q", checkResult.Labels[compv1alpha1.ComplianceScanLabel]) | ||
| } | ||
|
|
||
| t.Log("OpenSCAP metadata propagation verified:") |
There was a problem hiding this comment.
Similar comment here about logging successful test cases.
tests/e2e/parallel/main_test.go
Outdated
| if rule.Labels == nil { | ||
| rule.Labels = make(map[string]string) | ||
| } | ||
| rule.Labels["e2e-business-unit"] = "security-ops" |
There was a problem hiding this comment.
Similar comment here as above about putting these in a variable.
| if !exists { | ||
| cmdLog.Info("Creating object", "kind", kind, "name", name) | ||
| annotations = setTimestampAnnotations(owner, annotations) | ||
| if annotations != nil { |
There was a problem hiding this comment.
Could this potentially overwrite the list of custom labels and annotations we curated above? This is coming for a CheckResult and not the Rule?
There was a problem hiding this comment.
we add https://github.com/ComplianceAsCode/compliance-operator/pull/1091/changes#diff-2f2ea3af9b0b84d07bf17a91e568468b4dce17832cc0971d2523ba469613d36eR377 custom labels to compResult which is items in evalResultList. It should contain custom labels here
There was a problem hiding this comment.
What if we add an annotation that isn't prefixed? The user supplied one would overwrite it, wouldn't it?
There was a problem hiding this comment.
If I understand the SCAP result path, the aggregator will only add in the labels/annotations if they don't already exist. For the CEL path, it looks like it filters the operator annotations, but only based on the prefix, and doesn't use the same merge logic as the SCAP path? Is my understand correct there?
| t.Logf(" - Validated that rule %s has FAIL status", customRuleName) | ||
| } | ||
|
|
||
| func TestCustomRuleMetadataPropagation(t *testing.T) { |
There was a problem hiding this comment.
What happens if a user
- annotates a rule
- runs a scan
- observes the result has the annotation
- removes the annotation from the rule
- rescan the cluster
Does the logic remove that annotation from the existing check result, or is it sticky from the last (stale) check result?
There was a problem hiding this comment.
Custom metadata is not sticky. On rescan, results are created fresh from the current Rule metadata. If a user removes an annotation from the Rule, the next scan's results will not have it.
Introduce RuleMetadataCache and helper functions to extract, cache, and merge user-defined labels and annotations from Rule objects while preserving operator-managed keys.
Merge annotations instead of replacing them in the profileparser so custom labels and annotations added by users survive ProfileBundle content refreshes.
Build a RuleMetadataCache at scan result aggregation time and merge custom labels/annotations from Rule objects into each ComplianceCheckResult for OpenSCAP-based scans.
Extract custom labels/annotations from CustomRule objects and set them on the resulting ComplianceCheckResult for CEL-based scans.
Verify that user-defined labels and annotations on a CustomRule are propagated to the resulting ComplianceCheckResult after a CEL scan, and that operator-managed keys are not overridden.
Document the design, motivation, and usage examples for propagating user-defined labels and annotations from Rule/CustomRule objects to ComplianceCheckResults.
The RuleMetadataCache needs to list Rules in the namespace to build the custom metadata lookup table. Add the missing list verb for rules to the remediation-aggregator Role and regenerate bundle.
Verify that custom labels and annotations on an existing Rule are propagated to the ComplianceCheckResult through the aggregator path after an OpenSCAP scan completes.
Vincent056
force-pushed
the
custom-metadata-propagation
branch
from
4f86f24 to
c163361
Compare
|
🤖 To deploy this PR, run the following command: |
Filter kubernetes.io/ and k8s.io/ reserved keys from custom metadata, refactor e2e tests to use loops and named variables, add WaitForSuiteScansStatusAnyResult helper, and update enhancement doc reviewers.
|
🤖 To deploy this PR, run the following command: |
|
I can see both the label and annotations work for customrules. I will double check whether it works for rules. |
|
I tested below scenarios and all scenarios wok as expected. One reminder is: Don't update the annotations/labels for
|
|
/label qe-approved |
| return f.waitForSuiteScansStatusMulti(namespace, name, targetStatus, acceptableResults...) | ||
| } | ||
|
|
||
| func (f *Framework) waitForSuiteScansStatusMulti(namespace, name string, targetStatus compv1alpha1.ComplianceScanStatusPhase, acceptableResults ...compv1alpha1.ComplianceScanStatusResult) error { |
There was a problem hiding this comment.
@Anna-Koudelkova and @taimurhafeez are both adding variations of this in PRs for testing.
Unless one of those PRs is ready to land soon, we could break it out and merge it so that all three of you could use it on a rebase.
| if !exists { | ||
| cmdLog.Info("Creating object", "kind", kind, "name", name) | ||
| annotations = setTimestampAnnotations(owner, annotations) | ||
| if annotations != nil { |
There was a problem hiding this comment.
What if we add an annotation that isn't prefixed? The user supplied one would overwrite it, wouldn't it?
| if !exists { | ||
| cmdLog.Info("Creating object", "kind", kind, "name", name) | ||
| annotations = setTimestampAnnotations(owner, annotations) | ||
| if annotations != nil { |
There was a problem hiding this comment.
If I understand the SCAP result path, the aggregator will only add in the labels/annotations if they don't already exist. For the CEL path, it looks like it filters the operator annotations, but only based on the prefix, and doesn't use the same merge logic as the SCAP path? Is my understand correct there?
Align the CEL scanner path with the OpenSCAP aggregator by deferring custom metadata merge until after operator-managed labels/annotations are set. This uses MergeCustomMetadata so operator keys always win, preventing user-defined metadata from overwriting operator keys.
|
🤖 To deploy this PR, run the following command: |
|
/retest |
|
@Vincent056: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Propagate user-defined labels and annotations from Rule/CustomRule objects to their resulting ComplianceCheckResults, covering both OpenSCAP and CEL scan paths. Operator-managed keys are never overridden, and user metadata survives content updates.
enhancement doc for design details.