feat(#3097): use CRD validations for TCP/UDPIngress (#3136)

Modifies CRDs' annotations so they correctly validate their fields. An integration test suite is added to ensure that validations are in place indeed and prevent objects from creation. Parser's validations are removed.
Kong · Nov 7, 2022 · a3c40fc · a3c40fc
1 parent d26e653
commit a3c40fc
Show file tree

Hide file tree

Showing 11 changed files with 212 additions and 148 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -126,6 +126,9 @@ Adding a new version? You'll need three changes:
   [#3125](https://github.com/Kong/kubernetes-ingress-controller/pull/3125)
 - Warning events are recorded when annotations in services backing a single route do not match. 
   [#3130](https://github.com/Kong/kubernetes-ingress-controller/pull/3130)
+- CRDs' validations improvements: `UDPIngressRule.Port`, `IngressRule.Port` and `IngressBackend.ServiceName`
+  instead of being validated in the Parser, are validated by the Kubernetes API now.
+  [#3136](https://github.com/Kong/kubernetes-ingress-controller/pull/3136)
 
 ### Fixed
 

diff --git a/config/crd/bases/configuration.konghq.com_tcpingresses.yaml b/config/crd/bases/configuration.konghq.com_tcpingresses.yaml
@@ -59,6 +59,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -87,6 +88,7 @@ spec:
                       type: integer
                   required:
                   - backend
+                  - port
                   type: object
                 type: array
               tls:

diff --git a/config/crd/bases/configuration.konghq.com_udpingresses.yaml b/config/crd/bases/configuration.konghq.com_udpingresses.yaml
@@ -59,6 +59,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -74,6 +75,9 @@ spec:
                       description: Port indicates the port for the Kong proxy to accept
                         incoming traffic on, which will then be routed to the service
                         Backend.
+                      format: int32
+                      maximum: 65535
+                      minimum: 1
                       type: integer
                   required:
                   - backend

diff --git a/deploy/single/all-in-one-dbless-k4k8s-enterprise.yaml b/deploy/single/all-in-one-dbless-k4k8s-enterprise.yaml
@@ -787,6 +787,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -815,6 +816,7 @@ spec:
                       type: integer
                   required:
                   - backend
+                  - port
                   type: object
                 type: array
               tls:
@@ -969,6 +971,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -984,6 +987,9 @@ spec:
                       description: Port indicates the port for the Kong proxy to accept
                         incoming traffic on, which will then be routed to the service
                         Backend.
+                      format: int32
+                      maximum: 65535
+                      minimum: 1
                       type: integer
                   required:
                   - backend

diff --git a/deploy/single/all-in-one-dbless.yaml b/deploy/single/all-in-one-dbless.yaml
@@ -787,6 +787,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -815,6 +816,7 @@ spec:
                       type: integer
                   required:
                   - backend
+                  - port
                   type: object
                 type: array
               tls:
@@ -969,6 +971,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -984,6 +987,9 @@ spec:
                       description: Port indicates the port for the Kong proxy to accept
                         incoming traffic on, which will then be routed to the service
                         Backend.
+                      format: int32
+                      maximum: 65535
+                      minimum: 1
                       type: integer
                   required:
                   - backend

diff --git a/deploy/single/all-in-one-postgres-enterprise.yaml b/deploy/single/all-in-one-postgres-enterprise.yaml
@@ -787,6 +787,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -815,6 +816,7 @@ spec:
                       type: integer
                   required:
                   - backend
+                  - port
                   type: object
                 type: array
               tls:
@@ -969,6 +971,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -984,6 +987,9 @@ spec:
                       description: Port indicates the port for the Kong proxy to accept
                         incoming traffic on, which will then be routed to the service
                         Backend.
+                      format: int32
+                      maximum: 65535
+                      minimum: 1
                       type: integer
                   required:
                   - backend

diff --git a/deploy/single/all-in-one-postgres.yaml b/deploy/single/all-in-one-postgres.yaml
@@ -787,6 +787,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -815,6 +816,7 @@ spec:
                       type: integer
                   required:
                   - backend
+                  - port
                   type: object
                 type: array
               tls:
@@ -969,6 +971,7 @@ spec:
                       properties:
                         serviceName:
                           description: Specifies the name of the referenced service.
+                          minLength: 1
                           type: string
                         servicePort:
                           description: Specifies the port of the referenced service.
@@ -984,6 +987,9 @@ spec:
                       description: Port indicates the port for the Kong proxy to accept
                         incoming traffic on, which will then be routed to the service
                         Backend.
+                      format: int32
+                      maximum: 65535
+                      minimum: 1
                       type: integer
                   required:
                   - backend

diff --git a/internal/dataplane/parser/translate_kong_l4.go b/internal/dataplane/parser/translate_kong_l4.go
@@ -6,7 +6,6 @@ import (
 	"strconv"
 
 	"github.com/kong/go-kong/kong"
-	"github.com/sirupsen/logrus"
 
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/dataplane/kongstate"
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/util"
@@ -29,19 +28,10 @@ func (p *Parser) ingressRulesFromTCPIngressV1beta1() ingressRules {
 	for _, ingress := range ingressList {
 		ingressSpec := ingress.Spec
 
-		log := p.logger.WithFields(logrus.Fields{
-			"tcpingress_namespace": ingress.Namespace,
-			"tcpingress_name":      ingress.Name,
-		})
-
 		result.SecretNameToSNIs.addFromIngressV1beta1TLS(tcpIngressToNetworkingTLS(ingressSpec.TLS), ingress.Namespace)
 
 		var objectSuccessfullyParsed bool
 		for i, rule := range ingressSpec.Rules {
-			if !util.IsValidPort(rule.Port) {
-				log.Errorf("invalid TCPIngress: invalid port: %v", rule.Port)
-				continue
-			}
 			r := kongstate.Route{
 				Ingress: util.FromK8sObject(ingress),
 				Route: kong.Route{
@@ -58,14 +48,6 @@ func (p *Parser) ingressRulesFromTCPIngressV1beta1() ingressRules {
 			if host != "" {
 				r.SNIs = kong.StringSlice(host)
 			}
-			if rule.Backend.ServiceName == "" {
-				log.Errorf("invalid TCPIngress: empty serviceName")
-				continue
-			}
-			if !util.IsValidPort(rule.Backend.ServicePort) {
-				log.Errorf("invalid TCPIngress: invalid servicePort: %v", rule.Backend.ServicePort)
-				continue
-			}
 
 			serviceName := fmt.Sprintf("%s.%s.%d", ingress.Namespace, rule.Backend.ServiceName, rule.Backend.ServicePort)
 			service, ok := result.ServiceNameToServices[serviceName]
@@ -118,27 +100,8 @@ func (p *Parser) ingressRulesFromUDPIngressV1beta1() ingressRules {
 	for _, ingress := range ingressList {
 		ingressSpec := ingress.Spec
 
-		log := p.logger.WithFields(logrus.Fields{
-			"udpingress_namespace": ingress.Namespace,
-			"udpingress_name":      ingress.Name,
-		})
-
 		var objectSuccessfullyParsed bool
 		for i, rule := range ingressSpec.Rules {
-			// validate the ports and servicenames for the rule
-			if !util.IsValidPort(rule.Port) {
-				log.Errorf("invalid UDPIngress: invalid port: %d", rule.Port)
-				continue
-			}
-			if rule.Backend.ServiceName == "" {
-				log.Errorf("invalid UDPIngress: empty serviceName")
-				continue
-			}
-			if !util.IsValidPort(rule.Backend.ServicePort) {
-				log.Errorf("invalid UDPIngress: invalid servicePort: %d", rule.Backend.ServicePort)
-				continue
-			}
-
 			// generate the kong Route based on the listen port
 			route := kongstate.Route{
 				Ingress: util.FromK8sObject(ingress),

diff --git a/internal/dataplane/parser/translate_kong_l4_test.go b/internal/dataplane/parser/translate_kong_l4_test.go
@@ -97,69 +97,6 @@ func TestFromTCPIngressV1beta1(t *testing.T) {
 				},
 			},
 		},
-		// 4
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "foo",
-				Namespace: "default",
-				Annotations: map[string]string{
-					annotations.IngressClassKey: annotations.DefaultIngressClass,
-				},
-			},
-			Spec: configurationv1beta1.TCPIngressSpec{
-				Rules: []configurationv1beta1.IngressRule{
-					{
-						Port: 9000,
-						Backend: configurationv1beta1.IngressBackend{
-							ServiceName: "",
-							ServicePort: 80,
-						},
-					},
-				},
-			},
-		},
-		// 5
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "foo",
-				Namespace: "default",
-				Annotations: map[string]string{
-					annotations.IngressClassKey: annotations.DefaultIngressClass,
-				},
-			},
-			Spec: configurationv1beta1.TCPIngressSpec{
-				Rules: []configurationv1beta1.IngressRule{
-					{
-						Port: 0,
-						Backend: configurationv1beta1.IngressBackend{
-							ServiceName: "foo-svc",
-							ServicePort: 80,
-						},
-					},
-				},
-			},
-		},
-		// 6
-		{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "foo",
-				Namespace: "default",
-				Annotations: map[string]string{
-					annotations.IngressClassKey: annotations.DefaultIngressClass,
-				},
-			},
-			Spec: configurationv1beta1.TCPIngressSpec{
-				Rules: []configurationv1beta1.IngressRule{
-					{
-						Port: 9000,
-						Backend: configurationv1beta1.IngressBackend{
-							ServiceName: "foo-svc",
-							ServicePort: 0,
-						},
-					},
-				},
-			},
-		},
 	}
 	t.Run("no TCPIngress returns empty info", func(t *testing.T) {
 		store, err := store.NewFakeStore(store.FakeObjects{
@@ -260,49 +197,4 @@ func TestFromTCPIngressV1beta1(t *testing.T) {
 		assert.Equal(2, len(parsedInfo.SecretNameToSNIs["default/sooper-secret"]))
 		assert.Equal(2, len(parsedInfo.SecretNameToSNIs["default/sooper-secret2"]))
 	})
-	t.Run("TCPIngress without service name returns empty info", func(t *testing.T) {
-		store, err := store.NewFakeStore(store.FakeObjects{
-			TCPIngresses: []*configurationv1beta1.TCPIngress{
-				tcpIngressList[4],
-			},
-		})
-		assert.NoError(err)
-		p := mustNewParser(t, store)
-
-		parsedInfo := p.ingressRulesFromTCPIngressV1beta1()
-		assert.Equal(ingressRules{
-			ServiceNameToServices: make(map[string]kongstate.Service),
-			SecretNameToSNIs:      make(map[string][]string),
-		}, parsedInfo)
-	})
-	t.Run("TCPIngress with invalid port returns empty info", func(t *testing.T) {
-		store, err := store.NewFakeStore(store.FakeObjects{
-			TCPIngresses: []*configurationv1beta1.TCPIngress{
-				tcpIngressList[5],
-			},
-		})
-		assert.NoError(err)
-		p := mustNewParser(t, store)
-
-		parsedInfo := p.ingressRulesFromTCPIngressV1beta1()
-		assert.Equal(ingressRules{
-			ServiceNameToServices: make(map[string]kongstate.Service),
-			SecretNameToSNIs:      make(map[string][]string),
-		}, parsedInfo)
-	})
-	t.Run("empty TCPIngress with invalid service port returns empty info", func(t *testing.T) {
-		store, err := store.NewFakeStore(store.FakeObjects{
-			TCPIngresses: []*configurationv1beta1.TCPIngress{
-				tcpIngressList[6],
-			},
-		})
-		assert.NoError(err)
-		p := mustNewParser(t, store)
-
-		parsedInfo := p.ingressRulesFromTCPIngressV1beta1()
-		assert.Equal(ingressRules{
-			ServiceNameToServices: make(map[string]kongstate.Service),
-			SecretNameToSNIs:      make(map[string][]string),
-		}, parsedInfo)
-	})
 }