Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route #343

miheer · 2021-09-09T06:29:57Z

Bug 2002205 - HTTPS redirect happens even if there is a more specific http-only route

Let's say we have 2 routes like this:

HTTP-only route for "myapp.apps.example.com" domain and "/mypath" path
HTTPS-edge-redirect route for "myapp.apps.example.com" and no path

In such situation, redirect is sent for any query to "myapp.apps.example.com", including "myapp.apps.example.com/mypath", so the more specific path route is not taking precedence as expected.

miheer · 2021-09-09T08:05:32Z

/bugzilla refresh

openshift-ci · 2021-09-09T08:07:09Z

@miheer: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

miheer · 2021-09-09T08:14:26Z

/bugzilla refresh

openshift-ci · 2021-09-09T08:16:39Z

@miheer: This pull request references Bugzilla bug 1896474, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-09-09T08:16:41Z

@miheer: This pull request references Bugzilla bug 1896474, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

miheer · 2021-09-09T08:17:45Z

/bugzilla refresh

openshift-ci · 2021-09-09T08:17:53Z

@miheer: This pull request references Bugzilla bug 1896474, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @quarterpin

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

miheer · 2021-09-10T19:01:41Z

I am still working on to fix the unit test as the following is failing ->

=== RUN TestGenerateHAProxyMap
template_helper_test.go:503: TestGenerateHAProxyMap os_route_http_redirect.map error: sorted data ^zzz-production.wildcard.test.?(:[0-9]+)?(/.*)?$ 1 at index 0 did not match suffix expectation test:api-route
--- FAIL: TestGenerateHAProxyMap (0.00s)
FAIL
FAIL github.com/openshift/router/pkg/router/template 0.008s
testing: warning: no tests to run

miheer · 2021-09-11T02:26:41Z

/retest

Miciah · 2021-09-11T20:28:45Z

pkg/router/template/template_helper_test.go

-		"test:api-route",
-		"dev:reencrypt-route",
-		"prod:backend-route",
-		"stg:api-route",
-		"prod:api-route",
+		"1",
+		"0",
+		"1",
+		"0",
+		"0",
+		"1",
+		"1",
+		"0",
+		"1",
+		"0",
+		"0",
+		"0",
+		"0",
+		"0",


Why include the key and not the value as we discussed?

Miciah · 2021-09-11T20:29:48Z

pkg/router/template/util/haproxy/map_entry.go

+		if cfg.InsecurePolicy == routev1.InsecureEdgeTerminationPolicyRedirect {
+			return &HAProxyMapEntry{
+				Key:   templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),
+				Value: "1",
+			}
+		} else {
+			return &HAProxyMapEntry{
+				Key:   templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),
+				Value: "0"}


Please fix the formatting issue with the } at the end of the line, consider using switch, and reduce code duplication.

melvinjoseph86 · 2021-09-15T07:37:34Z

/bugzilla cc-qa

openshift-ci · 2021-09-15T07:37:38Z

@melvinjoseph86: This pull request references Bugzilla bug 1896474, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (mjoseph@redhat.com), skipping review request.

In response to this:

/bugzilla cc-qa

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

melvinjoseph86 · 2021-09-15T10:55:33Z

/lgtm
/label qe-approved

Verified via pre-merge verification workflow, more references related to the test can be found in:
https://bugzilla.redhat.com/show_bug.cgi?id=1896474#c13

Miciah · 2021-09-20T12:49:28Z

pkg/router/template/util/haproxy/map_entry.go

+			Key: templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),
+		}
+		switch cfg.InsecurePolicy {
+		case routev1.InsecureEdgeTerminationPolicyRedirect:
+			haproxyMapEntry.Value = "1"
+			return haproxyMapEntry
+		default:
+			haproxyMapEntry.Value = "0"
+			return haproxyMapEntry
 		}


This can be simplified:

Suggested change

Key: templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),

}

switch cfg.InsecurePolicy {

case routev1.InsecureEdgeTerminationPolicyRedirect:

haproxyMapEntry.Value = "1"

return haproxyMapEntry

default:

haproxyMapEntry.Value = "0"

return haproxyMapEntry

}

Key: templateutil.GenerateRouteRegexp(cfg.Host, cfg.Path, cfg.IsWildcard),

Value: "0",

}

switch cfg.InsecurePolicy {

case routev1.InsecureEdgeTerminationPolicyRedirect:

haproxyMapEntry.Value = "1"

}

return haproxyMapEntry

… http-only route Let's say we have 2 routes like this: - HTTP-only route for "myapp.apps.example.com" domain and "/mypath" path - HTTPS-edge-redirect route for "myapp.apps.example.com" and no path In such situation, redirect is sent for any query to "myapp.apps.example.com", including "myapp.apps.example.com/mypath", so the more specific path route is not taking precedence as expected.

melvinjoseph86 · 2021-09-24T05:44:47Z

/retest

melvinjoseph86 · 2021-09-24T12:27:33Z

/retest-required

melvinjoseph86 · 2021-09-27T08:17:13Z

/retest-required

melvinjoseph86 · 2021-09-27T12:50:17Z

/lgtm
/label qe-approved

Verified via pre-merge verification workflow, more references related to the test can be found in:
https://bugzilla.redhat.com/show_bug.cgi?id=1896474#c15

miheer · 2021-09-27T16:15:51Z

@Miciah PTAL. Can you please approve ?

Miciah · 2021-09-27T18:48:11Z

Thanks!
/approve

openshift-ci · 2021-09-27T18:48:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: melvinjoseph86, Miciah, miheer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-09-27T18:50:03Z

@miheer: All pull requests linked via external trackers have merged:

openshift/router#343

Bugzilla bug 1896474 has been moved to the MODIFIED state.

In response to this:

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

melvinjoseph86 · 2021-09-29T12:33:38Z

/bugzilla refresh

openshift-ci · 2021-09-29T12:33:40Z

@melvinjoseph86: Bugzilla bug 1896474 is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

miheer · 2022-01-10T09:55:27Z

/cherry-pick release-4.9

openshift-cherrypick-robot · 2022-01-10T09:56:06Z

@miheer: new pull request created: #367

In response to this:

/cherry-pick release-4.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot requested review from alebedev87 and frobware September 9, 2021 06:44

miheer force-pushed the path-based-route-issue branch from b2be003 to 5292e24 Compare September 9, 2021 06:46

miheer changed the title ~~Bug 2002205 - HTTPS redirect happens even if there is a more specific http-only route~~ Bug 1896474 - HTTPS redirect happens even if there is a more specific http-only route Sep 9, 2021

miheer changed the title ~~Bug 1896474 - HTTPS redirect happens even if there is a more specific http-only route~~ Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route Sep 9, 2021

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 9, 2021

openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Sep 9, 2021

openshift-ci bot requested a review from quarterpin September 9, 2021 08:17

miheer force-pushed the path-based-route-issue branch 3 times, most recently from 422fbc2 to c6c965d Compare September 10, 2021 19:00

miheer force-pushed the path-based-route-issue branch from c6c965d to e2b1441 Compare September 10, 2021 20:27

Miciah reviewed Sep 11, 2021

View reviewed changes

miheer force-pushed the path-based-route-issue branch from e2b1441 to 4b91bf4 Compare September 13, 2021 07:05

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 15, 2021

openshift-ci bot assigned melvinjoseph86 Sep 15, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2021

miheer force-pushed the path-based-route-issue branch from 4b91bf4 to 3fcaab2 Compare September 20, 2021 12:06

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2021

Miciah reviewed Sep 20, 2021

View reviewed changes

miheer force-pushed the path-based-route-issue branch from 3fcaab2 to 019c5ac Compare September 23, 2021 09:13

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2021

openshift-merge-robot merged commit ddca0a6 into openshift:master Sep 27, 2021

openshift-cherrypick-robot mentioned this pull request Jan 10, 2022

[release-4.9] Bug 2010227: HTTPS redirect happens even if there is a more specific http-only route #367

Merged

Miciah mentioned this pull request Apr 11, 2022

Bug 2074304: generateRouteHostRegexp: Escape blanks #381

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route #343

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route #343

miheer commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 10, 2021

miheer commented Sep 11, 2021 •

edited

Loading

Miciah Sep 11, 2021

Miciah Sep 11, 2021

melvinjoseph86 commented Sep 15, 2021

openshift-ci bot commented Sep 15, 2021

melvinjoseph86 commented Sep 15, 2021

Miciah Sep 20, 2021

melvinjoseph86 commented Sep 24, 2021

melvinjoseph86 commented Sep 24, 2021

melvinjoseph86 commented Sep 27, 2021

melvinjoseph86 commented Sep 27, 2021

miheer commented Sep 27, 2021

Miciah commented Sep 27, 2021

openshift-ci bot commented Sep 27, 2021

openshift-ci bot commented Sep 27, 2021

melvinjoseph86 commented Sep 29, 2021

openshift-ci bot commented Sep 29, 2021

miheer commented Jan 10, 2022

openshift-cherrypick-robot commented Jan 10, 2022

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route #343

Bug 1896474: HTTPS redirect happens even if there is a more specific http-only route #343

Conversation

miheer commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 9, 2021

openshift-ci bot commented Sep 9, 2021

miheer commented Sep 10, 2021

miheer commented Sep 11, 2021 • edited Loading

Miciah Sep 11, 2021

Choose a reason for hiding this comment

Miciah Sep 11, 2021

Choose a reason for hiding this comment

melvinjoseph86 commented Sep 15, 2021

openshift-ci bot commented Sep 15, 2021

melvinjoseph86 commented Sep 15, 2021

Miciah Sep 20, 2021

Choose a reason for hiding this comment

melvinjoseph86 commented Sep 24, 2021

melvinjoseph86 commented Sep 24, 2021

melvinjoseph86 commented Sep 27, 2021

melvinjoseph86 commented Sep 27, 2021

miheer commented Sep 27, 2021

Miciah commented Sep 27, 2021

openshift-ci bot commented Sep 27, 2021

openshift-ci bot commented Sep 27, 2021

melvinjoseph86 commented Sep 29, 2021

openshift-ci bot commented Sep 29, 2021

miheer commented Jan 10, 2022

openshift-cherrypick-robot commented Jan 10, 2022

miheer commented Sep 11, 2021 •

edited

Loading