From bf5735f29f065047f8cc9718a2ff1fc81df47c22 Mon Sep 17 00:00:00 2001 From: Chao Chen Date: Thu, 27 Jul 2023 15:18:12 -0700 Subject: [PATCH] endpoints.Interpret returns Host:port as ServerName Signed-off-by: Chao Chen --- client/v3/internal/endpoint/endpoint.go | 18 +++----- client/v3/internal/endpoint/endpoint_test.go | 24 +++++----- tests/e2e/ctl_v3_grpc_test.go | 46 +++++++++----------- tests/integration/grpc_test.go | 26 ++++++----- 4 files changed, 54 insertions(+), 60 deletions(-) diff --git a/client/v3/internal/endpoint/endpoint.go b/client/v3/internal/endpoint/endpoint.go index f6674235cd9e..35a3fe8c3374 100644 --- a/client/v3/internal/endpoint/endpoint.go +++ b/client/v3/internal/endpoint/endpoint.go @@ -41,10 +41,6 @@ func extractHostFromHostPort(ep string) string { return host } -func extractHostFromPath(pathStr string) string { - return extractHostFromHostPort(path.Base(pathStr)) -} - // mustSplit2 returns the values from strings.SplitN(s, sep, 2). // If sep is not found, it returns ("", "", false) instead. func mustSplit2(s, sep string) (string, string) { @@ -96,29 +92,29 @@ func translateEndpoint(ep string) (addr string, serverName string, requireCreds if strings.HasPrefix(ep, "unix:///") || strings.HasPrefix(ep, "unixs:///") { // absolute path case schema, absolutePath := mustSplit2(ep, "://") - return "unix://" + absolutePath, extractHostFromPath(absolutePath), schemeToCredsRequirement(schema) + return "unix://" + absolutePath, path.Base(absolutePath), schemeToCredsRequirement(schema) } if strings.HasPrefix(ep, "unix://") || strings.HasPrefix(ep, "unixs://") { // legacy etcd local path schema, localPath := mustSplit2(ep, "://") - return "unix:" + localPath, extractHostFromPath(localPath), schemeToCredsRequirement(schema) + return "unix:" + localPath, path.Base(localPath), schemeToCredsRequirement(schema) } schema, localPath := mustSplit2(ep, ":") - return "unix:" + localPath, extractHostFromPath(localPath), schemeToCredsRequirement(schema) + return "unix:" + localPath, path.Base(localPath), schemeToCredsRequirement(schema) } if strings.Contains(ep, "://") { url, err := url.Parse(ep) if err != nil { - return ep, extractHostFromHostPort(ep), CREDS_OPTIONAL + return ep, ep, CREDS_OPTIONAL } if url.Scheme == "http" || url.Scheme == "https" { - return url.Host, url.Hostname(), schemeToCredsRequirement(url.Scheme) + return url.Host, url.Host, schemeToCredsRequirement(url.Scheme) } - return ep, url.Hostname(), schemeToCredsRequirement(url.Scheme) + return ep, url.Host, schemeToCredsRequirement(url.Scheme) } // Handles plain addresses like 10.0.0.44:437. - return ep, extractHostFromHostPort(ep), CREDS_OPTIONAL + return ep, ep, CREDS_OPTIONAL } // RequiresCredentials returns whether given endpoint requires diff --git a/client/v3/internal/endpoint/endpoint_test.go b/client/v3/internal/endpoint/endpoint_test.go index bc6cd71399cc..7de9226ff19a 100644 --- a/client/v3/internal/endpoint/endpoint_test.go +++ b/client/v3/internal/endpoint/endpoint_test.go @@ -27,35 +27,35 @@ func Test_interpret(t *testing.T) { }{ {"127.0.0.1", "127.0.0.1", "127.0.0.1", CREDS_OPTIONAL}, {"localhost", "localhost", "localhost", CREDS_OPTIONAL}, - {"localhost:8080", "localhost:8080", "localhost", CREDS_OPTIONAL}, + {"localhost:8080", "localhost:8080", "localhost:8080", CREDS_OPTIONAL}, {"unix:127.0.0.1", "unix:127.0.0.1", "127.0.0.1", CREDS_OPTIONAL}, - {"unix:127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1", CREDS_OPTIONAL}, + {"unix:127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1:8080", CREDS_OPTIONAL}, {"unix://127.0.0.1", "unix:127.0.0.1", "127.0.0.1", CREDS_OPTIONAL}, - {"unix://127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1", CREDS_OPTIONAL}, + {"unix://127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1:8080", CREDS_OPTIONAL}, {"unixs:127.0.0.1", "unix:127.0.0.1", "127.0.0.1", CREDS_REQUIRE}, - {"unixs:127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1", CREDS_REQUIRE}, + {"unixs:127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1:8080", CREDS_REQUIRE}, {"unixs://127.0.0.1", "unix:127.0.0.1", "127.0.0.1", CREDS_REQUIRE}, - {"unixs://127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1", CREDS_REQUIRE}, + {"unixs://127.0.0.1:8080", "unix:127.0.0.1:8080", "127.0.0.1:8080", CREDS_REQUIRE}, {"http://127.0.0.1", "127.0.0.1", "127.0.0.1", CREDS_DROP}, - {"http://127.0.0.1:8080", "127.0.0.1:8080", "127.0.0.1", CREDS_DROP}, + {"http://127.0.0.1:8080", "127.0.0.1:8080", "127.0.0.1:8080", CREDS_DROP}, {"https://127.0.0.1", "127.0.0.1", "127.0.0.1", CREDS_REQUIRE}, - {"https://127.0.0.1:8080", "127.0.0.1:8080", "127.0.0.1", CREDS_REQUIRE}, - {"https://localhost:20000", "localhost:20000", "localhost", CREDS_REQUIRE}, + {"https://127.0.0.1:8080", "127.0.0.1:8080", "127.0.0.1:8080", CREDS_REQUIRE}, + {"https://localhost:20000", "localhost:20000", "localhost:20000", CREDS_REQUIRE}, {"unix:///tmp/abc", "unix:///tmp/abc", "abc", CREDS_OPTIONAL}, {"unixs:///tmp/abc", "unix:///tmp/abc", "abc", CREDS_REQUIRE}, - {"unix:///tmp/abc:1234", "unix:///tmp/abc:1234", "abc", CREDS_OPTIONAL}, - {"unixs:///tmp/abc:1234", "unix:///tmp/abc:1234", "abc", CREDS_REQUIRE}, + {"unix:///tmp/abc:1234", "unix:///tmp/abc:1234", "abc:1234", CREDS_OPTIONAL}, + {"unixs:///tmp/abc:1234", "unix:///tmp/abc:1234", "abc:1234", CREDS_REQUIRE}, {"etcd.io", "etcd.io", "etcd.io", CREDS_OPTIONAL}, {"http://etcd.io/abc", "etcd.io", "etcd.io", CREDS_DROP}, {"dns://something-other", "dns://something-other", "something-other", CREDS_OPTIONAL}, - {"http://[2001:db8:1f70::999:de8:7648:6e8]:100/", "[2001:db8:1f70::999:de8:7648:6e8]:100", "2001:db8:1f70::999:de8:7648:6e8", CREDS_DROP}, - {"[2001:db8:1f70::999:de8:7648:6e8]:100", "[2001:db8:1f70::999:de8:7648:6e8]:100", "2001:db8:1f70::999:de8:7648:6e8", CREDS_OPTIONAL}, + {"http://[2001:db8:1f70::999:de8:7648:6e8]:100/", "[2001:db8:1f70::999:de8:7648:6e8]:100", "[2001:db8:1f70::999:de8:7648:6e8]:100", CREDS_DROP}, + {"[2001:db8:1f70::999:de8:7648:6e8]:100", "[2001:db8:1f70::999:de8:7648:6e8]:100", "[2001:db8:1f70::999:de8:7648:6e8]:100", CREDS_OPTIONAL}, {"unix:unexpected-file_name#123$456", "unix:unexpected-file_name#123$456", "unexpected-file_name#123$456", CREDS_OPTIONAL}, } for _, tt := range tests { diff --git a/tests/e2e/ctl_v3_grpc_test.go b/tests/e2e/ctl_v3_grpc_test.go index 7059017dbfac..e00526a38dba 100644 --- a/tests/e2e/ctl_v3_grpc_test.go +++ b/tests/e2e/ctl_v3_grpc_test.go @@ -19,6 +19,7 @@ package e2e import ( "context" "fmt" + "net/url" "strings" "testing" "time" @@ -127,20 +128,21 @@ func TestAuthority(t *testing.T) { t.Fatalf("could not start etcd process cluster (%v)", err) } defer epc.Close() - endpoints := templateEndpoints(t, tc.clientURLPattern, epc) + endpoints := templateEndpoints(t, tc.clientURLPattern, epc) client, err := e2e.NewEtcdctl(cfg.Client, endpoints) assert.NoError(t, err) - err = client.Put(ctx, "foo", "bar", config.PutOptions{}) - if err != nil { - t.Fatal(err) + for i := 0; i < 100; i++ { + err = client.Put(ctx, "foo", "bar", config.PutOptions{}) + if err != nil { + t.Fatal(err) + } } testutils.ExecuteWithTimeout(t, 5*time.Second, func() { - assertAuthority(t, strings.ReplaceAll(tc.expectAuthorityPattern, "${MEMBER_PORT}", "20000"), epc) + assertAuthority(t, tc.expectAuthorityPattern, epc) }) }) - } } } @@ -156,26 +158,18 @@ func templateEndpoints(t *testing.T, pattern string, clus *e2e.EtcdProcessCluste return endpoints } -func assertAuthority(t *testing.T, expectAurhority string, clus *e2e.EtcdProcessCluster) { - var logs []e2e.LogsExpect - for _, proc := range clus.Procs { - logs = append(logs, proc.Logs()) - } - line := firstMatch(t, `http2: decoded hpack field header field ":authority"`, logs...) - line = strings.TrimSuffix(line, "\n") - line = strings.TrimSuffix(line, "\r") - expectLine := fmt.Sprintf(`http2: decoded hpack field header field ":authority" = %q`, expectAurhority) - assert.True(t, strings.HasSuffix(line, expectLine), fmt.Sprintf("Got %q expected suffix %q", line, expectLine)) -} +func assertAuthority(t *testing.T, expectAuthorityPattern string, clus *e2e.EtcdProcessCluster) { + for i := range clus.Procs { + line, _ := clus.Procs[i].Logs().ExpectWithContext(context.TODO(), `http2: decoded hpack field header field ":authority"`) + line = strings.TrimSuffix(line, "\n") + line = strings.TrimSuffix(line, "\r") -func firstMatch(t *testing.T, expectLine string, logs ...e2e.LogsExpect) string { - t.Helper() - match := make(chan string, len(logs)) - for i := range logs { - go func(l e2e.LogsExpect) { - line, _ := l.ExpectWithContext(context.TODO(), expectLine) - match <- line - }(logs[i]) + u, err := url.Parse(clus.Procs[i].EndpointsGRPC()[0]) + if err != nil { + t.Fatal(err) + } + expectAuthority := strings.ReplaceAll(expectAuthorityPattern, "${MEMBER_PORT}", u.Port()) + expectLine := fmt.Sprintf(`http2: decoded hpack field header field ":authority" = %q`, expectAuthority) + assert.True(t, strings.HasSuffix(line, expectLine), fmt.Sprintf("Got %q expected suffix %q", line, expectLine)) } - return <-match } diff --git a/tests/integration/grpc_test.go b/tests/integration/grpc_test.go index 618c63f71943..fcd56fbc1d91 100644 --- a/tests/integration/grpc_test.go +++ b/tests/integration/grpc_test.go @@ -103,12 +103,14 @@ func TestAuthority(t *testing.T) { defer kv.Close() putRequestMethod := "/etcdserverpb.KV/Put" - _, err := kv.Put(context.TODO(), "foo", "bar") - if err != nil { - t.Fatal(err) + for i := 0; i < 100; i++ { + _, err := kv.Put(context.TODO(), "foo", "bar") + if err != nil { + t.Fatal(err) + } } - assertAuthority(t, templateAuthority(t, tc.expectAuthorityPattern, clus.Members[0]), clus, putRequestMethod) + assertAuthority(t, tc.expectAuthorityPattern, clus, putRequestMethod) }) } } @@ -162,21 +164,23 @@ func templateAuthority(t *testing.T, pattern string, m *integration.Member) stri return authority } -func assertAuthority(t *testing.T, expectedAuthority string, clus *integration.Cluster, filterMethod string) { +func assertAuthority(t *testing.T, expectedAuthorityPattern string, clus *integration.Cluster, filterMethod string) { t.Helper() - requestsFound := 0 for _, m := range clus.Members { + requestsFound := 0 + expectedAuthority := templateAuthority(t, expectedAuthorityPattern, m) for _, r := range m.RecordedRequests() { if filterMethod != "" && r.FullMethod != filterMethod { continue } - requestsFound++ - if r.Authority != expectedAuthority { + if r.Authority == expectedAuthority { + requestsFound++ + } else { t.Errorf("Got unexpected authority header, member: %q, request: %q, got authority: %q, expected %q", m.Name, r.FullMethod, r.Authority, expectedAuthority) } } - } - if requestsFound == 0 { - t.Errorf("Expected at least one request") + if requestsFound == 0 { + t.Errorf("Expect at least one request with matched authority header value was recorded by the server intercepter on member %s but got 0", m.Name) + } } }