Improve CRD validations and logging (#3427)

docs/RELEASE-NOTES.rst

@@ -18,6 +18,7 @@ Bug Fixes
 * `Issue 3371 https://github.com/F5Networks/k8s-bigip-ctlr/issues/3371`_: CIS added irules cannot have "event disable all"
 * `Issue 3402 https://github.com/F5Networks/k8s-bigip-ctlr/issues/3402`_: TLS iRule fails after recent browser updates
 * Fix for EDNS pool member empty issue.
+* `Issue 3414 https://github.com/F5Networks/k8s-bigip-ctlr/issues/3414`_: CIS is deleting referenced SSL Profile it did not create
 
 
 2.16.1

docs/config_examples/customResourceDefinitions/incubator/customresourcedefinitions.yml

@@ -399,6 +399,9 @@ spec:
                   type: integer
                   minimum: 1
                   maximum: 65535
+              x-kubernetes-validations:
+                - rule: "self.partition != Common"
+                  message: "Partition cannot be set as Common"
             status:
               type: object
               properties:
@@ -757,6 +760,9 @@ spec:
                 - virtualServerPort
                 - pool
                 - mode
+              x-kubernetes-validations:
+                - rule: "self.partition != Common"
+                  message: "Partition cannot be set as Common"
             status:
               type: object
               properties:
@@ -974,6 +980,9 @@ spec:
                         type: string
                       type: object
                   type: object
+              x-kubernetes-validations:
+                - rule: "self.partition != Common"
+                  message: "Partition cannot be set as Common"
             status:
               type: object
               properties:

pkg/controller/backend.go

@@ -309,7 +309,9 @@ func (agent *Agent) agentWorker() {
 			agent.postTenantsDeclaration(decl, rsConfig, priorityTenants)
 		}
 		// Updating the remaining tenants
-		agent.postTenantsDeclaration(decl, rsConfig, updatedTenants)
+		if len(updatedTenants) > 0 {
+			agent.postTenantsDeclaration(decl, rsConfig, updatedTenants)
+		}
 
 		agent.declUpdate.Unlock()
 	}

pkg/controller/constants.go

@@ -42,4 +42,6 @@ const (
 	CALICO_K8S                  = "calico-k8s"
 	CALICO_API_BLOCK_AFFINITIES = "/apis/crd.projectcalico.org/v1/blockaffinities"
 	CALICONodeIPAnnotation      = "projectcalico.org/IPv4Address"
+
+	CommonPartition = "Common"
 )

pkg/controller/nativeResourceWorker.go

@@ -1040,7 +1040,12 @@ func (ctlr *Controller) processRouteConfigFromGlobalCM(es extendedSpec, isDelete
 		}
 		var partition string
 		if len(ergc.BigIpPartition) > 0 {
-			partition = ergc.BigIpPartition
+			if ergc.BigIpPartition != CommonPartition {
+				partition = ergc.BigIpPartition
+			} else {
+				return fmt.Errorf("partition Common is not allowed in RouteGroup, "+
+					"skipping RouteGroup: %s processing", routeGroup), false
+			}
 		} else {
 			partition = ctlr.Partition
 		}

pkg/controller/nativeResourceWorker_test.go

@@ -1421,6 +1421,44 @@ extendedRouteSpec:
 			Expect(ok).To(BeTrue())
 		})
 
+		It("Extended Route Spec with Common partition in RouteGroups and defaultRouteGroup of baseRouteSpec", func() {
+			// Verify Common partition is not allowed for defaultRouteGroup of baseRouteSpec
+			data["extendedSpec"] = `
+baseRouteSpec:
+    defaultRouteGroup:
+        bigIpPartition: Common
+extendedRouteSpec:
+    - namespace: default
+      vserverAddr: 10.8.3.11
+      vserverName: nextgenroutes
+      allowOverride: false
+    - namespace: new
+      vserverAddr: 10.8.3.12
+      allowOverride: false
+      bigIpPartition: Common
+`
+			err, ok := mockCtlr.processConfigMap(cm, false)
+			Expect(err).NotTo(BeNil(), "Common partition in defaultRouteGroup of baseRouteSpec should not be allowed")
+			Expect(ok).To(BeFalse(), "Common partition in defaultRouteGroup of baseRouteSpec should not be allowed")
+
+			// Verify Common partition is not allowed for RouteGroup
+			data["extendedSpec"] = `
+extendedRouteSpec:
+    - namespace: default
+      vserverAddr: 10.8.3.11
+      vserverName: nextgenroutes
+      allowOverride: false
+    - namespace: new
+      vserverAddr: 10.8.3.12
+      allowOverride: false
+      bigIpPartition: Common
+`
+			err, ok = mockCtlr.processConfigMap(cm, false)
+			Expect(err).NotTo(BeNil(), "Common partition in RouteGroups should not be allowed")
+			Expect(ok).To(BeFalse(), "Common partition in RouteGroups should not be allowed")
+
+		})
+
 		It("Extended Route Spec Allow local", func() {
 			data["extendedSpec"] = `
 extendedRouteSpec:

pkg/controller/validate.go

@@ -42,6 +42,13 @@ func (ctlr *Controller) checkValidVirtualServer(
 		log.Infof("VirtualServer %s is invalid", vsName)
 		return false
 	}
+
+	// Check if Partition is set as Common
+	if vsResource.Spec.Partition == CommonPartition {
+		log.Errorf("VirtualServer %s cannot be created in Common partition", vsName)
+		return false
+	}
+
 	// Check if HTTPTraffic is set for insecure VS
 	if vsResource.Spec.TLSProfileName == "" && vsResource.Spec.HTTPTraffic != "" {
 		log.Errorf("HTTPTraffic not allowed to be set for insecure VirtualServer: %v", vsName)
@@ -101,6 +108,12 @@ func (ctlr *Controller) checkValidTransportServer(
 		return false
 	}
 
+	// Check if Partition is set as Common
+	if tsResource.Spec.Partition == CommonPartition {
+		log.Errorf("TransportServer %s cannot be created in Common partition", vsName)
+		return false
+	}
+
 	bindAddr := tsResource.Spec.VirtualServerAddress
 
 	if ctlr.ipamCli == nil {
@@ -157,6 +170,12 @@ func (ctlr *Controller) checkValidIngressLink(
 		return false
 	}
 
+	// Check if Partition is set as Common
+	if il.Spec.Partition == CommonPartition {
+		log.Errorf("IngressLink %s cannot be created in Common partition", ilName)
+		return false
+	}
+
 	bindAddr := il.Spec.VirtualServerAddress
 
 	if ctlr.ipamCli == nil {

pkg/controller/worker.go

@@ -1532,7 +1532,7 @@ func (ctlr *Controller) getAssociatedVirtualServers(
 			}
 			if _, ok := uniquePaths[pool.Path]; ok {
 				// path already exists for the same host
-				log.Debugf("Discarding the VirtualServer %v/%v due to duplicate path",
+				log.Warningf("Discarding the VirtualServer %v/%v due to duplicate path",
 					vrt.ObjectMeta.Namespace, vrt.ObjectMeta.Name)
 				isUnique = false
 				break
@@ -4239,6 +4239,11 @@ func (ctlr *Controller) processConfigMap(cm *v1.ConfigMap, isDelete bool) (error
 	if err != nil {
 		return fmt.Errorf("invalid extended route spec in configmap: %v/%v error: %v", cm.Namespace, cm.Name, err), false
 	}
+	// Check if Partition is set to Common, which is not allowed
+	if es.DefaultRouteGroupConfig.BigIpPartition == CommonPartition {
+		return fmt.Errorf("invalid partition %v provided in defaultRouteGroup of configmap: %v/%v",
+			CommonPartition, cm.Namespace, cm.Name), false
+	}
 	// clusterConfigUpdated, oldClusterRatio and oldClusterAdminState are used for tracking cluster ratio and cluster Admin state updates
 	clusterConfigUpdated := false
 	oldClusterRatio := make(map[string]int)

pkg/controller/worker_test.go

@@ -790,6 +790,12 @@ var _ = Describe("Worker Tests", func() {
 				Expect(address).To(Equal("192.168.1.1"), "Should not return empty virtual address")
 				Expect(err).To(BeNil(), "error should be nil")
 			})
+
+			It("Verifies Common partition is not allowed in VS", func() {
+				vrt3.Spec.Partition = CommonPartition
+				Expect(mockCtlr.checkValidVirtualServer(vrt3)).To(BeFalse(), "VS with Common partition "+
+					"should not be allowed")
+			})
 		})
 	})
 	Describe("Endpoints", func() {
@@ -2517,6 +2523,9 @@ var _ = Describe("Worker Tests", func() {
 				Expect(len(mockCtlr.resources.ltmConfig[mockCtlr.Partition].ResourceMap)).To(Equal(2), "Invalid TS count")
 				Expect(len(mockCtlr.resources.ltmConfig["dev2"].ResourceMap)).To(Equal(1), "Invalid TS count")
 
+				// Verify TS with Common partition is not allowed
+				ts.Spec.Partition = CommonPartition
+				Expect(mockCtlr.checkValidTransportServer(ts)).To(BeFalse(), "TS with Common partition is not allowed")
 			})
 		})
 
@@ -2847,6 +2856,9 @@ var _ = Describe("Worker Tests", func() {
 				Expect(len(mockCtlr.resources.ltmConfig)).To(Equal(1), "Invalid Partition count")
 				Expect(len(mockCtlr.resources.ltmConfig[mockCtlr.Partition].ResourceMap)).To(Equal(2), "Invalid IL count")
 
+				// Verify IL with Common partition is not allowed
+				ingressLink1.Spec.Partition = CommonPartition
+				Expect(mockCtlr.checkValidIngressLink(ingressLink1)).To(BeFalse(), "IL with Common partition is not allowed")
 			})
 		})
 	})