Fix cluster peer HTTP SRV discovery

Signed-off-by: Brad Davidson <[email protected]>

embed/config.go

@@ -39,6 +39,7 @@ import (
 	"go.etcd.io/etcd/pkg/types"
 
 	bolt "go.etcd.io/bbolt"
+	"go.uber.org/multierr"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 	"golang.org/x/crypto/bcrypt"
@@ -93,6 +94,9 @@ var (
 
 	defaultHostname   string
 	defaultHostStatus error
+
+	// indirection for testing
+	getCluster = srv.GetCluster
 )
 
 var (
@@ -726,6 +730,8 @@ func (cfg *Config) PeerURLsMapAndToken(which string) (urlsmap types.URLsMap, tok
 			} else {
 				plog.Errorf("couldn't resolve during SRV discovery (%v)", cerr)
 			}
+		}
+		if len(clusterStrs) == 0 {
 			return nil, "", cerr
 		}
 		for _, s := range clusterStrs {
@@ -756,6 +762,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
@@ -770,7 +780,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.AdvertisePeerUrls)
+	clusterStrs, cerr = getCluster("https", "etcd-server-ssl"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.AdvertisePeerUrls)
 	if cerr != nil {
 		clusterStrs = make([]string, 0)
 	}
@@ -787,8 +797,8 @@ func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 		)
 	}
 
-	defaultHTTPClusterStrs, httpCerr := srv.GetCluster("http", "etcd-server"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.AdvertisePeerUrls)
-	if httpCerr != nil {
+	defaultHTTPClusterStrs, httpCerr := getCluster("http", "etcd-server"+serviceNameSuffix, cfg.Name, cfg.DNSCluster, cfg.AdvertisePeerUrls)
+	if httpCerr == nil {
 		clusterStrs = append(clusterStrs, defaultHTTPClusterStrs...)
 	}
 	if lg != nil {
@@ -804,7 +814,7 @@ func (cfg *Config) GetDNSClusterNames() ([]string, error) {
 		)
 	}
 
-	return clusterStrs, cerr
+	return clusterStrs, multierr.Combine(cerr, httpCerr)
 }
 
 func (cfg Config) InitialClusterFromName(name string) (ret string) {

embed/config_test.go

@@ -18,17 +18,25 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io/ioutil"
+	"net"
 	"net/url"
 	"os"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"go.etcd.io/etcd/pkg/srv"
 	"go.etcd.io/etcd/pkg/transport"
+	"go.etcd.io/etcd/pkg/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"}
@@ -86,7 +94,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)
 	}
@@ -281,3 +289,77 @@ func TestTLSVersionMinMax(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.AdvertisePeerUrls = 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)
+		}
+	}
+}