From b8f7eb54420592f8ada2078082cb49170cf364e0 Mon Sep 17 00:00:00 2001 From: Travis Raines <571832+rainest@users.noreply.github.com> Date: Mon, 29 Apr 2024 13:04:01 -0700 Subject: [PATCH] fix(targets) deduplicate nil-weight targets (#5946) Assume the Kong default 100 weight for targets that have nil weight when being deduplicated. --- internal/dataplane/translator/translate_upstreams.go | 12 +++++++++++- test/integration/ingress_test.go | 5 ++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/internal/dataplane/translator/translate_upstreams.go b/internal/dataplane/translator/translate_upstreams.go index 98857a3707..0b4cc62ea4 100644 --- a/internal/dataplane/translator/translate_upstreams.go +++ b/internal/dataplane/translator/translate_upstreams.go @@ -331,6 +331,16 @@ func getEndpoints( return upstreamServers } +// targetWeightOrDefault returns the effective value of a target weight pointer. If the pointer is non-nil, it returns +// the pointee. If the pointer is nil, it returns 100, the default Kong target weight. This allows us to sum +// deduplicated targets' weights if one happens to be unset in the controller. +func targetWeightOrDefault(in *int) int { + if in != nil { + return *in + } + return 100 +} + func updateTargetMap(targetMap map[string]kongstate.Target, t kongstate.Target) map[string]kongstate.Target { // See https://github.com/Kong/kubernetes-ingress-controller/issues/5761: // Duplicate targets will appear in configurations that use Services with the same selector, which are used @@ -341,7 +351,7 @@ func updateTargetMap(targetMap map[string]kongstate.Target, t kongstate.Target) // instead requires t.Target.Target. For consistency, everything below explicitly includes the nested object // name, so t.Target.Weight instead of t.Weight. if existing, ok := targetMap[*t.Target.Target]; ok { - sum := *existing.Target.Weight + *t.Target.Weight + sum := targetWeightOrDefault(existing.Target.Weight) + targetWeightOrDefault(t.Target.Weight) existing.Target.Weight = &sum targetMap[*t.Target.Target] = existing } else { diff --git a/test/integration/ingress_test.go b/test/integration/ingress_test.go index 66e155a23e..754f5af246 100644 --- a/test/integration/ingress_test.go +++ b/test/integration/ingress_test.go @@ -297,7 +297,7 @@ func TestIngressClassNameSpec(t *testing.T) { helpers.EventuallyExpectHTTP404WithNoRoute(t, proxyHTTPURL, proxyHTTPURL.Host, "/test_ingressclassname_spec", ingressWait, waitTick, nil) } -func TestIngressNamespaces(t *testing.T) { +func TestIngressServiceUpstream(t *testing.T) { t.Parallel() ctx := context.Background() @@ -308,12 +308,15 @@ func TestIngressNamespaces(t *testing.T) { t.Log("deploying a minimal HTTP container deployment to test Ingress routes") container := generators.NewContainer("httpbin", test.HTTPBinImage, test.HTTPBinPort) deployment := generators.NewDeploymentForContainer(container) + deployment.Spec.Replicas = lo.ToPtr(int32(3)) deployment, err := env.Cluster().Client().AppsV1().Deployments(ns.Name).Create(ctx, deployment, metav1.CreateOptions{}) require.NoError(t, err) cleaner.Add(deployment) t.Logf("exposing deployment %s via service", deployment.Name) service := generators.NewServiceForDeployment(deployment, corev1.ServiceTypeLoadBalancer) + service.ObjectMeta.Annotations = map[string]string{} + service.ObjectMeta.Annotations["ingress.kubernetes.io/service-upstream"] = "true" service, err = env.Cluster().Client().CoreV1().Services(ns.Name).Create(ctx, service, metav1.CreateOptions{}) require.NoError(t, err) cleaner.Add(service)