From cbaa538559e79e243cd6b5ac22334ba9a437f0f3 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Mon, 5 Jun 2023 11:10:51 -0700 Subject: [PATCH 1/2] Ensure uniqueness and validity of generated names and labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-, 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"}} ``` --- internal/provisioner/handler.go | 25 ++++-- internal/provisioner/handler_test.go | 117 ++++++++++++++++++--------- 2 files changed, 96 insertions(+), 46 deletions(-) diff --git a/internal/provisioner/handler.go b/internal/provisioner/handler.go index af0ff059b4..2d18e73a65 100644 --- a/internal/provisioner/handler.go +++ b/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,9 @@ 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 { + id := h.gatewayNextID + h.gatewayNextID++ + + return fmt.Sprintf("nginx-gateway-%d", id) } diff --git a/internal/provisioner/handler_test.go b/internal/provisioner/handler_test.go index 0df58c337a..96da6bcf9b 100644 --- a/internal/provisioner/handler_test.go +++ b/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"}) }) }) }) From 232e85030792ea035e31aab43e67c8d7cbc26e5b Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Tue, 6 Jun 2023 10:30:08 -0700 Subject: [PATCH 2/2] add a comment --- internal/provisioner/handler.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/provisioner/handler.go b/internal/provisioner/handler.go index 2d18e73a65..93092c8727 100644 --- a/internal/provisioner/handler.go +++ b/internal/provisioner/handler.go @@ -138,6 +138,8 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, batch events.EventB } 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++