IN PROGRESS - initial review updates

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

controllers/operatorpolicy_controller.go

@@ -17,6 +17,7 @@ import (
  "k8s.io/apimachinery/pkg/labels"
  "k8s.io/apimachinery/pkg/runtime"
  "k8s.io/apimachinery/pkg/runtime/schema"
+ "k8s.io/apimachinery/pkg/util/validation"
  ctrl "sigs.k8s.io/controller-runtime"
  "sigs.k8s.io/controller-runtime/pkg/builder"
  "sigs.k8s.io/controller-runtime/pkg/client"
@@ -172,78 +173,74 @@ func (r *OperatorPolicyReconciler) handleOpGroup(ctx context.Context, policy *po
  case 1:
  opGroup := foundOpGroups[0]
 
- if policy.Spec.OperatorGroup != nil {
- emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
-
- if opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch {
- // check whether the specs match
- desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
- if err != nil {
- return fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
- }
-
- if reflect.DeepEqual(desiredUnstruct["spec"], opGroup.Object["spec"]) {
- // Everything relevant matches! This is a "happy path".
- // Only update the condition if it reflects a NonCompliant state because emitting a
- // Compliant -> Compliant 'change' is noisy and would hide *why* this part of the
- // policy is currently compliant (eg that the policy previously updated the object).
- idx, existingCond := policy.Status.GetCondition(opGroupConditionType)
-
- if idx == -1 || existingCond.Status == metav1.ConditionFalse {
- err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
- if err != nil {
- return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
- }
- }
- } else {
- // The names match, but the specs don't: report NonCompliance
- err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))
- if err != nil {
- return fmt.Errorf("error updating the status for an OperatorGroup that does not match: %w", err)
- }
-
- if policy.Spec.RemediationAction.IsEnforce() {
- desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()
-
- err := r.Update(ctx, desiredOpGroup)
- if err != nil {
- return fmt.Errorf("error updating the OperatorGroup: %w", err)
- }
- desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information
-
- // It was updated and should match now, so report Compliance
- err = r.updateStatus(ctx, policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))
- if err != nil {
- return fmt.Errorf("error updating the status after updating the OperatorGroup: %w", err)
- }
- }
- }
- } else {
- // There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
- // Just creating a new one would cause the "TooManyOperatorGroups" failure.
- // So, just report a NonCompliant status.
- missing := missingWantedObj(desiredOpGroup)
- badExisting := mismatchedObj(&opGroup)
-
- err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), missing, badExisting)
- if err != nil {
- return fmt.Errorf("error updating the status for an OperatorGroup with the wrong name: %w", err)
- }
- }
- } else {
+ if policy.Spec.OperatorGroup == nil {
  // There is one operator group in the namespace, but the policy doesn't specify what it should look like.
-
  // FUTURE: check if the one operator group is compatible with the desired subscription.
-
  // For an initial implementation, assume if an OperatorGroup already exists, then it's a good one.
- // Only update the condition if it reflects a NonCompliant state, to prevent repeats.
- idx, existingCond := policy.Status.GetCondition(opGroupConditionType)
-
- if idx == -1 || existingCond.Status == metav1.ConditionFalse {
- err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
- if err != nil {
- return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
- }
+ err := r.updateStatus(ctx, policy, opGroupPreexistingCond, matchedObj(&opGroup))
+ if err != nil {
+ return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
+ }
+
+ return nil
+ }
+
+ // OperatorGroup is specified in the policy, check if what's on the cluster matches.
+ emptyNameMatch := desiredOpGroup.Name == "" && opGroup.GetGenerateName() == desiredOpGroup.GenerateName
+
+ if !(opGroup.GetName() == desiredOpGroup.Name || emptyNameMatch) {
+ // There is an OperatorGroup in the namespace that does not match the name of what is in the policy.
+ // Just creating a new one would cause the "TooManyOperatorGroups" failure.
+ // So, just report a NonCompliant status.
+ missing := missingWantedObj(desiredOpGroup)
+ badExisting := mismatchedObj(&opGroup)
+
+ err := r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), missing, badExisting)
+ if err != nil {
+ return fmt.Errorf("error updating the status for an OperatorGroup with the wrong name: %w", err)
+ }
+
+ return nil
+ }
+
+ // check whether the specs match
+ desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredOpGroup)
+ if err != nil {
+ return fmt.Errorf("error converting desired OperatorGroup to an Unstructured: %w", err)
+ }
+
+ // NOTE: this is more like mustonlyhave than musthave, and won't quite work.
+ // something from config-policy will be needed to check if the specs match.
+ if reflect.DeepEqual(desiredUnstruct["spec"], opGroup.Object["spec"]) {
+ // Everything relevant matches!
+ err := r.updateStatus(ctx, policy, matchesCond("OperatorGroup"), matchedObj(&opGroup))
+ if err != nil {
+ return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
+ }
+
+ return nil
+ }
+
+ // The names match, but the specs don't: report NonCompliance
+ err = r.updateStatus(ctx, policy, mismatchCond("OperatorGroup"), mismatchedObj(&opGroup))
+ if err != nil {
+ return fmt.Errorf("error updating the status for an OperatorGroup that does not match: %w", err)
+ }
+
+ if policy.Spec.RemediationAction.IsEnforce() {
+ desiredOpGroup.ResourceVersion = opGroup.GetResourceVersion()
+
+ err := r.Update(ctx, desiredOpGroup)
+ if err != nil {
+ return fmt.Errorf("error updating the OperatorGroup: %w", err)
+ }
+
+ desiredOpGroup.SetGroupVersionKind(operatorGroupGVK) // Update stripped this information
+
+ // It was updated and should match now, so report Compliance
+ err = r.updateStatus(ctx, policy, updatedCond("OperatorGroup"), updatedObj(desiredOpGroup))
+ if err != nil {
+ return fmt.Errorf("error updating the status after updating the OperatorGroup: %w", err)
  }
  }
  default:
@@ -280,6 +277,10 @@ func buildOperatorGroup(
  return nil, fmt.Errorf("namespace is required in spec.subscription")
  }
 
+ if validationErrs := validation.IsDNS1123Label(subNamespace); len(validationErrs) != 0 {
+ return nil, fmt.Errorf("the namespace specified in spec.subscription is not a valid namespace identifier")
+ }
+
  // Create a default OperatorGroup if one wasn't specified in the policy
  if policy.Spec.OperatorGroup == nil {
  operatorGroup.ObjectMeta.SetNamespace(subNamespace)
@@ -299,7 +300,7 @@ func buildOperatorGroup(
  // Fallback to the Subscription namespace if the OperatorGroup namespace is not specified in the policy.
  ogNamespace := subNamespace
 
- if specifiedNS, ok := opGroup["namespace"].(string); ok {
+ if specifiedNS, ok := opGroup["namespace"].(string); ok || specifiedNS == "" {
  ogNamespace = specifiedNS
  }
 
@@ -332,7 +333,7 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic
 
  foundSub, err := r.DynamicWatcher.Get(watcher, subscriptionGVK, desiredSub.Namespace, desiredSub.Name)
  if err != nil {
- return fmt.Errorf("error listing OperatorGroups: %w", err)
+ return fmt.Errorf("error getting the Subscription: %w", err)
  }
 
  if foundSub == nil {
@@ -356,44 +357,52 @@ func (r *OperatorPolicyReconciler) handleSubscription(ctx context.Context, polic
  return fmt.Errorf("error updating the status for a created Subscription: %w", err)
  }
  }
- } else {
- // Subscription found; check if specs match
- desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)
- if err != nil {
- return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
- }
 
- if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {
- // FUTURE: Check more details about the *status* of the Subscription
- // For now, (conditionally) mark it as compliant
- idx, existingCond := policy.Status.GetCondition(subConditionType)
+ return nil
+ }
 
- if idx == -1 || existingCond.Status == metav1.ConditionFalse {
- err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
- if err != nil {
- return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
- }
- }
- } else {
- err := r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub))
+ // Subscription found; check if specs match
+ desiredUnstruct, err := runtime.DefaultUnstructuredConverter.ToUnstructured(desiredSub)
+ if err != nil {
+ return fmt.Errorf("error converting desired Subscription to an Unstructured: %w", err)
+ }
+
+ // NOTE: this is more like mustonlyhave than musthave, and won't quite work.
+ // something from config-policy will be needed to check if the specs match.
+ if reflect.DeepEqual(desiredUnstruct["spec"], foundSub.Object["spec"]) {
+ // FUTURE: Check more details about the *status* of the Subscription
+ // For now, (conditionally) mark it as compliant
+ idx, existingCond := policy.Status.GetCondition(subConditionType)
+
+ if idx == -1 || existingCond.Status == metav1.ConditionFalse {
+ err := r.updateStatus(ctx, policy, matchesCond("Subscription"), matchedObj(foundSub))
  if err != nil {
- return fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
+ return fmt.Errorf("error updating the status for an OperatorGroup that matches: %w", err)
  }
+ }
 
-  if policy.Spec.RemediationAction.IsEnforce() {
-  desiredSub.ResourceVersion = foundSub.GetResourceVersion()
+ return nil
+ }
 
-  err := r.Update(ctx, desiredSub)
-  if err != nil {
-  return fmt.Errorf("error updating the Subscription: %w", err)
-  }
-  desiredSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information
+ // Specs don't match.
+ err = r.updateStatus(ctx, policy, mismatchCond("Subscription"), mismatchedObj(foundSub))
+ if err != nil {
+ return fmt.Errorf("error updating status for a mismatched Subscription: %w", err)
+ }
 
- err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(desiredSub))
- if err != nil {
- return fmt.Errorf("error updating status after updating the Subscription: %w", err)
- }
- }
+ if policy.Spec.RemediationAction.IsEnforce() {
+ desiredSub.ResourceVersion = foundSub.GetResourceVersion()
+
+ err := r.Update(ctx, desiredSub)
+ if err != nil {
+ return fmt.Errorf("error updating the Subscription: %w", err)
+ }
+
+ desiredSub.SetGroupVersionKind(subscriptionGVK) // Update stripped this information
+
+ err = r.updateStatus(ctx, policy, updatedCond("Subscription"), updatedObj(desiredSub))
+ if err != nil {
+ return fmt.Errorf("error updating status after updating the Subscription: %w", err)
  }
  }
 

controllers/operatorpolicy_status.go

@@ -24,7 +24,7 @@ import (
 // condition has changed. If not provided, the `lastTransitionTime` will use "now". It also handles
 // preserving the `CreatedByPolicy` property on relatedObjects.
 //
-// This function assumes that all given related objects are of the same kind.
+// This function requires that all given related objects are of the same kind.
 //
 // Note that only changing the related objects will not emit a new compliance event, but will update
 // the status.
@@ -81,37 +81,42 @@ func (r *OperatorPolicyReconciler) updateStatus(
 
  relObjsChanged := false
 
- prevRelObjs := policy.Status.RelatedObjsOfKind(updatedRelatedObjs[0].Object.Kind)
- if len(prevRelObjs) == len(updatedRelatedObjs) {
- for _, prevObj := range prevRelObjs {
- nameFound := false
-
- for i, updatedObj := range updatedRelatedObjs {
- if prevObj.Object.Metadata.Name == updatedObj.Object.Metadata.Name {
- nameFound = true
-
- if updatedObj.Properties != nil && prevObj.Properties != nil {
- if updatedObj.Properties.UID != prevObj.Properties.UID {
- relObjsChanged = true
- } else if prevObj.Properties.CreatedByPolicy != nil {
- // There is an assumption here that it will never need to transition to false.
- updatedRelatedObjs[i].Properties.CreatedByPolicy = prevObj.Properties.CreatedByPolicy
- }
- }
-
- if prevObj.Compliant != updatedObj.Compliant {
- relObjsChanged = true
- } else if prevObj.Reason != updatedObj.Reason {
- relObjsChanged = true
- }
+ prevRelObjs := make(map[int]policyv1.RelatedObject)
+ if len(updatedRelatedObjs) != 0 {
+ prevRelObjs = policy.Status.RelatedObjsOfKind(updatedRelatedObjs[0].Object.Kind)
+ }
+
+ for _, prevObj := range prevRelObjs {
+ nameFound := false
+
+ for i, updatedObj := range updatedRelatedObjs {
+ if prevObj.Object.Metadata.Name != updatedObj.Object.Metadata.Name {
+ continue
+ }
+
+ nameFound = true
+
+ if updatedObj.Properties != nil && prevObj.Properties != nil {
+ if updatedObj.Properties.UID != prevObj.Properties.UID {
+ relObjsChanged = true
+ } else if prevObj.Properties.CreatedByPolicy != nil {
+ // There is an assumption here that it will never need to transition to false.
+ updatedRelatedObjs[i].Properties.CreatedByPolicy = prevObj.Properties.CreatedByPolicy
  }
  }
 
- if !nameFound {
+ if prevObj.Compliant != updatedObj.Compliant || prevObj.Reason != updatedObj.Reason {
  relObjsChanged = true
  }
  }
- } else {
+
+ if !nameFound {
+ relObjsChanged = true
+ }
+ }
+
+ // Catch the case where there is a new object in updatedRelatedObjs
+ if len(prevRelObjs) != len(updatedRelatedObjs) {
  relObjsChanged = true
  }
 
@@ -340,6 +345,14 @@ func updatedCond(kind string) metav1.Condition {
  }
 }
 
+var opGroupPreexistingCond = metav1.Condition{
+ Type: opGroupConditionType,
+ Status: metav1.ConditionTrue,
+ Reason: "PreexistingOperatorGroupFound",
+ Message: "the policy does not specify an OperatorGroup but one already exists in the namespace - " +
+ "assuming that OperatorGroup is correct",
+}
+
 // opGroupTooManyCond is a NonCompliant condition with a Reason like 'TooManyOperatorGroups',
 // and a Message like 'there is more than one OperatorGroup in the namespace'
 var opGroupTooManyCond = metav1.Condition{

test/e2e/case38_install_operator_test.go

@@ -2,7 +2,6 @@ package e2e
 
 import (
  "encoding/json"
- "fmt"
 
  . "github.com/onsi/ginkgo/v2"
  . "github.com/onsi/gomega"
@@ -14,7 +13,7 @@ import (
  "open-cluster-management.io/config-policy-controller/test/utils"
 )
 
-var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, func() {
+var _ = FDescribe("Test installing an operator from OperatorPolicy", Ordered, func() {
  const (
  opPolTestNS = "operator-policy-testns"
  parentPolicyYAML = "../resources/case38_operator_install/parent-policy.yaml"
@@ -36,16 +35,8 @@ var _ = Describe("Test installing an operator from OperatorPolicy", Ordered, fun
  policyJSON, err := json.MarshalIndent(unstructPolicy.Object, "", " ")
  g.Expect(err).NotTo(HaveOccurred())
 
- RegisterFailHandler(func(msg string, callerSkip ...int) {
- GinkgoHelper()
- fmt.Fprintf(GinkgoWriter, "Debug info for failure.\npolicy JSON: %s\nwanted related objects: %+v\n"+
- "wanted condition: %+v", string(policyJSON), expectedRelatedObjs, expectedCondition)
- fmt.Fprintf(GinkgoWriter, "Failure: %s", msg)
- })
-
- DeferCleanup(func() {
- RegisterFailHandler(Fail) // Restore the default fail handler
- })
+ GinkgoWriter.Printf("Debug info for failure.\npolicy JSON: %s\nwanted related objects: %+v\n"+
+ "wanted condition: %+v\n", string(policyJSON), expectedRelatedObjs, expectedCondition)
 
  policy := policyv1beta1.OperatorPolicy{}
  err = json.Unmarshal(policyJSON, &policy)