Skip to content

reverseproxy: Remove deprecated lookup_srv #5396

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.

Sign up for GitHub

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

Merged
mholt merged 2 commits into from
Apr 10, 2023
Merged

reverseproxy: Remove deprecated lookup_srv #5396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f11bcb
reverseproxy: Remove deprecated `lookup_srv`
francislavoie Feb 26, 2023
95b7fb5
Merge branch 'master' into drop-proxy-lookupsrv
mholt Apr 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 4 additions & 91 deletions caddytest/integration/reverseproxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,10 @@ func TestSRVReverseProxy(t *testing.T) {
"handle": [
{
"handler": "reverse_proxy",
"upstreams": [
{
"lookup_srv": "srv.host.service.consul"
}
]
"dynamic_upstreams": {
"source": "srv",
"name": "srv.host.service.consul"
}
}
]
}
Expand All @@ -57,47 +56,6 @@ func TestSRVReverseProxy(t *testing.T) {
`, "json")
}

func TestSRVWithDial(t *testing.T) {
caddytest.AssertLoadError(t, `
{
"apps": {
"pki": {
"certificate_authorities": {
"local": {
"install_trust": false
}
}
},
"http": {
"grace_period": 1,
"servers": {
"srv0": {
"listen": [
":18080"
],
"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 TestDialWithPlaceholderUnix(t *testing.T) {

if runtime.GOOS == "windows" {
Expand Down Expand Up @@ -369,51 +327,6 @@ func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) {
tester.AssertResponse(req, 200, "Hello, World!")
}

func TestSRVWithActiveHealthcheck(t *testing.T) {
caddytest.AssertLoadError(t, `
{
"apps": {
"pki": {
"certificate_authorities" : {
"local" : {
"install_trust": false
}
}
},
"http": {
"grace_period": 1,
"servers": {
"srv0": {
"listen": [
":18080"
],
"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
19 changes: 2 additions & 17 deletions modules/caddyhttp/reverseproxy/caddyfile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
package reverseproxy

import (
"net"
"net/http"
"reflect"
"strconv"
Expand Down Expand Up @@ -142,15 +141,8 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
h.responseMatchers = make(map[string]caddyhttp.ResponseMatcher)

// appendUpstream creates an upstream for address and adds
// it to the list. If the address starts with "srv+" it is
// treated as a SRV-based upstream, and any port will be
// dropped.
// it to the list.
appendUpstream := func(address string) error {
isSRV := strings.HasPrefix(address, "srv+")
if isSRV {
address = strings.TrimPrefix(address, "srv+")
}

dialAddr, scheme, err := parseUpstreamDialAddress(address)
if err != nil {
return d.WrapErr(err)
Expand All @@ -165,14 +157,7 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
}
commonScheme = scheme

if isSRV {
if host, _, err := net.SplitHostPort(dialAddr); err == nil {
dialAddr = host
}
h.Upstreams = append(h.Upstreams, &Upstream{LookupSRV: dialAddr})
} else {
h.Upstreams = append(h.Upstreams, &Upstream{Dial: dialAddr})
}
h.Upstreams = append(h.Upstreams, &Upstream{Dial: dialAddr})
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion modules/caddyhttp/reverseproxy/healthchecks.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ func (h *Handler) doActiveHealthCheckForAllHosts() {
}
addr.StartPort, addr.EndPort = hcp, hcp
}
if upstream.LookupSRV == "" && addr.PortRangeSize() != 1 {
if addr.PortRangeSize() != 1 {
h.HealthChecks.Active.logger.Error("multiple addresses (upstream must map to only one address)",
zap.String("address", networkAddr),
)
Expand Down
49 changes: 11 additions & 38 deletions modules/caddyhttp/reverseproxy/hosts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package reverseproxy
import (
"context"
"fmt"
"net"
"net/http"
"net/netip"
"strconv"
Expand Down Expand Up @@ -48,15 +47,6 @@ type Upstream struct {
// backends is down. Also be aware of open proxy vulnerabilities.
Dial string `json:"dial,omitempty"`

// DEPRECATED: Use the SRVUpstreams module instead
// (http.reverse_proxy.upstreams.srv). This field will be
// removed in a future version of Caddy. TODO: Remove this field.
//
// If DNS SRV records are used for service discovery with this
// upstream, specify the DNS name for which to look up SRV
// records here, instead of specifying a dial address.
LookupSRV string `json:"lookup_srv,omitempty"`

// The maximum number of simultaneous requests to allow to
// this upstream. If set, overrides the global passive health
// check UnhealthyRequestCount value.
Expand All @@ -74,9 +64,6 @@ type Upstream struct {
}

func (u Upstream) String() string {
if u.LookupSRV != "" {
return u.LookupSRV
}
return u.Dial
}

Expand Down Expand Up @@ -110,35 +97,21 @@ func (u *Upstream) Full() bool {
}

// fillDialInfo returns a filled DialInfo for upstream u, using the request
// context. If the upstream has a SRV lookup configured, that is done and a
// returned address is chosen; otherwise, the upstream's regular dial address
// field is used. Note that the returned value is not a pointer.
// context. Note that the returned value is not a pointer.
func (u *Upstream) fillDialInfo(r *http.Request) (DialInfo, error) {
repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer)
var addr caddy.NetworkAddress

if u.LookupSRV != "" {
// perform DNS lookup for SRV records and choose one - TODO: deprecated
srvName := repl.ReplaceAll(u.LookupSRV, "")
_, records, err := net.DefaultResolver.LookupSRV(r.Context(), "", "", srvName)
if err != nil {
return DialInfo{}, err
}
addr.Network = "tcp"
addr.Host = records[0].Target
addr.StartPort, addr.EndPort = uint(records[0].Port), uint(records[0].Port)
} else {
// use provided dial address
var err error
dial := repl.ReplaceAll(u.Dial, "")
addr, err = caddy.ParseNetworkAddress(dial)
if err != nil {
return DialInfo{}, fmt.Errorf("upstream %s: invalid dial address %s: %v", u.Dial, dial, err)
}
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)
}
// use provided dial address
var err error
dial := repl.ReplaceAll(u.Dial, "")
addr, err = caddy.ParseNetworkAddress(dial)
if err != nil {
return DialInfo{}, fmt.Errorf("upstream %s: invalid dial address %s: %v", u.Dial, dial, err)
}
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)
}

return DialInfo{
Expand Down
14 changes: 0 additions & 14 deletions modules/caddyhttp/reverseproxy/reverseproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,20 +243,6 @@ func (h *Handler) Provision(ctx caddy.Context) error {
h.logger.Warn("UNLIMITED BUFFERING: buffering is enabled without any cap on buffer size, which can result in OOM crashes")
}

// verify SRV compatibility - TODO: LookupSRV deprecated; will be removed
for i, v := range h.Upstreams {
if v.LookupSRV == "" {
continue
}
h.logger.Warn("DEPRECATED: lookup_srv: will be removed in a near-future version of Caddy; use the http.reverse_proxy.upstreams.srv module instead")
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