Skip to content

Fix cluster peer HTTP SRV discovery when no HTTPS records exist #11776

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
ptabor merged 1 commit into from
Feb 3, 2021
+198 −29
Merged

Fix cluster peer HTTP SRV discovery when no HTTPS records exist #11776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
118 changes: 94 additions & 24 deletions pkg/srv/srv_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package srv

import (
"errors"
"fmt"
"net"
"reflect"
"strings"
Expand All @@ -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",
Expand All @@ -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
records []*net.SRV
urls []string

expected string
service string
scheme string
withSSL []*net.SRV
withoutSSL []*net.SRV
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,
},
}

Expand All @@ -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)
if err != nil {
t.Fatalf("%d: err: %#v", i, err)
str, err := GetCluster(tt.scheme, tt.service, name, "example.com", urls)

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)
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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) {
t.Errorf("#%d: endpoints = %v, want %v", i, srvs.Endpoints, tt.expected)
if hasErr(err) != tt.werr {
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)
}
}

}
}

Expand Down
18 changes: 14 additions & 4 deletions server/embed/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -91,6 +92,9 @@ var (

defaultHostname string
defaultHostStatus error

// indirection for testing
getCluster = srv.GetCluster
)

var (
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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)
}
Expand All @@ -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(
Expand All @@ -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) {
Expand Down
Loading