From fafd9001b4ccf37e27aad570e32c9c602ca0d148 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Wed, 7 Feb 2024 15:08:11 -0500 Subject: [PATCH 1/3] Consider init container resources when determining if existing + desired deployments are equal --- .../api-gateway/gatekeeper/deployment.go | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/control-plane/api-gateway/gatekeeper/deployment.go b/control-plane/api-gateway/gatekeeper/deployment.go index 6bc1402eed..ffca257289 100644 --- a/control-plane/api-gateway/gatekeeper/deployment.go +++ b/control-plane/api-gateway/gatekeeper/deployment.go @@ -6,6 +6,7 @@ package gatekeeper import ( "context" + "github.com/google/go-cmp/cmp" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" @@ -164,10 +165,26 @@ func mergeDeployments(gcc v1alpha1.GatewayClassConfig, a, b *appsv1.Deployment) return b } +// compareDeployments determines whether two Deployments are equal for all +// of the fields that we care about. There are some differences between a +// Deployment returned by the Kubernetes API and one that we would create +// in memory which are perfectly fine. We want to ignore those differences. func compareDeployments(a, b *appsv1.Deployment) bool { - // since K8s adds a bunch of defaults when we create a deployment, check that - // they don't differ by the things that we may actually change, namely container - // ports + if len(b.Spec.Template.Spec.InitContainers) != len(a.Spec.Template.Spec.InitContainers) { + return false + } + + for i, containerA := range a.Spec.Template.Spec.InitContainers { + containerB := b.Spec.Template.Spec.InitContainers[i] + if !cmp.Equal(containerA.Resources.Limits, containerB.Resources.Limits) { + return false + } + + if !cmp.Equal(containerA.Resources.Requests, containerB.Resources.Requests) { + return false + } + } + if len(b.Spec.Template.Spec.Containers) != len(a.Spec.Template.Spec.Containers) { return false } From f5918d80da5fcbe75c300f2c562e6e9afafdefcc Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Wed, 7 Feb 2024 15:31:03 -0500 Subject: [PATCH 2/3] Add test coverage for compareDeployments --- .../api-gateway/gatekeeper/deployment.go | 1 + .../api-gateway/gatekeeper/deployment_test.go | 216 ++++++++++++++++++ 2 files changed, 217 insertions(+) create mode 100644 control-plane/api-gateway/gatekeeper/deployment_test.go diff --git a/control-plane/api-gateway/gatekeeper/deployment.go b/control-plane/api-gateway/gatekeeper/deployment.go index ffca257289..d85fbf8ef1 100644 --- a/control-plane/api-gateway/gatekeeper/deployment.go +++ b/control-plane/api-gateway/gatekeeper/deployment.go @@ -188,6 +188,7 @@ func compareDeployments(a, b *appsv1.Deployment) bool { if len(b.Spec.Template.Spec.Containers) != len(a.Spec.Template.Spec.Containers) { return false } + for i, container := range a.Spec.Template.Spec.Containers { otherPorts := b.Spec.Template.Spec.Containers[i].Ports if len(container.Ports) != len(otherPorts) { diff --git a/control-plane/api-gateway/gatekeeper/deployment_test.go b/control-plane/api-gateway/gatekeeper/deployment_test.go new file mode 100644 index 0000000000..600f5ce392 --- /dev/null +++ b/control-plane/api-gateway/gatekeeper/deployment_test.go @@ -0,0 +1,216 @@ +package gatekeeper + +import ( + "testing" + + "github.com/stretchr/testify/assert" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + + "github.com/hashicorp/consul-k8s/control-plane/api-gateway/common" +) + +func Test_compareDeployments(t *testing.T) { + testCases := []struct { + name string + a, b *appsv1.Deployment + shouldBeEqual bool + }{ + { + name: "zero-state deployments", + a: &appsv1.Deployment{}, + b: &appsv1.Deployment{}, + shouldBeEqual: true, + }, + { + name: "different replicas", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: common.PointerTo(int32(1)), + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: common.PointerTo(int32(2)), + }, + }, + shouldBeEqual: false, + }, + { + name: "same replicas", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: common.PointerTo(int32(1)), + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Replicas: common.PointerTo(int32(1)), + }, + }, + shouldBeEqual: true, + }, + { + name: "different init container resources", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "cpu": requireQuantity(t, "111m"), + "memory": requireQuantity(t, "111Mi"), + }, + }, + }, + }, + }, + }, + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "cpu": requireQuantity(t, "222m"), + "memory": requireQuantity(t, "111Mi"), + }, + }, + }, + }, + }, + }, + }, + }, + shouldBeEqual: false, + }, + { + name: "different init container resources", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "cpu": requireQuantity(t, "111m"), + "memory": requireQuantity(t, "111Mi"), + }, + }, + }, + }, + }, + }, + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + InitContainers: []corev1.Container{ + { + Resources: corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + "cpu": requireQuantity(t, "111m"), + "memory": requireQuantity(t, "111Mi"), + }, + }, + }, + }, + }, + }, + }, + }, + shouldBeEqual: true, + }, + { + name: "different container ports", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Ports: []corev1.ContainerPort{ + {ContainerPort: 7070}, + {ContainerPort: 9090}, + }, + }, + }, + }, + }, + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Ports: []corev1.ContainerPort{ + {ContainerPort: 8080}, + {ContainerPort: 9090}, + }, + }, + }, + }, + }, + }, + }, + shouldBeEqual: false, + }, + { + name: "same container ports", + a: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Ports: []corev1.ContainerPort{ + {ContainerPort: 8080}, + {ContainerPort: 9090}, + }, + }, + }, + }, + }, + }, + }, + b: &appsv1.Deployment{ + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Ports: []corev1.ContainerPort{ + {ContainerPort: 8080}, + {ContainerPort: 9090}, + }, + }, + }, + }, + }, + }, + }, + shouldBeEqual: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + if testCase.shouldBeEqual { + assert.True(t, compareDeployments(testCase.a, testCase.b), "expected deployments to be equal but they were not") + } else { + assert.False(t, compareDeployments(testCase.a, testCase.b), "expected deployments to be different but they were not") + } + }) + } +} From 366ebec379cc03902a50298ef168115f099be4a5 Mon Sep 17 00:00:00 2001 From: Nathan Coleman Date: Wed, 7 Feb 2024 16:21:33 -0500 Subject: [PATCH 3/3] Update control-plane/api-gateway/gatekeeper/deployment_test.go --- control-plane/api-gateway/gatekeeper/deployment_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/api-gateway/gatekeeper/deployment_test.go b/control-plane/api-gateway/gatekeeper/deployment_test.go index 600f5ce392..0e4c2f53f0 100644 --- a/control-plane/api-gateway/gatekeeper/deployment_test.go +++ b/control-plane/api-gateway/gatekeeper/deployment_test.go @@ -91,7 +91,7 @@ func Test_compareDeployments(t *testing.T) { shouldBeEqual: false, }, { - name: "different init container resources", + name: "same init container resources", a: &appsv1.Deployment{ Spec: appsv1.DeploymentSpec{ Template: corev1.PodTemplateSpec{