Repository: haproxytech/kubernetes-ingress

Issue:
Ingress pathType "Prefix" can override pathType "Exact"
Let's assume we have a web service with prefix `/foo`. There are many sub-paths for `/foo/...`. Let's also assume we want to have different timeouts for `/foo` and `/foo/.*`.

We would expect the following to work:

```yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: subpath-foo
  annotations:
    haproxy.org/standalone-backend: "true"
    haproxy.org/timeout-server: 5s
spec:
  ingressClassName: haproxy
  rules:
  - host: example.tld 
    http:
      paths:
      - path: /foo
        pathType: Prefix
        backend:
          service:
            name: foo-service
            port:
              name: http
---
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: just-foo
  annotations:
    haproxy.org/standalone-backend: "true"
    haproxy.org/timeout-server: 10s
spec:
  ingressClassName: haproxy
  rules:
  - host: example.tld 
    http:
      paths:
      - path: /foo
        pathType: Exact
        backend:
          service:
            name: foo-service
            port:
              name: http
```

The annotation `haproxy.org/standalone-backend`ensures two separate backends are created so that the different timeouts do not fight each other.
Now we have the problem that `/etc/haproxy/maps/path-exact.map` has the following contents:

```
example.tld/foo          ing_default_subpath-foo_http.dbe4b28f9c4404fc36a7ae76ead9a37d
example.tld/foo          ing_default_just-foo_http.dbe4b28f9c4404fc36a7ae76ead9a37d
```

That means the actual exact match from ingress `just-foo` will never match since pathType `Prefix` patterns are also put into this pattern file.
[Here](https://github.com/haproxytech/kubernetes-ingress/blob/04d271ccc641fb9295b0ea1c7cf7af5f9397e52d/pkg/route/route.go#L90) we can find the responsible code:

```golang
	path := route.Path.Path
	switch {
	case route.Path.PathTypeMatch == store.PATH_TYPE_EXACT:
		mapFiles.MapAppend(PATH_EXACT, route.Host+path+"\t\t\t"+value)
	case path == "" || path == "/":
		mapFiles.MapAppend(PATH_PREFIX, route.Host+"/"+"\t\t\t"+value)
	case route.Path.PathTypeMatch == store.PATH_TYPE_PREFIX:
		path = strings.TrimSuffix(path, "/")
		mapFiles.MapAppend(PATH_EXACT, route.Host+path+"\t\t\t"+value)
		mapFiles.MapAppend(PATH_PREFIX, route.Host+path+"/"+"\t\t\t"+value)
	case route.Path.PathTypeMatch == store.PATH_TYPE_IMPLEMENTATION_SPECIFIC:
		path = strings.TrimSuffix(path, "/")
		mapFiles.MapAppend(PATH_EXACT, route.Host+path+"\t\t\t"+value)
		mapFiles.MapAppend(PATH_PREFIX, route.Host+path+"\t\t\t"+value)
	default:
		return fmt.Errorf("unknown path type '%s' with backend '%s'", route.Path.PathTypeMatch, route.BackendName)
	}
```

I do understand why `Prefix` paths must have an entry in `path-exact.map`: The [definition](https://kubernetes.io/docs/concepts/services-networking/ingress/#examples) of ingress resources mandates that `/foo` must match `/foo` and `/foo/bar` but never `/foobar`. Therefore we add `/foo` to exact matches and `/foo/` to prefix matches (via `map_beg`).

The only way I see to resolve this issue in a clean and backward-compatible way is to introduce a new map file which will contain all exact matches from pathType `Prefix` only and it will be matched if there was no match in `path-exact.map` which will then contain ONLY pathType `Exact` patterns.

I will follow up with a PR.

Discussion:
@dosmanak: Just a while ago I wondered the Implementation Specific variant could by also implemented in haproxy with regex matching available.@phihos: Hi @dosmanak, if an ingress controller wants to implement regex-based parsing then pathType `ImplementationSpecfic` is the way to go. But this is beside the point of this issue. Here I discussed that prefix paths can sometimes override exact paths, which should not happen.
If you would like to discuss regex-based path matching feel free to open another issue for that. I am not a maintainer of this project, but I certainly can contribute some caveats to be aware of.@stale: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@phihos: Bump to remove stale label. There is still an open PR for this and it is still discussed by the maintainers AFAIK.@stale: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
@phihos: @oktalz Do you have a suggestion on how to improve the naming of the new map file?@stale: This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.


Solution patch:
diff --git a/pkg/annotations/common/main.go b/pkg/annotations/common/main.go
index e56fae0..6b4e61e 100644
--- a/pkg/annotations/common/main.go
+++ b/pkg/annotations/common/main.go
@@ -75,3 +75,21 @@ func GetValuesAndIndices(annotationName string, annotations ...map[string]string
 	}
 	return result
 }
+
+// EnsureQuoted ensures that a string starts and ends with a double quote.
+// It adds a quote to the beginning if one is not already present,
+// and adds a quote to the end if one is not already present.
+func EnsureQuoted(s string) string {
+	newS := s
+	if s == "\"" || s == "" {
+		newS = "\"\""
+		return newS
+	}
+	if !strings.HasPrefix(newS, "\"") {
+		newS = "\"" + newS
+	}
+	if !strings.HasSuffix(newS, "\"") {
+		newS += "\""
+	}
+	return newS
+}
diff --git a/pkg/annotations/ingress/reqSetHdr.go b/pkg/annotations/ingress/reqSetHdr.go
index 8161b05..0539828 100644
--- a/pkg/annotations/ingress/reqSetHdr.go
+++ b/pkg/annotations/ingress/reqSetHdr.go
@@ -42,7 +42,7 @@ func (a *SetHdr) Process(k store.K8s, annotations ...map[string]string) (err err
 		}
 		a.rules.Add(&rules.SetHdr{
 			HdrName:   param[:indexSpace],
-			HdrFormat: "\"" + param[indexSpace+1:] + "\"",
+			HdrFormat: common.EnsureQuoted(param[indexSpace+1:]),
 			Response:  a.response,
 		})
 	}
diff --git a/pkg/annotations/ingress/resSetCORS.go b/pkg/annotations/ingress/resSetCORS.go
index a907f30..9cd6a23 100644
--- a/pkg/annotations/ingress/resSetCORS.go
+++ b/pkg/annotations/ingress/resSetCORS.go
@@ -93,7 +93,7 @@ func (a ResSetCORSAnn) Process(k store.K8s, annotations ...map[string]string) (e
 					return fmt.Errorf("unsupported HTTP method '%s' in cors-allow-methods configuration", methods[i])
 				}
 			}
-			input = "\"" + strings.Join(methods, ", ") + "\""
+			input = common.EnsureQuoted(strings.Join(methods, ", "))
 		}
 		a.parent.rules.Add(&rules.SetHdr{
 			HdrName:       "Access-Control-Allow-Methods",
@@ -109,7 +109,7 @@ func (a ResSetCORSAnn) Process(k store.K8s, annotations ...map[string]string) (e
 		input = strings.Join(strings.Fields(input), "") // strip spaces
 		a.parent.rules.Add(rules.SetHdr{
 			HdrName:       "Access-Control-Allow-Headers",
-			HdrFormat:     "\"" + input + "\"",
+			HdrFormat:     common.EnsureQuoted(input),
 			AfterResponse: true,
 			CondTest:      a.parent.acl,
 			Cond:          "if",
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
index 46a1ca4..a8d72d0 100644
--- a/pkg/controller/controller.go
+++ b/pkg/controller/controller.go
@@ -298,6 +298,12 @@ func (c *HAProxyController) setupHAProxyRules() error {
 				Scope:      "txn",
 				Expression: fmt.Sprintf("var(txn.host_match),concat(,txn.path,),map(%s)", maps.GetPath(route.PATH_EXACT)),
 			}, false),
+			c.haproxy.AddRule(frontend, rules.ReqSetVar{
+				Name:       "path_match",
+				Scope:      "txn",
+				Expression: fmt.Sprintf("var(txn.host_match),concat(,txn.path,),map(%s)", maps.GetPath(route.PATH_PREFIX_EXACT)),
+				CondTest:   "!{ var(txn.path_match) -m found }",
+			}, false),
 			c.haproxy.AddRule(frontend, rules.ReqSetVar{
 				Name:       "path_match",
 				Scope:      "txn",
diff --git a/pkg/haproxy/main.go b/pkg/haproxy/main.go
index f9c945b..b4f40f6 100644
--- a/pkg/haproxy/main.go
+++ b/pkg/haproxy/main.go
@@ -53,6 +53,7 @@ func New(osArgs utils.OSArgs, env env.Env, cfgFile []byte, p process.Process, cl
 		route.SNI,
 		route.HOST,
 		route.PATH_EXACT,
+		route.PATH_PREFIX_EXACT,
 		route.PATH_PREFIX,
 	}
 	if h.Maps, err = maps.New(env.MapsDir, persistentMaps); err != nil {
diff --git a/pkg/route/route.go b/pkg/route/route.go
index 69b1e54..30b5ab8 100644
--- a/pkg/route/route.go
+++ b/pkg/route/route.go
@@ -33,10 +33,11 @@ const (
 	FrontendHTTP  = "http"
 	FrontendHTTPS = "https"
 	// Routing Maps
-	SNI         maps.Name = "sni"
-	HOST        maps.Name = "host"
-	PATH_EXACT  maps.Name = "path-exact"
-	PATH_PREFIX maps.Name = "path-prefix"
+	SNI               maps.Name = "sni"
+	HOST              maps.Name = "host"
+	PATH_EXACT        maps.Name = "path-exact"
+	PATH_PREFIX_EXACT maps.Name = "path-prefix-exact"
+	PATH_PREFIX       maps.Name = "path-prefix"
 )
 
 var (
@@ -87,11 +88,11 @@ func AddHostPathRoute(route Route, mapFiles maps.Maps) error {
 		mapFiles.MapAppend(PATH_PREFIX, route.Host+"/"+"\t\t\t"+value)
 	case route.Path.PathTypeMatch == store.PATH_TYPE_PREFIX:
 		path = strings.TrimSuffix(path, "/")
-		mapFiles.MapAppend(PATH_EXACT, route.Host+path+"\t\t\t"+value)
+		mapFiles.MapAppend(PATH_PREFIX_EXACT, route.Host+path+"\t\t\t"+value)
 		mapFiles.MapAppend(PATH_PREFIX, route.Host+path+"/"+"\t\t\t"+value)
 	case route.Path.PathTypeMatch == store.PATH_TYPE_IMPLEMENTATION_SPECIFIC:
 		path = strings.TrimSuffix(path, "/")
-		mapFiles.MapAppend(PATH_EXACT, route.Host+path+"\t\t\t"+value)
+		mapFiles.MapAppend(PATH_PREFIX_EXACT, route.Host+path+"\t\t\t"+value)
 		mapFiles.MapAppend(PATH_PREFIX, route.Host+path+"\t\t\t"+value)
 	default:
 		return fmt.Errorf("unknown path type '%s' with backend '%s'", route.Path.PathTypeMatch, route.BackendName)



Test patch:
diff --git a/deploy/tests/e2e/client.go b/deploy/tests/e2e/client.go
index 425436e..b711203 100644
--- a/deploy/tests/e2e/client.go
+++ b/deploy/tests/e2e/client.go
@@ -202,7 +202,7 @@ func runtimeCommand(command string) (result []byte, err error) {
 	if err != nil {
 		return
 	}
-	result = make([]byte, 1024)
+	result = make([]byte, 2048)
 	_, err = conn.Read(result)
 	conn.Close()
 	return
@@ -217,7 +217,7 @@ func GetHAProxyMapCount(mapName string) (count int, err error) {
 	scanner := bufio.NewScanner(bytes.NewReader(result))
 	for scanner.Scan() {
 		line := scanner.Text()
-		if strings.Contains(line, mapName) {
+		if strings.Contains(line, mapName+".map") {
 			r := regexp.MustCompile("entry_cnt=[0-9]*")
 			match := r.FindString(line)
 			nbr := strings.Split(match, "=")[1]
diff --git a/deploy/tests/e2e/cors/cors_test.go b/deploy/tests/e2e/cors/cors_test.go
index 9cdb056..1898f37 100644
--- a/deploy/tests/e2e/cors/cors_test.go
+++ b/deploy/tests/e2e/cors/cors_test.go
@@ -20,6 +20,7 @@ import (
 	"net/http"
 
 	"github.com/haproxytech/kubernetes-ingress/deploy/tests/e2e"
+	"github.com/haproxytech/kubernetes-ingress/pkg/annotations/common"
 )
 
 const (
@@ -104,7 +105,7 @@ func (suite *CorsSuite) eventuallyReturnsWithNoContentOption(expectedHeaders, un
 }
 
 func q(value string) string {
-	return "\"" + value + "\""
+	return common.EnsureQuoted(value)
 }
 
 func (suite *CorsSuite) Default(ingressCors bool) func() {
diff --git a/deploy/tests/e2e/map-updates/update_test.go b/deploy/tests/e2e/map-updates/update_test.go
index 8777f6d..12db74a 100644
--- a/deploy/tests/e2e/map-updates/update_test.go
+++ b/deploy/tests/e2e/map-updates/update_test.go
@@ -30,17 +30,27 @@ func (suite *MapUpdateSuite) Test_Update() {
 			suite.tmplData.Paths = append(suite.tmplData.Paths, strconv.Itoa(i))
 		}
 		oldInfo, err := e2e.GetGlobalHAProxyInfo()
-		oldCount, err := e2e.GetHAProxyMapCount("path-prefix")
+		oldCountExact, err := e2e.GetHAProxyMapCount("path-exact")
+		suite.Require().NoError(err)
+		oldCountPrefixExact, err := e2e.GetHAProxyMapCount("path-prefix-exact")
+		suite.Require().NoError(err)
+		oldCountPrefix, err := e2e.GetHAProxyMapCount("path-prefix")
 		suite.Require().NoError(err)
 		suite.Require().NoError(suite.test.Apply("config/ingress.yaml.tmpl", suite.test.GetNS(), suite.tmplData))
 		suite.Require().Eventually(func() bool {
 			newInfo, err := e2e.GetGlobalHAProxyInfo()
 			suite.Require().NoError(err)
-			count, err := e2e.GetHAProxyMapCount("path-prefix")
+			countExact, err := e2e.GetHAProxyMapCount("path-exact")
+			suite.Require().NoError(err)
+			countPrefixExact, err := e2e.GetHAProxyMapCount("path-prefix-exact")
+			suite.Require().NoError(err)
+			countPrefix, err := e2e.GetHAProxyMapCount("path-prefix")
 			suite.Require().NoError(err)
-			numOfAddedEntries := count - oldCount + 1 // We add one because there's already an entry at the begining which will be removed
-			suite.T().Logf("oldInfo.Pid(%s) == newInfo.Pid(%s) && additional path-prefix.count(%d) == %d", oldInfo.Pid, newInfo.Pid, numOfAddedEntries, n)
-			return oldInfo.Pid == newInfo.Pid && numOfAddedEntries == n
+			numOfAddedEntriesExact := countExact - oldCountExact
+			numOfAddedEntriesPrefixExact := countPrefixExact - oldCountPrefixExact
+			numOfAddedEntriesPrefix := countPrefix - oldCountPrefix + 1 // We add one because there's already an entry at the beginning which will be removed
+			suite.T().Logf("oldInfo.Pid(%s) == newInfo.Pid(%s) && additional path-exact.count(%d) == %d && additional path-prefix-exact.count(%d) == %d && additional path-prefix.count(%d) == %d", oldInfo.Pid, newInfo.Pid, numOfAddedEntriesExact, 0, numOfAddedEntriesPrefixExact, n, numOfAddedEntriesPrefix, n)
+			return oldInfo.Pid == newInfo.Pid && numOfAddedEntriesExact == 0 && numOfAddedEntriesPrefixExact == n && numOfAddedEntriesPrefix == n
 		}, e2e.WaitDuration, e2e.TickDuration)
 	})
 }
diff --git a/test/annotations/common_test.go b/test/annotations/common_test.go
new file mode 100644
index 0000000..a231420
--- /dev/null
+++ b/test/annotations/common_test.go
@@ -0,0 +1,69 @@
+package annotations_test
+
+import (
+	"testing"
+
+	"github.com/haproxytech/kubernetes-ingress/pkg/annotations/common"
+)
+
+func TestEnsureQuoted(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{
+			name:  "simple string",
+			input: "test",
+			want:  "\"test\"",
+		},
+		{
+			name:  "already quoted",
+			input: "\"test\"",
+			want:  "\"test\"",
+		},
+		{
+			name:  "empty string",
+			input: "",
+			want:  "\"\"",
+		},
+		{
+			name:  "starts with quote",
+			input: "\"test",
+			want:  "\"test\"",
+		},
+		{
+			name:  "ends with quote",
+			input: "test\"",
+			want:  "\"test\"",
+		},
+		{
+			name:  "single quote",
+			input: "\"",
+			want:  "\"\"",
+		},
+		{
+			name:  "empty quoted string",
+			input: "\"\"",
+			want:  "\"\"",
+		},
+		{
+			name:  "string with internal quotes",
+			input: "te\"st",
+			want:  "\"te\"st\"",
+		},
+		{
+			name:  "string with leading/trailing spaces",
+			input: " test ",
+			want:  "\" test \"",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := common.EnsureQuoted(tt.input); got != tt.want {
+				t.Errorf("EnsureQuoted() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}