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

v2: Support SRV DNS discovery for proxy upstreams #3179

Closed
danlsgiga opened this issue Mar 23, 2020 · 14 comments · Fixed by #3180
Closed

v2: Support SRV DNS discovery for proxy upstreams #3179

danlsgiga opened this issue Mar 23, 2020 · 14 comments · Fixed by #3180
Labels
feature ⚙️ New feature or request

Comments

@danlsgiga
Copy link
Contributor

This is to replicate the same feature existing in v1 here: https://caddyserver.com/v1/docs/proxy

@danlsgiga
Copy link
Contributor Author

@mholt, I created this issue to track the work we initiated on Twitter

https://twitter.com/mholt6/status/1242175606172704770?s=20

@danlsgiga
Copy link
Contributor Author

So... I gave the srv-proxy branch a shot here and it fails because SRV upstreams should not provide ports. Here's the error I get:

2020/03/23 21:02:04.515	ERROR	http.log.error	making dial info: upstream app.service.consul: invalid dial address app.service.consul: address app.service.consul: missing port in address	{"request": {"method": "GET", "uri": "/", "proto": "HTTP/1.1", "remote_addr": "[::1]:38028", "host": "localhost", "headers": {"Accept": ["*/*"], "User-Agent": ["curl/7.29.0"]}, "tls": {"resumed": false, "version": 771, "ciphersuite": 49196, "proto": "", "proto_mutual": true, "server_name": "localhost"}}}

@francislavoie francislavoie added feature ⚙️ New feature or request in progress 🏃‍♂️ Being actively worked on labels Mar 23, 2020
@francislavoie
Copy link
Member

Branch in question, for reference: https://github.com/caddyserver/caddy/tree/srv-proxy

@mholt
Copy link
Member

mholt commented Mar 23, 2020

@danlsgiga Thanks -- for now, just put a port in there, it won't be used after the SRV lookup, I just wanna see if the SRV lookup is doing its job, basically.

@danlsgiga
Copy link
Contributor Author

yup... added the port and it works as expected! ;)

@mholt
Copy link
Member

mholt commented Mar 23, 2020

Great! Okay imma refactor some stuff, brb

@danlsgiga
Copy link
Contributor Author

This app specifically runs on an HTTP only port but most of all the other ones are listening on HTTPS... so something like srv+https and an option to provide insecure_skip_verify would be awesome as well!

@mholt
Copy link
Member

mholt commented Mar 23, 2020

Whether TLS is used is a different matter, orthogonal to SRV lookups. I'll figure something out for v2 and clean it up and tag you in a PR to review later!

@mholt
Copy link
Member

mholt commented Mar 23, 2020

Do SRV records have advantages over multiple A/AAAA records, like in #1545? Would it be necessary to implement both? Seems like they do the same thing but SRV is actually built for it...

@mholt
Copy link
Member

mholt commented Mar 23, 2020

@danlsgiga Okay, I've polished it up a bit and created a PR in #3180. Please try it out!

@danlsgiga
Copy link
Contributor Author

Do SRV records have advantages over multiple A/AAAA records, like in #1545? Would it be necessary to implement both? Seems like they do the same thing but SRV is actually built for it...

Multiple A/AAAA records are basically for load balancing when you know what port the service is running and when that port is the same for all targets. SRV records on the other hand are able to do dynamic service discovery and load balacing without the need to provide ports and it allows us to have dynamic port allocation across all nodes (the new normal in schedulers like k8s / nomad / mesos).

@mholt
Copy link
Member

mholt commented Mar 24, 2020

Gotcha, that makes sense.

I'll release beta 20 today probably, with this feature!

mholt added a commit that referenced this issue Mar 24, 2020
* reverse_proxy: Begin SRV lookup support (WIP)

* reverse_proxy: Finish adding support for SRV-based backends (#3179)
@mholt mholt removed the in progress 🏃‍♂️ Being actively worked on label Mar 24, 2020
@danlsgiga
Copy link
Contributor Author

Hey @mholt, just noticed the docs don't have this feature included yet. Since it is included in the latest beta I just want to make sure it ends there and does not get lost in the wild. ;)

@mholt
Copy link
Member

mholt commented Mar 31, 2020

Thanks for the reminder, I just updated the docs locally and it'll go out next deploy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants