Handle preexisting operator #192

JustinKuli · 2024-01-25T16:45:03Z

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

In addition to handling more situations around whether the Subscription or OperatorGroup already exist, this adds condition reporting, related objects, and compliance events.

This PR also slims the CRD to what we believe can be functional in an initial release.

mprahl · 2024-01-25T22:07:57Z

/cc @mprahl

mprahl · 2024-01-25T22:12:26Z

controllers/configurationpolicy_controller.go

@@ -1741,7 +1741,7 @@ func (r *ConfigurationPolicyReconciler) handleSingleObj(

 if exists && !obj.shouldExist {
 // it is a mustnothave but it exist, so it must be deleted
- if strings.EqualFold(string(remediation), string(policyv1.Enforce)) {
+ if remediation.IsEnforce() {


@JustinKuli is the plan to keep mustnothave to remove what the operator created but nothing beyond that or is the plant to still remove mustnothave until the whole uninstall logic is added?

The plan for OperatorPolicy is to only support musthave in the initial release. I believe I removed all logic for it in the operator-policy-controller code, although there could still be remnants somewhere.

(This comment is attached to config-policy code, which of course will not be changed here)

mprahl · 2024-01-25T22:18:34Z

controllers/operatorpolicy_controller.go


- return reconcile.Result{}, err
+ foundOpGroups, err := r.DynamicWatcher.List(
+ watcher, operatorGroupGVK, desiredOpGroup.Namespace, labels.Everything())


Is labels.Everything() basically no selector? If so, you can optionally provide nil instead for the label selector.

Yes. I felt like labels.Everything() was more descriptive. I find I often pause and check what a nil selector does, because an empty selector would match nothing, but nil != empty in this case.

mprahl · 2024-01-25T22:20:04Z

controllers/operatorpolicy_controller.go

- exists := subExists && ogExists
- shouldExist := strings.EqualFold(string(policy.Spec.ComplianceType), string(policyv1.MustHave))
+ if policy.Spec.RemediationAction.IsEnforce() {
+ err = r.Create(ctx, desiredOpGroup)


Did you verify that this creates the object with strict field validation? I couldn't find anything in the controller-runtime code.

Strict field validation would return an error if there were any unknown or duplicated fields. (Any request will return an error if the provided fields fail CRD validation, eg if a required field is missing.) Since this request is using an "actual" operatorv1.OperatorGroup typed object for the request, it won't have any unknown or duplicate fields.

Extra context for other reviewers: we have been discussing what to do to handle version skew around the "embedded" types like OperatorGroup and Subscription. The OperatorPolicy CRD now uses a RawExtension to possibly help us handle the situation in the future. The kind of change we will need to worry about is if a field is added to the type we import, and a policy using that field is deployed on a cluster using an older CRD. In that case, I think the field would be dropped by the API server without an error.

Neither the OperatorGroup type not the Subscription type have had any recent changes that would cause version skews we need to worry about right now.

Instead of field validation on the API request, I think we should do the same kind of validation when we unmarshal the RawExtension into the type. (I think I missed that in this PR.)

@JustinKuli the oldest version we need to support is OCP 4.12 (https://access.redhat.com/articles/7027073). This seems to map to OLM API v0.17.3:
https://github.com/openshift/operator-framework-olm/blob/release-4.12/go.mod#L16

If you go to this link to compare v0.17.3 to the latest version, you can see that a couple of minor fields were added to the subscription:
operator-framework/api@v0.17.3...v0.21.0

It's easiest to check the crds/operators.coreos.com_subscriptions.yaml file.

So this would mean that using the latest github.com/operator-framework/api is fine but you need to error if those new fields were provided on OCP 4.12.

mprahl · 2024-01-25T22:26:29Z

controllers/operatorpolicy_controller.go

+ if policy.Spec.RemediationAction.IsEnforce() {
+ desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()
+
+ err := r.Update(ctx, desiredOpGroup)


This seems like "must only have" since I don't see any checking to see if the desired operator group is a subset of the existing operator group.

That's a good point, I missed considering that.

mprahl · 2024-01-25T22:29:47Z

controllers/operatorpolicy_controller.go

+ if policy.Spec.RemediationAction.IsEnforce() {
+ desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()
+
+ err := r.Update(ctx, desiredOpGroup)


Same comment as above, please ensure field validation is strict on the update.

mprahl · 2024-01-25T22:32:03Z

controllers/operatorpolicy_controller.go

- // Future implementation: Verify the specs of the object matches the one on the cluster
- OpLog.Info("The object already exists. Checking fields to verify matching specs")
+ if idx == -1 || existingCond.Status == metav1.ConditionFalse {
+ err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))


Would it be possible to include in a message that there is a preexisting operator group that was not verified since it was not specified in the policy?

Yes, I can adjust the condition for that easily (and then it will be reflected in the compliance event as well).

I have created a condition for this case with the message the policy does not specify an OperatorGroup but one already exists in the namespace - assuming that OperatorGroup is correct

controllers/operatorpolicy_controller.go

mprahl · 2024-01-25T22:35:48Z

controllers/operatorpolicy_controller.go

- r.setCompliance(ctx, policy, policyv1.NonCompliant)
- }
+ subNamespace, ok := sub["namespace"].(string)
+ if !ok {


Do you also want to check the length too?

To make sure that it is a valid kubernetes namespace identifier? Yeah, that seems reasonable.

mprahl · 2024-01-25T22:37:20Z

controllers/operatorpolicy_controller.go


- return reconcile.Result{}, err
- }
+ opGroup := make(map[string]interface{})


Same comment as for the subscription. I recommend using unstructured. It's the same amount of code and you don't have to type assert.

mprahl · 2024-01-25T22:37:36Z

controllers/operatorpolicy_controller.go

+ // Fallback to the Subscription namespace if the OperatorGroup namespace is not specified in the policy.
+ ogNamespace := subNamespace
+
+ if specifiedNS, ok := opGroup["namespace"].(string); ok {


Suggested change

if specifiedNS, ok := opGroup["namespace"].(string); ok {

if specifiedNS, ok := opGroup["namespace"].(string); ok || specifiedNS == "" {

mprahl · 2024-01-25T22:42:09Z

controllers/operatorpolicy_controller.go

+
+ spec := new(operatorv1.OperatorGroupSpec)
+
+ err = json.Unmarshal(policy.Spec.OperatorGroup.Raw, spec)


If you follow my other comments, you can use unstructured for the conversion but it does create an extra json.Marshal that your implementation does not.

err = k8sruntime.DefaultUnstructuredConverter.FromUnstructured(opgroup.UnstructuredContent(), &spec)

controllers/operatorpolicy_controller.go

mprahl · 2024-01-25T22:45:38Z

controllers/operatorpolicy_controller.go

 if err != nil {
- OpLog.Info(fmt.Sprintf("Failed to update policy status: %s", err))
+ return fmt.Errorf("error listing OperatorGroups: %w", err)


Suggested change

return fmt.Errorf("error listing OperatorGroups: %w", err)

return fmt.Errorf("error getting the subscription: %w", err)

mprahl · 2024-01-25T22:46:02Z

controllers/operatorpolicy_controller.go

+ }
+
+ if policy.Spec.RemediationAction.IsEnforce() {
+ err := r.Create(ctx, desiredSub)


Same comment about field validation.

mprahl · 2024-01-25T22:47:02Z

controllers/operatorpolicy_controller.go

+ return fmt.Errorf("error updating the status for a created Subscription: %w", err)
+ }
+ }
+ } else {


Optional: You could return nil at the end of the if block to prevent the indentation of else.

mprahl · 2024-01-25T22:49:54Z

controllers/operatorpolicy_controller.go

+ return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
+ }
+
+ if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {


This seems like a "must only have" check. Is the intention to only support "must only have"?

It wasn't my intention. It seems like I will need to bring in the heavy machinery from config-policy to handle this.

mprahl · 2024-01-25T22:50:58Z

controllers/operatorpolicy_controller.go

+ return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
+ }
+
+ if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {


Also, are there any omitempty that the user could specify in desiredSub but not be returned in foundSub?

Since we are dealing with unstructured here, I think you could move out some of the config-policy-controller code from checkAndUpdateResource to do the comparison for both must have and must only have.

mprahl · 2024-01-25T22:54:41Z

controllers/operatorpolicy_controller.go

+ // For now, (conditionally) mark it as compliant
+ idx, existingCond := policy.Status.GetCondition(subConditionType)
+
+ if idx == -1 || existingCond.Status == metav1.ConditionFalse {


Does updateStatus already check this? If so, you can remove this and I believe there's another place where this comment is relevant.

This is slightly different than what updateStatus checks. The code comment in the OperatorGroup section explains it more, and I should copy it down here.

This check prevents a previous "created" message from being overwritten with a "matches" message. My feeling was that the created message is more context and is generally more useful, but now I'm thinking that it might be confusing this way, because that's not how the config-policy status and events do it...

I have removed the if preventing the “matches” condition from being added if the previous condition was “created”. This should match how the config-policy works. The tests will need some changes to check this.

mprahl · 2024-01-25T22:57:03Z

controllers/operatorpolicy_controller.go

+ if policy.Spec.RemediationAction.IsEnforce() {
+ desiredSub.ResourceVersion = foundSub.GetResourceVersion()
+
+ err := r.Update(ctx, desiredSub)


Same comment about field validation.

mprahl · 2024-01-25T23:03:16Z

controllers/operatorpolicy_status.go

+//
+// Note that only changing the related objects will not emit a new compliance event, but will update
+// the status.
+func (r *OperatorPolicyReconciler) updateStatus(


I think the controller should send one compliance event per compliance change like the config-policy-controller to prevent so many compliance changes on the policy.

The config-policy-controller does it with "event batches" at the end of handleObjectTemplates. I imagine it'd be a lot simpler in the operator policy case since there is no namespace selector and not multiple object templates.

The number of status updates and compliance events is a valid critique. I wanted to send the updates and event immediately, because I felt it simplified error handling and limited the amount of information that needed to be returned by other functions.

I find what the config policy does with event batches very difficult to reason through... Can you carefully define "one compliance event per compliance change"?

Imagine a fresh enforced policy. With this implementation we get 4 status updates and events that look kinda like this:

NonCompliant; the operator group is missing (subscription details omitted because they are still unknown)

NonCompliant; the operator group was created (subscription details omitted because they are still unknown)

NonCompilant; the operator group was created and the subscription is missing

Compliant; the operator group was created and the subscription was created

Are you asking for just two events like these?

NonCompliant; the operator group is missing, and the subscription is missing,

Compliant; the operator group was created, and the subscription was created

Yeah, exactly. If I have time today, I can try to make the event batching logic more generic since I wrote that code.

The gist of the approach is that each handle method would return a list of events. So say operator group returned three events (two noncompliant) and subscription returned two event (one noncompliant), you would end up with three events in this way:

noncompliant operator group + existing state

noncompliant operator group and noncompliant subscription

compliant operator group and compliant subscription

mprahl · 2024-01-25T23:04:15Z

controllers/operatorpolicy_controller.go


- OpLog.Info("Creating the subscription", "kind", subscription.Kind,
- "namespace", subscription.Namespace)
+ err := json.Unmarshal(policy.Spec.Subscription.Raw, &sub)


Optional: Similar comments as in buildOperatorGroup for using unstructured.

mprahl · 2024-01-25T23:05:08Z

controllers/operatorpolicy_controller.go


- err := r.updatePolicyStatus(ctx, policy)
+ err = json.Unmarshal(policy.Spec.Subscription.Raw, spec)


Optional: Like I mentioned in buildOperatorGroup, are you able to just use this immediately rather than go the map route and then the typed route?

mprahl · 2024-01-25T23:06:31Z

controllers/operatorpolicy_controller.go


- err := r.updatePolicyStatus(ctx, policy)
+ err = json.Unmarshal(policy.Spec.Subscription.Raw, spec)


I don't think this will catch invalid/unknown fields. It looks like the unstructured package's converter to typed has some unknown field validation.

mprahl · 2024-01-26T14:36:03Z

deploy/crds/kustomize_operatorpolicy/kustomization.yaml

 patches:
- path: ns-selector-validation.json
+# The OperatorPolicy currently only supports "musthave"
+- path: allowed-compliance-types.json


This is a good workaround.

ComplianceType and RemediationActions were previously checked inline with string comparisons. This is more easily readable, and potentially better able to be changed in the future. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

It was decided to use RawExtension for the "embedded" Subscription and OperatorGroup, this can better allow for possible version skew in the future. The CRD validation would be limited inside the policy framework anyway because in a Policy, the raw content has no validation. This also removes some fields which are not expected to be functional in the initial release. The `build*` functions have been updated to use the RawExtension, and may have been adjusted for some other incoming changes. Some other temporary changes were made to resolve compilation errors and unit tests, but the OperatorPolicy "e2e" tests were not considered. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

The previous version of this function was only applicable to a Policy owning a ConfigurationPolicy. Now it is more broadly applicable. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli · 2024-01-26T20:40:45Z

I'm expecting the e2e tests to be failing. One of my first revisions to the PR was to add debugging to the test, but actually the way I tried to do that made it so the test could never register its failure 😆 (resolved as of 2024-01-30)

Work Item 1: As pointed out, some of the logic currently works more like "mustonlyhave" than "musthave", and with some of the type/untyped changes more recently, defaulted fields need to be considered more carefully. I will bring in some machinery from config-policy to help with this, and I expect it will also bring options for strict field validation. The build functions might return Unstructureds in the future.

Work Item 2: fewer compliance events / status update API calls. @mprahl in your opinion, is this required for this implementation, or is it something we could look at improving in the future?

mprahl · 2024-01-26T20:59:25Z

I'm expecting the e2e tests to be failing. One of my first revisions to the PR was to add debugging to the test, but actually the way I tried to do that made it so the test could never register its failure 😆

Work Item 1: As pointed out, some of the logic currently works more like "mustonlyhave" than "musthave", and with some of the type/untyped changes more recently, defaulted fields need to be considered more carefully. I will bring in some machinery from config-policy to help with this, and I expect it will also bring options for strict field validation. The build functions might return Unstructureds in the future.

Work Item 2: fewer compliance events / status update API calls. @mprahl in your opinion, is this required for this implementation, or is it something we could look at improving in the future?

I think work item 2 can be delayed. It shouldn't change QE's tests so it should be safe to handle early next sprint and get it in by the release. I'd be happy to do it if you create a ticket.

yiraeChristineKim · 2024-01-30T20:55:36Z

api/v1beta1/operatorpolicy_types.go

+// Type is found.
+func (status OperatorPolicyStatus) GetCondition(condType string) (int, metav1.Condition) {
+ for i, cond := range status.Conditions {
+ if cond.Type == condType {


What if multiple conditions match? is it okay to return only the first match?

I think it's ok to just return the first match. Ideally we would have some way of enforcing that there are not multiple conditions of the same type (the actual k8s APIs can use listType=map like this, but that didn't seem to be available for CRDs). We'll just need to be careful not to accidentally put the same condition twice.

good to learn Thanks

controllers/configurationpolicy_controller.go

controllers/operatorpolicy_controller.go

The general plan is to continue adding `handle*` functions for the other resources that an OperatorPolicy needs to examine. Each handle function will update the policy's status (with conditions and relatedObjects), and possibly emit compliance events. This may cause more compliance events "than usual" compared to other controllers, but I think the separation of concerns will help each function be more maintainable. My hope is that some of the `*Cond` and `*Obj` functions in the status section can be reused in the future handlers. There was already overlap between the Subscription and OperatorGroup, so this seemed reasonable. To check if the Subscription/OperatorGroup on the cluster matches what is desired by the policy, a function `handleKeys` was created that can be used for OperatorPolicies and ConfigurationPolicies. Refs: - https://issues.redhat.com/browse/ACM-9283 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

test/e2e/case38_install_operator_test.go

yiraeChristineKim · 2024-01-31T15:23:30Z

test/e2e/case38_install_operator_test.go

+ )
+ })
+ It("Should match when the OperatorGroup is manually corrected", func() {
+ utils.Kubectl("delete", "operatorgroup", incorrectOpGroupName, "-n", opPolTestNS)


OperatorGroup is specified and another OperatorGroup also exists, then what happens?

Can we add one more case(it)

openshift-ci · 2024-01-31T16:22:40Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

dhaiducek

Taking a look at the new PR also, but I wanted to give this one a look first. 🙂

dhaiducek · 2024-01-31T23:45:09Z

api/v1/configurationpolicy_types.go

+func (ra RemediationAction) IsInform() bool {
+ return strings.EqualFold(string(ra), string(Inform))
+}
+
+func (ra RemediationAction) IsEnforce() bool {
+ return strings.EqualFold(string(ra), string(Enforce))
+}
+


Oooh, these are nice. 😄

dhaiducek · 2024-01-31T23:53:21Z

controllers/configurationpolicy_controller.go

- mergedAnnotations, _, _ := unstructured.NestedStringMap(mdMap, "annotations")
- mergedLabels, _, _ := unstructured.NestedStringMap(mdMap, "labels")
+ if updateNeeded {
+ mismatchLog := "Detected value mismatch"


I see the key was removed from the log, which I think is fine since I was debating the value of providing a root key anyway. The ideal in my mind would to be able to accumulate the JSON path through handleKeys()/handleSingleKey() and log that, but that's just my own musing.

openshift-ci bot added the dco-signoff: yes label Jan 25, 2024

openshift-ci bot requested review from dhaiducek and gparvin January 25, 2024 16:45

openshift-ci bot added the approved label Jan 25, 2024

JustinKuli force-pushed the 9283-handle-preexisting-operator branch from 095cc77 to 2d800eb Compare January 25, 2024 18:22

openshift-ci bot requested a review from mprahl January 25, 2024 22:07

mprahl reviewed Jan 25, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Outdated Show resolved Hide resolved

mprahl reviewed Jan 25, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Show resolved Hide resolved

mprahl reviewed Jan 25, 2024

View reviewed changes

mprahl reviewed Jan 26, 2024

View reviewed changes

JustinKuli added 3 commits January 26, 2024 14:35

Add helpers for some enums

e246345

ComplianceType and RemediationActions were previously checked inline with string comparisons. This is more easily readable, and potentially better able to be changed in the future. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

Adjust test helper for owned objects

2978f4e

The previous version of this function was only applicable to a Policy owning a ConfigurationPolicy. Now it is more broadly applicable. Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>

JustinKuli force-pushed the 9283-handle-preexisting-operator branch from 2d800eb to a8fc211 Compare January 26, 2024 19:36

JustinKuli force-pushed the 9283-handle-preexisting-operator branch 4 times, most recently from 097b7c8 to 3546ee8 Compare January 30, 2024 19:30

yiraeChristineKim reviewed Jan 30, 2024

View reviewed changes

controllers/configurationpolicy_controller.go Show resolved Hide resolved

yiraeChristineKim reviewed Jan 30, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Show resolved Hide resolved

yiraeChristineKim reviewed Jan 30, 2024

View reviewed changes

controllers/operatorpolicy_controller.go Show resolved Hide resolved

JustinKuli force-pushed the 9283-handle-preexisting-operator branch from 3546ee8 to c358a12 Compare January 30, 2024 22:03

yiraeChristineKim reviewed Jan 31, 2024

View reviewed changes

test/e2e/case38_install_operator_test.go Show resolved Hide resolved

yiraeChristineKim reviewed Jan 31, 2024

View reviewed changes

yiraeChristineKim approved these changes Jan 31, 2024

View reviewed changes

openshift-ci bot assigned yiraeChristineKim Jan 31, 2024

openshift-ci bot added the lgtm label Jan 31, 2024

openshift-merge-bot bot merged commit 56ace86 into open-cluster-management-io:main Jan 31, 2024
4 checks passed

This was referenced Jan 31, 2024

🤖 Sync from open-cluster-management-io/config-policy-controller: #192 stolostron/config-policy-controller#695

Closed

😿 Failed to sync the upstream PRs: #192 stolostron/config-policy-controller#696

Closed

dhaiducek reviewed Feb 1, 2024

View reviewed changes

This was referenced Feb 1, 2024

Synchronize OperatorPolicy CRD open-cluster-management-io/governance-policy-addon-controller#125

Merged

Sync 192 193 194 stolostron/config-policy-controller#701

Merged

JustinKuli deleted the 9283-handle-preexisting-operator branch July 25, 2024 13:41

	if specifiedNS, ok := opGroup["namespace"].(string); ok {
	if specifiedNS, ok := opGroup["namespace"].(string); ok \|\| specifiedNS == "" {


		spec := new(operatorv1.OperatorGroupSpec)

		err = json.Unmarshal(policy.Spec.OperatorGroup.Raw, spec)

	return fmt.Errorf("error listing OperatorGroups: %w", err)
	return fmt.Errorf("error getting the subscription: %w", err)


		err := r.updatePolicyStatus(ctx, policy)
		err = json.Unmarshal(policy.Spec.Subscription.Raw, spec)

Handle preexisting operator #192

Handle preexisting operator #192

Conversation

JustinKuli commented Jan 25, 2024

mprahl commented Jan 25, 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

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

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 commented Jan 26, 2024 • edited Loading

mprahl commented Jan 26, 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

openshift-ci bot commented Jan 31, 2024

dhaiducek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinKuli commented Jan 26, 2024 •

edited

Loading