Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new case insensitive matcher #553

Merged
merged 4 commits into from
Oct 29, 2018
Merged

Conversation

herbrandson
Copy link
Contributor

This adds the ability to have case insensitive matching for routes. This is important for projects that need to migrate from other load balancers that are case insensitive.

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2018

CLA assistant check
All committers have signed the CLA.

route/matcher.go Outdated
@@ -12,6 +12,7 @@ type matcher func(uri string, r *Route) bool
var Matcher = map[string]matcher{
"prefix": prefixMatcher,
"glob": globMatcher,
"noCase": noCaseMatcher,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this might be a terrible name, but it's the best I could come up with this evening. I'm open to suggestions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe nocase

@herbrandson
Copy link
Contributor Author

I've never done GoLang before, but this seemed pretty straight forward based on the other examples. Let me know if I missed anything.

Copy link
Contributor

@magiconair magiconair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks simple enough. Lets drop the camel case and call it nocase. Also, please add a test in config/load_test.go, update fabio.properties and the documentation.

route/matcher.go Outdated
@@ -12,6 +12,7 @@ type matcher func(uri string, r *Route) bool
var Matcher = map[string]matcher{
"prefix": prefixMatcher,
"glob": globMatcher,
"noCase": noCaseMatcher,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe nocase

route/matcher.go Outdated
func noCaseMatcher(uri string, r *Route) bool {
lowerURI := strings.ToLower(uri)
lowerPath := strings.ToLower(r.Path)
return strings.HasPrefix(lowerURI, lowerPath)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at this again, I think this should be a case insensitive prefix match. Originally I was using EqualFold. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// todo(fs): if this turns out to be a performance issue we should cache
// todo(fs): strings.ToLower(r.Path) in r.PathLower
return strings.HasPrefix(strings.ToLower(uri), strings.ToLower(r.Path))

config/load.go Outdated
@@ -247,7 +247,7 @@ func load(cmdline, environ, envprefix []string, props *properties.Properties) (c
return nil, fmt.Errorf("invalid proxy.strategy: %s", cfg.Proxy.Strategy)
}

if cfg.Proxy.Matcher != "prefix" && cfg.Proxy.Matcher != "glob" && cfg.Proxy.Matcher != "noCase" {
if cfg.Proxy.Matcher != "prefix" && cfg.Proxy.Matcher != "glob" && cfg.Proxy.Matcher != "nocase" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With respect to the case-insensitive prefix matching I think iprefix is a better name.

fabio.properties Outdated
@@ -297,6 +297,7 @@
#
# prefix: prefix matching
# glob: glob matching
# nocase: matches using a case insensitive test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# iprefix: case-insensitive prefix matching

Copy link
Contributor

@magiconair magiconair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some more comments.

@herbrandson
Copy link
Contributor Author

I believe I've addressed all of your comments. I just wanted to make sure there wasn't something I missed that you're still waiting on. Are there any other changes you'd like to see on this?

@herbrandson
Copy link
Contributor Author

Any update on this? We really need this in order to migrate to fabio

@magiconair magiconair merged commit cba6849 into fabiolb:master Oct 29, 2018
@magiconair
Copy link
Contributor

Thank you and sorry that it took so long!

@magiconair magiconair added this to the 1.5.11 milestone Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants