Skip to content

Commit

Permalink
Ensure uniqueness and validity of generated names and labels (#716)
Browse files Browse the repository at this point in the history 
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"}}
```
  • Loading branch information
@pleshakov
pleshakov authored Jun 6, 2023
1 parent edaa5ff commit f188b4c
Show file tree
Hide file tree
Showing 2 changed files with 98 additions and 46 deletions.
27 changes: 19 additions & 8 deletions internal/provisioner/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ type eventHandler struct {
logger logr.Logger

staticModeDeploymentYAML []byte

gatewayNextID int64
}

func newEventHandler(
Expand All @@ -47,6 +49,7 @@ func newEventHandler(
k8sClient: k8sClient,
logger: logger,
staticModeDeploymentYAML: staticModeDeploymentYAML,
gatewayNextID: 1,
}
}

Expand Down Expand Up @@ -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))
}
Expand All @@ -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
Expand All @@ -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,
)
}
}

Expand All @@ -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)
}
117 changes: 79 additions & 38 deletions internal/provisioner/handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package provisioner

import (
"context"
"fmt"

. "github.com/onsi/ginkgo/v2"
v1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -32,9 +33,6 @@ var _ = Describe("handler", func() {

statusUpdater status.Updater
k8sclient client.Client

gwNsName, depNsName types.NamespacedName
gw *v1beta1.Gateway
)

BeforeEach(OncePerOrdered, func() {
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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),
},
}

Expand All @@ -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,
Expand All @@ -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,
},
}

Expand All @@ -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",
Expand All @@ -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,
Expand Down Expand Up @@ -275,7 +316,7 @@ var _ = Describe("handler", func() {

When("upserting Gateway when GatewayClass doesn't exist", func() {
It("should panic", func() {
itShouldPanicWhenUpsertingGateway()
itShouldPanicWhenUpsertingGateway(gwNsName)
})
})

Expand All @@ -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",
},
}

Expand Down Expand Up @@ -364,7 +405,7 @@ var _ = Describe("handler", func() {
)

itShouldUpsertGatewayClass()
itShouldPanicWhenUpsertingGateway()
itShouldPanicWhenUpsertingGateway(types.NamespacedName{Namespace: "test-ns", Name: "test-gw"})
})
})
})

0 comments on commit f188b4c

Please sign in to comment.