diff --git a/internal/provisioner/handler.go b/internal/provisioner/handler.go index af0ff059b4..93092c8727 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,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) } 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"}) }) }) })