🤖 Sync from open-cluster-management-io/config-policy-controller: #230, #231

api/v1beta1/operatorpolicy_types.go

@@ -51,9 +51,9 @@ func (ra RemovalAction) IsDeleteIfUnused() bool {
 
 type RemovalBehavior struct {
  //+kubebuilder:default=DeleteIfUnused
- //+kubebuilder:validation:Enum=Keep;Delete;DeleteIfUnused
+ //+kubebuilder:validation:Enum=Keep;DeleteIfUnused
  // Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
- // will only delete the OperatorGroup if there is not another Subscription using it.
+ // will only delete the OperatorGroup if there is not another resource using it.
  OperatorGroups RemovalAction `json:"operatorGroups,omitempty"`
 
  //+kubebuilder:default=Delete

controllers/operatorpolicy_controller.go

@@ -226,7 +226,12 @@ func (r *OperatorPolicyReconciler) handleResources(ctx context.Context, policy *
  return earlyComplianceEvents, condChanged, err
  }
 
- earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG)
+ desiredSubName := ""
+ if desiredSub != nil {
+ desiredSubName = desiredSub.Name
+ }
+
+ earlyConds, changed, err := r.handleOpGroup(ctx, policy, desiredOG, desiredSubName)
  earlyComplianceEvents = append(earlyComplianceEvents, earlyConds...)
  condChanged = condChanged || changed
 
@@ -567,7 +572,10 @@ func buildOperatorGroup(
 }
 
 func (r *OperatorPolicyReconciler) handleOpGroup(
- ctx context.Context, policy *policyv1beta1.OperatorPolicy, desiredOpGroup *operatorv1.OperatorGroup,
+ ctx context.Context,
+ policy *policyv1beta1.OperatorPolicy,
+ desiredOpGroup *operatorv1.OperatorGroup,
+ desiredSubName string,
 ) ([]metav1.Condition, bool, error) {
  watcher := opPolIdentifier(policy.Namespace, policy.Name)
 
@@ -586,7 +594,7 @@ func (r *OperatorPolicyReconciler) handleOpGroup(
  return r.musthaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
  }
 
- return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups)
+ return r.mustnothaveOpGroup(ctx, policy, desiredOpGroup, foundOpGroups, desiredSubName)
 }
 
 func (r *OperatorPolicyReconciler) musthaveOpGroup(
@@ -716,44 +724,72 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
  ctx context.Context,
  policy *policyv1beta1.OperatorPolicy,
  desiredOpGroup *operatorv1.OperatorGroup,
- foundOpGroups []unstructured.Unstructured,
+ allFoundOpGroups []unstructured.Unstructured,
+ desiredSubName string,
 ) ([]metav1.Condition, bool, error) {
- if len(foundOpGroups) == 0 {
+ if len(allFoundOpGroups) == 0 {
  // Missing OperatorGroup: report Compliance
  changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
 
  return nil, changed, nil
  }
 
- var foundOpGroupName string
- var foundOpGroup *unstructured.Unstructured
+ if len(allFoundOpGroups) > 1 {
+ // Don't try to choose one, just report this as NonCompliant.
+ return nil, updateStatus(policy, opGroupTooManyCond, opGroupTooManyObjs(allFoundOpGroups)...), nil
+ }
 
- for _, opGroup := range foundOpGroups {
- emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
+ foundOpGroup := allFoundOpGroups[0]
 
- if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
- foundOpGroupName = opGroup.GetName()
- foundOpGroup = opGroup.DeepCopy()
+ removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
+ keep := removalBehavior.OperatorGroups.IsKeep()
 
- break
+ // if DeleteIfUnused and there are other subscriptions, then keep the operator group
+ if removalBehavior.OperatorGroups.IsDeleteIfUnused() {
+ // Check the namespace for any subscriptions, including the sub for this mustnothave policy,
+ // since deleting the OperatorGroup before that could cause problems
+ watcher := opPolIdentifier(policy.Namespace, policy.Name)
+
+ foundSubscriptions, err := r.DynamicWatcher.List(
+ watcher, subscriptionGVK, desiredOpGroup.Namespace, labels.Everything())
+ if err != nil {
+ return nil, false, fmt.Errorf("error listing Subscriptions: %w", err)
+ }
+
+ anotherSubFound := false
+
+ for _, sub := range foundSubscriptions {
+ if sub.GetName() != desiredSubName {
+ anotherSubFound = true
+
+ break
+ }
  }
+
+ if anotherSubFound {
+ keep = true
+ }
+ }
+
+ if keep {
+ return nil, updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(&foundOpGroup)), nil
  }
 
- if foundOpGroupName == "" {
+ emptyNameMatch := desiredOpGroup.Name == "" && foundOpGroup.GetGenerateName() == desiredOpGroup.GenerateName
+
+ if !(foundOpGroup.GetName() == desiredOpGroup.Name || emptyNameMatch) {
  // no found OperatorGroup matches what the policy is looking for, report Compliance.
  changed := updateStatus(policy, missingNotWantedCond("OperatorGroup"), missingNotWantedObj(desiredOpGroup))
 
  return nil, changed, nil
  }
 
- desiredOpGroup.SetName(foundOpGroupName)
-
- removalBehavior := policy.Spec.RemovalBehavior.ApplyDefaults()
+ desiredOpGroup.SetName(foundOpGroup.GetName()) // set it for the generateName case
 
- if removalBehavior.OperatorGroups.IsKeep() {
- changed := updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(desiredOpGroup))
-
- return nil, changed, nil
+ if len(foundOpGroup.GetOwnerReferences()) != 0 {
+ // the OperatorGroup specified in the policy might be used or managed by something else
+ // so we will keep it.
+ return nil, updateStatus(policy, keptCond("OperatorGroup"), leftoverObj(desiredOpGroup)), nil
  }
 
  // The found OperatorGroup matches what is *not* wanted by the policy. Report NonCompliance.
@@ -763,22 +799,6 @@ func (r *OperatorPolicyReconciler) mustnothaveOpGroup(
  return nil, changed, nil
  }
 
- if removalBehavior.OperatorGroups.IsDeleteIfUnused() {
- // Check the namespace for any subscriptions, including the sub for this mustnothave policy,
- // since deleting the OperatorGroup before that could cause problems
- watcher := opPolIdentifier(policy.Namespace, policy.Name)
-
- foundSubscriptions, err := r.DynamicWatcher.List(
- watcher, subscriptionGVK, desiredOpGroup.Namespace, labels.Everything())
- if err != nil {
- return nil, false, fmt.Errorf("error listing Subscriptions: %w", err)
- }
-
- if len(foundSubscriptions) != 0 {
- return nil, changed, nil
- }
- }
-
  if foundOpGroup.GetDeletionTimestamp() != nil {
  // No "early" condition because that would cause the status to flap
  return nil, updateStatus(policy, deletingCond("OperatorGroup"), deletingObj(desiredOpGroup)), nil
@@ -1393,11 +1413,14 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV(
  return nil, updateStatus(policy, keptCond("ClusterServiceVersion"), relatedCSVs...), nil
  }
 
+ csvNames := make([]string, 0, len(csvList))
+
  for i := range csvList {
  relatedCSVs = append(relatedCSVs, foundNotWantedObj(&csvList[i]))
+ csvNames = append(csvNames, csvList[i].GetName())
  }
 
- changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...)
+ changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion", csvNames...), relatedCSVs...)
 
  if policy.Spec.RemediationAction.IsInform() {
  return nil, changed, nil
@@ -1421,7 +1444,7 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV(
 
  err := r.Delete(ctx, &csvList[i])
  if err != nil {
- changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion"), relatedCSVs...)
+ changed := updateStatus(policy, foundNotWantedCond("ClusterServiceVersion", csvNames...), relatedCSVs...)
 
  if anyAlreadyDeleting {
  // reset the "early" conditions to avoid flapping
@@ -1439,10 +1462,10 @@ func (r *OperatorPolicyReconciler) mustnothaveCSV(
  // reset the "early" conditions to avoid flapping
  earlyConds = []metav1.Condition{}
 
- return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion"), relatedCSVs...), nil
+ return earlyConds, updateStatus(policy, deletingCond("ClusterServiceVersion", csvNames...), relatedCSVs...), nil
  }
 
- updateStatus(policy, deletedCond("ClusterServiceVersion"), relatedCSVs...)
+ updateStatus(policy, deletedCond("ClusterServiceVersion", csvNames...), relatedCSVs...)
 
  return earlyConds, true, nil
 }

controllers/operatorpolicy_status.go

@@ -428,12 +428,22 @@ func missingNotWantedCond(kind string) metav1.Condition {
 
 // foundNotWantedCond returns a NonCompliant condition with a Reason like '____Present'
 // and a Message like 'the ____ is present'
-func foundNotWantedCond(kind string) metav1.Condition {
+func foundNotWantedCond(kind string, identifiers ...string) metav1.Condition {
+ var extraInfo string
+
+ if len(identifiers) == 1 {
+ extraInfo = fmt.Sprintf(" (%s) is", identifiers[0])
+ } else if len(identifiers) > 1 {
+ extraInfo = fmt.Sprintf("s (%s) are", strings.Join(identifiers, ", "))
+ } else {
+ extraInfo = " is"
+ }
+
  return metav1.Condition{
  Type: condType(kind),
  Status: metav1.ConditionFalse,
  Reason: kind + "Present",
- Message: "the " + kind + " is present",
+ Message: "the " + kind + extraInfo + " present",
  }
 }
 
@@ -450,23 +460,43 @@ func createdCond(kind string) metav1.Condition {
 
 // deletedCond returns a Compliant condition, with a Reason like '____Deleted',
 // and a Message like 'the ____ was deleted'
-func deletedCond(kind string) metav1.Condition {
+func deletedCond(kind string, identifiers ...string) metav1.Condition {
+ var extraInfo string
+
+ if len(identifiers) == 1 {
+ extraInfo = fmt.Sprintf(" (%s) was", identifiers[0])
+ } else if len(identifiers) > 1 {
+ extraInfo = fmt.Sprintf("s (%s) were", strings.Join(identifiers, ", "))
+ } else {
+ extraInfo = " was"
+ }
+
  return metav1.Condition{
  Type: condType(kind),
  Status: metav1.ConditionTrue,
  Reason: kind + "Deleted",
- Message: "the " + kind + " was deleted",
+ Message: "the " + kind + extraInfo + " deleted",
  }
 }
 
 // deletingCond returns a NonCompliant condition, with a Reason like '____Deleting',
 // and a Message like 'the ____ has a deletion timestamp'
-func deletingCond(kind string) metav1.Condition {
+func deletingCond(kind string, identifiers ...string) metav1.Condition {
+ var extraInfo string
+
+ if len(identifiers) == 1 {
+ extraInfo = fmt.Sprintf(" (%s) has", identifiers[0])
+ } else if len(identifiers) > 1 {
+ extraInfo = fmt.Sprintf("s (%s) have", strings.Join(identifiers, ", "))
+ } else {
+ extraInfo = " has"
+ }
+
  return metav1.Condition{
  Type: condType(kind),
  Status: metav1.ConditionFalse,
  Reason: kind + "Deleting",
- Message: "the " + kind + " has a deletion timestamp",
+ Message: "the " + kind + extraInfo + " a deletion timestamp",
  }
 }
 
@@ -687,7 +717,7 @@ func buildCSVCond(csv *operatorv1alpha1.ClusterServiceVersion) metav1.Condition
  Type: condType(csv.Kind),
  Status: status,
  Reason: string(csv.Status.Reason),
- Message: "ClusterServiceVersion - " + csv.Status.Message,
+ Message: "ClusterServiceVersion (" + csv.Name + ") - " + csv.Status.Message,
  }
 }
 
@@ -696,23 +726,23 @@ var noCSVCond = metav1.Condition{
  Type: csvConditionType,
  Status: metav1.ConditionFalse,
  Reason: "RelevantCSVNotFound",
- Message: "A relevant installed ClusterServiceVersion could not be found",
+ Message: "a relevant installed ClusterServiceVersion could not be found",
 }
 
 // noCRDCond is a Compliant condition for when no CRDs are found
 var noCRDCond = metav1.Condition{
  Type: crdConditionType,
  Status: metav1.ConditionTrue,
  Reason: "RelevantCRDNotFound",
- Message: "No CRDs were found for the operator",
+ Message: "no CRDs were found for the operator",
 }
 
 // crdFoundCond is a Compliant condition for when CRDs are found
 var crdFoundCond = metav1.Condition{
  Type: crdConditionType,
  Status: metav1.ConditionTrue,
  Reason: "RelevantCRDFound",
- Message: "There are CRDs present for the operator",
+ Message: "there are CRDs present for the operator",
 }
 
 // buildDeploymentCond creates a Condition for deployments. If any are not at their
@@ -758,7 +788,7 @@ var noDeploymentsCond = metav1.Condition{
  Type: deploymentConditionType,
  Status: metav1.ConditionTrue,
  Reason: "NoRelevantDeployments",
- Message: "The ClusterServiceVersion is missing, thus meaning there are no relevant deployments",
+ Message: "there are no relevant deployments because the ClusterServiceVersion is missing",
 }
 
 // catalogSourceFindCond is a conditionally compliant condition with reason
@@ -793,7 +823,7 @@ var catalogSourceUnknownCond = metav1.Condition{
  Type: "CatalogSourcesUnknownState",
  Status: metav1.ConditionTrue,
  Reason: "LastObservedUnknown",
- Message: "Could not determine last observed state of CatalogSource",
+ Message: "could not determine last observed state of CatalogSource",
 }
 
 // missingWantedObj returns a NonCompliant RelatedObject with reason = 'Resource not found but should exist'
@@ -850,6 +880,8 @@ func deletedObj(obj client.Object) policyv1.RelatedObject {
  }
 }
 
+// deletingObj returns a NonCompliant RelatedObject with
+// reason = 'The object is being deleted but has not been removed yet'
 func deletingObj(obj client.Object) policyv1.RelatedObject {
  return policyv1.RelatedObject{
  Object: policyv1.ObjectResourceFromObj(obj),

deploy/crds/kustomize_operatorpolicy/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -105,10 +105,9 @@ spec:
  default: DeleteIfUnused
  description: |-
  Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
- will only delete the OperatorGroup if there is not another Subscription using it.
+ will only delete the OperatorGroup if there is not another resource using it.
  enum:
  - Keep
- - Delete
  - DeleteIfUnused
  type: string
  subscriptions:

deploy/crds/policy.open-cluster-management.io_operatorpolicies.yaml

@@ -100,10 +100,9 @@ spec:
  default: DeleteIfUnused
  description: |-
  Specifies whether to delete the OperatorGroup; defaults to 'DeleteIfUnused' which
- will only delete the OperatorGroup if there is not another Subscription using it.
+ will only delete the OperatorGroup if there is not another resource using it.
  enum:
  - Keep
- - Delete
  - DeleteIfUnused
  type: string
  subscriptions: