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

Add raw tags #455

Open
magiconair opened this issue Feb 27, 2018 · 13 comments · May be fixed by #454
Open

Add raw tags #455

magiconair opened this issue Feb 27, 2018 · 13 comments · May be fixed by #454

Comments

@magiconair
Copy link
Contributor

Fabio currently uses specially crafted Consul tags (urlprefix-host/path opt opt ...) to generate route commands which build the fabio routing table. There is a certain impedance mismatch between the syntax of the urlprefix- tags and the route add commands they generate, e.g.

urlprefix-/
route add <svcname> / http://<svcaddr>/

urlprefix-/ strip=/foo
route add <svcname> / http://<svcaddr>/ opts "strip=/foo"

urlprefix-:1234 proto=tcp
route add <svcname> :1234 tcp://<svcaddr>/

urlprefix-/ proto=https
route add <svcname> / https://<svcaddr>/

It would be useful if the urlprefix- tags are just a convenient shorthand notation for common more complex route commands but users could choose to express the same thing as a route command as well. This would limit the syntax of the urlprefix- commands to what is necessary for the most common uses cases.

For this to work, the code would need to distinguish between two types of tags: short and raw

raw tags are added verbatim (without the tag prefix) to the routing table whereas the short tags provide a special shorthand notation syntax which needs to be parsed and transformed.

Additionally, the raw tags need pseudo-variables to inject the service name and service address which are currently generated from the urlprefix- tags to be fully equivalent.

urlprefix-/ could then also be written as fabio route add $service / http://$addr/ assuming that fabio is the raw prefix tag.

The open questions are whether there should be a different prefix for the raw tags or whether this should be rolled into the urlprefix- syntax. Since the urlprefix- syntax is already used for defining TCP services I think we shouldn't overload it even further.

I'd like to gather some ideas on the syntax of a fabio tag for defining routes and allowing raw route commands and then we start with the route commands.

Note

There is an open issue/pr somewhere which has these ideas but I can't find it right now. Will update when I do.

Note2

Part of the discussion is in #87 (comment)

@magiconair magiconair linked a pull request Feb 27, 2018 that will close this issue
@ctlajoie
Copy link
Contributor

I think you would want to use a different prefix for the raw tags.

What is the real impetus for adding this capability? Are you intending for there to be new extensions to the route syntax that can't be expressed using the "short" urlprefix- style tags?

@aaronhurt
Copy link
Member

I could see this being useful as fabio gradually moves to supplant the major reasons people give for deploying another more traditional reverse proxy like nginx or haproxy before fabio. These primarily seem to revolve around header mangling which could be tricky to express with the short tags unless there it becomes yet another csv style option.

Take the following example for header manipulation with pseudo variables:

route add <svc> / http://<addr>/ opts "headers_front=x-forwarded-for=$remote,x-my-header=my static value,remove-some-header,append-some-header+=with this value,headers_back=remove-from-response-"

Versus something maybe more legible and dynamic...

route add <svc> / http://<addr> opts fe.x-forwarded-for=$remote fe.x-my-header="my static value" fe.remove-some-header- fe.append-some-header+="with this value" be.remove-from-response-

Maybe not the best example?

@magiconair
Copy link
Contributor Author

@ctlajoie My main motivation is that there are two ways of expressing the same thing and there is a slight difference in syntax. Also, urlprefix- doesn't sit well with tcp services. Tech debt from the early days :)

Maybe it is just me wanting to clean up but in the process I make things more complex. What do you think?

@ctlajoie
Copy link
Contributor

ctlajoie commented Mar 1, 2018

You can think about the difference between the two as being declarative vs imperative.

If I add a tag urlprefix-example.com/ to a service, I am declaring that I want requests for example.com to be routed to this service.
A tag like fabio route add $service / http://$addr/ is imperative. You are issuing a command to fabio, as opposed to just declaring "route requests here".

Allowing route commands like that in tags also implies other things are also possible to do with the tag, such as fabio route del someothersvc. Doing so would be stupid, but you know someone will try it.

IMO it makes more sense to use a declarative syntax for service tags.

@ctlajoie
Copy link
Contributor

ctlajoie commented Mar 2, 2018

I am probably biased because I have never needed to manually add routes using the route add syntax. I consider the urlprefix- tags to be the "normal" method of adding routes.

@holtwilkins
Copy link
Contributor

So we've starting using route add syntax for lots of stuff right now, but we shove it into the fabio.config in consul. I agreee that it doesn't make sense to support route add in a tag for the declarative vs imperative split.

I guess I'm back to my original request of just wanting a catch-all https-redirector, and I'd prefer putting route add command in fabio.config to putting it in a job's tags - it applies to everything anyway, not just a single job, and definitely "feels" right to put it in fabio.config.

@holtwilkins
Copy link
Contributor

Hey @magiconair had a chance to think any more about a global https redirector?

@aaronhurt
Copy link
Member

@holtwilkins you can already do the "catch all" HTTP->HTTPS redirect. We're doing that today with the following route:

route add http-redirect host.domain.tld:80 https://host.domain.tld$path opts "redirect=301"

@aaronhurt
Copy link
Member

aaronhurt commented Jul 2, 2018

To get exactly what you are wanting we could probably add another pseudo-variable for $host that could be used similarly to the existing $path variable. Then you could basically add a route for :80 that would target https://$host$path with your redirect code specified in the options.

@holtwilkins
Copy link
Contributor

Yup @leprechau that would be great.

@aaronhurt
Copy link
Member

aaronhurt commented Jul 2, 2018

So, after actually looking at the code I think we could do this without introducing another variable. I think we could just add a single code block to the end fo the BuildRedirectURL function in route/target.go that would use the request host as the redirect host if the redirect host is empty. This would allow you to specify something like...

route add global-redirect :80 https://$path opts "redirect=301"

Does that look reasonable? Should we add the pseudo-variable just to make it more explicit?

@aaronhurt
Copy link
Member

@holtwilkins would you care to open a new issue around this specifically?

@holtwilkins
Copy link
Contributor

Hey @leprechau I'm happy too, but not sure how to word it. It seems like the old PR at #451 was meant to do something similar to this but was seemingly forgotten about? Should I open an issue calling out host-less redirects?

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 a pull request may close this issue.

4 participants