-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
reverseproxy: Dynamic upstreams (with support for SRV and A/AAAA lookups) #4470
Conversation
if h.DynamicUpstreams != nil { | ||
dUpstreams, err := h.DynamicUpstreams.GetUpstreams(r) | ||
if err != nil { | ||
h.logger.Error("failed getting dynamic upstreams; falling back to static upstreams", zap.Error(err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it a bit more, I don't see the value in doing this fallback. I think it should just be either or. Actually like I mentioned before I think static upstreams should be an implementation of DynamicUpstreams that we implicitly provision for backwards compatibility. Would streamline this code.
If someone wants fallback, then they can implement their own module that wraps any other upstream source, first trying the wrapped one and falling back to whatever the wrapping module is configured to do. We could provide a fallback one as a standard module for fun 🤷♂️
Also we could (for backwards compatibility) turn an old-way lookup_srv
configured upstream into a SRVUpstreams (instead of StaticUpstreams, as above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, one code/config path would be nice. Let me give this some more thought.
I was sent here from #1630. One thing I was thinking of doing was adding support for a Would doing this be possible with the existing codebase or this MR? Should I open an issue or discuss this sort of work somewhere? For what its worth, my use case is adding a simple integration for the Dokku paas, and avoiding the issue we have with nginx where docker restarts may result in container IPs/Ports changing. |
You can just proxy to
This would require Caddy to actually talk to the Docker API, possibly via the docker unix socket. That's out of scope of this PR specifically, but it could possibly be implemented as a custom module which implements the dynamic upstreams interface. It wouldn't be done with a special scheme, but rather with a separate module (which is what this PR makes possible). FYI, your usecase might be already covered by https://github.com/lucaslorentz/caddy-docker-proxy though, depending on your goals. It generates Caddyfile config dynamically based on the labels on the containers in your docker network. |
I added Caddyfile support real quick, a couple examples here in the tests: |
Wow, thanks Francis! 😁 Nice job for "real quick", heh. |
@jjiang-stripe, @cds2-stripe - this is ready for an initial review / trial whenever you're ready 👍 |
Thank you! We'll set aside some time next week to test this internally. |
Also get upstreams at every retry loop iteration instead of just once before the loop. See #4442.
Still WIP, preparing to preserve health checker functionality
Move active health check results into handler-specific Upstreams. Improve documentation regarding health checks and upstreams.
0986c51
to
9a28420
Compare
Could this support fetching the upstream from an HTTP request or a database read (maybe coordinates with the storage module)? |
@yroc92 Yes, any function that, given the request, can return a list of upstreams, is compatible. And any Caddy modules can get access to Caddy's global storage configured here: https://caddyserver.com/docs/json/storage/ |
Alright, I'm going to merge this in preparation for the pre-release of v2.5. If needed, we can tweak these features in a separate PR and continue discussion in another issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely an awesome step forwards.
I have a few concerns, but we talked off-github and I think we're on the same page that we can probably sort them out in followup PRs before 2.5 stable (this being in the beta). In particular, I think the GET /reverse_proxy/upstreams API will be broken with these changes, and I think we should take the refactor a step further with making static upstreams an UpstreamSource
as well so we only have a single codepath in the main proxy code (instead of it being split depending on whether static or dynamic is configured).
Why no aaaa source that restricts it to just IPv6? Would be handy for my NAT |
@xeruf you'll need to elaborate, your question is severely lacking detail. A merged PR is not the right place for further discussion. If you have a feature request, open a new issue. If you have a usage question, open a topic on the forums https://caddy.community. |
@xeruf I would be happy to hear the use case for that in a new issue! |
Right now, proxy upstreams have to be hard-coded into the config (with the only dyanamism coming from placeholders, which all act as a single upstream anyway). This change adds supports for truly dynamic upstreams, with the potential for every request to have different upstreams -- not only every request, but every retry within a single request, too.
Instead of (or in addition to) specifying
upstreams
in your config, you could specifydynamic_upstreams
and then define your upstream source module. Currently I'm implementing SRV and A/AAAA lookups as sources.Fixes or related to:
When we're done, it will hopefully close or supercede #4350, #3801, #4446, and #4245. Closes #4341 too.
For dynamic SRV lookups:
For dynamic A/AAAA DNS lookups:
The default refresh interval for lookups is 1 minute, but can be configured with the
refresh
property. SRV lookups can also optionally be configured by their individual service, proto, and name components, or the entire domain as the name by itself.Still need to integrate health checks, add docs and tests, and cache eviction.
May be of interest to @danlsgiga, @dazoot, @sorenisanerd, and @furkanmustafa.
CC: @jjiang-stripe and @cds2-stripe