Fix cluster peer HTTP SRV discovery

Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

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)
-	if httpCerr != nil {
+	defaultHTTPClusterStrs, httpCerr := getCluster("http", "etcd-server"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.APUrls)
+	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