fix: fix namespaceSelectors of the NetworkPolicies argocd components

argocd-operator/controllers/argocd/networkpolicies.go

@@ -45,6 +45,16 @@ const (
 	ArgoCDApplicationSetControllerNetworkPolicy = "applicationset-controller-network-policy"
 	// ImageUpdaterNetworkPolicy is the name of the network policy which controls Image Updater traffic
 	ImageUpdaterNetworkPolicy = "image-updater-network-policy"
+
+	// NetworkPolicyGroupLabelSelectorKey is applied by OpenShift to platform namespaces
+	// (monitoring and ingress).
+	NetworkPolicyGroupLabelSelectorKey = "network.openshift.io/policy-group"
+	MonitoringLabelSelectorValue       = "monitoring"
+	IngressLabelSelectorValue          = "ingress"
+
+	// openShiftIngressOVNPolicyGroupLabel is the OVN-Kubernetes recommended selector
+	// for namespaces that host the ingress controller (including hostNetwork pods).
+	openShiftIngressOVNPolicyGroupLabel = "policy-group.network.openshift.io/ingress"
 )
 
 func (r *ReconcileArgoCD) ReconcileNetworkPolicies(cr *argoproj.ArgoCD) error {
@@ -140,7 +150,8 @@ func (r *ReconcileArgoCD) deleteArgoCDNetworkPolicies(cr *argoproj.ArgoCD) error
 }
 
 // ReconcileDexServerNetworkPolicy creates and reconciles network policy for Dex Server
-// This network policy allows ingress traffic to the dex server from the server and any namespace.
+// This network policy allows ingress traffic to the dex server from the Argo CD server
+// and metrics scraping from OpenShift monitoring namespaces.
 // Referenced from https://github.com/argoproj/argo-cd/blob/master/manifests/base/dex/argocd-dex-server-network-policy.yaml
 func (r *ReconcileArgoCD) ReconcileDexServerNetworkPolicy(cr *argoproj.ArgoCD) error {
 
@@ -179,7 +190,7 @@ func (r *ReconcileArgoCD) ReconcileDexServerNetworkPolicy(cr *argoproj.ArgoCD) e
 			{
 				From: []networkingv1.NetworkPolicyPeer{
 					{
-						NamespaceSelector: &metav1.LabelSelector{},
+						NamespaceSelector: getMonitoringNamespaceLabelSelector(),
 					},
 				},
 				Ports: []networkingv1.NetworkPolicyPort{
@@ -262,7 +273,8 @@ func (r *ReconcileArgoCD) ReconcileDexServerNetworkPolicy(cr *argoproj.ArgoCD) e
 }
 
 // ReconcileApplicationSetControllerNetworkPolicy creates and reconciles network policy for ApplicationSet Controller
-// This network policy allows ingress traffic to the applicationset controller from any namespace.
+// This network policy allows webhook traffic from the OpenShift ingress controller and
+// metrics scraping from OpenShift monitoring namespaces.
 // Referenced from https://github.com/argoproj/argo-cd/blob/master/manifests/base/applicationset-controller/argocd-applicationset-controller-network-policy.yaml
 func (r *ReconcileArgoCD) ReconcileApplicationSetControllerNetworkPolicy(cr *argoproj.ArgoCD) error {
 
@@ -278,16 +290,21 @@ func (r *ReconcileArgoCD) ReconcileApplicationSetControllerNetworkPolicy(cr *arg
 		},
 		Ingress: []networkingv1.NetworkPolicyIngressRule{
 			{
-				From: []networkingv1.NetworkPolicyPeer{
-					{
-						NamespaceSelector: &metav1.LabelSelector{},
-					},
-				},
+				From: getIngressNamespacePeers(),
 				Ports: []networkingv1.NetworkPolicyPort{
 					{
 						Protocol: TCPProtocol,
 						Port:     &intstr.IntOrString{Type: intstr.Int, IntVal: 7000},
 					},
+				},
+			},
+			{
+				From: []networkingv1.NetworkPolicyPeer{
+					{
+						NamespaceSelector: getMonitoringNamespaceLabelSelector(),
+					},
+				},
+				Ports: []networkingv1.NetworkPolicyPort{
 					{
 						Protocol: TCPProtocol,
 						Port:     &intstr.IntOrString{Type: intstr.Int, IntVal: 8080},
@@ -634,7 +651,7 @@ func (r *ReconcileArgoCD) ReconcileRedisHANetworkPolicy(cr *argoproj.ArgoCD) err
 }
 
 // ReconcileNotificationsControllerNetworkPolicy creates and reconciles network policy for Notifications Controller
-// This network policy allows ingress traffic to the notifications controller from any namespace.
+// This network policy allows metrics scraping from OpenShift monitoring namespaces.
 // Referenced from https://github.com/argoproj/argo-cd/blob/master/manifests/base/notifications-controller/argocd-notifications-controller-network-policy.yaml
 func (r *ReconcileArgoCD) ReconcileNotificationsControllerNetworkPolicy(cr *argoproj.ArgoCD) error {
 
@@ -652,7 +669,7 @@ func (r *ReconcileArgoCD) ReconcileNotificationsControllerNetworkPolicy(cr *argo
 			{
 				From: []networkingv1.NetworkPolicyPeer{
 					{
-						NamespaceSelector: &metav1.LabelSelector{},
+						NamespaceSelector: getMonitoringNamespaceLabelSelector(),
 					},
 				},
 				Ports: []networkingv1.NetworkPolicyPort{
@@ -815,7 +832,7 @@ func (r *ReconcileArgoCD) ReconcileArgoCDServerNetworkPolicy(cr *argoproj.ArgoCD
 }
 
 // ReconcileArgoCDApplicationControllerNetworkPolicy creates and reconciles network policy for Argo CD Application Controller
-// This network policy allows ingress traffic to the application controller from any namespace.
+// This network policy allows metrics scraping from OpenShift monitoring namespaces.
 // Referenced from https://github.com/argoproj/argo-cd/blob/master/manifests/base/application-controller/argocd-application-controller-network-policy.yaml
 func (r *ReconcileArgoCD) ReconcileArgoCDApplicationControllerNetworkPolicy(cr *argoproj.ArgoCD) error {
 	desired := returnNetworkPolicyHeaders(cr, ArgoCDApplicationControllerNetworkPolicy)
@@ -833,7 +850,7 @@ func (r *ReconcileArgoCD) ReconcileArgoCDApplicationControllerNetworkPolicy(cr *
 			{
 				From: []networkingv1.NetworkPolicyPeer{
 					{
-						NamespaceSelector: &metav1.LabelSelector{},
+						NamespaceSelector: getMonitoringNamespaceLabelSelector(),
 					},
 				},
 				Ports: []networkingv1.NetworkPolicyPort{
@@ -910,7 +927,8 @@ func (r *ReconcileArgoCD) ReconcileArgoCDApplicationControllerNetworkPolicy(cr *
 }
 
 // ReconcileArgoCDRepoServerNetworkPolicy creates and reconciles network policy for Argo CD Repo Server
-// This network policy allows ingress traffic to the repo server from the application controller, server, notifications controller, applicationset controller, and any namespace.
+// This network policy allows ingress traffic to the repo server from Argo CD control-plane
+// components, plus metrics scraping from OpenShift monitoring namespaces.
 // Referenced from https://github.com/argoproj/argo-cd/blob/master/manifests/base/repo-server/argocd-repo-server-network-policy.yaml
 func (r *ReconcileArgoCD) ReconcileArgoCDRepoServerNetworkPolicy(cr *argoproj.ArgoCD) error {
 
@@ -975,7 +993,7 @@ func (r *ReconcileArgoCD) ReconcileArgoCDRepoServerNetworkPolicy(cr *argoproj.Ar
 			{
 				From: []networkingv1.NetworkPolicyPeer{
 					{
-						NamespaceSelector: &metav1.LabelSelector{},
+						NamespaceSelector: getMonitoringNamespaceLabelSelector(),
 					},
 				},
 				Ports: []networkingv1.NetworkPolicyPort{
@@ -1143,3 +1161,60 @@ func returnNetworkPolicyHeaders(cr *argoproj.ArgoCD, NetworkPolicyName string) *
 		},
 	}
 }
+
+// getMonitoringNamespaceLabelSelector returns the namespace label selector for selecting
+// the namespace where prometheus related pods run.
+// for openshift, it is going to be network.openshift.io/policy-group: monitoring
+// for non-openshift clusters, all namespaces will be selected.
+func getMonitoringNamespaceLabelSelector() *metav1.LabelSelector {
+	if !IsOpenShiftCluster() {
+		return &metav1.LabelSelector{}
+	}
+	return &metav1.LabelSelector{
+		MatchLabels: map[string]string{
+			NetworkPolicyGroupLabelSelectorKey: MonitoringLabelSelectorValue,
+		},
+	}
+}
+
+// getIngressNamespaceLabelSelector returns the namespace label selector for selecting the
+// namespace where ingress controller related pods run.
+// for openshift clusters, it is going to be network.openshift.io/policy-group: ingress
+// for non-openshift clusters, all namespaces will be selected.
+func getIngressNamespaceLabelSelector() *metav1.LabelSelector {
+	if !IsOpenShiftCluster() {
+		return &metav1.LabelSelector{}
+	}
+	return &metav1.LabelSelector{
+		MatchLabels: map[string]string{
+			NetworkPolicyGroupLabelSelectorKey: IngressLabelSelectorValue,
+		},
+	}
+}
+
+// getIngressNamespacePeers allows traffic from OpenShift router pods. On OpenShift
+// this is both network.openshift.io/policy-group=ingress and the OVN-Kubernetes
+// policy-group.network.openshift.io/ingress="" label. An empty PodSelector is
+// required so hostNetwork ingress-controller pods match.
+func getIngressNamespacePeers() []networkingv1.NetworkPolicyPeer {
+	nsSelector := getIngressNamespaceLabelSelector()
+	if !IsOpenShiftCluster() {
+		return []networkingv1.NetworkPolicyPeer{
+			{NamespaceSelector: nsSelector},
+		}
+	}
+	return []networkingv1.NetworkPolicyPeer{
+		{
+			NamespaceSelector: nsSelector,
+			PodSelector:       &metav1.LabelSelector{},
+		},
+		{
+			NamespaceSelector: &metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					openShiftIngressOVNPolicyGroupLabel: "",
+				},
+			},
+			PodSelector: &metav1.LabelSelector{},
+		},
+	}
+}

argocd-operator/controllers/argocd/networkpolicies_test.go

@@ -359,14 +359,22 @@ func TestApplicationSetControllerNetworkPolicy(t *testing.T) {
 	assert.Equal(t, "argocd-applicationset-controller", np.Spec.PodSelector.MatchLabels["app.kubernetes.io/name"])
 	assert.Equal(t, networkingv1.PolicyTypeIngress, np.Spec.PolicyTypes[0])
 
-	// ingress: from any namespace on 7000 and 8080
-	assert.Equal(t, 1, len(np.Spec.Ingress))
-	assert.Equal(t, 1, len(np.Spec.Ingress[0].From))
+	expectedWebhookPeers := 1
+	if IsOpenShiftCluster() {
+		expectedWebhookPeers = 2
+	}
+	// ingress: webhook on 7000, metrics on 8080
+	assert.Equal(t, 2, len(np.Spec.Ingress))
+	assert.Equal(t, expectedWebhookPeers, len(np.Spec.Ingress[0].From))
 	assert.NotNil(t, np.Spec.Ingress[0].From[0].NamespaceSelector)
 	assert.Equal(t, metav1.LabelSelector{}, *np.Spec.Ingress[0].From[0].NamespaceSelector)
-	assert.Equal(t, 2, len(np.Spec.Ingress[0].Ports))
+	assert.Equal(t, 1, len(np.Spec.Ingress[0].Ports))
 	assert.Equal(t, intstr.FromInt(7000), *np.Spec.Ingress[0].Ports[0].Port)
-	assert.Equal(t, intstr.FromInt(8080), *np.Spec.Ingress[0].Ports[1].Port)
+	assert.Equal(t, 1, len(np.Spec.Ingress[1].From))
+	assert.NotNil(t, np.Spec.Ingress[1].From[0].NamespaceSelector)
+	assert.Equal(t, metav1.LabelSelector{}, *np.Spec.Ingress[1].From[0].NamespaceSelector)
+	assert.Equal(t, 1, len(np.Spec.Ingress[1].Ports))
+	assert.Equal(t, intstr.FromInt(8080), *np.Spec.Ingress[1].Ports[0].Port)
 }
 
 func TestApplicationSetControllerNetworkPolicyDisabledDeletesExisting(t *testing.T) {

argocd-operator/tests/ginkgo/parallel/1-124_validate_networkpolicies_test.go

@@ -120,6 +120,15 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				Eventually(&networkingv1.NetworkPolicy{ObjectMeta: metav1.ObjectMeta{Name: npName, Namespace: nsObj.Name}}, "3m", "5s").Should(k8sFixture.ExistByName())
 			}
 
+			if fixture.RunningOnOpenShift() {
+				By("verifying no NetworkPolicy uses an unscoped namespaceSelector")
+				for _, npName := range expectedNPs {
+					np := &networkingv1.NetworkPolicy{ObjectMeta: metav1.ObjectMeta{Name: npName, Namespace: nsObj.Name}}
+					Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(np), np)).To(Succeed())
+					Expect(networkPolicyHasUnscopedNamespaceSelector(np)).To(BeFalse(), npName+" has namespaceSelector: {}")
+				}
+			}
+
 			By("verifying repo-server NetworkPolicy ingress peers/ports")
 			repoNP := &networkingv1.NetworkPolicy{ObjectMeta: metav1.ObjectMeta{Name: "example-argocd-repo-server-network-policy", Namespace: nsObj.Name}}
 			Eventually(func() bool {
@@ -146,8 +155,8 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				if !foundAppSet {
 					return false
 				}
-				// Rule 2: any namespace on 8084 (metrics)
-				if repoNP.Spec.Ingress[1].From == nil || len(repoNP.Spec.Ingress[1].From) != 1 || repoNP.Spec.Ingress[1].From[0].NamespaceSelector == nil {
+				// Rule 2: OpenShift monitoring on 8084 (metrics)
+				if repoNP.Spec.Ingress[1].From == nil || len(repoNP.Spec.Ingress[1].From) != 1 || !networkPolicyPeerSelectsMonitoring(repoNP.Spec.Ingress[1].From[0]) {
 					return false
 				}
 				if len(repoNP.Spec.Ingress[1].Ports) != 1 || repoNP.Spec.Ingress[1].Ports[0].Port == nil || repoNP.Spec.Ingress[1].Ports[0].Port.IntVal != 8084 {
@@ -180,6 +189,9 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				if len(dexNP.Spec.Ingress[1].Ports) != 1 || dexNP.Spec.Ingress[1].Ports[0].Port == nil || dexNP.Spec.Ingress[1].Ports[0].Port.IntVal != common.ArgoCDDefaultDexMetricsPort {
 					return false
 				}
+				if len(dexNP.Spec.Ingress[1].From) != 1 || !networkPolicyPeerSelectsMonitoring(dexNP.Spec.Ingress[1].From[0]) {
+					return false
+				}
 				return true
 			}, "3m", "5s").Should(BeTrue())
 
@@ -195,7 +207,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				if len(notifNP.Spec.Ingress) != 1 {
 					return false
 				}
-				if notifNP.Spec.Ingress[0].From == nil || len(notifNP.Spec.Ingress[0].From) != 1 || notifNP.Spec.Ingress[0].From[0].NamespaceSelector == nil {
+				if notifNP.Spec.Ingress[0].From == nil || len(notifNP.Spec.Ingress[0].From) != 1 || !networkPolicyPeerSelectsMonitoring(notifNP.Spec.Ingress[0].From[0]) {
 					return false
 				}
 				if len(notifNP.Spec.Ingress[0].Ports) != 1 || notifNP.Spec.Ingress[0].Ports[0].Port == nil || notifNP.Spec.Ingress[0].Ports[0].Port.IntVal != 9001 {
@@ -214,19 +226,22 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				if appSetNP.Spec.PodSelector.MatchLabels[common.ArgoCDKeyName] != expectedAppSetSelectorLabel {
 					return false
 				}
-				if len(appSetNP.Spec.Ingress) != 1 {
+				if len(appSetNP.Spec.Ingress) != 2 {
 					return false
 				}
-				if appSetNP.Spec.Ingress[0].From == nil || len(appSetNP.Spec.Ingress[0].From) != 1 || appSetNP.Spec.Ingress[0].From[0].NamespaceSelector == nil {
+				if appSetNP.Spec.Ingress[0].From == nil || !networkPolicyPeerSelectsIngress(appSetNP.Spec.Ingress[0].From) {
 					return false
 				}
-				if len(appSetNP.Spec.Ingress[0].Ports) != 2 {
+				if len(appSetNP.Spec.Ingress[0].Ports) != 1 {
 					return false
 				}
 				if appSetNP.Spec.Ingress[0].Ports[0].Port == nil || appSetNP.Spec.Ingress[0].Ports[0].Port.IntVal != 7000 {
 					return false
 				}
-				if appSetNP.Spec.Ingress[0].Ports[1].Port == nil || appSetNP.Spec.Ingress[0].Ports[1].Port.IntVal != 8080 {
+				if len(appSetNP.Spec.Ingress[1].From) != 1 || !networkPolicyPeerSelectsMonitoring(appSetNP.Spec.Ingress[1].From[0]) {
+					return false
+				}
+				if len(appSetNP.Spec.Ingress[1].Ports) != 1 || appSetNP.Spec.Ingress[1].Ports[0].Port == nil || appSetNP.Spec.Ingress[1].Ports[0].Port.IntVal != 8080 {
 					return false
 				}
 				return true
@@ -263,7 +278,7 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 				if len(appControllerNP.Spec.Ingress) != 1 {
 					return false
 				}
-				if appControllerNP.Spec.Ingress[0].From == nil || len(appControllerNP.Spec.Ingress[0].From) != 1 || appControllerNP.Spec.Ingress[0].From[0].NamespaceSelector == nil {
+				if appControllerNP.Spec.Ingress[0].From == nil || len(appControllerNP.Spec.Ingress[0].From) != 1 || !networkPolicyPeerSelectsMonitoring(appControllerNP.Spec.Ingress[0].From[0]) {
 					return false
 				}
 				if len(appControllerNP.Spec.Ingress[0].Ports) != 1 || appControllerNP.Spec.Ingress[0].Ports[0].Port == nil || appControllerNP.Spec.Ingress[0].Ports[0].Port.IntVal != 8082 {
@@ -376,3 +391,49 @@ var _ = Describe("GitOps Operator Parallel E2E Tests", func() {
 		})
 	})
 })
+
+func networkPolicyPeerSelectsMonitoring(peer networkingv1.NetworkPolicyPeer) bool {
+	if peer.NamespaceSelector == nil {
+		return false
+	}
+	if fixture.RunningOnOpenShift() {
+		return peer.NamespaceSelector.MatchLabels["network.openshift.io/policy-group"] == "monitoring"
+	}
+	return len(peer.NamespaceSelector.MatchLabels) == 0 && len(peer.NamespaceSelector.MatchExpressions) == 0
+}
+
+func networkPolicyPeerSelectsIngress(peers []networkingv1.NetworkPolicyPeer) bool {
+	if !fixture.RunningOnOpenShift() {
+		return len(peers) == 1 && peers[0].NamespaceSelector != nil &&
+			len(peers[0].NamespaceSelector.MatchLabels) == 0 &&
+			len(peers[0].NamespaceSelector.MatchExpressions) == 0
+	}
+	foundPolicyGroup := false
+	foundOVNPolicyGroup := false
+	for _, peer := range peers {
+		if peer.NamespaceSelector == nil {
+			continue
+		}
+		if peer.NamespaceSelector.MatchLabels["network.openshift.io/policy-group"] == "ingress" {
+			foundPolicyGroup = true
+		}
+		if value, ok := peer.NamespaceSelector.MatchLabels["policy-group.network.openshift.io/ingress"]; ok && value == "" {
+			foundOVNPolicyGroup = true
+		}
+	}
+	return foundPolicyGroup && foundOVNPolicyGroup
+}
+
+func networkPolicyHasUnscopedNamespaceSelector(np *networkingv1.NetworkPolicy) bool {
+	for _, rule := range np.Spec.Ingress {
+		for _, peer := range rule.From {
+			if peer.NamespaceSelector == nil {
+				continue
+			}
+			if len(peer.NamespaceSelector.MatchLabels) == 0 && len(peer.NamespaceSelector.MatchExpressions) == 0 {
+				return true
+			}
+		}
+	}
+	return false
+}