Skip to content

Commit

Permalink
Merge pull request #3786 from Shopify/rewrite-x-forwarded-prefix
Browse files Browse the repository at this point in the history
Fix x-forwarded-prefix annotation
  • Loading branch information
k8s-ci-robot authored Mar 31, 2019
2 parents 662a4d0 + 6cbfd4c commit b87cc5a
Show file tree
Hide file tree
Showing 8 changed files with 121 additions and 22 deletions.
15 changes: 15 additions & 0 deletions cmd/plugin/lints/ingress.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,24 @@ func GetIngressLints() []IngressLint {
message: "Contains an annotation with the prefix 'nginx.com'. This is a prefix for https://github.com/nginxinc/kubernetes-ingress",
f: annotationPrefixIsNginxCom,
},
{
message: "The x-forwarded-prefix annotation value is a boolean instead of a string",
issue: 3786,
version: "0.24.0",
f: xForwardedPrefixIsBool,
},
}
}

func xForwardedPrefixIsBool(ing v1beta1.Ingress) bool {
for name, val := range ing.Annotations {
if strings.HasSuffix(name, "/x-forwarded-prefix") && (val == "true" || val == "false") {
return true
}
}
return false
}

func annotationPrefixIsNginxCom(ing v1beta1.Ingress) bool {
for name := range ing.Annotations {
if strings.HasPrefix(name, "nginx.com/") {
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/annotations/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ type Ingress struct {
LoadBalancing string
UpstreamVhost string
Whitelist ipwhitelist.SourceRange
XForwardedPrefix bool
XForwardedPrefix string
SSLCiphers string
Logs log.Config
LuaRestyWAF luarestywaf.Config
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/annotations/xforwardedprefix/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,5 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
// Parse parses the annotations contained in the ingress rule
// used to add an x-forwarded-prefix header to the request
func (cbbs xforwardedprefix) Parse(ing *extensions.Ingress) (interface{}, error) {
return parser.GetBoolAnnotation("x-forwarded-prefix", ing)
return parser.GetStringAnnotation("x-forwarded-prefix", ing)
}
12 changes: 6 additions & 6 deletions internal/ingress/annotations/xforwardedprefix/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ func TestParse(t *testing.T) {

testCases := []struct {
annotations map[string]string
expected bool
expected string
}{
{map[string]string{annotation: "true"}, true},
{map[string]string{annotation: "1"}, true},
{map[string]string{annotation: ""}, false},
{map[string]string{}, false},
{nil, false},
{map[string]string{annotation: "true"}, "true"},
{map[string]string{annotation: "1"}, "1"},
{map[string]string{annotation: ""}, ""},
{map[string]string{}, ""},
{nil, ""},
}

ing := &extensions.Ingress{
Expand Down
4 changes: 2 additions & 2 deletions internal/ingress/controller/template/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -503,8 +503,8 @@ func buildProxyPass(host string, b interface{}, loc interface{}) string {
if len(location.Rewrite.Target) > 0 {
var xForwardedPrefix string

if location.XForwardedPrefix {
xForwardedPrefix = fmt.Sprintf("proxy_set_header X-Forwarded-Prefix \"%s\";\n", path)
if len(location.XForwardedPrefix) > 0 {
xForwardedPrefix = fmt.Sprintf("proxy_set_header X-Forwarded-Prefix \"%s\";\n", location.XForwardedPrefix)
}

return fmt.Sprintf(`
Expand Down
22 changes: 11 additions & 11 deletions internal/ingress/controller/template/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ var (
Location string
ProxyPass string
Sticky bool
XForwardedPrefix bool
XForwardedPrefix string
SecureBackend bool
enforceRegex bool
}{
Expand All @@ -58,7 +58,7 @@ var (
"/",
"proxy_pass https://upstream_balancer;",
false,
false,
"",
true,
false,
},
Expand All @@ -68,7 +68,7 @@ var (
"/",
"proxy_pass https://upstream_balancer;",
false,
false,
"",
true,
false,
},
Expand All @@ -78,7 +78,7 @@ var (
"/",
"proxy_pass https://upstream_balancer;",
true,
false,
"",
true,
false,
},
Expand All @@ -88,7 +88,7 @@ var (
"/",
"proxy_pass http://upstream_balancer;",
false,
false,
"",
false,
false,
},
Expand All @@ -98,7 +98,7 @@ var (
"/",
"proxy_pass http://upstream_balancer;",
false,
false,
"",
false,
false,
},
Expand All @@ -110,7 +110,7 @@ var (
rewrite "(?i)/" /jenkins break;
proxy_pass http://upstream_balancer;`,
false,
false,
"",
false,
true,
},
Expand All @@ -122,7 +122,7 @@ proxy_pass http://upstream_balancer;`,
rewrite "(?i)/" /something break;
proxy_pass http://upstream_balancer;`,
true,
false,
"",
false,
true,
},
Expand All @@ -134,7 +134,7 @@ proxy_pass http://upstream_balancer;`,
rewrite "(?i)/" /something break;
proxy_pass http://upstream_balancer;`,
true,
false,
"",
false,
true,
},
Expand All @@ -147,7 +147,7 @@ rewrite "(?i)/there" /something break;
proxy_set_header X-Forwarded-Prefix "/there";
proxy_pass http://upstream_balancer;`,
true,
true,
"/there",
false,
true,
},
Expand All @@ -157,7 +157,7 @@ proxy_pass http://upstream_balancer;`,
`~* "^/something"`,
"proxy_pass http://upstream_balancer;",
false,
false,
"",
false,
true,
},
Expand Down
2 changes: 1 addition & 1 deletion internal/ingress/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ type Location struct {
// XForwardedPrefix allows to add a header X-Forwarded-Prefix to the request with the
// original location.
// +optional
XForwardedPrefix bool `json:"xForwardedPrefix,omitempty"`
XForwardedPrefix string `json:"xForwardedPrefix,omitempty"`
// Logs allows to enable or disable the nginx logs
// By default access logs are enabled and rewrite logs are disabled
Logs log.Config `json:"logs,omitempty"`
Expand Down
84 changes: 84 additions & 0 deletions test/e2e/annotations/xforwardedprefix.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
/*
Copyright 2019 The Kubernetes Authors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package annotations

import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/parnurzeal/gorequest"
"k8s.io/ingress-nginx/test/e2e/framework"
"net/http"
)

var _ = framework.IngressNginxDescribe("Annotations - X-Forwarded-Prefix", func() {
f := framework.NewDefaultFramework("xforwardedprefix")

BeforeEach(func() {
f.NewEchoDeployment()
})

AfterEach(func() {
})

It("should set the X-Forwarded-Prefix to the annotation value", func() {
host := "xfp.baz.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/x-forwarded-prefix": "/test/value",
"nginx.ingress.kubernetes.io/rewrite-target": "/foo",
}

f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, &annotations))
f.WaitForNginxServer(host,
func(server string) bool {
return Expect(server).Should(ContainSubstring("proxy_set_header X-Forwarded-Prefix \"/test/value\";"))
})

uri := "/"
resp, body, errs := gorequest.New().
Get(f.GetURL(framework.HTTP)+uri).
Set("Host", host).
End()

Expect(errs).Should(BeEmpty())
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(body).To(ContainSubstring("x-forwarded-prefix=/test/value"))
})

It("should not add X-Forwarded-Prefix if the annotation value is empty", func() {
host := "noxfp.baz.com"
annotations := map[string]string{
"nginx.ingress.kubernetes.io/x-forwarded-prefix": "",
"nginx.ingress.kubernetes.io/rewrite-target": "/foo",
}

f.EnsureIngress(framework.NewSingleIngress(host, "/", host, f.Namespace, "http-svc", 80, &annotations))
f.WaitForNginxServer(host,
func(server string) bool {
return Expect(server).Should(And(ContainSubstring(host), Not(ContainSubstring("proxy_set_header X-Forwarded-Prefix"))))
})

uri := "/"
resp, body, errs := gorequest.New().
Get(f.GetURL(framework.HTTP)+uri).
Set("Host", host).
End()

Expect(errs).Should(BeEmpty())
Expect(resp.StatusCode).Should(Equal(http.StatusOK))
Expect(body).To(Not(ContainSubstring("x-forwarded-prefix")))
})
})

0 comments on commit b87cc5a

Please sign in to comment.