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

core/config: add support for stripping the port for matching routes #5085

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

calebdoxsey
Copy link
Contributor

@calebdoxsey calebdoxsey commented Apr 24, 2024

Summary

Change the way route matching works so that if a route is defined without an explicit port that route will match a request on any port.

The current behavior is that a route defined without an explicit port will only match a request with no port, or port :443.

For example, with the current behavior a route like:

from: https://www.example.com

Would match incoming requests for https://www.example.com or https://www.example.com:443, but not https://www.example.com:8443.

With the new behavior all 3 routes would match.

This new behavior can be disabled via a runtime flag (match_any_incoming_port ), but the default is enabled since we believe its unlikely to break existing configurations.

TCP routes should behave as they do currently. The port is always required for them and must match explicitly.

Related issues

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@calebdoxsey calebdoxsey added the enhancement New feature or request label Apr 24, 2024
@calebdoxsey calebdoxsey requested a review from a team as a code owner April 24, 2024 22:43
@coveralls
Copy link

coveralls commented Apr 24, 2024

Coverage Status

coverage: 57.265% (+0.06%) from 57.207%
when pulling e5d9e5e on cdoxsey/strip-port
into 05e077f on main.

Copy link
Contributor

@kenjenkins kenjenkins left a comment

Choose a reason for hiding this comment

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

Thanks! I think this makes sense.

config/runtime_flags.go Outdated Show resolved Hide resolved
config/from.go Outdated Show resolved Hide resolved
"from-url: %s\nrequest-url: %s", tc.pattern, tc.input)
}
}

func TestWildcardToRegex(t *testing.T) {
t.Parallel()

re, err := regexp.Compile(WildcardToRegex("*.internal.*.example.com"))
re, err := regexp.Compile(WildcardToRegex("*.internal.*.example.com", true))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding a unit test exercise the WildcardToRegex(..., false) code path too?

@calebdoxsey calebdoxsey merged commit 5373e25 into main Apr 26, 2024
16 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/strip-port branch April 26, 2024 14:24
@calebdoxsey calebdoxsey added the docs Docs update required label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Docs update required enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants