Ensure uniqueness and validity of generated names and labels (#716)

Problem:

Provisioner could generate an invalid name or a label for NKG static
mode deployment. As a result, the provisioner would fail with an error
like below:
```
{“level”:“info”,“ts”:“2023-06-05T17:26:04Z”,“logger”:“eventLoop”,“msg”:“added an event to the next batch”,“type”:“*events.UpsertEvent”,“total”:1}
panic: failed to create deployment: Deployment.apps “nginx-gateway-gateway-conformance-infra-same-namespace-with-http-listener-on-8080" is invalid: [spec.selector.matchLabels: Invalid value: “nginx-gateway-gateway-conformance-infra-same-namespace-with-http-listener-on-8080”: must be no more than 63 characters, spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{“app”:“nginx-gateway-gateway-conformance-infra-same-namespace-with-http-listener-on-8080”}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: invalid label selector]
```


Solution:

Provisioner will use the following format for names and labels of
Deployments:
nginx-gateway-<number>, where the number if an interger >= 1. For every
new Gateway resource, the provisioner will increment the number by one.

This approach will break if the provisioner is restarted, but we don't
support provisioner for production yet, so it is acceptable.


Note: correlation between a Gateway resource and the corresponding
Deployment can be found in the provisioner log. For example:
```
{"level":"info","ts":"2023-06-05T21:23:33Z","logger":"eventHandler","msg":"Created deployment","deployment":{"name":"nginx-gateway-1","namespace":"nginx-gateway"},"gateway":{"name":"same-namespace","namespace":"gateway-conformance-infra"}}
```

Author: pleshakov

internal/provisioner/handler.go

@@ -30,6 +30,8 @@ type eventHandler struct {
 	logger        logr.Logger
 
 	staticModeDeploymentYAML []byte
+
+	gatewayNextID int64
 }
 
 func newEventHandler(
@@ -47,6 +49,7 @@ func newEventHandler(
 		k8sClient:                k8sClient,
 		logger:                   logger,
 		staticModeDeploymentYAML: staticModeDeploymentYAML,
+		gatewayNextID:            1,
 	}
 }
 
@@ -91,7 +94,7 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {
 	// Create new deployments
 
 	for _, nsname := range gwsWithoutDeps {
-		deployment, err := prepareDeployment(h.staticModeDeploymentYAML, generateDeploymentID(nsname), nsname)
+		deployment, err := prepareDeployment(h.staticModeDeploymentYAML, h.generateDeploymentID(), nsname)
 		if err != nil {
 			panic(fmt.Errorf("failed to prepare deployment: %w", err))
 		}
@@ -103,7 +106,10 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {
 
 		h.provisions[nsname] = deployment
 
-		h.logger.Info("Created deployment", "deployment", client.ObjectKeyFromObject(deployment))
+		h.logger.Info("Created deployment",
+			"deployment", client.ObjectKeyFromObject(deployment),
+			"gateway", nsname,
+		)
 	}
 
 	// Remove unnecessary deployments
@@ -118,7 +124,10 @@ func (h *eventHandler) ensureDeploymentsMatchGateways(ctx context.Context) {
 
 		delete(h.provisions, nsname)
 
-		h.logger.Info("Deleted deployment", "deployment", client.ObjectKeyFromObject(deployment))
+		h.logger.Info("Deleted deployment",
+			"deployment", client.ObjectKeyFromObject(deployment),
+			"gateway", nsname,
+		)
 	}
 }
 
@@ -128,9 +137,11 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, batch events.EventB
 	h.ensureDeploymentsMatchGateways(ctx)
 }
 
-func generateDeploymentID(gatewayNsName types.NamespacedName) string {
-	// for production, make sure the ID is:
-	// - a valid resource name (ex. can't be too long);
-	// - unique among all Gateway resources (Gateways test-test/test and test/test-test should not have the same ID)
-	return fmt.Sprintf("nginx-gateway-%s-%s", gatewayNsName.Namespace, gatewayNsName.Name)
+func (h *eventHandler) generateDeploymentID() string {
+	// This approach will break if the provisioner is restarted, because the existing Gateways might get
+	// IDs different from the previous replica of the provisioner.
+	id := h.gatewayNextID
+	h.gatewayNextID++
+
+	return fmt.Sprintf("nginx-gateway-%d", id)
 }

internal/provisioner/handler_test.go

@@ -2,6 +2,7 @@ package provisioner
 
 import (
 	"context"
+	"fmt"
 
 	. "github.com/onsi/ginkgo/v2"
 	v1 "k8s.io/api/apps/v1"
@@ -32,9 +33,6 @@ var _ = Describe("handler", func() {
 
 		statusUpdater status.Updater
 		k8sclient     client.Client
-
-		gwNsName, depNsName types.NamespacedName
-		gw                  *v1beta1.Gateway
 	)
 
 	BeforeEach(OncePerOrdered, func() {
@@ -64,12 +62,10 @@ var _ = Describe("handler", func() {
 			PodIP:                    "1.2.3.4",
 			UpdateGatewayClassStatus: true,
 		})
+	})
 
-		gwNsName = types.NamespacedName{
-			Namespace: "test-ns",
-			Name:      "test-gw",
-		}
-		gw = &v1beta1.Gateway{
+	createGateway := func(gwNsName types.NamespacedName) *v1beta1.Gateway {
+		return &v1beta1.Gateway{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: gwNsName.Namespace,
 				Name:      gwNsName.Name,
@@ -78,12 +74,7 @@ var _ = Describe("handler", func() {
 				GatewayClassName: gcName,
 			},
 		}
-
-		depNsName = types.NamespacedName{
-			Namespace: "nginx-gateway",
-			Name:      "nginx-gateway-test-ns-test-gw",
-		}
-	})
+	}
 
 	itShouldUpsertGatewayClass := func() {
 		// Add GatewayClass to the cluster
@@ -127,31 +118,37 @@ var _ = Describe("handler", func() {
 		Expect(clusterGc.Status.Conditions).To(Equal(expectedConditions))
 	}
 
-	itShouldUpsertGateway := func() {
+	itShouldUpsertGateway := func(gwNsName types.NamespacedName, seqNumber int64) {
 		batch := []interface{}{
 			&events.UpsertEvent{
-				Resource: gw,
+				Resource: createGateway(gwNsName),
 			},
 		}
 
 		handler.HandleEventBatch(context.Background(), batch)
 
+		depNsName := types.NamespacedName{
+			Namespace: "nginx-gateway",
+			Name:      fmt.Sprintf("nginx-gateway-%d", seqNumber),
+		}
+
 		dep := &v1.Deployment{}
 		err := k8sclient.Get(context.Background(), depNsName, dep)
 
 		Expect(err).ShouldNot(HaveOccurred())
 
 		Expect(dep.ObjectMeta.Namespace).To(Equal("nginx-gateway"))
-		Expect(dep.ObjectMeta.Name).To(Equal("nginx-gateway-test-ns-test-gw"))
+		Expect(dep.ObjectMeta.Name).To(Equal(depNsName.Name))
 		Expect(dep.Spec.Template.Spec.Containers[0].Args).To(ContainElement("static-mode"))
-		Expect(dep.Spec.Template.Spec.Containers[0].Args).To(ContainElement("--gateway=test-ns/test-gw"))
+		expectedGwFlag := fmt.Sprintf("--gateway=%s", gwNsName.String())
+		Expect(dep.Spec.Template.Spec.Containers[0].Args).To(ContainElement(expectedGwFlag))
 		Expect(dep.Spec.Template.Spec.Containers[0].Args).To(ContainElement("--update-gatewayclass-status=false"))
 	}
 
-	itShouldPanicWhenUpsertingGateway := func() {
+	itShouldPanicWhenUpsertingGateway := func(gwNsName types.NamespacedName) {
 		batch := []interface{}{
 			&events.UpsertEvent{
-				Resource: gw,
+				Resource: createGateway(gwNsName),
 			},
 		}
 
@@ -163,7 +160,18 @@ var _ = Describe("handler", func() {
 	}
 
 	Describe("Core cases", Ordered, func() {
+		var gwNsName1, gwNsName2 types.NamespacedName
+
 		BeforeAll(func() {
+			gwNsName1 = types.NamespacedName{
+				Namespace: "test-ns-1",
+				Name:      "test-gw-1",
+			}
+			gwNsName2 = types.NamespacedName{
+				Namespace: "test-ns-2",
+				Name:      "test-gw-2",
+			}
+
 			handler = newEventHandler(
 				gcName,
 				statusUpdater,
@@ -179,24 +187,50 @@ var _ = Describe("handler", func() {
 			})
 		})
 
-		When("upserting Gateway", func() {
-			It("should create Deployment", func() {
-				itShouldUpsertGateway()
+		When("upserting first Gateway", func() {
+			It("should create first Deployment", func() {
+				itShouldUpsertGateway(gwNsName1, 1)
 			})
 		})
 
-		When("upserting Gateway again", func() {
+		When("upserting first Gateway again", func() {
 			It("must retain Deployment", func() {
-				itShouldUpsertGateway()
+				itShouldUpsertGateway(gwNsName1, 1)
 			})
 		})
 
-		When("deleting Gateway", func() {
-			It("should remove Deployment", func() {
+		When("upserting second Gateway", func() {
+			It("should create second Deployment", func() {
+				itShouldUpsertGateway(gwNsName2, 2)
+			})
+		})
+
+		When("deleting first Gateway", func() {
+			It("should remove first Deployment", func() {
 				batch := []interface{}{
 					&events.DeleteEvent{
 						Type:           &v1beta1.Gateway{},
-						NamespacedName: gwNsName,
+						NamespacedName: gwNsName1,
+					},
+				}
+
+				handler.HandleEventBatch(context.Background(), batch)
+				deps := &v1.DeploymentList{}
+
+				err := k8sclient.List(context.Background(), deps)
+
+				Expect(err).ShouldNot(HaveOccurred())
+				Expect(deps.Items).To(HaveLen(1))
+				Expect(deps.Items[0].ObjectMeta.Name).To(Equal("nginx-gateway-2"))
+			})
+		})
+
+		When("deleting second Gateway", func() {
+			It("should remove second Deployment", func() {
+				batch := []interface{}{
+					&events.DeleteEvent{
+						Type:           &v1beta1.Gateway{},
+						NamespacedName: gwNsName2,
 					},
 				}
 
@@ -215,8 +249,8 @@ var _ = Describe("handler", func() {
 			It("should not create Deployment", func() {
 				gw := &v1beta1.Gateway{
 					ObjectMeta: metav1.ObjectMeta{
-						Name:      "test-gw-2",
-						Namespace: "test-ns-2",
+						Name:      "test-gw-3",
+						Namespace: "test-ns-3",
 					},
 					Spec: v1beta1.GatewaySpec{
 						GatewayClassName: "some-class",
@@ -241,7 +275,14 @@ var _ = Describe("handler", func() {
 	})
 
 	Describe("Edge cases", func() {
+		var gwNsName types.NamespacedName
+
 		BeforeEach(func() {
+			gwNsName = types.NamespacedName{
+				Namespace: "test-ns",
+				Name:      "test-gw",
+			}
+
 			handler = newEventHandler(
 				gcName,
 				statusUpdater,
@@ -275,7 +316,7 @@ var _ = Describe("handler", func() {
 
 		When("upserting Gateway when GatewayClass doesn't exist", func() {
 			It("should panic", func() {
-				itShouldPanicWhenUpsertingGateway()
+				itShouldPanicWhenUpsertingGateway(gwNsName)
 			})
 		})
 
@@ -287,29 +328,29 @@ var _ = Describe("handler", func() {
 
 				dep := &v1.Deployment{
 					ObjectMeta: metav1.ObjectMeta{
-						Namespace: depNsName.Namespace,
-						Name:      depNsName.Name,
+						Namespace: "nginx-gateway",
+						Name:      "nginx-gateway-1",
 					},
 				}
 
 				err := k8sclient.Create(context.Background(), dep)
 				Expect(err).ShouldNot(HaveOccurred())
 
-				itShouldPanicWhenUpsertingGateway()
+				itShouldPanicWhenUpsertingGateway(gwNsName)
 			})
 		})
 
 		When("deleting Gateway when Deployment can't be deleted", func() {
 			It("should panic", func() {
 				itShouldUpsertGatewayClass()
-				itShouldUpsertGateway()
+				itShouldUpsertGateway(gwNsName, 1)
 
 				// Delete the deployment so that the Handler will fail to delete it because it doesn't exist.
 
 				dep := &v1.Deployment{
 					ObjectMeta: metav1.ObjectMeta{
-						Namespace: depNsName.Namespace,
-						Name:      depNsName.Name,
+						Namespace: "nginx-gateway",
+						Name:      "nginx-gateway-1",
 					},
 				}
 
@@ -364,7 +405,7 @@ var _ = Describe("handler", func() {
 			)
 
 			itShouldUpsertGatewayClass()
-			itShouldPanicWhenUpsertingGateway()
+			itShouldPanicWhenUpsertingGateway(types.NamespacedName{Namespace: "test-ns", Name: "test-gw"})
 		})
 	})
 })