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

Simplified route config #24

Merged
merged 3 commits into from
Mar 25, 2020
Merged

Simplified route config #24

merged 3 commits into from
Mar 25, 2020

Conversation

Tratcher
Copy link
Member

#9 The existing routing rules system uses a single config entry named ‘rule’ and then uses a secondary parser to parse out entries for Host, Path, Methods, etc.. I've worked at simplifying this to use the normal config system with separate entries for the various components.

Before:

      "Routes": [
        {
          "RouteId": "backend1/route1",
          "BackendId": "backend1",
          "Rule": "Host('localhost') && Path('/{**catchall}') && Method(‘GET’, ‘POST’)"
       }
      ]

After:

       "Routes": [
         {
           "RouteId": "backend1/route1",
           "BackendId": "backend1",
           "Methods": [ "GET", "POST" ],
           "Host": "localhost",
           "Path": "/{**catchall}"
         }
       ]

Benefits:
• Slightly reduced complexity for the config
• Unified syntax
• Significant reduction in the code required to read and process config, everything uses the default config binder
• Removes a dependency on Superpower 2.3.0 that was being used to parse the custom format
• You don’t have to deal with invalid combinations like having duplicate entries for Host.
• Removes incomplete code paths like support for OR (“||”). That scenario can be supported by adding a second route.

Downsides:
• I haven’t found any yet

I have not added support yet for things that weren't already supported like headers, but have noted where they'd be added.

private static bool ValidateHostMatcher(HostMatcher hostMatcher, out string errorMessage)
{
if (!_hostNameRegex.IsMatch(hostMatcher.Host))
if (!_hostNameRegex.IsMatch(host))
Copy link
Member

@stephentoub stephentoub Mar 24, 2020

Choose a reason for hiding this comment

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

We should consider constructing this Regex with RegexOptions.Compiled. We wouldn't do that if this code is rarely used, if the perf of this doesn't matter, or if we expect lots of these _hostNameRegex instances and are worried about higher memory consumption.

(Though looking again, this is actually static, so number of these shouldn't be an issue. I was thrown off by the style difference from dotnet/runtime, where a s_ prefix would be used for static and _ is used for instance.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's come back to that. I have my doubts that this level of validation is even needed. Filed #25

@davidni
Copy link
Contributor

davidni commented Mar 24, 2020

@Tratcher I'd really prefer if all fields related to inoming request matching were grouped together. Have you considered putting them in a matcher or rule section within the route? Since routes can have additional settings such as rate limiting etc. things could get messy very fast.

@Tratcher
Copy link
Member Author

@Tratcher I'd really prefer if all fields related to inoming request matching were grouped together. Have you considered putting them in a matcher or rule section within the route? Since routes can have additional settings such as rate limiting etc. things could get messy very fast.

Like this?

       "Routes": [
         {
           "RouteId": "backend1/route1",
           "BackendId": "backend1",
           "Match": {
             "Methods": [ "GET", "POST" ],
             "Host": "localhost",
             "Path": "/{**catchall}"
           }
         }
       ]

Sure, that's easy. @halter73, this is also what you'd proposed on the mail thread?

@Tratcher Tratcher merged commit 8428ae4 into master Mar 25, 2020
@Tratcher Tratcher deleted the tratcher/config branch March 25, 2020 16:54
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.

4 participants