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

reverseproxy: Add ID field for upstreams #4350

Closed
wants to merge 2 commits into from

Conversation

francislavoie
Copy link
Member

First step for #4341, Caddyfile support would be needed to complete it. /cc @ulrichSchreiner

This makes it possible to specify upstreams in JSON with an "id" field, to disambiguate upstreams with the same dial address:

"upstreams": [
    {
        "id": "second",
        "dial": "httpbin.org:443"
    }
],

Given the JSON config from https://caddy.community/t/healthcheck-not-working-with-multiple-servers/13563, with just "id" fields added, we see that it works, the internal state has separate upstreams:

$ curl -s localhost:2019/reverse_proxy/upstreams | jq
[
  {
    "id": "second",
    "address": "second",
    "healthy": true,
    "num_requests": 0,
    "fails": 0
  },
  {
    "id": "first",
    "address": "first",
    "healthy": false,
    "num_requests": 0,
    "fails": 0
  }
]

To avoid it being a breaking change, I left in the "address" field in the admin API, deprecated for now, to be removed at a later date. It's more accurate to call it "ID" even though it could be an ID, a dial address, or a SRV address, depending on how it was configured (i.e. whatever the upstream's String() function returns).

@francislavoie francislavoie added the under review 🧐 Review is pending before merging label Sep 13, 2021
@francislavoie francislavoie added this to the v2.5.0 milestone Sep 13, 2021
@francislavoie
Copy link
Member Author

I made a second commit with a proposed way to specify IDs in the Caddyfile, by using | as a separator. I consider this part still up for debate though. I think it's an okay solution, but 🤷‍♂️

@mholt
Copy link
Member

mholt commented Sep 20, 2021

Ok, so I appreciate the thought going into this. I think I want to see about coming up with a different fix though that doesn't require the user to name the upstream separate from the upstream's... well, its name (address, like DNS name or IP). I will try to give it some brain cycles this week.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Oct 20, 2021
@mholt mholt added declined 🚫 Not a fit for this project and removed under review 🧐 Review is pending before merging labels Dec 13, 2021
@mholt
Copy link
Member

mholt commented Dec 13, 2021

(With all due respect) I will be sunsetting this PR in favor of #4470 which will properly distinguish health check state from upstreams, so IDs will not be needed. (We can reopen if that's not the case for some reason, but I'm pretty sure this will be superseded by that.)

Thank you for working on it. 🙏

@mholt mholt closed this Dec 13, 2021
@mholt mholt removed the bug 🐞 Something isn't working label Dec 13, 2021
@francislavoie
Copy link
Member Author

francislavoie commented Dec 13, 2021

@mholt I don't see how #4470 will distinguish the upstreams from two different reverse_proxy handler instances. There's still no identifier to disambiguate two hosts with the same upstream address in var hosts = caddy.NewUsagePool(). That's the crux of this issue. upstream.fillHost() will still return the same host for both, so they'll still share the health state. The only way they'll return something different is if upstream.String() returns something different for the two instances. This is what this PR does, it allows for an optional ID to be specified to disambiguate them.

@mholt
Copy link
Member

mholt commented Dec 13, 2021

I'm not finished yet. When I am, the hosts in the pool will not be storing their health state. The crux of the problem will be fixed. Edit: Done now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
declined 🚫 Not a fit for this project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants