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

Refactor urlprefix tags #111

Closed
magiconair opened this issue Jun 14, 2016 · 7 comments
Closed

Refactor urlprefix tags #111

magiconair opened this issue Jun 14, 2016 · 7 comments
Milestone

Comments

@magiconair
Copy link
Contributor

magiconair commented Jun 14, 2016

Related open issues: #1, #42, #44, #56, #75, #80, #105, #110, #129, #164
Related closed issues: #35, #45, #53, #87, #90, #93, #107

fabio currently uses the urlprefix- tag prefix to identify routes. The format is limited to specifying a host/path prefix which makes it difficult to allow optional features for a given route. Issue #42 mentions an alternative syntax which provides more options but is still primarily focussed on prefixes.

consul issue 1107 mentions a revamp of the API to provide generic KV support for services but doesn't provide a timeline. However, one suggestion is something like RFC 1464 which is used to store arbitrary k/v data in DNS TXT records.

fabio could use this as follows which would provide a generic key/value config mechanism which can also be moved to something else later. Note that RFC 1464 uses the backtick for escaping the =, and the backtick itself.

This would allow a generic mechanism to add options to routes and protocols other than HTTP like TCP to be configured properly.

fabio key=value key=value key=value

e.g.
fabio uri=host/path
fabio uri=host/path proto=http,https
fabio uri=host/path log=true cors=true strip=/path
fabio addr=1.2.3.4:6789 proto=tcp

Several design questions come to mind:

  • Should multiple prefixes be specified as a multiple uri attributes in a single tag or as multiple tags?

The challenge here would be a length restriction in the tags but we could allow both multiple tags and multiple uri attributes to work around this.

  • Should the protocol be specified separately or as part of the URL?

As part of the URL has the benefit of being familiar but will require duplication if a path should be exposed via HTTP and HTTPS but not WS. Therefore, uri=host/path proto=http,https might be more compact.

Another approach would be to use JSON but the quoting makes this more difficult.

@far-blue
Copy link

far-blue commented Aug 19, 2016

Each service in Consul is a single ip/port combination so this is really a 1-2-many mapping problem. This one ip/port could be routed differently (in theory) for http, https, wss and tcp. Clearly not all combinations of these make sense but you get the idea.

So I suggest 1 tag per protocol with each tag starting with the protocol and then providing the protocol-specific details. For http this would be the hostname/path to be routed, for tcp I assume it would just be the listening port, then followed by generic details such as weighting and logging etc.

So you'd have something like:

fabio-http foo.com/wibble
fabio-wss foo.com/api strip=/path
fabio-tcp 6789 log=true

you could also support mixed protocols such as:
fabio-http-https foo.com/wibble

If tags can't contain spaces then still best to avoid commas because of how Registrator works. Maybe use something like:
fabio-http=foo.com/wibble;log=true

@far-blue
Copy link

you could use fabio-proxy to refer to http & https & wss all at once.

@magiconair
Copy link
Contributor Author

Another approach is to leave the urlprefix-host/path unchanged and allow for urlprefix-host/path key=val key=val ... This would provide the same flexibility will full backwards compatibility.

@MilesBehind
Copy link

MilesBehind commented Oct 30, 2016

Fantastic work on the project @magiconair!
When do you think, this key=val key=val may be implemented? Since spring-cloud-consul supports only this configuration in an application.yml file:

spring:
  cloud:
    consul:
      discovery:
        tags: foo=bar, baz=foobar

it would be nice if these key=val key=val expressions would be supported.

@ghost ghost mentioned this issue Nov 15, 2016
@magiconair
Copy link
Contributor Author

OK, here is what I am going to do:

Step 1

Add support for key=val options to the existing urlprefix- tags. This keeps them backwards compatible and won't break any existing setups. The first option after the tag prefix is the route target. This should also work for a TCP or UDP proxy although it reads a bit awkward.

urlprefix-host/path
urlprefix-host/path proto=https strip=/path
urlprefix-1.2.3.4:5555 proto=tcp

Step 2

Add support for multiple tag prefixes to allow migration from urlprefix- to something more generic like fabio and add support for a route= option as an alias for the route target so that the previous example reads as:

fabio route=host/path
fabio route=host/path proto=https strip=/path
fabio route=1.2.3.4:5555 proto=tcp

which is equivalent to

fabio host/path
fabio host/path proto=https strip=/path
fabio 1.2.3.4:5555 proto=tcp

which is equivalent to

urlprefix-host/path
urlprefix-host/path proto=https strip=/path
urlprefix-1.2.3.4:5555 proto=tcp

Maybe step 2 won't be needed but I'd like to add step 1 ASAP to support additional use cases without breaking existing setups.

magiconair added a commit that referenced this issue Dec 6, 2016
Add support for generic 'key=val key=val ...' options after an
'urlprefix-' tag. The parser still needs to be updated to recognize
them. This will happen in the next commit.
magiconair added a commit that referenced this issue Dec 6, 2016
Add a new route parser which is better structured and less
strict on whitespace within commands. It supports the route
options which have been added in the previous commit.

The old parser is still around but disabled in case I have
to switch back quickly. It ignores the options and will be
removed in a later commit.
magiconair added a commit that referenced this issue Dec 6, 2016
Use the RouteDef struct instead of individual arguments
which have the same type.
magiconair added a commit that referenced this issue Dec 6, 2016
Add support for generic 'key=val key=val ...' options after an
'urlprefix-' tag. The parser still needs to be updated to recognize
them. This will happen in the next commit.
magiconair added a commit that referenced this issue Dec 6, 2016
Add a new route parser which is better structured and less
strict on whitespace within commands. It supports the route
options which have been added in the previous commit.

The old parser is still around but disabled in case I have
to switch back quickly. It ignores the options and will be
removed in a later commit.
magiconair added a commit that referenced this issue Dec 6, 2016
Use the RouteDef struct instead of individual arguments
which have the same type.
@magiconair
Copy link
Contributor Author

Merged new route parser to master

@magiconair magiconair added this to the 1.4 milestone Dec 6, 2016
magiconair added a commit that referenced this issue Dec 8, 2016
The TestParse() function was not testing the old and the
new parser but only the new parser. Skipped tests are now
also properly reported.
magiconair added a commit that referenced this issue Dec 8, 2016
Move the regexp out of the matching functions so that
they are compiled only once and during startup and
not at runtime.
magiconair added a commit that referenced this issue Dec 8, 2016
magiconair added a commit that referenced this issue Dec 8, 2016
Merge the TableWeight and TableRoute test since they
were doing similar things but in different ways. Use
only text routing tables instead of internal structs.
magiconair added a commit that referenced this issue Dec 8, 2016
magiconair added a commit that referenced this issue Dec 8, 2016
Merge the TableWeight and TableRoute test since they
were doing similar things but in different ways. Use
only text routing tables instead of internal structs.
@magiconair magiconair modified the milestones: 1.4, 1.3.6 Mar 13, 2017
@magiconair
Copy link
Contributor Author

The new route parser is in use. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants