Added permanent-redirect-code #2834

Stono · 2018-07-23T13:48:09Z

Hey,
This PR addresses #2715 (I hope).
It makes the permanent redirect code 308 by default (as this is more aligned to a permanent redirect) but also gives the ability to override it.

<< Golang noob... so check with caution

Please could you do me a build so I can test it out?

Stono · 2018-07-23T13:48:51Z

/assign @aledbf

codecov-io · 2018-07-23T14:16:46Z

Codecov Report

Merging #2834 into master will increase coverage by 0.21%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #2834      +/-   ##
==========================================
+ Coverage   47.34%   47.56%   +0.21%     
==========================================
  Files          75       76       +1     
  Lines        5413     5489      +76     
==========================================
+ Hits         2563     2611      +48     
- Misses       2525     2540      +15     
- Partials      325      338      +13

Impacted Files	Coverage Δ
internal/ingress/annotations/redirect/redirect.go	`32.72% <66.66%> (ø)`
internal/ingress/controller/template/template.go	`66.25% <0%> (-0.28%)`	⬇️
internal/ingress/metric/collectors/socket.go	`79.81% <0%> (+0.09%)`	⬆️
internal/ingress/controller/nginx.go	`11.28% <0%> (+0.4%)`	⬆️
internal/ingress/metric/collectors/controller.go	`90.67% <0%> (+13.75%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 43aabfc...d19c717. Read the comment docs.

Stono · 2018-07-24T14:44:20Z

Works a treat, without the code annotation:

> Accept: */*
>
< HTTP/1.1 308 Permanent Redirect
< Date: Tue, 24 Jul 2018 14:41:10 GMT
< Content-Type: text/html
< Content-Length: 180
< Connection: keep-alive
< Location: https://www.google.com
<
<html>
<head><title>308 Permanent Redirect</title></head>
<body bgcolor="white">
<center><h1>308 Permanent Redirect</h1></center>
<hr><center>nginx</center>
</body>

With the code annotation:

>
< HTTP/1.1 301 Moved Permanently
< Date: Tue, 24 Jul 2018 14:44:03 GMT
< Content-Type: text/html
< Content-Length: 178
< Connection: keep-alive
< Location: https://www.google.com
<
<html>
<head><title>301 Moved Permanently</title></head>
<body bgcolor="white">
<center><h1>301 Moved Permanently</h1></center>
<hr><center>nginx</center>
</body>
</html>

Could we get this merged?
Any change on a 0.18.0 ETA?

aledbf · 2018-07-24T14:56:24Z

Any change on a 0.18.0 ETA?

at least 2 weeks

Edit: you can check the status here https://github.com/kubernetes/ingress-nginx/projects/25

antoineco

Looks good overall 👍

antoineco · 2018-07-24T15:31:04Z

docs/user-guide/nginx-configuration/annotations.md

@@ -366,6 +367,10 @@ To configure this setting globally for all Ingress rules, the `limit-rate-after`

 This annotation allows to return a permanent redirect instead of sending data to the upstream.  For example `nginx.ingress.kubernetes.io/permanent-redirect: https://www.google.com` would redirect everything to Google.

+### Permanent Redirect Code
+
+This annotation allows you to modify the status code used for permanent redirects.  For example `nginx.ingress.kubernetes.io/permanent-redirect-code: 301` would return your redirect with a 301.


Annotations can't be int, you have to use single quotes around numeric values.

antoineco · 2018-07-24T15:32:19Z

internal/ingress/annotations/redirect/redirect.go

@@ -73,14 +74,28 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) {
 		return nil, err
 	}

+	prc, err := parser.GetStringAnnotation("permanent-redirect-code", ing)


Maybe an int would be more suitable, no need to call Atoi then.

antoineco · 2018-07-24T15:36:43Z

internal/ingress/annotations/redirect/redirect.go

+	}
+
+	var prci = http.StatusPermanentRedirect
+	if prc != "" {


If you do use an int, the validation of the annotation could be something like

// this is set at the top of the file const defaultPermanentRedirectCode = http.StatusPermanentRedirect if prc < http.StatusMultipleChoices || prc > http.StatusPermanentRedirect { prc = defaultPermanentRedirectCode }

(depending on what HTTP codes you believe are appropriate).

The const just makes the code mode readable, but more importantly you don't need 2 variables anymore.

Stono · 2018-07-27T19:58:28Z

Hey @antoineco - I've pushed pushed your suggestions, however one thing that doesn't sit right with me is that i have renamed the annotation to http-redirect-code, as that makes sense, but really permanent-redirect should be renamed to http-redirect too, as having the type of redirect in that name infers the response code.

However, I didn't want to make such a breaking change without understanding what, if any strategy you have for depreciating annotations.

antoineco · 2018-07-28T01:13:26Z

internal/ingress/annotations/redirect/redirect.go

@@ -73,14 +73,25 @@ func (a redirect) Parse(ing *extensions.Ingress) (interface{}, error) {
 		return nil, err
 	}

+	const defaultPermanentRedirectCode = http.StatusPermanentRedirect


The default should remain http.StatusMovedPermanently for backward compatibility reasons.

I'd move this right above the definition of the Config struct, defaults should be clearly visible.

antoineco · 2018-07-28T01:33:09Z

@Stono thanks for addressing my comments. You're completely right about the naming, we should not put the name of the http code inside the annotation name if the code is configurable.

I'd discuss the renaming of existing annotations in a different PR though, because, as you said, this change would be breaking backward compatibility unless we keep supporting the old and new annotations side by side for a little while.

Stono · 2018-07-28T07:22:19Z

@antoineco that's done, if you're happy i'd like to get this merged to ensure it's in the next release as it's super critical for me.

antoineco

Looking good but we need a simple unit test for it before merging.

I'll add the corresponding e2e test as soon as you're done.

antoineco · 2018-07-28T07:30:11Z

internal/ingress/annotations/redirect/redirect.go

+		return nil, err
+	}
+
+	const defaultPermanentRedirectCode = http.StatusMovedPermanently


Please move the const declaration after the import block as suggested before, otherwise there is little value in declaring a default as a constant.

antoineco · 2018-07-28T07:31:33Z

docs/user-guide/nginx-configuration/annotations.md

@@ -43,6 +43,7 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
 |[nginx.ingress.kubernetes.io/limit-connections](#rate-limiting)|number|
 |[nginx.ingress.kubernetes.io/limit-rps](#rate-limiting)|number|
 |[nginx.ingress.kubernetes.io/permanent-redirect](#permanent-redirect)|string|
+|[nginx.ingress.kubernetes.io/permanent-redirect-code](#permanent-redirect-code)|number|


Outdated annotation name.

antoineco · 2018-07-28T07:41:07Z

Answer to deleted comment

Saddened to read that. Both my comments were already mentioned before, you could have addressed them right away, in a single round trip.

All features need a test before we can merge them, I'm kindly referring you to the contribution guidelines.

If you felt the original name was more aligned with the feature I would have been happy to let you convince me, reviews are a discussion. As a reviewer I do have to question why names do not follow the existing scheme, but I doesn't mean this can't be challenged.

antoineco · 2018-07-28T08:00:16Z

Answer to deleted comment

You said you're not familiar with Go so I tried to help and share precise directions and good practices. A bit unfair to come and blame the reviewers afterwards for introducing confusion, isn't it?

I didn't make you use any name, I asked you about your opinion. Re-read my answer above.

Again, please check the contribution guidelines and show more empathy next time. We all try to help and deliver good software, and we all do it purely on our free time.

antoineco · 2018-07-28T12:28:56Z

@Stono e2e tests are here: [PATCH] Add e2e test for redirect annotations

Tests assume the annotation is called permanent-redirect-code (not tested).

edit: I already spotted a bug without even running the test.

noRedirectPolicyFunc should return http.ErrUseLastResponse, not some nil error.

Stono · 2018-07-28T13:06:04Z

Hey,
Thanks. I'll add that later as I'm out at the moment. Are we using "permanent-redirect-code" for now then? It feels like it makes sense because of the undocumented temporal redirect and then things can be ratified in the other issue I raised.

antoineco · 2018-07-28T13:07:27Z

The choice is yours, both make sense to me according to what was discussed already.

Stono · 2018-07-28T18:47:18Z

I've moved back to permanent-redirect-code and the unit tests pass, I've applied your patch and pushed that too. However, I don't really understand the e2e tests, and don't follow your comment about http.ErrUseLastResponse either! It also doesn't help that I don't know how to actually run them either.

I've just added you as a collaborator on my fork rather than messing about with patches.

antoineco · 2018-07-29T10:23:47Z

/assign antoineco
/lgtm

E2E tests are still failing because 404 is always returned. I'll investigate later.

Stono · 2018-07-29T15:05:16Z

Thanks @antoineco

Minor refactoring of parser and unit tests

antoineco · 2018-07-29T20:58:28Z

Found it. It was a simple misuse of the gorequest library. The function that determines the HTTP method (Get(...)) has to come first, otherwise the custom headers (Set("Host", ...)) and other parameters get reset.

/lgtm

k8s-ci-robot · 2018-07-29T20:58:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, Stono

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

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

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 23, 2018

k8s-ci-robot assigned aledbf Jul 23, 2018

antoineco suggested changes Jul 24, 2018

View reviewed changes

antoineco suggested changes Jul 28, 2018

View reviewed changes

Stono mentioned this pull request Jul 28, 2018

Discuss rename of permanent-redirect and temporal-redirect annotation #2864

Closed

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 28, 2018

Add permanent-redirect-code annotation

0983255

k8s-ci-robot assigned antoineco Jul 29, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 29, 2018

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2018

Add e2e test for redirect annotations

39966f4

Minor refactoring of parser and unit tests

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2018

k8s-ci-robot merged commit 5e4137c into kubernetes:master Jul 29, 2018

superseb mentioned this pull request Oct 24, 2018

nginx.ingress.kubernetes.io/permanent-redirect-code annotation not working rancher/rancher#16260

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added permanent-redirect-code #2834

Added permanent-redirect-code #2834

Stono commented Jul 23, 2018

Stono commented Jul 23, 2018

codecov-io commented Jul 23, 2018 •

edited

Loading

Stono commented Jul 24, 2018

aledbf commented Jul 24, 2018 •

edited

Loading

antoineco left a comment

antoineco Jul 24, 2018

antoineco Jul 24, 2018

antoineco Jul 24, 2018 •

edited

Loading

Stono commented Jul 27, 2018 •

edited

Loading

antoineco Jul 28, 2018

antoineco Jul 28, 2018

antoineco commented Jul 28, 2018

Stono commented Jul 28, 2018

antoineco left a comment

antoineco Jul 28, 2018 •

edited

Loading

antoineco Jul 28, 2018

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

Stono commented Jul 28, 2018

antoineco commented Jul 28, 2018 •

edited

Loading

Stono commented Jul 28, 2018

antoineco commented Jul 29, 2018 •

edited

Loading

Stono commented Jul 29, 2018

antoineco commented Jul 29, 2018

k8s-ci-robot commented Jul 29, 2018

Added permanent-redirect-code #2834

Added permanent-redirect-code #2834

Conversation

Stono commented Jul 23, 2018

Stono commented Jul 23, 2018

codecov-io commented Jul 23, 2018 • edited Loading

Codecov Report

Stono commented Jul 24, 2018

aledbf commented Jul 24, 2018 • edited Loading

antoineco left a comment

Choose a reason for hiding this comment

antoineco Jul 24, 2018

Choose a reason for hiding this comment

antoineco Jul 24, 2018

Choose a reason for hiding this comment

antoineco Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Stono commented Jul 27, 2018 • edited Loading

antoineco Jul 28, 2018

Choose a reason for hiding this comment

antoineco Jul 28, 2018

Choose a reason for hiding this comment

antoineco commented Jul 28, 2018

Stono commented Jul 28, 2018

antoineco left a comment

Choose a reason for hiding this comment

antoineco Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

antoineco Jul 28, 2018

Choose a reason for hiding this comment

antoineco commented Jul 28, 2018 • edited Loading

antoineco commented Jul 28, 2018 • edited Loading

antoineco commented Jul 28, 2018 • edited Loading

Stono commented Jul 28, 2018

antoineco commented Jul 28, 2018 • edited Loading

Stono commented Jul 28, 2018

antoineco commented Jul 29, 2018 • edited Loading

Stono commented Jul 29, 2018

antoineco commented Jul 29, 2018

k8s-ci-robot commented Jul 29, 2018

codecov-io commented Jul 23, 2018 •

edited

Loading

aledbf commented Jul 24, 2018 •

edited

Loading

antoineco Jul 24, 2018 •

edited

Loading

Stono commented Jul 27, 2018 •

edited

Loading

antoineco Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 28, 2018 •

edited

Loading

antoineco commented Jul 29, 2018 •

edited

Loading