Skip to content

Commit

Permalink
fix: ingress rule matching too much (#41)
Browse files Browse the repository at this point in the history 
PDOK-17273
  • Loading branch information
@roelarents
roelarents authored Nov 21, 2024
1 parent ca0edec commit 9ee216b
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 29 deletions.
4 changes: 2 additions & 2 deletions internal/controller/ogcapi_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,7 @@ func (r *OGCAPIReconciler) mutateIngressRoute(ogcAPI *pdoknlv1alpha1.OGCAPI, ing
"uptime.pdok.nl/url": uptimeURL,
"uptime.pdok.nl/tags": "public-stats,ogcapi",
}
matchRule, _ := createIngressRuleAndStripPrefixForURL(*ogcAPI.Spec.Service.BaseURL.URL, true, true, true)
matchRule, _ := createIngressRuleAndStripPrefixForBaseURL(*ogcAPI.Spec.Service.BaseURL.URL, true, true, true)
ingressRoute.Spec = traefikiov1alpha1.IngressRouteSpec{
Routes: []traefikiov1alpha1.Route{
{
Expand Down Expand Up @@ -509,7 +509,7 @@ func (r *OGCAPIReconciler) mutateStripPrefixMiddleware(ogcAPI *pdoknlv1alpha1.OG
if err := setImmutableLabels(r.Client, middleware, labels); err != nil {
return err
}
_, stripPrefixRegex := createIngressRuleAndStripPrefixForURL(*ogcAPI.Spec.Service.BaseURL.URL, true, true, true)
_, stripPrefixRegex := createIngressRuleAndStripPrefixForBaseURL(*ogcAPI.Spec.Service.BaseURL.URL, true, true, true)
middleware.Spec = traefikiov1alpha1.MiddlewareSpec{
StripPrefixRegex: &traefikdynamic.StripPrefixRegex{
Regex: []string{
Expand Down
39 changes: 24 additions & 15 deletions internal/controller/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ func strategicMergePatch[T, P any](obj *T, patch *P) (*T, error) {
return &newObj, nil
}

func createIngressRuleAndStripPrefixForURL(url url.URL, includeLocalhost, matchUnderscoreVersions, traefikV2 bool) (string, string) {
func createIngressRuleAndStripPrefixForBaseURL(url url.URL, includeLocalhost, matchUnderscoreVersions, traefikV2 bool) (string, string) {
var hostMatch string
if includeLocalhost {
hostMatch = fmt.Sprintf("(Host(`localhost`) || Host(`%s`))", url.Hostname())
Expand All @@ -128,33 +128,42 @@ func createIngressRuleAndStripPrefixForURL(url url.URL, includeLocalhost, matchU
}

path := url.EscapedPath()
trailingSlash := strings.HasSuffix(path, "/")
path = strings.Trim(path, "/")
if path == "" {
return hostMatch, ""
}

var pathMatch, stripPrefixRegexp string
var pathRegexp string
if matchUnderscoreVersions {
pathRegexp := createRegexpForUnderscoreVersions(path)
if traefikV2 {
// Traefik v2: embed a regex in Path by using {name: regex}
pathMatch = fmt.Sprintf("PathPrefix(`/{path:%s}`)", pathRegexp)
} else {
// Traefik v3: match all as a regex
pathMatch = fmt.Sprintf("PathRegexp(`^/%s`)", pathRegexp)
}
stripPrefixRegexp = fmt.Sprintf("^/%s", pathRegexp) //nolint:perfsprint
pathRegexp = createRegexpForUnderscoreVersions(path)
} else {
pathMatch = fmt.Sprintf("PathPrefix(`%s`)", path)
stripPrefixRegexp = fmt.Sprintf("^%s", regexp.QuoteMeta(path)) //nolint:perfsprint
pathRegexp = regexp.QuoteMeta(path)
}

trailingRegexp := "(/|$)" // to prevent matching too much after the last segment
if trailingSlash {
trailingRegexp = "/"
}

var pathMatch, stripPrefixRegexp string
if traefikV2 {
// Traefik v2: embed a regex in Path by using {name: regex}
pathMatch = fmt.Sprintf("PathPrefix(`/{path:%s%s}`)", pathRegexp, trailingRegexp)
} else {
// Traefik v3: match all as a regex
pathMatch = fmt.Sprintf("PathRegexp(`^/%s%s`)", pathRegexp, trailingRegexp)
}
stripPrefixRegexp = fmt.Sprintf("^/%s", pathRegexp) //nolint:perfsprint
if trailingSlash {
stripPrefixRegexp += "/"
}

matchRule := fmt.Sprintf("%s && %s", hostMatch, pathMatch)
return matchRule, stripPrefixRegexp
}

// (leading slash is stripped)
func createRegexpForUnderscoreVersions(path string) string {
path = strings.TrimLeft(path, "/")
// luckily Traefik also uses golang regular expressions syntax
// first create a regexp that literally matches the path
pathRegexp := regexp.QuoteMeta(path)
Expand Down
43 changes: 31 additions & 12 deletions internal/controller/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,24 +46,34 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
url: mustURLParse(t, "http://example.com/some/path"),
},
wants: wants{
rule: "Host(`example.com`) && PathPrefix(`/some/path`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path(/|$)`)",
prefix: "^/some/path"},
}, {
name: "v1, no replacing",
args: args{
url: mustURLParse(t, "http://example.com/some/path/v1"),
url: mustURLParse(t, "http://example.com/some/path/v1"),
matchUnderscoreVersions: false,
},
wants: wants{
rule: "Host(`example.com`) && PathPrefix(`/some/path/v1`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(/|$)`)",
prefix: "^/some/path/v1"},
}, {
name: "v1, no replacing, trailing slash",
args: args{
url: mustURLParse(t, "http://example.com/some/path/v1/"),
matchUnderscoreVersions: false,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1/`)",
prefix: "^/some/path/v1/"},
}, {
name: "v1",
args: args{
url: mustURLParse(t, "http://example.com/some/path/v1"),
matchUnderscoreVersions: true,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(_\\d+)?`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(_\\d+)?(/|$)`)",
prefix: "^/some/path/v1(_\\d+)?"},
}, {
name: "v1, for traefikV2",
Expand All @@ -73,7 +83,7 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
traefikV2: true,
},
wants: wants{
rule: "Host(`example.com`) && PathPrefix(`/{path:some/path/v1(_\\d+)?}`)",
rule: "Host(`example.com`) && PathPrefix(`/{path:some/path/v1(_\\d+)?(/|$)}`)",
prefix: "^/some/path/v1(_\\d+)?"},
}, {
name: "v2, followed by other segment",
Expand All @@ -82,7 +92,7 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
matchUnderscoreVersions: true,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v2(_\\d+)?/foo`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v2(_\\d+)?/foo(/|$)`)",
prefix: "^/some/path/v2(_\\d+)?/foo"},
}, {
name: "v2, followed by other segment, for traefikV2",
Expand All @@ -92,7 +102,7 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
traefikV2: true,
},
wants: wants{
rule: "Host(`example.com`) && PathPrefix(`/{path:some/path/v2(_\\d+)?/foo}`)",
rule: "Host(`example.com`) && PathPrefix(`/{path:some/path/v2(_\\d+)?/foo(/|$)}`)",
prefix: "^/some/path/v2(_\\d+)?/foo"},
}, {
name: "v1_3",
Expand All @@ -101,7 +111,7 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
matchUnderscoreVersions: true,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(_3)?`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(_3)?(/|$)`)",
prefix: "^/some/path/v1(_3)?"},
}, {
name: "combined (never happens)",
Expand All @@ -110,17 +120,26 @@ func Test_createIngressRuleAndStripPrefixForURL(t *testing.T) {
matchUnderscoreVersions: true,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v345(_\\d+)?/v666(_78)?`)",
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v345(_\\d+)?/v666(_78)?(/|$)`)",
prefix: "^/some/path/v345(_\\d+)?/v666(_78)?"},
}, {
name: "trailing slash in base url",
args: args{
url: mustURLParse(t, "http://example.com/some/path/v1/"),
matchUnderscoreVersions: true,
},
wants: wants{
rule: "Host(`example.com`) && PathRegexp(`^/some/path/v1(_\\d+)?/`)",
prefix: "^/some/path/v1(_\\d+)?/"},
}}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
rule, prefix := createIngressRuleAndStripPrefixForURL(tt.args.url, tt.args.includelocalhost, tt.args.matchUnderscoreVersions, tt.args.traefikV2)
rule, prefix := createIngressRuleAndStripPrefixForBaseURL(tt.args.url, tt.args.includelocalhost, tt.args.matchUnderscoreVersions, tt.args.traefikV2)
if rule != tt.wants.rule {
t.Errorf("createIngressRuleAndStripPrefixForURL() = `%v`, _, want rule `%v`", rule, tt.wants.rule)
t.Errorf("createIngressRuleAndStripPrefixForBaseURL() = `%v`, _,\nwant rule `%v`", rule, tt.wants.rule)
}
if prefix != tt.wants.prefix {
t.Errorf("createIngressRuleAndStripPrefixForURL() = _, `%v`, want prefix `%v`", prefix, tt.wants.prefix)
t.Errorf("createIngressRuleAndStripPrefixForBaseURL() = _, `%v`,\nwant prefix `%v`", prefix, tt.wants.prefix)
}
})
}
Expand Down

0 comments on commit 9ee216b

Please sign in to comment.