Better handling of OperatorPolicy advanced fields #414
Conversation
It seems like this was a typo in the original implementation - the controller correctly updates the resource to match the "merged" version, but was then using the "desired" version of the resource (which only specifies what parts the user was interested in, and does not reflect the full live state) when updating the status of the policy. The updated section in `musthaveOpGroup` now more closely matches the analogous section in `musthaveSubscription`. Signed-off-by: Justin Kulikauskas <[email protected]>
| unstructured.SetNestedMap(existing.Object, desiredConfig, "spec", "config") | ||
| } else if eq, _ := deeplyEquivalent(desiredConfig, existingConfig, true); !eq { | ||
| forceUpdate = true | ||
| unstructured.SetNestedMap(existing.Object, desiredConfig, "spec", "config") |
There was a problem hiding this comment.
I guess you will update this? return value
There was a problem hiding this comment.
Yeah the linter complained 😅
The error should be impossible because the earlier check verified that .spec.config was a Map.
| } | ||
| } | ||
|
|
||
| func (r *OperatorPolicyReconciler) mergeOpGroups( |
There was a problem hiding this comment.
Are you going to add unit tests for MergeOgGroups and MergeSubscriptions?
There was a problem hiding this comment.
I wasn't planning to, do you think it's necessary?
| func (r *OperatorPolicyReconciler) mergeObjects( | ||
| ctx context.Context, | ||
| desired map[string]interface{}, | ||
| desired map[string]any, |
There was a problem hiding this comment.
curious interface any are same from doc said.. why did you change?
There was a problem hiding this comment.
Just my editor complaining about it.
|
|
||
| //nolint:dogsled | ||
| _, errMsg, updateNeeded, _, _ := handleKeys(desiredObj, existing, existingObjectCopy, complianceType, "") | ||
| _, errMsg, updateNeeded, _, _ := handleKeys(desiredObj, existing, existingObjectCopy, policyv1.MustHave, "") |
There was a problem hiding this comment.
At first, the user added a OperatorGroup to Operatorpolicy. Later, they changed their mind and wanted to use the default OperatorGroup instead. So they deleted the OperatorGroup from the operator policy and set mutUserOnlyHave.
In this case, will the OperatorGroup be removed and a new one created?
There was a problem hiding this comment.
OperatorPolicy can not be set to mustonlyhave: https://github.com/open-cluster-management-io/config-policy-controller/blob/main/api/v1beta1/operatorpolicy_types.go#L156
Also, OperatorPolicy will try to never create a second OperatorGroup in a namespace: https://github.com/open-cluster-management-io/config-policy-controller/blob/main/controllers/operatorpolicy_controller.go#L1138 which might answer part of your question.
But generally if a user declared an OperatorGroup with specific configurations, and then wanted to revert to a more normal one, they would need to edit the policy to specify what they wanted in the group. If that part of the policy is just removed, it will just consider the existing OperatorGroup acceptable.
| } | ||
|
|
||
| // when specified, manually handle targetNamespaces as if it's "mustonlyhave" | ||
| if len(desired.Spec.TargetNamespaces) > 0 { |
There was a problem hiding this comment.
If users want to delete targetNamespace can user use mustOnlyHave?
There was a problem hiding this comment.
If they initially set targetNamespaces, but then wanted to remove it from the OperatorGroup, I think they would need to use a ConfigurationPolicy (or manually apply the change on the cluster)
There was a problem hiding this comment.
Oh one more thing If users changed the TargetNamespaces then what happen to old targetnamespace. Do we remove resources in previous targetnamespace?
The optional `config` field in a Subscription, and the optional `targetNamespaces` and `selector` fields in an OperatorGroup often need to exactly match what the user has specified, rather than being merged with what might already be there. That is, when they are specified, it is usually better to treat them with `mustonlyhave` semantics. Previously, if a user adjusted the `targetNamespaces` to change it from one namespace to another (maybe they had a typo originally), the existing list and the new list would be merged, and the operator would likely not deploy as intended. Similarly, if a user had a nodeSelector in their `config`, adjustments would often result in a selector that did not match any nodes. Now, when one of those specific fields is set, it is handled better. When the field is not set, the existing configuration is still used. Refs: - https://issues.redhat.com/browse/ACM-22555 Signed-off-by: Justin Kulikauskas <[email protected]>
JustinKuli
force-pushed
the
22555-operatorpolicy-advanced-fields
branch
from
0144f8b to
a378f03
Compare
|
[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:
Approvers can indicate their approval by writing |
|
/hold |
2 participants
Mainly
For https://issues.redhat.com/browse/ACM-22555