Merge pull request #3 from ssttehrani/main

fix: ingress packet drop issue for non-vpn-accessible services
snapp-incubator · Apr 6, 2024 · 80568c5 · 80568c5
2 parents 855b4ae + 17d6576
commit 80568c5
Show file tree

Hide file tree

Showing 5 changed files with 19 additions and 65 deletions.
diff --git a/Makefile b/Makefile
@@ -184,7 +184,7 @@ ENVTEST ?= $(LOCALBIN)/setup-envtest
 
 ## Tool Versions
 KUSTOMIZE_VERSION ?= v4.5.7
-CONTROLLER_TOOLS_VERSION ?= v0.11.1
+CONTROLLER_TOOLS_VERSION ?= v0.14.0
 
 KUSTOMIZE_INSTALL_SCRIPT ?= "https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/hack/install_kustomize.sh"
 .PHONY: kustomize

diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml
@@ -2,7 +2,6 @@
 apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
-  creationTimestamp: null
   name: manager-role
 rules:
 - apiGroups:

diff --git a/internal/controller/controller_test.go b/internal/controller/controller_test.go
@@ -50,15 +50,14 @@ const (
 
 // Global variables for test configuration
 var (
-	CiliumNetworkPolicyControllerLabels                  map[string]string             = map[string]string{"snappcloud.io/controller-managed": "true", "snappcloud.io/controller": "svc-lb-to-cilium-netpolicy"}
-	CiliumNetworkPolicyAllowedEntities                   cilium_policy_api.EntitySlice = []cilium_policy_api.Entity{cilium_policy_api.EntityCluster}
-	CiliumNetworkPolicyAllowedEntitiesForExposedServices cilium_policy_api.EntitySlice = []cilium_policy_api.Entity{cilium_policy_api.EntityCluster, cilium_policy_api.EntityWorld}
-	CiliumNetworkPolicyAnnotations                       map[string]string             = map[string]string{"snappcloud.io/team": "snappcloud", "snappcloud.io/sub-team": "network"}
-	ExcludedNamespaceLabelsViaLabelsSet                  map[string]string             = map[string]string{"k1": "v1", "k2": "v2"} // Set based on the configuration
-	MetallbAnnotations                                   map[string]string             = map[string]string{"metallb.universe.tf/address-pool": "vpn-access"}
-	ServiceLabels                                        map[string]string             = map[string]string{"app.kubernetes.io/name": "test", "k2": "v2"}
-	ServicePorts                                         []corev1.ServicePort          = []corev1.ServicePort{{Name: "http", Port: 80}}
-	ServiceSelector                                      map[string]string             = map[string]string{"app.kubernetes.io/name": "test", "k2": "v2"}
+	CiliumNetworkPolicyControllerLabels map[string]string             = map[string]string{"snappcloud.io/controller-managed": "true", "snappcloud.io/controller": "svc-lb-to-cilium-netpolicy"}
+	CiliumNetworkPolicyAllowedEntities  cilium_policy_api.EntitySlice = []cilium_policy_api.Entity{cilium_policy_api.EntityCluster, cilium_policy_api.EntityWorld}
+	CiliumNetworkPolicyAnnotations      map[string]string             = map[string]string{"snappcloud.io/team": "snappcloud", "snappcloud.io/sub-team": "network"}
+	ExcludedNamespaceLabelsViaLabelsSet map[string]string             = map[string]string{"k1": "v1", "k2": "v2"} // Set based on the configuration
+	MetallbAnnotations                  map[string]string             = map[string]string{"metallb.universe.tf/address-pool": "vpn-access"}
+	ServiceLabels                       map[string]string             = map[string]string{"app.kubernetes.io/name": "test", "k2": "v2"}
+	ServicePorts                        []corev1.ServicePort          = []corev1.ServicePort{{Name: "http", Port: 80}}
+	ServiceSelector                     map[string]string             = map[string]string{"app.kubernetes.io/name": "test", "k2": "v2"}
 )
 
 // Main block for testing 'LoadBalancer Service' to CiliumNetworkPolicy controller
@@ -122,23 +121,6 @@ var _ = Describe("Testing LoadBalancer Service to CiliumNetworkPolicy Controller
 			}
 		}
 
-		// Utility function to get a sample exposed Service
-		getSampleExposedService := func(name, namespace string) *corev1.Service {
-			return &corev1.Service{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace:   namespace,
-					Name:        name,
-					Labels:      ServiceLabels,
-					Annotations: MetallbAnnotations,
-				},
-				Spec: corev1.ServiceSpec{
-					Ports:    ServicePorts,
-					Type:     corev1.ServiceTypeLoadBalancer,
-					Selector: ServiceSelector,
-				},
-			}
-		}
-
 		// Utility function to delete a Service
 		deleteService := func(service *corev1.Service) {
 			Expect(k8sClient.Delete(context.Background(), service)).To(Succeed())
@@ -621,8 +603,8 @@ var _ = Describe("Testing LoadBalancer Service to CiliumNetworkPolicy Controller
 			deleteService(serviceObj)
 		})
 
-		// Test case: Verifies that the CiliumNetworkPolicy object's Spec.Ingress[0].FromEntities are set based on the CiliumNetworkPolicyAllowedEntities variable for non-exposed services
-		It("should set CiliumNetworkPolicy object \"Spec.Ingress[0].FromEntities\" based on CiliumNetworkPolicyAllowedEntities variable if the Service is not exposed", func() {
+		// Test case: Verifies that the CiliumNetworkPolicy object's Spec.Ingress[0].FromEntities are set based on the CiliumNetworkPolicyAllowedEntities variable
+		It("should set CiliumNetworkPolicy object \"Spec.Ingress[0].FromEntities\" based on CiliumNetworkPolicyAllowedEntities variable", func() {
 			// Create and verify a sample LoadBalancer Service in the default namespace
 			serviceObj := getSampleService(ServiceName, DefaultNamespace)
 			Expect(k8sClient.Create(context.Background(), serviceObj)).To(Succeed())
@@ -634,7 +616,7 @@ var _ = Describe("Testing LoadBalancer Service to CiliumNetworkPolicy Controller
 			// Wait for a specified interval to ensure the state is stable
 			time.Sleep(WaitInterval * time.Second)
 
-			//  Retrieve the created CiliumNetworkPolicy object and verify its Ingress FromEntities
+			// Retrieve the created CiliumNetworkPolicy object and verify its Ingress FromEntities
 			ciliumNetworkPolicyObj := ciliumv2.CiliumNetworkPolicy{}
 			Expect(k8sClient.Get(context.Background(), types.NamespacedName{Namespace: DefaultNamespace, Name: CiliumNetworkPolicyName}, &ciliumNetworkPolicyObj)).To(Succeed())
 			Expect(ciliumNetworkPolicyObj.Spec.Ingress[0].FromEntities).To(Equal(CiliumNetworkPolicyAllowedEntities))
@@ -644,29 +626,6 @@ var _ = Describe("Testing LoadBalancer Service to CiliumNetworkPolicy Controller
 			deleteService(serviceObj)
 		})
 
-		// Test case: Verifies that the CiliumNetworkPolicy object's Spec.Ingress[0].FromEntities are set based on the CiliumNetworkPolicyAllowedEntitiesForExposedServices variable for exposed services
-		It("should set CiliumNetworkPolicy object \"Spec.Ingress[0].FromEntities\" based on CiliumNetworkPolicyAllowedEntities variable if the Service is exposed", func() {
-			// Create and verify a sample LoadBalancer Service in the default namespace
-			serviceObj := getSampleExposedService(ServiceName, DefaultNamespace)
-			Expect(k8sClient.Create(context.Background(), serviceObj)).To(Succeed())
-
-			// Create a dummy CiliumNetworkPolicy object to bypass the 'NoPolicyRule' rule and verify its creation
-			dummyCiliumNetworkPolicyObj := getSampleCiliumNetworkPolicy(DummyName, DefaultNamespace)
-			Expect(k8sClient.Create(context.Background(), dummyCiliumNetworkPolicyObj)).To(Succeed())
-
-			// Wait for a specified interval to ensure the state is stable
-			time.Sleep(WaitInterval * time.Second)
-
-			// Retrieve the created CiliumNetworkPolicy object and verify its Ingress FromEntities for exposed services
-			ciliumNetworkPolicyObj := ciliumv2.CiliumNetworkPolicy{}
-			Expect(k8sClient.Get(context.Background(), types.NamespacedName{Namespace: DefaultNamespace, Name: CiliumNetworkPolicyName}, &ciliumNetworkPolicyObj)).To(Succeed())
-			Expect(ciliumNetworkPolicyObj.Spec.Ingress[0].FromEntities).To(Equal(CiliumNetworkPolicyAllowedEntitiesForExposedServices))
-
-			// Cleanup: Delete the dummy CiliumNetworkPolicy and Service objects
-			deleteCiliumNetworkPolicy(dummyCiliumNetworkPolicyObj)
-			deleteService(serviceObj)
-		})
-
 		// Test case: Verifies that a CiliumNetworkPolicy object's annotations are set based on a predefined variable
 		It("should set CiliumNetworkPolicy object 'ObjectMeta.Annotations' based on CiliumNetworkPolicyAnnotations variable", func() {
 			// Create and verify a sample LoadBalancer Service in the default namespace

diff --git a/internal/controller/service/controller.go b/internal/controller/service/controller.go
@@ -245,9 +245,11 @@ func (re *ReconcilerExtended) addFinalizer(ctx context.Context) (*ctrl.Result, e
 			if err := re.Client.Get(ctx, re.request.NamespacedName, re.service); err != nil {
 				return err
 			}
+
 			if controllerutil.ContainsFinalizer(re.service, consts.ServiceFinalizerString) {
 				return nil
 			}
+
 			controllerutil.AddFinalizer(re.service, consts.ServiceFinalizerString)
 
 			return re.Client.Update(ctx, re.service)
@@ -268,9 +270,11 @@ func (re *ReconcilerExtended) removeFinalizer(ctx context.Context) (*ctrl.Result
 			if err := re.Client.Get(ctx, re.request.NamespacedName, re.service); err != nil {
 				return err
 			}
+
 			if !controllerutil.ContainsFinalizer(re.service, consts.ServiceFinalizerString) {
 				return nil
 			}
+
 			controllerutil.RemoveFinalizer(re.service, consts.ServiceFinalizerString)
 
 			return re.Client.Update(ctx, re.service)

diff --git a/internal/controller/service/helpers.go b/internal/controller/service/helpers.go
@@ -51,17 +51,9 @@ var (
 
 // buildCNP builds and returns a CiliumNetworkPolicy struct based on the Service object.
 func (re *ReconcilerExtended) buildCNP() *ciliumv2.CiliumNetworkPolicy {
-	var fromEntities cilium_policy_api.EntitySlice
-
-	if re.service.Annotations["metallb.universe.tf/address-pool"] == "vpn-access" {
-		fromEntities = cilium_policy_api.EntitySlice{
-			cilium_policy_api.EntityCluster,
-			cilium_policy_api.EntityWorld,
-		}
-	} else {
-		fromEntities = cilium_policy_api.EntitySlice{
-			cilium_policy_api.EntityCluster,
-		}
+	fromEntities := cilium_policy_api.EntitySlice{
+		cilium_policy_api.EntityCluster,
+		cilium_policy_api.EntityWorld,
 	}
 
 	endpointSelectorLabelsMap := make(map[string]string, len(re.service.Spec.Selector))