From 422e9c4c4ec410f183f75a9cc62c8df179746b10 Mon Sep 17 00:00:00 2001 From: Adrian Moisey Date: Sun, 21 Apr 2024 15:01:48 +0200 Subject: [PATCH 1/4] Fix incorrect service name being set Fix: https://github.com/kubernetes/ingress-nginx/issues/10210 This handles the case where multiple rules have identical paths, but differing types. --- .../ingress/controller/template/template.go | 12 +++- .../controller/template/template_test.go | 69 ++++++++++++++++++- rootfs/etc/nginx/template/nginx.tmpl | 2 +- 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index ed052e4ec..5a6b75654 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -37,6 +37,7 @@ import ( text_template "text/template" networkingv1 "k8s.io/api/networking/v1" + v1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" @@ -1051,7 +1052,7 @@ func (info *ingressInformation) Equal(other *ingressInformation) bool { return true } -func getIngressInformation(i, h, p interface{}) *ingressInformation { +func getIngressInformation(i, h, p, t interface{}) *ingressInformation { ing, ok := i.(*ingress.Ingress) if !ok { klog.Errorf("expected an '*ingress.Ingress' type but %T was returned", i) @@ -1070,6 +1071,11 @@ func getIngressInformation(i, h, p interface{}) *ingressInformation { return &ingressInformation{} } + ingressPathType, ok := t.(*v1.PathType) + if !ok { + klog.Errorf("expected a '*v1.PathType' type but %T was returned", t) + } + if ing == nil { return &ingressInformation{} } @@ -1118,6 +1124,10 @@ func getIngressInformation(i, h, p interface{}) *ingressInformation { continue } + if *ingressPathType != *rPath.PathType { + continue + } + if rPath.Backend.Service == nil { continue } diff --git a/internal/ingress/controller/template/template_test.go b/internal/ingress/controller/template/template_test.go index 6553f5daf..3bf5ef0f8 100644 --- a/internal/ingress/controller/template/template_test.go +++ b/internal/ingress/controller/template/template_test.go @@ -55,6 +55,7 @@ func init() { var ( pathPrefix networking.PathType = networking.PathTypePrefix + pathExact networking.PathType = networking.PathTypeExact // TODO: add tests for SSLPassthrough tmplFuncTestcases = map[string]struct { @@ -1154,18 +1155,21 @@ func TestGetIngressInformation(t *testing.T) { Ingress interface{} Host string Path interface{} + PathType interface{} Expected *ingressInformation }{ "wrong ingress type": { "wrongtype", "host1", "/ok", + "", &ingressInformation{}, }, "wrong path type": { &ingress.Ingress{}, "host1", 10, + "", &ingressInformation{}, }, "valid ingress definition with name validIng in namespace default using a service with name a-svc port number 8080": { @@ -1192,6 +1196,7 @@ func TestGetIngressInformation(t *testing.T) { }, "host1", "", + "", &ingressInformation{ Namespace: "default", Rule: "validIng", @@ -1227,6 +1232,7 @@ func TestGetIngressInformation(t *testing.T) { }, "host1", "", + "", &ingressInformation{ Namespace: "default", Rule: "validIng", @@ -1259,6 +1265,7 @@ func TestGetIngressInformation(t *testing.T) { }, "host1", "", + "", &ingressInformation{ Namespace: "default", Rule: "validIng", @@ -1309,6 +1316,7 @@ func TestGetIngressInformation(t *testing.T) { }, "foo.bar", "/ok", + &pathPrefix, &ingressInformation{ Namespace: "something", Rule: "demo", @@ -1359,6 +1367,7 @@ func TestGetIngressInformation(t *testing.T) { }, "foo.bar", "/ok", + &pathPrefix, &ingressInformation{ Namespace: "something", Rule: "demo", @@ -1404,6 +1413,7 @@ func TestGetIngressInformation(t *testing.T) { }, "foo.bar", "/ok", + &pathPrefix, &ingressInformation{ Namespace: "something", Rule: "demo", @@ -1459,6 +1469,7 @@ func TestGetIngressInformation(t *testing.T) { }, "foo.bar", "/oksvc", + &pathPrefix, &ingressInformation{ Namespace: "something", Rule: "demo", @@ -1469,10 +1480,66 @@ func TestGetIngressInformation(t *testing.T) { ServicePort: "b-svc-80", }, }, + "valid ingress definition with name demo in namespace something and two path / with Prefix and Exact": { + &ingress.Ingress{ + Ingress: networking.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "demo", + Namespace: "something", + Annotations: map[string]string{ + "ingress.annotation": "ok", + }, + }, + Spec: networking.IngressSpec{ + Rules: []networking.IngressRule{ + { + Host: "foo.bar", + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{ + Paths: []networking.HTTPIngressPath{ + { + Path: "/", + PathType: &pathPrefix, + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: "a-svc", + }, + }, + }, + { + Path: "/", + PathType: &pathExact, + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: "b-svc", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + "foo.bar", + "/", + &pathExact, + &ingressInformation{ + Path: "/", + Namespace: "something", + Rule: "demo", + Annotations: map[string]string{ + "ingress.annotation": "ok", + }, + Service: "b-svc", + }, + }, } for title, testCase := range testcases { - info := getIngressInformation(testCase.Ingress, testCase.Host, testCase.Path) + info := getIngressInformation(testCase.Ingress, testCase.Host, testCase.Path, testCase.PathType) if !info.Equal(testCase.Expected) { t.Fatalf("%s: expected '%v' but returned %v", title, testCase.Expected, info) diff --git a/rootfs/etc/nginx/template/nginx.tmpl b/rootfs/etc/nginx/template/nginx.tmpl index 6b8e750b0..bdaf46d34 100644 --- a/rootfs/etc/nginx/template/nginx.tmpl +++ b/rootfs/etc/nginx/template/nginx.tmpl @@ -1110,7 +1110,7 @@ stream { {{ end }} location {{ $path }} { - {{ $ing := (getIngressInformation $location.Ingress $server.Hostname $location.IngressPath) }} + {{ $ing := (getIngressInformation $location.Ingress $server.Hostname $location.IngressPath $location.PathType) }} set $namespace {{ $ing.Namespace | quote}}; set $ingress_name {{ $ing.Rule | quote }}; set $service_name {{ $ing.Service | quote }}; From 5b60f4e2d78cd21e5f3f50634fc3194e305b9742 Mon Sep 17 00:00:00 2001 From: Adrian Moisey Date: Mon, 22 Apr 2024 14:48:55 +0200 Subject: [PATCH 2/4] Remove duplicate import --- internal/ingress/controller/template/template.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/ingress/controller/template/template.go b/internal/ingress/controller/template/template.go index 5a6b75654..00fe9aea3 100644 --- a/internal/ingress/controller/template/template.go +++ b/internal/ingress/controller/template/template.go @@ -37,7 +37,6 @@ import ( text_template "text/template" networkingv1 "k8s.io/api/networking/v1" - v1 "k8s.io/api/networking/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" @@ -1071,7 +1070,7 @@ func getIngressInformation(i, h, p, t interface{}) *ingressInformation { return &ingressInformation{} } - ingressPathType, ok := t.(*v1.PathType) + ingressPathType, ok := t.(*networkingv1.PathType) if !ok { klog.Errorf("expected a '*v1.PathType' type but %T was returned", t) } From cc63aeec41cfd5e9a0f5a1233e02c7aad8cca3cd Mon Sep 17 00:00:00 2001 From: Adrian Moisey Date: Thu, 21 Nov 2024 07:11:52 +0200 Subject: [PATCH 3/4] Add e2e test --- test/e2e/framework/framework.go | 31 +++++++++++++++++++ test/e2e/framework/metrics.go | 17 ++++++++++ test/e2e/metrics/metrics.go | 55 +++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+) diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 204da7df0..a28405155 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -732,6 +732,37 @@ func NewSingleIngressWithMultiplePaths(name string, paths []string, host, ns, se return newSingleIngress(name, ns, annotations, spec) } +func NewSingleIngressWithMultiplePathsOfDifferentTypes(name string, host, ns string, services map[string]networking.PathType, port int, annotations map[string]string) *networking.Ingress { + spec := networking.IngressSpec{ + IngressClassName: GetIngressClassName(ns), + Rules: []networking.IngressRule{ + { + Host: host, + IngressRuleValue: networking.IngressRuleValue{ + HTTP: &networking.HTTPIngressRuleValue{}, + }, + }, + }, + } + + for service, pathType := range services { + spec.Rules[0].IngressRuleValue.HTTP.Paths = append(spec.Rules[0].IngressRuleValue.HTTP.Paths, networking.HTTPIngressPath{ + Path: "/", + PathType: &pathType, + Backend: networking.IngressBackend{ + Service: &networking.IngressServiceBackend{ + Name: service, + Port: networking.ServiceBackendPort{ + Number: int32(port), + }, + }, + }, + }) + } + + return newSingleIngress(name, ns, annotations, spec) +} + func newSingleIngressWithRules(name, path, host, ns, service string, port int, annotations map[string]string, tlsHosts []string) *networking.Ingress { pathtype := networking.PathTypePrefix spec := networking.IngressSpec{ diff --git a/test/e2e/framework/metrics.go b/test/e2e/framework/metrics.go index 774f1bd7e..dfc3a8754 100644 --- a/test/e2e/framework/metrics.go +++ b/test/e2e/framework/metrics.go @@ -56,3 +56,20 @@ func (f *Framework) GetMetric(metricName, ip string) (*dto.MetricFamily, error) return nil, fmt.Errorf("there is no metric with name %v", metricName) } + +func (f *Framework) GetLabelValue(metric *dto.Metric, labelName string) (string, bool) { + // Use the proto descriptor of the metric + // metricProto := &dto.Metric{} + // if err := metric.Write(metricProto); err != nil { + // fmt.Println("Error writing metric:", err) + // return "", false + // } + + // Iterate through the label pairs + for _, label := range metric.Label { + if label.GetName() == labelName { + return label.GetValue(), true + } + } + return "", false +} diff --git a/test/e2e/metrics/metrics.go b/test/e2e/metrics/metrics.go index bec09bb37..12b3f4ac3 100644 --- a/test/e2e/metrics/metrics.go +++ b/test/e2e/metrics/metrics.go @@ -26,6 +26,7 @@ import ( "github.com/onsi/ginkgo/v2" "github.com/stretchr/testify/assert" appsv1 "k8s.io/api/apps/v1" + networking "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/ingress-nginx/test/e2e/framework" @@ -139,3 +140,57 @@ var _ = framework.IngressNginxDescribe("[metrics] exported prometheus metrics", assert.Nil(ginkgo.GinkgoT(), reqMetrics) }) }) + +var _ = framework.IngressNginxDescribe("[metrics] exported prometheus metrics when multiple services share a path", func() { + f := framework.NewDefaultFramework("metrics") + host := "foo.com" + + configuredServices := map[string]networking.PathType{ + framework.EchoService: networking.PathTypePrefix, + framework.HTTPBunService: networking.PathTypeExact, + } + + ginkgo.BeforeEach(func() { + f.NewEchoDeployment() + f.NewHttpbunDeployment() + f.EnsureIngress(framework.NewSingleIngressWithMultiplePathsOfDifferentTypes("multiplepaths", host, f.Namespace, configuredServices, 80, nil)) + f.WaitForNginxServer(host, + func(server string) bool { + return strings.Contains(server, fmt.Sprintf("server_name %s ;", host)) && + strings.Contains(server, "proxy_pass http://upstream_balancer;") + }) + }) + + ginkgo.It("ensures that each service has metrics reporting", func() { + f.HTTPTestClient(). + GET("/"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + f.HTTPTestClient(). + GET("/path"). + WithHeader("Host", host). + Expect(). + Status(http.StatusOK) + + time.Sleep(waitForMetrics) + + fetchedServices := []string{} + + ip := f.GetNginxPodIP() + mf, err := f.GetMetric("nginx_ingress_controller_requests", ip) + assert.Len(ginkgo.GinkgoT(), mf.Metric, 2) + + for _, metric := range mf.Metric { + label, _ := f.GetLabelValue(metric, "service") + fetchedServices = append(fetchedServices, label) + } + + assert.Equal(ginkgo.GinkgoT(), len(configuredServices), len(fetchedServices)) + + for _, service := range fetchedServices { + assert.Contains(ginkgo.GinkgoT(), configuredServices, service) + } + assert.NoError(ginkgo.GinkgoT(), err) + }) +}) From 0be3a5a0ecff37c827e9b13e71964a019db4aeda Mon Sep 17 00:00:00 2001 From: Adrian Moisey Date: Thu, 21 Nov 2024 07:41:41 +0200 Subject: [PATCH 4/4] Fix lint --- test/e2e/framework/framework.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index a28405155..f8633b3e5 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -732,7 +732,7 @@ func NewSingleIngressWithMultiplePaths(name string, paths []string, host, ns, se return newSingleIngress(name, ns, annotations, spec) } -func NewSingleIngressWithMultiplePathsOfDifferentTypes(name string, host, ns string, services map[string]networking.PathType, port int, annotations map[string]string) *networking.Ingress { +func NewSingleIngressWithMultiplePathsOfDifferentTypes(name, host, ns string, services map[string]networking.PathType, port int32, annotations map[string]string) *networking.Ingress { spec := networking.IngressSpec{ IngressClassName: GetIngressClassName(ns), Rules: []networking.IngressRule{ @@ -753,7 +753,7 @@ func NewSingleIngressWithMultiplePathsOfDifferentTypes(name string, host, ns str Service: &networking.IngressServiceBackend{ Name: service, Port: networking.ServiceBackendPort{ - Number: int32(port), + Number: port, }, }, },