Implement new ComplianceConfig field #252

zyjjay · 2024-05-23T20:22:38Z

Users can define how specific resource statuses affect the overall OperatorPolicy compliance events.

ref: https://issues.redhat.com/browse/ACM-11023

JustinKuli

Looks good overall to me! I think the one e2e test, combined with the unit tests is sufficient for verifying the behavior.

I think we're considering changing the default behaviors, which look easy to adjust here.

JustinKuli

Just some early comments - they might be things you're already looking at changing.

JustinKuli · 2024-05-30T14:09:17Z

controllers/operatorpolicy_status.go

 // FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`.
 // For now this condition assumes it is set to 'NonCompliant'
+ canModifyComp := complianceConfig == "NonCompliant"


I think I'd prefer using complianceConfig == "NonCompliant" directly in the condition below, it might make it easier in the future if we add other options. Also just a reminder to remove the // FUTURE comment.

JustinKuli · 2024-05-30T14:11:23Z

controllers/operatorpolicy_status.go

@@ -775,6 +794,11 @@ func buildDeploymentCond(
 message = fmt.Sprintf("the deployments %s do not have their minimum availability", names)
 }

+ if !canModifyComp {
+ status = metav1.ConditionTrue
+ message += " - ignore, compliance overridden by complianceConfig"


Can we add a conditional to only add this message if it is overriding the status? It looks to me like it would be adding this even if all of the deployments are available.

JustinKuli · 2024-05-30T14:13:29Z

controllers/operatorpolicy_status.go

@@ -1044,7 +1079,7 @@ func noInstallPlansObj(namespace string) policyv1.RelatedObject {
 // existingInstallPlanObj returns a RelatedObject for the InstallPlan, with a reason
 // like 'The InstallPlan is ____' based on the phase. When the InstallPlan is in phase
 // 'Complete', the object will be Compliant, otherwise it will be NonCompliant.
-func existingInstallPlanObj(ip client.Object, phase string) policyv1.RelatedObject {
+func existingInstallPlanObj(ip client.Object, phase string, complianceConfig string) policyv1.RelatedObject {


I would prefer if this took type policyv1beta1.ComplianceConfigAction, there are two conversions to string that would be unnecessary.

mprahl · 2024-05-31T13:07:25Z

Could you please squash the commits and update the PR title and commit title to mention complianceConfig instead of statusConfig?

mprahl · 2024-05-31T13:08:40Z

api/v1beta1/operatorpolicy_types.go

+ // Compliant is a ComplianceConfigAction that only shows the status message and
+ // sets thje compliance to Compliant


Suggested change

// Compliant is a ComplianceConfigAction that only shows the status message and

// sets thje compliance to Compliant

// Compliant is a ComplianceConfigAction that only shows the status message and

// does not affect the overall compliance.

mprahl · 2024-05-31T13:09:16Z

api/v1beta1/operatorpolicy_types.go

+ // NonCompliant is a ComplianceConfigAction that shows the status message and sets
 // the compliance to NonCompliant


Suggested change

// NonCompliant is a ComplianceConfigAction that shows the status message and sets

// the compliance to NonCompliant

// NonCompliant is a ComplianceConfigAction that shows the status message and sets

// the overall compliance to NonCompliant when the condition is met

mprahl · 2024-05-31T13:09:32Z

api/v1beta1/operatorpolicy_types.go

- DeploymentsUnavailable StatusConfigAction `json:"deploymentsUnavailable,omitempty"`
- UpgradesAvailable StatusConfigAction `json:"upgradesAvailable,omitempty"`
- UpgradesProgressing StatusConfigAction `json:"upgradesProgressing,omitempty"`
+// ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance


Suggested change

// ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance

// ComplianceConfig defines how resource statuses affect the OperatorPolicy compliance

mprahl · 2024-05-31T13:11:55Z

api/v1beta1/operatorpolicy_types.go

- UpgradesProgressing StatusConfigAction `json:"upgradesProgressing,omitempty"`
+// ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance
+type ComplianceConfig struct {
+ // +kubebuilder:default=Compliant


I like the idea of having a default, but this does mean the template sync equivalentPolicies will have to be modified to not think the OperatorPolicy on the cluster is different than the one specified in the Policy object if the defaults aren't specified in the Policy object. See how this is handled for ConfigurationPolicy:
https://github.com/open-cluster-management-io/governance-policy-framework-addon/blob/c34eec6923b9f4079977c116ecd513f1eb9f22b7/controllers/templatesync/template_sync.go#L958

Seems like I'll need to do some similar changes like this for some other fields, sorry I missed it!

mprahl · 2024-05-31T13:13:34Z

api/v1beta1/operatorpolicy_types.go

@@ -130,7 +133,7 @@ type OperatorPolicySpec struct {
 // in 'inform' mode, and which installPlans are approved when in 'enforce' mode
 Versions []policyv1.NonEmptyString `json:"versions,omitempty"`

- //+kubebuilder:default={}
+ // +kubebuilder:default={}


Generally, comments related to autogenerating code don't have a space after // but I'm not strict about this. Some others on the team may be 😉

mprahl · 2024-05-31T13:15:59Z

api/v1beta1/operatorpolicy_types.go

+ // ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance.
+ // Options include Compliant, which will essentially ignore NonCompliance for certain cases,
+ // and NonCompliant, which will report NonCompliance for those cases.


Suggested change

// ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance.

// Options include Compliant, which will essentially ignore NonCompliance for certain cases,

// and NonCompliant, which will report NonCompliance for those cases.

// ComplianceConfig defines how resource statuses affect the OperatorPolicy compliance.

// When set to Compliant, the condition does not impact the OperatorPolicy compliance. When

// set to NonCompliant, the condition causes the OperatorPolicy to become NonComplaint.

mprahl · 2024-05-31T13:18:21Z

controllers/operatorpolicy_controller.go

@@ -1356,7 +1356,9 @@ func (r *OperatorPolicyReconciler) musthaveInstallPlan(
 }
 }

- relatedInstallPlans = append(relatedInstallPlans, existingInstallPlanObj(&ownedInstallPlans[i], phase))
+ complianceConfig := policy.Spec.ComplianceConfig.UpgradesAvailable


Could you set this once outside of the loop so it's not redefined every loop iteration and the code below can reference it as well?

controllers/operatorpolicy_status.go

controllers/operatorpolicy_controller.go

mprahl · 2024-05-31T13:31:17Z

controllers/operatorpolicy_controller.go

@@ -1680,15 +1685,19 @@ func (r *OperatorPolicyReconciler) handleDeployment(
 if policy.Spec.ComplianceType.IsMustNotHave() {
 relatedObjects = append(relatedObjects, foundNotApplicableObj(&dep))
 } else {
- relatedObjects = append(relatedObjects, existingDeploymentObj(&dep))
+ complianceConfig := policy.Spec.ComplianceConfig.DeploymentsUnavailable


Consider defining this once outside of the loop.

mprahl · 2024-05-31T13:32:33Z

controllers/operatorpolicy_status.go

@@ -669,12 +675,13 @@ var installPlansNoApprovals = metav1.Condition{

 // installPlanUpgradeCond is a NonCompliant condition with Reason 'InstallPlanRequiresApproval'
 // and a message detailing which possible updates are available


The docstring needs to be updated to mention the new argument.

mprahl · 2024-05-31T13:34:38Z

controllers/operatorpolicy_status.go

@@ -693,6 +700,13 @@ func installPlanUpgradeCond(versions []string, approvableIPs []unstructured.Unst
 cond.Message += " but multiple of those match the versions specified in the policy"
 }

+ if complianceConfig == "NonCompliant" {


Optional: It doesn't really matter in this case because there are only two values, but I prefer checking specifically for "Compliant" and if it's not, assume it's "NonCompliant". If we add additional options in the future, this is clearer.

mprahl · 2024-05-31T13:35:54Z

controllers/operatorpolicy_status.go

+ cond.Status = metav1.ConditionFalse
+ } else {
+ cond.Status = metav1.ConditionTrue
+ cond.Message += " - ignore, compliance overridden by complianceConfig"


I don't think this additional message is required. Now that the default is "Compliant", I don't think overridden is the correct term.

I think because this condition is a NonCompliant condition, it would make sense to to say it was overridden. Especially since the condition message makes it to the final policy compliance event, it might be useful to see this detail.

I think I somehow agree with both of you 😆 - saying its overriden doesn't feel right, but I don't want to not mention that the possible upgrade is being ignored.

Maybe a rephrasing would help, something like " - not triggering noncompliance because of complianceConfig".

JustinKuli

Looking good overall, I'd be ok merging this if Matt's discussions are all resolved... I do have some nits, and I'm wondering if we should add info to the related objects when their compliance is affected by the complianceConfig. But that's not strictly necessary to do now.

controllers/operatorpolicy_status.go

JeffeyL · 2024-06-05T15:07:12Z

Per Matt's previous mention, this PR (open-cluster-management-io/governance-policy-framework-addon#145) in the framework-addon addresses the template sync for default values of complianceConfig

JustinKuli

/hold for @mprahl

LGTM

mprahl · 2024-06-05T15:36:52Z

api/v1beta1/operatorpolicy_types.go

+ //+kubebuilder:default={}
+ // ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance.
+ // When set to Compliant, the condition does not impact the OperatorPolicy compliance.
+ // When set to NonCompliant, the condition cause the OperatorPolicy to become NonCompliant.


Suggested change

// When set to NonCompliant, the condition cause the OperatorPolicy to become NonCompliant.

// When set to NonCompliant, the condition causes the OperatorPolicy to become NonCompliant.

Can I assume you meant "causes"?

Yup, you caught it before the edit 😆

mprahl · 2024-06-05T15:42:04Z

controllers/operatorpolicy_status.go

+ relObj.Compliant = string(policyv1.NonCompliant)
+ } else {
+ relObj.Compliant = string(policyv1.Compliant)
+ relObj.Reason += " (policy compliance is not impacted due to spec.complianceConfig.upgradesAvailable)"


I find this message to be quite noisy for a default behavior. My thought is that we only need to include a message such as this when the default is overridden.

@JustinKuli what do you think?

I don't think it's prohibitively noisy, but I see what you mean with default behaviors. So then should we have a message on the NonCompliant case? Maybe "(in violation due to spec.complianceConfig.upgradesAvailable)"?

mprahl · 2024-06-05T15:42:20Z

controllers/operatorpolicy_status.go

+ relObj.Compliant = string(policyv1.NonCompliant)
+ } else {
+ relObj.Compliant = string(policyv1.Compliant)
+ relObj.Reason += " (policy compliance is not impacted due to spec.complianceConfig.upgradesAvailable)"


Same as above.

JustinKuli · 2024-06-05T16:45:11Z

controllers/operatorpolicy_status.go

+ if complianceConfig != "Compliant" {
+ compliance = string(policyv1.NonCompliant)
+ } else {
+ reason += " (policy compliance is not impacted due to spec.complianceConfig.catalogSourceUnhealthy)"


@mprahl how do you feel about this one. I think "___ exists but is unhealthy" on its own in a Compliant object could be confusing. It is the default behavior, but I feel like mentioning why "unhealthy" is ok in this case is good.

Users can define how specific resource statuses affect the overall OperatorPolicy status and compliance events. ref: https://issues.redhat.com/browse/ACM-11023 Signed-off-by: Jason Zhang <jaszhang@redhat.com>

JustinKuli

/unhold

LGTM, I think you covered Matt's suggestions, thanks

openshift-ci · 2024-06-05T20:17:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JustinKuli, zyjjay

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:

~~OWNERS~~ [JustinKuli]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added do-not-merge/work-in-progress dco-signoff: yes labels May 23, 2024

JustinKuli reviewed May 23, 2024

View reviewed changes

zyjjay force-pushed the ACM-11023 branch from 394498a to 75726b3 Compare May 27, 2024 17:27

JeffeyL force-pushed the ACM-11023 branch from d0b0c15 to 949034e Compare May 29, 2024 20:21

zyjjay force-pushed the ACM-11023 branch from 949034e to d0b0c15 Compare May 29, 2024 20:41

JustinKuli reviewed May 30, 2024

View reviewed changes

JeffeyL force-pushed the ACM-11023 branch 4 times, most recently from fedc6e1 to 28b795e Compare May 30, 2024 20:17

zyjjay marked this pull request as ready for review May 30, 2024 21:08

openshift-ci bot removed the do-not-merge/work-in-progress label May 30, 2024

openshift-ci bot requested review from gparvin and mprahl May 30, 2024 21:09

mprahl reviewed May 31, 2024

View reviewed changes

controllers/operatorpolicy_status.go Show resolved Hide resolved

mprahl reviewed May 31, 2024

View reviewed changes

controllers/operatorpolicy_status.go Show resolved Hide resolved

mprahl reviewed May 31, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Show resolved Hide resolved

mprahl reviewed May 31, 2024

View reviewed changes

JeffeyL force-pushed the ACM-11023 branch from 2b0866a to 108242a Compare June 4, 2024 17:16

JustinKuli reviewed Jun 4, 2024

View reviewed changes

controllers/operatorpolicy_status.go Show resolved Hide resolved

controllers/operatorpolicy_status.go Outdated Show resolved Hide resolved

controllers/operatorpolicy_status.go Show resolved Hide resolved

JeffeyL force-pushed the ACM-11023 branch from 108242a to 85091bf Compare June 4, 2024 20:37

zyjjay force-pushed the ACM-11023 branch from 85091bf to 29915d9 Compare June 4, 2024 20:39

JeffeyL force-pushed the ACM-11023 branch from 29915d9 to b790ecb Compare June 5, 2024 14:07

JustinKuli previously approved these changes Jun 5, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Jun 5, 2024

openshift-ci bot assigned JustinKuli Jun 5, 2024

openshift-ci bot added lgtm approved labels Jun 5, 2024

mprahl reviewed Jun 5, 2024

View reviewed changes

JustinKuli reviewed Jun 5, 2024

View reviewed changes

JeffeyL dismissed JustinKuli’s stale review via 7da280f June 5, 2024 19:56

JeffeyL force-pushed the ACM-11023 branch from b790ecb to 7da280f Compare June 5, 2024 19:56

openshift-ci bot removed lgtm approved labels Jun 5, 2024

Implement new complianceConfig field

6dba49a

Users can define how specific resource statuses affect the overall OperatorPolicy status and compliance events. ref: https://issues.redhat.com/browse/ACM-11023 Signed-off-by: Jason Zhang <jaszhang@redhat.com>

JeffeyL force-pushed the ACM-11023 branch from 7da280f to 6dba49a Compare June 5, 2024 20:01

JustinKuli approved these changes Jun 5, 2024

View reviewed changes

openshift-ci bot added lgtm and removed do-not-merge/hold labels Jun 5, 2024

openshift-ci bot added the approved label Jun 5, 2024

openshift-merge-bot bot merged commit 4992fdc into open-cluster-management-io:main Jun 5, 2024
9 checks passed

magic-mirror-bot bot mentioned this pull request Jun 5, 2024

🤖 Sync from open-cluster-management-io/config-policy-controller: #252 stolostron/config-policy-controller#889

Merged

dhaiducek mentioned this pull request Jun 6, 2024

Sync OperatorPolicy CRD complianceConfig field open-cluster-management-io/governance-policy-addon-controller#159

Merged

mprahl mentioned this pull request Jun 7, 2024

Add support for approving InstallPlans with multiple CSVs #260

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new ComplianceConfig field #252

Implement new ComplianceConfig field #252

zyjjay commented May 23, 2024 •

edited

Loading

JustinKuli left a comment

JustinKuli left a comment

JustinKuli May 30, 2024

JustinKuli May 30, 2024

JustinKuli May 30, 2024

mprahl commented May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

JustinKuli Jun 3, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

mprahl May 31, 2024

zyjjay May 31, 2024

JustinKuli Jun 3, 2024

JustinKuli left a comment

JeffeyL commented Jun 5, 2024 •

edited

Loading

JustinKuli left a comment

mprahl Jun 5, 2024 •

edited

Loading

JeffeyL Jun 5, 2024

mprahl Jun 5, 2024

mprahl Jun 5, 2024

mprahl Jun 5, 2024

JustinKuli Jun 5, 2024

mprahl Jun 5, 2024

JustinKuli Jun 5, 2024

JustinKuli left a comment

openshift-ci bot commented Jun 5, 2024

		// Compliant is a ComplianceConfigAction that only shows the status message and
		// sets thje compliance to Compliant

		// NonCompliant is a ComplianceConfigAction that shows the status message and sets
		// the compliance to NonCompliant

	// ComplianceConfig defines how resource statuses affect the OperatorPolicy status and compliance
	// ComplianceConfig defines how resource statuses affect the OperatorPolicy compliance

		@@ -669,12 +675,13 @@ var installPlansNoApprovals = metav1.Condition{

		// installPlanUpgradeCond is a NonCompliant condition with Reason 'InstallPlanRequiresApproval'
		// and a message detailing which possible updates are available

	// When set to NonCompliant, the condition cause the OperatorPolicy to become NonCompliant.
	// When set to NonCompliant, the condition causes the OperatorPolicy to become NonCompliant.

Implement new ComplianceConfig field #252

Implement new ComplianceConfig field #252

Conversation

zyjjay commented May 23, 2024 • edited Loading

JustinKuli left a comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mprahl commented May 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

JeffeyL commented Jun 5, 2024 • edited Loading

JustinKuli left a comment

Choose a reason for hiding this comment

mprahl Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jun 5, 2024

zyjjay commented May 23, 2024 •

edited

Loading

JeffeyL commented Jun 5, 2024 •

edited

Loading

mprahl Jun 5, 2024 •

edited

Loading