reverseproxy: return validation error if upstream address has port range size of more than 1

[mohammed90]

Currently the Provision-er of the reverse proxy handler accepts upstream network address with range size more than 1. This means the following Caddyfile is accepted and validates successfully:

localhost
reverse_proxy localhost:2000-2020

This is despite the comment on the Dial field of the Upstream struct saying only a single socket is accepted:

caddy/modules/caddyhttp/reverseproxy/hosts.go

Lines 68 to 78 in e2f913b

	// The [network address](/docs/conventions#network-addresses)
	// to dial to connect to the upstream. Must represent precisely
	// one socket (i.e. no port ranges). A valid network address
	// either has a host and port or is a unix socket address.
	//
	// Placeholders may be used to make the upstream dynamic, but be
	// aware of the health check implications of this: a single
	// upstream that represents numerous (perhaps arbitrary) backends
	// can be considered down if one or enough of the arbitrary
	// backends is down. Also be aware of open proxy vulnerabilities.
	Dial string `json:"dial,omitempty"`

The code doesn't validate that until request time, when the method fillDialInfo is called:

caddy/modules/caddyhttp/reverseproxy/hosts.go

Lines 161 to 164 in e2f913b

	if numPorts := addr.PortRangeSize(); numPorts != 1 {
	return DialInfo{}, fmt.Errorf("upstream %s: dial address must represent precisely one socket: %s represents %d",
	u.Dial, dial, numPorts)
	}

This PR adds validation of the port range size of an upstream address to the Provision step. We can sugar the Caddyfile upstream to accept port range and convert it to the JSON array for the entire specified range.

[francislavoie]

Hmm, yeah I think we should continue to support that, not reject it. It was a thing in Caddy v1 and was intended to work in v2.

[mohammed90]

It didn't work before. It would've just failed at runtime rather than during validation.

[francislavoie]

Understood - but we should actually fix it instead of rejecting it 🙂

[mohammed90]

Hmm okay, how should we handle them? Should we expand them into the upstream slice? This makes it simpler for the selection policy to work.

The other approach is to treat network addresses whose port range size is greater than 1 as a single unit, but the selection policy might need to be applied recursively or at random.

[francislavoie]

🤔 I think the Caddyfile adapter can expand it out. Probably the easiest way to deal with it.

[mohammed90]

Oh so you're saying the JSON should reject upstream network addresses whose port range size is greater than 1, but the Caddyfile should accept it, right? I understood your earlier comment to apply to both JSON and Caddyfile.

[francislavoie]

Either that, or we expand it out early at provisioning time so it's supported everywhere?

[mholt]

The tricky thing about expanding them out into multiple upstreams is the user might expect all of them to act as one upstream, because they were configured together. This has implications with regards to load balancing and health checks.

So to keep things simple, for now I disallowed port ranges. I remember initially thinking that expanding them out would be no problem (and thus didn't put in this check here), but when I got around to HC and LB it got complicated so I changed my mind.

Anyway, this is a good change I think. Thanks!

[mholt]

Excellent, look good! Thank you as always, Mohammed!