Merge pull request #343 from miheer/path-based-route-issue

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route
openshift · Sep 27, 2021 · ddca0a6 · ddca0a6
2 parents b34df4c + 019c5ac
commit ddca0a6
Show file tree

Hide file tree

Showing 4 changed files with 33 additions and 16 deletions.
diff --git a/images/router/haproxy/conf/haproxy-config.template b/images/router/haproxy/conf/haproxy-config.template
@@ -231,7 +231,7 @@ frontend public
     {{- end }}
 
   # check if we need to redirect/force using https.
-  acl secure_redirect base,map_reg(/var/lib/haproxy/conf/os_route_http_redirect.map) -m found
+  acl secure_redirect base,map_reg_int(/var/lib/haproxy/conf/os_route_http_redirect.map) -m bool
   redirect scheme https if secure_redirect
 
   use_backend %[base,map_reg(/var/lib/haproxy/conf/os_http_be.map)]

diff --git a/pkg/router/template/template_helper_test.go b/pkg/router/template/template_helper_test.go
@@ -479,12 +479,23 @@ func TestGenerateHAProxyMap(t *testing.T) {
 		t.Errorf("TestGenerateHAProxyMap os_edge_reencrypt_be.map error: %v", err)
 	}
 
+	// Need to add these as now we add all the routes in the redirect map which don't have Redirect policy
+	// but we differentiate them based on  values 1 and 0 where 1 means Redirect Policy is enabled.
 	httpRedirectOrder := []string{
-		"test:api-route",
-		"dev:reencrypt-route",
-		"prod:backend-route",
-		"stg:api-route",
-		"prod:api-route",
+		`^zzz-production\.wildcard\.test\.?(:[0-9]+)?(/.*)?$ 1`,
+		`^zed\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^reencrypt-dev\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 1`,
+		`^passthrough-prod\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^passthrough-dev\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^backend-app\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 1`,
+		`^api-stg\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 1`,
+		`^api-prod\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?/x/y/z(/.*)?$ 0`,
+		`^api-prod\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 1`,
+		`^3dev\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^3app-admin\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^[^\.]*\.foo\.wildcard\.test\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^[^\.]*\.foo\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
+		`^[^\.]*\.127\.0\.0\.1\.nip\.io\.?(:[0-9]+)?(/.*)?$ 0`,
 	}
 
 	lines = generateHAProxyMap("os_route_http_redirect.map", td)

diff --git a/pkg/router/template/util/haproxy/map_entry.go b/pkg/router/template/util/haproxy/map_entry.go
@@ -58,13 +58,17 @@ func generateEdgeReencryptMapEntry(cfg *BackendConfig) *HAProxyMapEntry {
 
 // generateHttpRedirectMapEntry generates a map entry for redirecting insecure/http hosts.
 func generateHttpRedirectMapEntry(cfg *BackendConfig) *HAProxyMapEntry {
-	if len(cfg.Host) > 0 && cfg.InsecurePolicy == routev1.InsecureEdgeTerminationPolicyRedirect {
-		return &HAProxyMapEntry{
+	if len(cfg.Host) > 0 {
+		haproxyMapEntry := &HAProxyMapEntry{
 			Key:   templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),
-			Value: cfg.Name,
+			Value: "0",
+		}
+		switch cfg.InsecurePolicy {
+		case routev1.InsecureEdgeTerminationPolicyRedirect:
+			haproxyMapEntry.Value = "1"
 		}
+		return haproxyMapEntry
 	}
-
 	return nil
 }
 

diff --git a/pkg/router/template/util/haproxy/map_entry_test.go b/pkg/router/template/util/haproxy/map_entry_test.go
@@ -444,15 +444,17 @@ func TestGenerateHttpRedirectMapEntry(t *testing.T) {
 		expectation *HAProxyMapEntry
 	}
 
-	buildTestExpectation := func(name, key string, policy routev1.InsecureEdgeTerminationPolicyType) *HAProxyMapEntry {
+	buildTestExpectation := func(hostname, name, key string, policy routev1.InsecureEdgeTerminationPolicyType) *HAProxyMapEntry {
 		if len(key) == 0 {
 			return nil
 		}
-
-		if policy == routev1.InsecureEdgeTerminationPolicyRedirect {
-			return &HAProxyMapEntry{Key: key, Value: name}
+		if len(hostname) != 0 {
+			if policy == routev1.InsecureEdgeTerminationPolicyRedirect {
+				return &HAProxyMapEntry{Key: key, Value: "1"}
+			} else {
+				return &HAProxyMapEntry{Key: key, Value: "0"}
+			}
 		}
-
 		return nil
 	}
 
@@ -464,7 +466,7 @@ func TestGenerateHttpRedirectMapEntry(t *testing.T) {
 					name: fmt.Sprintf("%s:termination=%s:policy=%s", tt.name, termination, policy),
 					cfg:  testBackendConfig(tt.backendKey, tt.hostname, tt.path, tt.wildcard, termination, policy, false),
 
-					expectation: buildTestExpectation(tt.backendKey, tt.expectedKey, policy),
+					expectation: buildTestExpectation(tt.hostname, tt.backendKey, tt.expectedKey, policy),
 				})
 			}
 		}