MustNotHave mode for OperatorPolicy #222

JustinKuli · 2024-04-02T15:55:34Z

For https://issues.redhat.com/browse/ACM-9287

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Also adjusts CSV reporting to handle when the CSV exists but the subscription does not, by using a label that OLM adds to these objects. The authorino operator was chosen because it has more than one CRD, but not too many to make a concise test. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

When the subscription has a ConstraintsNotSatisfiable condition, the exact content of the message is not always consistent. We have added other things to sort the message better, but it looks like we missed this test which was still assuming a certain part was first. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli · 2024-04-05T14:04:53Z

Tests passed twice, I'm finally feeling good about this 😆

yiraeChristineKim · 2024-04-05T19:38:27Z

api/v1beta1/operatorpolicy_types.go

+ //+kubebuilder:default=DeleteIfUnused
+ //+kubebuilder:validation:Enum=Keep;Delete;DeleteIfUnused
+ // Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
+ // will only delete the OperatorGroup if there is not another Subscription using it.


So "Delete" means delete even though another sub uses this?

Right, it means "Delete, regardless of any consequences". Kind of like a mustnothave configuration policy.

yiraeChristineKim · 2024-04-05T19:39:57Z

api/v1beta1/operatorpolicy_types.go

+
+ //+kubebuilder:default=Delete
+ //+kubebuilder:validation:Enum=Keep;Delete
+ // Specifies whether to delete the ClusterServiceVersion; defaults to 'Delete'


Can we add when this happens? maybe top of RemovalBehavior

In the CRD description for removalBehavior, it says "RemovalBehavior defines what resources will be removed by enforced mustnothave policies ..."

Is that what you meant? Or what's another place to put it?

yiraeChristineKim · 2024-04-05T19:41:57Z

api/v1beta1/operatorpolicy_types.go

- // Kind APIServiceDefinitions
- APIServiceDefinitions RemovalAction `json:"apiServiceDefinitions,omitempty"`
+
+ // Future?


😆 the whole feature was a TODO ...

This type / option was something I thought would be good during the design, but I haven't found anyone asking for it, or any examples of it yet, so I want to hold off on it until we can understand it better.

test/e2e/case38_install_operator_test.go

controllers/operatorpolicy_controller.go

mprahl · 2024-04-08T13:33:37Z

api/v1beta1/operatorpolicy_types.go

- // Kind APIServiceDefinitions
- APIServiceDefinitions RemovalAction `json:"apiServiceDefinitions,omitempty"`
+
+ // Future?


Let's just remove this commented out code

controllers/operatorpolicy_controller.go

mprahl · 2024-04-08T15:22:47Z

controllers/operatorpolicy_controller.go

+ return nil, updateStatus(policy, keptCond("ClusterServiceVersion"), relatedCSVs...), nil
+ }
+
+ for i := range csvList {


Is it possible to have more than one CSV?

In general, there might be more than one CSV: I think if one fails it might stay around even while another one is created. However, it's not clear to me if/when OLM will remove the label from an old CSV. So I would say it's very likely there would only be one CSV in this list, but it should handle the case where there are more.

controllers/operatorpolicy_status.go

mprahl · 2024-04-08T15:28:31Z

controllers/operatorpolicy_status.go

@@ -489,6 +560,17 @@ func subResFailedCond(subFailedCond operatorv1alpha1.SubscriptionCondition) meta
 return cond
 }

+// irrelevantCond returns a Compliant condition, with a Reason like '____Irrelevant',


Is NotApplicable a little better?

That does sound better

mprahl · 2024-04-08T16:58:17Z

@JustinKuli does the order of operations matter? To me, it seems like we should wait for the subscription to be fully deleted before deleting anything else so that we don't conflict with OLM. What do you think?

mprahl · 2024-04-08T17:08:43Z

test/e2e/case38_install_operator_test.go

+ opPolName = "oppol-authorino"
+ )
+ BeforeAll(func() {
+ utils.Kubectl("delete", "crd", "--selector=olm.managed=true")


Can this be more specific?

JustinKuli · 2024-04-08T17:10:46Z

@JustinKuli does the order of operations matter? To me, it seems like we should wait for the subscription to be fully deleted before deleting anything else so that we don't conflict with OLM. What do you think?

@mprahl that could be nice. I think that even if stuff was deleted in the wrong order, eventually this would get to the correct state, but it could be a lot of churn on the cluster: for example if the CSV was deleted before the Subscription was removed, OLM would probably re-create the CSV. The policy would reconcile several times and (probably) eventually get to a stable status.

This also makes me think that we might want to validate some relationships between the removalBehavior options: if Subscriptions is set to Keep, than it should probably be an error to set ClusterServiceVersions to Delete, since that would cause a loop with OLM.

mprahl · 2024-04-08T17:18:07Z

controllers/operatorpolicy_status.go

+ return metav1.Condition{
+ Type: condType(kind),
+ Status: metav1.ConditionTrue,
+ Reason: kind + "Excluded",


What about this?

Suggested change

Reason: kind + "Excluded",

Reason: kind + "NotPresent",

Updated this. "Included" is also now "Present" and the deleted and kept conditions are updated so that their messages are shorter and avoid the "checked by the mustnothave" language.

mprahl · 2024-04-08T17:19:50Z

controllers/operatorpolicy_status.go

+ Type: condType(kind),
+ Status: metav1.ConditionTrue,
+ Reason: kind + "Excluded",
+ Message: "the " + kind + " checked by the mustnothave policy was not found",


Perhaps we can get straight to the point. The user should know this is desired based on the compliance type they set.

Suggested change

Message: "the " + kind + " checked by the mustnothave policy was not found",

Message: "the " + kind + " is not present",

Enforced mustnohave policies will delete things based on the new RemovalBehavior field. In inform mode, resources that would be removed by an enforced policy will be causes for NonCompliance. This gives control to users in order to prevent unintended side effects. Refs: - https://issues.redhat.com/browse/ACM-9287 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

mprahl

/hold for other reviews and assuming that another PR will change deletes to be guarded by if a deletion timestamp is not set.

openshift-ci · 2024-04-08T20:32:34Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,mprahl]

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

zyjjay · 2024-04-09T19:20:43Z

controllers/operatorpolicy_status.go

@@ -600,13 +680,33 @@ func buildCSVCond(csv *operatorv1alpha1.ClusterServiceVersion) metav1.Condition
 }
 }

+// noCSVCond is a NonCompliant condition with Reason 'RelevantCSVNotFound'
 var noCSVCond = metav1.Condition{
 Type: csvConditionType,
 Status: metav1.ConditionFalse,


Maybe I missed something, but wouldn't this imply that the reason "RelevantCSVNotFound" is false here given the status of the condition?

Conditions and reasons can be confusing, and that's not limited to just our use of them. I think the status goes with the type, not the reason.

So I read them like <Type>? <Status>, because <Reason>. Extra info: <Message>

So in this case, "ClusterServiceVersionCompliant? False, because RelevantCSVNotFound. Extra info: A relevant installed ClusterServiceVersion could not be found"

Right, that makes sense.

zyjjay · 2024-04-09T19:22:45Z

api/v1beta1/operatorpolicy_types.go

+ //+kubebuilder:default=Keep
+ //+kubebuilder:validation:Enum=Keep;Delete
+ // Specifies whether to delete any CustomResourceDefinitions associated with the operator;
+ // defaults to 'Keep' because deleting them should be done deliberately
 CRDs RemovalAction `json:"customResourceDefinitions,omitempty"`


Do these CRDs refer to CRDs created by OLM as a prerequisite for installing the desired operator?

They are the CRDs created by OLM, as defined in the PackageManifest/ClusterServiceVersion for the operator.

JustinKuli · 2024-04-10T19:16:46Z

/unhold

This was added to the compliance message in open-cluster-management-io/config-policy-controller#222 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

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

openshift-merge-robot added the needs-rebase label Apr 2, 2024

openshift-ci bot added the approved label Apr 2, 2024

JustinKuli force-pushed the 9287-mustnothave branch from c94f6a1 to ff1f704 Compare April 2, 2024 16:49

openshift-merge-robot removed the needs-rebase label Apr 2, 2024

Add/Update some comment strings

59a18f8

Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli force-pushed the 9287-mustnothave branch from 2e1d948 to 268dc5d Compare April 4, 2024 15:39

JustinKuli marked this pull request as ready for review April 4, 2024 16:53

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

openshift-ci bot requested review from dhaiducek and gparvin April 4, 2024 16:53

JustinKuli requested review from mprahl, JeffeyL and zyjjay April 4, 2024 16:53

JustinKuli force-pushed the 9287-mustnothave branch from 268dc5d to 350f6fa Compare April 4, 2024 19:47

JustinKuli force-pushed the 9287-mustnothave branch from 350f6fa to 1cd841d Compare April 4, 2024 21:19

yiraeChristineKim reviewed Apr 5, 2024

View reviewed changes

test/e2e/case38_install_operator_test.go Outdated Show resolved Hide resolved

yiraeChristineKim reviewed Apr 5, 2024

View reviewed changes

test/e2e/case38_install_operator_test.go Outdated Show resolved Hide resolved

mprahl reviewed Apr 5, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Show resolved Hide resolved

mprahl reviewed Apr 5, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Outdated Show resolved Hide resolved