Implement support for HTTPS Redirects #1206

spencerhance · 2020-08-05T09:01:53Z

Adds support for using HTTPS Redirects with L7 ELB via a Redirects Config in FrontendConfig
Also includes a new status annotation for the additional UrlMap that needs to be synced

e2e tests will be added in a follow up

k8s-ci-robot · 2020-08-05T09:02:00Z

Hi @spencerhance. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

rramkumar1 · 2020-08-05T10:52:06Z

/ok-to-test

rramkumar1

LGTM, would like one more pair of eyes on it though.

spencerhance · 2020-08-05T22:58:01Z

@freehan can you also take a look?

/assign @freehan

freehan

How about only supporting GC for the v2 naming scheme.

One more thing, there is one corner case: If someone enabled the HTTP redirect and disable later. Will the RedirectURLMap get trimmed? From the code, I do not think so, right?

Maybe handle this as part of the ensureRedirectUrlMap code?

freehan · 2020-08-05T23:52:06Z

pkg/utils/namer/frontendnamer.go

@@ -88,6 +90,11 @@ func (ln *V1IngressFrontendNamer) UrlMap() string {
 	return ln.namer.UrlMap(ln.lbName)
 }

+// UrlMap implements IngressFrontendNamer.
+func (ln *V1IngressFrontendNamer) RedirectUrlMap() string {
+	return ln.namer.RedirectUrlMap(ln.lbName)


I strongly recommend NOT to support this feature in V1 Namer.

Return an error event. Asking user to recreate ingress and it will come with new naming scheme.

freehan · 2020-08-05T23:55:28Z

pkg/utils/namer/interfaces.go

@@ -25,6 +25,8 @@ type IngressFrontendNamer interface {
 	TargetProxy(protocol NamerProtocol) string
 	// UrlMap returns the name of the URL Map.
 	UrlMap() string
+	// RedirectUrlMap returns the name of the URL Map.
+	RedirectUrlMap() string


// RedirectUrlMap returns the name of the URL Map and if the namer supports naming redirectUrlMap RedirectUrlMap() (string, bool)

spencerhance · 2020-08-06T04:51:57Z

@freehan

One more thing, there is one corner case: If someone enabled the HTTP redirect and disable later. Will the RedirectURLMap get trimmed? From the code, I do not think so, right?

Good point, I added in code to delete it when disabled

freehan · 2020-08-06T21:20:34Z

pkg/loadbalancers/url_maps.go

+		return err
+	}
+
+	currentMap, err := composite.GetUrlMap(l.cloud, key, l.Versions().UrlMap)


Might worth avoiding this GET call every time an irrelevant ingress was synced.

One idea is to check if the the ingress status annotation has the redirectUrlMap

freehan · 2020-08-06T21:36:32Z

pkg/loadbalancers/url_maps.go

+		return err
+	}
+
+	if expectedMap == nil {


add a comment here to say "do not expect to have a RedirectUrlMap"

freehan · 2020-08-06T21:36:46Z

pkg/loadbalancers/url_maps.go

+				return err
+			}
+		}
+	} else {


probably no need for the else block

Adds support for using HTTPS Redirects via a Redirects Config in FrontendConfig Also includes a new status annotation for the additional UrlMap that needs to be synced

freehan

/lgtm
/approve

k8s-ci-robot · 2020-08-06T22:57:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, spencerhance

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~~ [freehan]

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

coderaven · 2020-08-09T09:00:25Z

Good to know this has been merged finally! I'm new to this, when is the expected timeline for this to be available in GKE? And is there any documentation that comes along with it?

peterldowns · 2020-09-02T18:37:05Z

Yes, would really love to use this, when will it be available in GKE?

artvichi · 2020-10-22T02:16:11Z

Is this going to be live at some point in this century ?

spencerhance · 2020-10-22T07:57:11Z

Hi folks, please follow the discussion at #1075

We are doing everything we can to launch this ASAP

rhzs · 2020-10-28T12:13:35Z

pkg/loadbalancers/target_proxies.go

-	tr := translator.NewTranslator(isL7ILB, l.namer)
+	// Get UrlMap Name, could be the url map or the redirect url map
+	// TODO(shance): move to translator
+	var umName string


Why didn't this code written as follow with less branches?

umName := l.um.Name if flags.F.EnableFrontendConfig { // TODO(shance): check for empty name? if l.redirectUm != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps != nil && l.runtimeInfo.FrontendConfig.Spec.RedirectToHttps.Enabled { umName = l.redirectUm.Name } }

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 5, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2020

k8s-ci-robot requested review from freehan and rramkumar1 August 5, 2020 09:02

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 5, 2020

rramkumar1 reviewed Aug 5, 2020

View reviewed changes

geun mentioned this pull request Aug 5, 2020

HTTP to HTTPS redirection #1075

Closed

k8s-ci-robot assigned freehan Aug 5, 2020

freehan reviewed Aug 5, 2020

View reviewed changes

spencerhance force-pushed the https-redirects-demo branch from 58914c0 to a2f591d Compare August 6, 2020 04:50

spencerhance force-pushed the https-redirects-demo branch from a2f591d to ad3c93e Compare August 6, 2020 05:40

freehan reviewed Aug 6, 2020

View reviewed changes

Implements support for HTTPS Redirects for L7 ELB

da73204

Adds support for using HTTPS Redirects via a Redirects Config in FrontendConfig Also includes a new status annotation for the additional UrlMap that needs to be synced

spencerhance force-pushed the https-redirects-demo branch from ad3c93e to da73204 Compare August 6, 2020 22:47

freehan reviewed Aug 6, 2020

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 6, 2020

k8s-ci-robot merged commit 97f0158 into kubernetes:master Aug 6, 2020

spencerhance deleted the https-redirects-demo branch August 6, 2020 23:26

rhzs reviewed Oct 28, 2020

View reviewed changes

irbekrm mentioned this pull request Jan 29, 2021

Ingress that set ".../ingress.allow-http: false" hang forever with no messages cert-manager/cert-manager#3549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for HTTPS Redirects #1206

Implement support for HTTPS Redirects #1206

spencerhance commented Aug 5, 2020 •

edited

Loading

k8s-ci-robot commented Aug 5, 2020

rramkumar1 commented Aug 5, 2020

rramkumar1 left a comment

spencerhance commented Aug 5, 2020

freehan left a comment •

edited

Loading

freehan Aug 5, 2020

freehan Aug 5, 2020

spencerhance Aug 6, 2020

freehan Aug 5, 2020

spencerhance Aug 6, 2020

spencerhance commented Aug 6, 2020

freehan Aug 6, 2020

spencerhance Aug 6, 2020

freehan Aug 6, 2020

spencerhance Aug 6, 2020

freehan Aug 6, 2020

spencerhance Aug 6, 2020

freehan left a comment

k8s-ci-robot commented Aug 6, 2020

coderaven commented Aug 9, 2020

peterldowns commented Sep 2, 2020

artvichi commented Oct 22, 2020

spencerhance commented Oct 22, 2020

rhzs Oct 28, 2020

Implement support for HTTPS Redirects #1206

Implement support for HTTPS Redirects #1206

Conversation

spencerhance commented Aug 5, 2020 • edited Loading

k8s-ci-robot commented Aug 5, 2020

rramkumar1 commented Aug 5, 2020

rramkumar1 left a comment

Choose a reason for hiding this comment

spencerhance commented Aug 5, 2020

freehan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spencerhance commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 6, 2020

coderaven commented Aug 9, 2020

peterldowns commented Sep 2, 2020

artvichi commented Oct 22, 2020

spencerhance commented Oct 22, 2020

Choose a reason for hiding this comment

spencerhance commented Aug 5, 2020 •

edited

Loading

freehan left a comment •

edited

Loading