From 4f2950febb8ec7f3ce7dd8880954858ff25fe8d0 Mon Sep 17 00:00:00 2001
From: Gordon Honda <1594377+ailurarctos@users.noreply.github.com>
Date: Fri, 16 Jun 2023 17:02:15 +0900
Subject: [PATCH] Fix duplicate location in NGINX config (#10090)

When creating ingresses that use the same path with a mix of
Exact and Prefix path types nginx-ingress-controller can
generate an NGINX config with a duplicate location:

    nginx: [emerg] duplicate location "/a/" in /tmp/nginx/nginx-cfg2287376726:693
    nginx: configuration file /tmp/nginx/nginx-cfg2287376726 test failed

The issue is that nginx-ingress-controller stops looking for
duplicates once it sees the same path with a different path type.
---
 internal/ingress/controller/controller.go     |   2 +-
 .../ingress/controller/controller_test.go     | 143 ++++++++++++++++++
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/internal/ingress/controller/controller.go b/internal/ingress/controller/controller.go
index cb8d3712c..eeeb95eb5 100644
--- a/internal/ingress/controller/controller.go
+++ b/internal/ingress/controller/controller.go
@@ -795,7 +795,7 @@ func (n *NGINXController) getBackendServers(ingresses []*ingress.Ingress) ([]*in
 					// Same paths but different types are allowed
 					// (same type means overlap in the path definition)
 					if !apiequality.Semantic.DeepEqual(loc.PathType, path.PathType) {
-						break
+						continue
 					}
 
 					addLoc = false
diff --git a/internal/ingress/controller/controller_test.go b/internal/ingress/controller/controller_test.go
index c07fcfadf..315525cee 100644
--- a/internal/ingress/controller/controller_test.go
+++ b/internal/ingress/controller/controller_test.go
@@ -2468,6 +2468,149 @@ func TestGetBackendServers(t *testing.T) {
 				}
 			},
 		},
+		{
+			Ingresses: []*ingress.Ingress{
+				{
+					Ingress: networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: networking.IngressSpec{
+							Rules: []networking.IngressRule{
+								{
+									Host: "example.com",
+									IngressRuleValue: networking.IngressRuleValue{
+										HTTP: &networking.HTTPIngressRuleValue{
+											Paths: []networking.HTTPIngressPath{
+												{
+													Path:     "/a",
+													PathType: &pathTypeExact,
+													Backend: networking.IngressBackend{
+														Service: &networking.IngressServiceBackend{
+															Name: "http-svc-1",
+															Port: networking.ServiceBackendPort{
+																Number: 80,
+															},
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{},
+				},
+				{
+					Ingress: networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: networking.IngressSpec{
+							Rules: []networking.IngressRule{
+								{
+									Host: "example.com",
+									IngressRuleValue: networking.IngressRuleValue{
+										HTTP: &networking.HTTPIngressRuleValue{
+											Paths: []networking.HTTPIngressPath{
+												{
+													Path:     "/a",
+													PathType: &pathTypePrefix,
+													Backend: networking.IngressBackend{
+														Service: &networking.IngressServiceBackend{
+															Name: "http-svc-2",
+															Port: networking.ServiceBackendPort{
+																Number: 80,
+															},
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{},
+				},
+				{
+					Ingress: networking.Ingress{
+						ObjectMeta: metav1.ObjectMeta{
+							Namespace: "example",
+						},
+						Spec: networking.IngressSpec{
+							Rules: []networking.IngressRule{
+								{
+									Host: "example.com",
+									IngressRuleValue: networking.IngressRuleValue{
+										HTTP: &networking.HTTPIngressRuleValue{
+											Paths: []networking.HTTPIngressPath{
+												{
+													Path:     "/a",
+													PathType: &pathTypePrefix,
+													Backend: networking.IngressBackend{
+														Service: &networking.IngressServiceBackend{
+															Name: "http-svc-3",
+															Port: networking.ServiceBackendPort{
+																Number: 80,
+															},
+														},
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+					ParsedAnnotations: &annotations.Ingress{},
+				},
+			},
+			Validate: func(ingresses []*ingress.Ingress, upstreams []*ingress.Backend, servers []*ingress.Server) {
+				if len(servers) != 2 {
+					t.Errorf("servers count should be 2, got %d", len(servers))
+					return
+				}
+
+				s := servers[0]
+				if s.Hostname != "_" {
+					t.Errorf("server hostname should be '_', got '%s'", s.Hostname)
+				}
+				if len(s.Locations) != 1 {
+					t.Errorf("servers locations count should be 1, got %d", len(s.Locations))
+					return
+				}
+				if !s.Locations[0].IsDefBackend {
+					t.Errorf("server location 0 should be default backend")
+				}
+				if s.Locations[0].Backend != defUpstreamName {
+					t.Errorf("location backend should be '%s', got '%s'", defUpstreamName, s.Locations[0].Backend)
+				}
+
+				s = servers[1]
+				if s.Hostname != "example.com" {
+					t.Errorf("server hostname should be 'example.com', got '%s'", s.Hostname)
+				}
+				if len(s.Locations) != 3 {
+					t.Errorf("servers locations count should be 2, got %d", len(s.Locations))
+					return
+				}
+				if s.Locations[0].Backend != "example-http-svc-1-80" {
+					t.Errorf("location 0 backend should be 'example-http-svc-1-80', got '%s'", s.Locations[0].Backend)
+				}
+				if s.Locations[1].Backend != "example-http-svc-2-80" {
+					t.Errorf("location 1 backend should be 'example-http-svc-2-80', got '%s'", s.Locations[1].Backend)
+				}
+				if s.Locations[2].Backend != defUpstreamName {
+					t.Errorf("location 2 backend should be '%s', got '%s'", defUpstreamName, s.Locations[2].Backend)
+				}
+			},
+			SetConfigMap: testConfigMap,
+		},
 	}
 
 	for _, testCase := range testCases {