Merge pull request #11776 from brandond/fix_srv_11321

Fix cluster peer HTTP SRV discovery when no HTTPS records exist

pkg/srv/srv_test.go

@@ -16,6 +16,7 @@ package srv
 
 import (
 	"errors"
+	"fmt"
 	"net"
 	"reflect"
 	"strings"
@@ -24,12 +25,21 @@ import (
 	"go.etcd.io/etcd/pkg/v3/testutil"
 )
 
+func notFoundErr(service, proto, domain string) error {
+	name := fmt.Sprintf("_%s._%s.%s", service, proto, domain)
+	return &net.DNSError{Err: "no such host", Name: name, Server: "10.0.0.53:53", IsTimeout: false, IsTemporary: false, IsNotFound: true}
+}
+
 func TestSRVGetCluster(t *testing.T) {
 	defer func() {
 		lookupSRV = net.LookupSRV
 		resolveTCPAddr = net.ResolveTCPAddr
 	}()
 
+	hasErr := func(err error) bool {
+		return err != nil
+	}
+
 	name := "dnsClusterTest"
 	dns := map[string]string{
 		"1.example.com.:2480": "10.0.0.1:2480",
@@ -42,57 +52,72 @@ func TestSRVGetCluster(t *testing.T) {
 		{Target: "2.example.com.", Port: 2480},
 		{Target: "3.example.com.", Port: 2480},
 	}
+	srvNone := []*net.SRV{}
 
 	tests := []struct {
-		scheme  string
+		service    string
-		records []*net.SRV
+		scheme     string
-		urls    []string
+		withSSL    []*net.SRV
-
+		withoutSSL []*net.SRV
-		expected string
+		urls       []string
+		expected   string
+		werr       bool
 	}{
 		{
+			"etcd-server-ssl",
 			"https",
-			[]*net.SRV{},
+			srvNone,
+			srvNone,
 			nil,
-
 			"",
+			true,
 		},
 		{
+			"etcd-server-ssl",
 			"https",
 			srvAll,
+			srvNone,
 			nil,
-
 			"0=https://1.example.com:2480,1=https://2.example.com:2480,2=https://3.example.com:2480",
+			false,
 		},
 		{
+			"etcd-server",
 			"http",
+			srvNone,
 			srvAll,
 			nil,
-
 			"0=http://1.example.com:2480,1=http://2.example.com:2480,2=http://3.example.com:2480",
+			false,
 		},
 		{
+			"etcd-server-ssl",
 			"https",
 			srvAll,
+			srvNone,
 			[]string{"https://10.0.0.1:2480"},
-
 			"dnsClusterTest=https://1.example.com:2480,0=https://2.example.com:2480,1=https://3.example.com:2480",
+			false,
 		},
 		// matching local member with resolved addr and return unresolved hostnames
 		{
+			"etcd-server-ssl",
 			"https",
 			srvAll,
+			srvNone,
 			[]string{"https://10.0.0.1:2480"},
-
 			"dnsClusterTest=https://1.example.com:2480,0=https://2.example.com:2480,1=https://3.example.com:2480",
+			false,
 		},
 		// reject if apurls are TLS but SRV is only http
 		{
+			"etcd-server",
 			"http",
+			srvNone,
 			srvAll,
 			[]string{"https://10.0.0.1:2480"},
-
 			"0=http://2.example.com:2480,1=http://3.example.com:2480",
+			false,
 		},
 	}
 
@@ -109,12 +134,26 @@ func TestSRVGetCluster(t *testing.T) {
 
 	for i, tt := range tests {
 		lookupSRV = func(service string, proto string, domain string) (string, []*net.SRV, error) {
-			return "", tt.records, nil
+			if service == "etcd-server-ssl" {
+				if len(tt.withSSL) > 0 {
+					return "", tt.withSSL, nil
+				}
+				return "", nil, notFoundErr(service, proto, domain)
+			}
+			if service == "etcd-server" {
+				if len(tt.withoutSSL) > 0 {
+					return "", tt.withoutSSL, nil
+				}
+				return "", nil, notFoundErr(service, proto, domain)
+			}
+			return "", nil, errors.New("unknown service in mock")
 		}
+
 		urls := testutil.MustNewURLs(t, tt.urls)
-		str, err := GetCluster(tt.scheme, "etcd-server", name, "example.com", urls)
+		str, err := GetCluster(tt.scheme, tt.service, name, "example.com", urls)
-		if err != nil {
+
-			t.Fatalf("%d: err: %#v", i, err)
+		if hasErr(err) != tt.werr {
+			t.Fatalf("%d: err = %#v, want = %#v", i, err, tt.werr)
 		}
 		if strings.Join(str, ",") != tt.expected {
 			t.Errorf("#%d: cluster = %s, want %s", i, str, tt.expected)
@@ -125,15 +164,31 @@ func TestSRVGetCluster(t *testing.T) {
 func TestSRVDiscover(t *testing.T) {
 	defer func() { lookupSRV = net.LookupSRV }()
 
+	hasErr := func(err error) bool {
+		return err != nil
+	}
+
 	tests := []struct {
 		withSSL    []*net.SRV
 		withoutSSL []*net.SRV
 		expected   []string
+		werr       bool
 	}{
 		{
 			[]*net.SRV{},
 			[]*net.SRV{},
 			[]string{},
+			true,
+		},
+		{
+			[]*net.SRV{},
+			[]*net.SRV{
+				{Target: "10.0.0.1", Port: 2480},
+				{Target: "10.0.0.2", Port: 2480},
+				{Target: "10.0.0.3", Port: 2480},
+			},
+			[]string{"http://10.0.0.1:2480", "http://10.0.0.2:2480", "http://10.0.0.3:2480"},
+			false,
 		},
 		{
 			[]*net.SRV{
@@ -143,6 +198,7 @@ func TestSRVDiscover(t *testing.T) {
 			},
 			[]*net.SRV{},
 			[]string{"https://10.0.0.1:2480", "https://10.0.0.2:2480", "https://10.0.0.3:2480"},
+			false,
 		},
 		{
 			[]*net.SRV{
@@ -154,6 +210,7 @@ func TestSRVDiscover(t *testing.T) {
 				{Target: "10.0.0.1", Port: 7001},
 			},
 			[]string{"https://10.0.0.1:2480", "https://10.0.0.2:2480", "https://10.0.0.3:2480", "http://10.0.0.1:7001"},
+			false,
 		},
 		{
 			[]*net.SRV{
@@ -165,6 +222,7 @@ func TestSRVDiscover(t *testing.T) {
 				{Target: "10.0.0.1", Port: 7001},
 			},
 			[]string{"https://10.0.0.1:2480", "https://10.0.0.2:2480", "https://10.0.0.3:2480", "http://10.0.0.1:7001"},
+			false,
 		},
 		{
 			[]*net.SRV{
@@ -174,29 +232,41 @@ func TestSRVDiscover(t *testing.T) {
 			},
 			[]*net.SRV{},
 			[]string{"https://a.example.com:2480", "https://b.example.com:2480", "https://c.example.com:2480"},
+			false,
 		},
 	}
 
 	for i, tt := range tests {
 		lookupSRV = func(service string, proto string, domain string) (string, []*net.SRV, error) {
 			if service == "etcd-client-ssl" {
-				return "", tt.withSSL, nil
+				if len(tt.withSSL) > 0 {
+					return "", tt.withSSL, nil
+				}
+				return "", nil, notFoundErr(service, proto, domain)
 			}
 			if service == "etcd-client" {
-				return "", tt.withoutSSL, nil
+				if len(tt.withoutSSL) > 0 {
+					return "", tt.withoutSSL, nil
+				}
+				return "", nil, notFoundErr(service, proto, domain)
 			}
 			return "", nil, errors.New("unknown service in mock")
 		}
 
 		srvs, err := GetClient("etcd-client", "example.com", "")
-		if err != nil {
-			t.Fatalf("%d: err: %#v", i, err)
-		}
 
-		if !reflect.DeepEqual(srvs.Endpoints, tt.expected) {
+		if hasErr(err) != tt.werr {
-			t.Errorf("#%d: endpoints = %v, want %v", i, srvs.Endpoints, tt.expected)
+			t.Fatalf("%d: err = %#v, want = %#v", i, err, tt.werr)
+		}
+		if srvs == nil {
+			if len(tt.expected) > 0 {
+				t.Errorf("#%d: srvs = nil, want non-nil", i)
+			}
+		} else {
+			if !reflect.DeepEqual(srvs.Endpoints, tt.expected) {
+				t.Errorf("#%d: endpoints = %v, want = %v", i, srvs.Endpoints, tt.expected)
+			}
 		}
-
 	}
 }
 

server/embed/config.go

@@ -37,6 +37,7 @@ import (
 	"go.etcd.io/etcd/server/v3/etcdserver/api/v3compactor"
 
 	bolt "go.etcd.io/bbolt"
+	"go.uber.org/multierr"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 	"golang.org/x/crypto/bcrypt"
@@ -91,6 +92,9 @@ var (
 
 	defaultHostname   string
 	defaultHostStatus error
+
+	// indirection for testing
+	getCluster = srv.GetCluster
 )
 
 var (
@@ -630,6 +634,8 @@ func (cfg *Config) PeerURLsMapAndToken(which string) (urlsmap types.URLsMap, tok
 		lg := cfg.logger
 		if cerr != nil {
 			lg.Warn("failed to resolve during SRV discovery", zap.Error(cerr))
+		}
+		if len(clusterStrs) == 0 {
 			return nil, "", cerr
 		}
 		for _, s := range clusterStrs {
@@ -656,6 +662,10 @@ func (cfg *Config) PeerURLsMapAndToken(which string) (urlsmap types.URLsMap, tok
 }
 
 // GetDNSClusterNames uses DNS SRV records to get a list of initial nodes for cluster bootstrapping.
+// This function will return a list of one or more nodes, as well as any errors encountered while
+// performing service discovery.
+// Note: Because this checks multiple sets of SRV records, discovery should only be considered to have
+// failed if the returned node list is empty.
 func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 	var (
 		clusterStrs       []string
@@ -670,7 +680,7 @@ func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 
 	// Use both etcd-server-ssl and etcd-server for discovery.
 	// Combine the results if both are available.
-	clusterStrs, cerr = srv.GetCluster("https", "etcd-server-ssl"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.APUrls)
+	clusterStrs, cerr = getCluster("https", "etcd-server-ssl"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.APUrls)
 	if cerr != nil {
 		clusterStrs = make([]string, 0)
 	}
@@ -685,8 +695,8 @@ func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 		zap.Error(cerr),
 	)
 
-	defaultHTTPClusterStrs, httpCerr := srv.GetCluster("http", "etcd-server"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.APUrls)
+	defaultHTTPClusterStrs, httpCerr := getCluster("http", "etcd-server"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.APUrls)
-	if httpCerr != nil {
+	if httpCerr == nil {
 		clusterStrs = append(clusterStrs, defaultHTTPClusterStrs...)
 	}
 	lg.Info(
@@ -700,7 +710,7 @@ func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 		zap.Error(httpCerr),
 	)
 
-	return clusterStrs, cerr
+	return clusterStrs, multierr.Combine(cerr, httpCerr)
 }
 
 func (cfg Config) InitialClusterFromName(name string) (ret string) {

server/embed/config_test.go

@@ -17,16 +17,24 @@ package embed
 import (
 	"fmt"
 	"io/ioutil"
+	"net"
 	"net/url"
 	"os"
 	"testing"
 	"time"
 
+	"go.etcd.io/etcd/pkg/v3/srv"
 	"go.etcd.io/etcd/pkg/v3/transport"
+	"go.etcd.io/etcd/pkg/v3/types"
 
 	"sigs.k8s.io/yaml"
 )
 
+func notFoundErr(service, domain string) error {
+	name := fmt.Sprintf("_%s._tcp.%s", service, domain)
+	return &net.DNSError{Err: "no such host", Name: name, Server: "10.0.0.53:53", IsTimeout: false, IsTemporary: false, IsNotFound: true}
+}
+
 func TestConfigFileOtherFields(t *testing.T) {
 	ctls := securityConfig{TrustedCAFile: "cca", CertFile: "ccert", KeyFile: "ckey"}
 	ptls := securityConfig{TrustedCAFile: "pca", CertFile: "pcert", KeyFile: "pkey"}
@@ -84,7 +92,7 @@ func TestUpdateDefaultClusterFromName(t *testing.T) {
 
 	// in case of 'etcd --name=abc'
 	exp := fmt.Sprintf("%s=%s://localhost:%s", cfg.Name, oldscheme, lpport)
-	cfg.UpdateDefaultClusterFromName(defaultInitialCluster)
+	_, _ = cfg.UpdateDefaultClusterFromName(defaultInitialCluster)
 	if exp != cfg.InitialCluster {
 		t.Fatalf("initial-cluster expected %q, got %q", exp, cfg.InitialCluster)
 	}
@@ -201,3 +209,83 @@ func TestAutoCompactionModeParse(t *testing.T) {
 		}
 	}
 }
+
+func TestPeerURLsMapAndTokenFromSRV(t *testing.T) {
+	defer func() { getCluster = srv.GetCluster }()
+
+	tests := []struct {
+		withSSL    []string
+		withoutSSL []string
+		apurls     []string
+		wurls      string
+		werr       bool
+	}{
+		{
+			[]string{},
+			[]string{},
+			[]string{"http://localhost:2380"},
+			"",
+			true,
+		},
+		{
+			[]string{"1.example.com=https://1.example.com:2380", "0=https://2.example.com:2380", "1=https://3.example.com:2380"},
+			[]string{},
+			[]string{"https://1.example.com:2380"},
+			"0=https://2.example.com:2380,1.example.com=https://1.example.com:2380,1=https://3.example.com:2380",
+			false,
+		},
+		{
+			[]string{"1.example.com=https://1.example.com:2380"},
+			[]string{"0=http://2.example.com:2380", "1=http://3.example.com:2380"},
+			[]string{"https://1.example.com:2380"},
+			"0=http://2.example.com:2380,1.example.com=https://1.example.com:2380,1=http://3.example.com:2380",
+			false,
+		},
+		{
+			[]string{},
+			[]string{"1.example.com=http://1.example.com:2380", "0=http://2.example.com:2380", "1=http://3.example.com:2380"},
+			[]string{"http://1.example.com:2380"},
+			"0=http://2.example.com:2380,1.example.com=http://1.example.com:2380,1=http://3.example.com:2380",
+			false,
+		},
+	}
+
+	hasErr := func(err error) bool {
+		return err != nil
+	}
+
+	for i, tt := range tests {
+		getCluster = func(serviceScheme string, service string, name string, dns string, apurls types.URLs) ([]string, error) {
+			var urls []string
+			if serviceScheme == "https" && service == "etcd-server-ssl" {
+				urls = tt.withSSL
+			} else if serviceScheme == "http" && service == "etcd-server" {
+				urls = tt.withoutSSL
+			}
+			if len(urls) > 0 {
+				return urls, nil
+			}
+			return urls, notFoundErr(service, dns)
+		}
+
+		cfg := NewConfig()
+		cfg.Name = "1.example.com"
+		cfg.InitialCluster = ""
+		cfg.InitialClusterToken = ""
+		cfg.DNSCluster = "example.com"
+		cfg.APUrls = types.MustNewURLs(tt.apurls)
+
+		if err := cfg.Validate(); err != nil {
+			t.Errorf("#%d: failed to validate test Config: %v", i, err)
+			continue
+		}
+
+		urlsmap, _, err := cfg.PeerURLsMapAndToken("etcd")
+		if urlsmap.String() != tt.wurls {
+			t.Errorf("#%d: urlsmap = %s, want = %s", i, urlsmap.String(), tt.wurls)
+		}
+		if hasErr(err) != tt.werr {
+			t.Errorf("#%d: err = %v, want = %v", i, err, tt.werr)
+		}
+	}
+}

server/go.mod

@@ -32,6 +32,7 @@ require (
 	go.etcd.io/etcd/client/v3 v3.5.0-pre
 	go.etcd.io/etcd/pkg/v3 v3.5.0-pre
 	go.etcd.io/etcd/raft/v3 v3.5.0-pre
+	go.uber.org/multierr v1.5.0
 	go.uber.org/zap v1.16.0
 	golang.org/x/crypto v0.0.0-20201002170205-7f63de1d35b0
 	golang.org/x/lint v0.0.0-20200302205851-738671d3881b // indirect