CM-763: Fix infinite update loop for user-defined network policies

pkg/controller/deployment/cert_manager_networkpolicy.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 
 	networkingv1 "k8s.io/api/networking/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
@@ -133,12 +134,15 @@ func (c *CertManagerNetworkPolicyUserDefinedController) sync(ctx context.Context
 	}
 
 	// Apply user-defined network policies
-	if err := c.reconcileUserNetworkPolicies(ctx, certManager); err != nil {
+	changed, err := c.reconcileUserNetworkPolicies(ctx, certManager)
+	if err != nil {
 		c.eventRecorder.Warningf("UserNetworkPolicyReconcileFailed", "Failed to reconcile user network policies: %v", err)
 		return fmt.Errorf("failed to reconcile user network policies: %w", err)
 	}
 
-	c.eventRecorder.Event("UserNetworkPolicyReconcileSuccess", "Successfully reconciled user-defined network policies")
+	if changed {
+		c.eventRecorder.Event("UserNetworkPolicyReconcileSuccess", "Successfully reconciled user-defined network policies")
+	}
 	return nil
 }
 
@@ -165,16 +169,21 @@ func (c *CertManagerNetworkPolicyUserDefinedController) validateComponentName(co
 	}
 }
 
-func (c *CertManagerNetworkPolicyUserDefinedController) reconcileUserNetworkPolicies(ctx context.Context, certManager *v1alpha1.CertManager) error {
+func (c *CertManagerNetworkPolicyUserDefinedController) reconcileUserNetworkPolicies(ctx context.Context, certManager *v1alpha1.CertManager) (bool, error) {
+	changed := false
 	// Apply each user-defined network policy
 	for _, userPolicy := range certManager.Spec.NetworkPolicies {
 		policy := c.createUserNetworkPolicy(userPolicy)
-		if err := c.createOrUpdateNetworkPolicy(ctx, policy); err != nil {
-			return fmt.Errorf("failed to create/update user network policy %s: %w", policy.Name, err)
+		policyChanged, err := c.createOrUpdateNetworkPolicy(ctx, policy)
+		if err != nil {
+			return changed, fmt.Errorf("failed to create/update user network policy %s: %w", policy.Name, err)
+		}
+		if policyChanged {
+			changed = true
 		}
 	}
 
-	return nil
+	return changed, nil
 }
 
 func (c *CertManagerNetworkPolicyUserDefinedController) createUserNetworkPolicy(userPolicy v1alpha1.NetworkPolicy) *networkingv1.NetworkPolicy {
@@ -227,30 +236,32 @@ func (c *CertManagerNetworkPolicyUserDefinedController) getPodSelectorForCompone
 	}
 }
 
-func (c *CertManagerNetworkPolicyUserDefinedController) createOrUpdateNetworkPolicy(ctx context.Context, policy *networkingv1.NetworkPolicy) error {
+func (c *CertManagerNetworkPolicyUserDefinedController) createOrUpdateNetworkPolicy(ctx context.Context, policy *networkingv1.NetworkPolicy) (bool, error) {
 	existing, err := c.kubeClient.NetworkingV1().NetworkPolicies(policy.Namespace).Get(ctx, policy.Name, metav1.GetOptions{})
 	if err != nil {
 		if errors.IsNotFound(err) {
-			// Create new policy
 			_, err := c.kubeClient.NetworkingV1().NetworkPolicies(policy.Namespace).Create(ctx, policy, metav1.CreateOptions{})
 			if err != nil {
-				return fmt.Errorf("failed to create network policy: %w", err)
+				return false, fmt.Errorf("failed to create network policy: %w", err)
 			}
 			c.eventRecorder.Eventf("NetworkPolicyCreated", "Created user-defined network policy %s", policy.Name)
-			return nil
+			return true, nil
 		}
-		return fmt.Errorf("failed to get existing network policy: %w", err)
+		return false, fmt.Errorf("failed to get existing network policy: %w", err)
+	}
+
+	if equality.Semantic.DeepEqual(existing.Spec, policy.Spec) &&
+		equality.Semantic.DeepEqual(existing.Labels, policy.Labels) {
+		return false, nil
 	}
 
-	// Update existing policy
 	existing.Spec = policy.Spec
 	existing.Labels = policy.Labels
 	_, err = c.kubeClient.NetworkingV1().NetworkPolicies(policy.Namespace).Update(ctx, existing, metav1.UpdateOptions{})
 	if err != nil {
-		return fmt.Errorf("failed to update network policy: %w", err)
+		return false, fmt.Errorf("failed to update network policy: %w", err)
 	}
 
 	c.eventRecorder.Eventf("NetworkPolicyUpdated", "Updated user-defined network policy %s", policy.Name)
-
-	return nil
+	return true, nil
 }

pkg/controller/deployment/cert_manager_networkpolicy_test.go

@@ -0,0 +1,130 @@
+package deployment
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	networkingv1 "k8s.io/api/networking/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/utils/clock"
+
+	"github.com/openshift/library-go/pkg/operator/events"
+)
+
+func newTestController() (*CertManagerNetworkPolicyUserDefinedController, events.InMemoryRecorder) {
+	recorder := events.NewInMemoryRecorder("test", clock.RealClock{})
+	c := &CertManagerNetworkPolicyUserDefinedController{
+		kubeClient:    fake.NewSimpleClientset(),
+		eventRecorder: recorder.WithComponentSuffix("cert-manager-networkpolicy-user-defined"),
+	}
+	return c, recorder
+}
+
+func newTestNetworkPolicy(name string, port int) *networkingv1.NetworkPolicy {
+	proto := corev1.ProtocolTCP
+	return &networkingv1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: certManagerNamespace,
+			Labels: map[string]string{
+				networkPolicyOwnerLabel: "cert-manager",
+			},
+		},
+		Spec: networkingv1.NetworkPolicySpec{
+			PodSelector: metav1.LabelSelector{
+				MatchLabels: map[string]string{"app": "cert-manager"},
+			},
+			PolicyTypes: []networkingv1.PolicyType{
+				networkingv1.PolicyTypeEgress,
+			},
+			Egress: []networkingv1.NetworkPolicyEgressRule{
+				{
+					Ports: []networkingv1.NetworkPolicyPort{
+						{
+							Port:     &intstr.IntOrString{Type: intstr.Int, IntVal: int32(port)},
+							Protocol: &proto,
+						},
+					},
+				},
+			},
+		},
+	}
+}
+
+func TestCreateOrUpdateNetworkPolicy_Create(t *testing.T) {
+	c, recorder := newTestController()
+	ctx := context.Background()
+
+	policy := newTestNetworkPolicy("cert-manager-user-test", 443)
+	changed, err := c.createOrUpdateNetworkPolicy(ctx, policy)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !changed {
+		t.Fatal("expected changed=true for new policy creation")
+	}
+
+	evts := recorder.Events()
+	if len(evts) != 1 {
+		t.Fatalf("expected 1 event, got %d", len(evts))
+	}
+	if evts[0].Reason != "NetworkPolicyCreated" {
+		t.Errorf("expected reason NetworkPolicyCreated, got %s", evts[0].Reason)
+	}
+}
+
+func TestCreateOrUpdateNetworkPolicy_NoOpUpdate(t *testing.T) {
+	c, recorder := newTestController()
+	ctx := context.Background()
+
+	policy := newTestNetworkPolicy("cert-manager-user-test", 443)
+
+	_, err := c.kubeClient.NetworkingV1().NetworkPolicies(certManagerNamespace).Create(ctx, policy, metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("setup: unexpected error: %v", err)
+	}
+
+	changed, err := c.createOrUpdateNetworkPolicy(ctx, policy)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if changed {
+		t.Fatal("expected changed=false when spec and labels are identical")
+	}
+
+	evts := recorder.Events()
+	if len(evts) != 0 {
+		t.Fatalf("expected 0 events for no-op update, got %d", len(evts))
+	}
+}
+
+func TestCreateOrUpdateNetworkPolicy_UpdateOnSpecChange(t *testing.T) {
+	c, recorder := newTestController()
+	ctx := context.Background()
+
+	original := newTestNetworkPolicy("cert-manager-user-test", 443)
+	_, err := c.kubeClient.NetworkingV1().NetworkPolicies(certManagerNamespace).Create(ctx, original, metav1.CreateOptions{})
+	if err != nil {
+		t.Fatalf("setup: unexpected error: %v", err)
+	}
+
+	updated := newTestNetworkPolicy("cert-manager-user-test", 8443)
+	changed, err := c.createOrUpdateNetworkPolicy(ctx, updated)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !changed {
+		t.Fatal("expected changed=true when spec differs")
+	}
+
+	evts := recorder.Events()
+	if len(evts) != 1 {
+		t.Fatalf("expected 1 event, got %d", len(evts))
+	}
+	if evts[0].Reason != "NetworkPolicyUpdated" {
+		t.Errorf("expected reason NetworkPolicyUpdated, got %s", evts[0].Reason)
+	}
+}