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: fix breakage in handling SRV lookup introduced by d55d50b #3756

Merged
merged 7 commits into from
Oct 1, 2020
103 changes: 103 additions & 0 deletions caddytest/integration/reverseproxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,109 @@ import (
"github.com/caddyserver/caddy/v2/caddytest"
)

func TestSRVReverseProxy(t *testing.T) {
tester := caddytest.NewTester(t)
tester.InitServer(`
{
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":8080"
],
"routes": [
{
"handle": [
{
"handler": "reverse_proxy",
"upstreams": [
{
"lookup_srv": "srv.host.service.consul"
}
]
}
]
}
]
}
}
}
}
}
`, "json")
}

func TestSRVWithDial(t *testing.T) {
caddytest.AssertLoadError(t, `
{
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":8080"
],
"routes": [
{
"handle": [
{
"handler": "reverse_proxy",
"upstreams": [
{
"dial": "tcp/address.to.upstream:80",
"lookup_srv": "srv.host.service.consul"
}
]
}
]
}
]
}
}
}
}
}
`, "json", `upstream: specifying dial address is incompatible with lookup_srv: 0: {\"dial\": \"tcp/address.to.upstream:80\", \"lookup_srv\": \"srv.host.service.consul\"}`)
}

func TestSRVWithActiveHealthcheck(t *testing.T) {
caddytest.AssertLoadError(t, `
{
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":8080"
],
"routes": [
{
"handle": [
{
"handler": "reverse_proxy",
"health_checks": {
"active": {
"path": "/ok"
}
},
"upstreams": [
{
"lookup_srv": "srv.host.service.consul"
}
]
}
]
}
]
}
}
}
}
}
`, "json", `upstream: lookup_srv is incompatible with active health checks: 0: {\"dial\": \"\", \"lookup_srv\": \"srv.host.service.consul\"}`)
}

func TestReverseProxyHealthCheck(t *testing.T) {
tester := caddytest.NewTester(t)
tester.InitServer(`
Expand Down
34 changes: 25 additions & 9 deletions modules/caddyhttp/reverseproxy/reverseproxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,21 @@ func (h *Handler) Provision(ctx caddy.Context) error {
h.ctx = ctx
h.logger = ctx.Logger(h)

// get validation out of the way
for i, v := range h.Upstreams {
// Having LookupSRV non-empty conflicts with 2 other config parameters: active health checks, and upstream dial address.
// Therefore if LookupSRV is empty, then there are no immediately apparent config conflicts, and the iteration can be skipped.
if v.LookupSRV == "" {
continue
}
if h.HealthChecks != nil && h.HealthChecks.Active != nil {
return fmt.Errorf(`upstream: lookup_srv is incompatible with active health checks: %d: {"dial": %q, "lookup_srv": %q}`, i, v.Dial, v.LookupSRV)
}
if v.Dial != "" {
return fmt.Errorf(`upstream: specifying dial address is incompatible with lookup_srv: %d: {"dial": %q, "lookup_srv": %q}`, i, v.Dial, v.LookupSRV)
}
}

// start by loading modules
if h.TransportRaw != nil {
mod, err := ctx.LoadModule(h, "TransportRaw")
Expand Down Expand Up @@ -204,17 +219,18 @@ func (h *Handler) Provision(ctx caddy.Context) error {

// set up upstreams
for _, upstream := range h.Upstreams {
addr, err := caddy.ParseNetworkAddress(upstream.Dial)
if err != nil {
return err
}

if addr.PortRangeSize() != 1 {
return fmt.Errorf("multiple addresses (upstream must map to only one address): %v", addr)
}
if upstream.LookupSRV == "" {
addr, err := caddy.ParseNetworkAddress(upstream.Dial)
if err != nil {
return err
}

upstream.networkAddress = addr
if addr.PortRangeSize() != 1 {
return fmt.Errorf("multiple addresses (upstream must map to only one address): %v", addr)
}

upstream.networkAddress = addr
}
// create or get the host representation for this upstream
var host Host = new(upstreamHost)
existingHost, loaded := hosts.LoadOrStore(upstream.String(), host)
Expand Down