From 1362711a5a5bb230d75d592d37ab25fd7f1df12f Mon Sep 17 00:00:00 2001
From: Easwar Swaminathan <easwars@google.com>
Date: Fri, 8 Jul 2022 12:58:33 -0700
Subject: [PATCH] xdsclient: handle empty authority in new style resource names

---
 xds/internal/xdsclient/xdsresource/name.go    |  2 +-
 .../xdsclient/xdsresource/name_test.go        | 86 ++++++++++++-------
 2 files changed, 57 insertions(+), 31 deletions(-)

diff --git a/xds/internal/xdsclient/xdsresource/name.go b/xds/internal/xdsclient/xdsresource/name.go
index eb1ee323cee9..80c0efd37b39 100644
--- a/xds/internal/xdsclient/xdsresource/name.go
+++ b/xds/internal/xdsclient/xdsresource/name.go
@@ -119,7 +119,7 @@ func (n *Name) String() string {
 
 	path := n.Type
 	if n.ID != "" {
-		path = path + "/" + n.ID
+		path = "/" + path + "/" + n.ID
 	}
 
 	tempURL := &url.URL{
diff --git a/xds/internal/xdsclient/xdsresource/name_test.go b/xds/internal/xdsclient/xdsresource/name_test.go
index 8ef9d894840c..cfab669c54bc 100644
--- a/xds/internal/xdsclient/xdsresource/name_test.go
+++ b/xds/internal/xdsclient/xdsresource/name_test.go
@@ -18,54 +18,76 @@
 package xdsresource
 
 import (
-	"reflect"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"google.golang.org/grpc/internal/envconfig"
 )
 
 func TestParseName(t *testing.T) {
 	tests := []struct {
-		name string
-		env  bool // Whether federation env is set to true.
-		in   string
-		want *Name
+		name    string
+		env     bool // Whether federation env is set to true.
+		in      string
+		want    *Name
+		wantStr string
 	}{
 		{
-			name: "env off",
-			env:  false,
-			in:   "xdstp://auth/type/id",
-			want: &Name{ID: "xdstp://auth/type/id"},
+			name:    "env off",
+			env:     false,
+			in:      "xdstp://auth/type/id",
+			want:    &Name{ID: "xdstp://auth/type/id"},
+			wantStr: "xdstp://auth/type/id",
 		},
 		{
-			name: "old style name",
-			env:  true,
-			in:   "test-resource",
-			want: &Name{ID: "test-resource"},
+			name:    "old style name",
+			env:     true,
+			in:      "test-resource",
+			want:    &Name{ID: "test-resource"},
+			wantStr: "test-resource",
 		},
 		{
-			name: "invalid not url",
-			env:  true,
-			in:   "a:/b/c",
-			want: &Name{ID: "a:/b/c"},
+			name:    "invalid not url",
+			env:     true,
+			in:      "a:/b/c",
+			want:    &Name{ID: "a:/b/c"},
+			wantStr: "a:/b/c",
 		},
 		{
-			name: "invalid no resource type",
-			env:  true,
-			in:   "xdstp://auth/id",
-			want: &Name{ID: "xdstp://auth/id"},
+			name:    "invalid no resource type",
+			env:     true,
+			in:      "xdstp://auth/id",
+			want:    &Name{ID: "xdstp://auth/id"},
+			wantStr: "xdstp://auth/id",
 		},
 		{
-			name: "valid no ctx params",
-			env:  true,
-			in:   "xdstp://auth/type/id",
-			want: &Name{Scheme: "xdstp", Authority: "auth", Type: "type", ID: "id"},
+			name:    "valid with no authority",
+			env:     true,
+			in:      "xdstp:///type/id",
+			want:    &Name{Scheme: "xdstp", Authority: "", Type: "type", ID: "id"},
+			wantStr: "xdstp:///type/id",
 		},
 		{
-			name: "valid with ctx params",
-			env:  true,
-			in:   "xdstp://auth/type/id?a=1&b=2",
-			want: &Name{Scheme: "xdstp", Authority: "auth", Type: "type", ID: "id", ContextParams: map[string]string{"a": "1", "b": "2"}},
+			name:    "valid no ctx params",
+			env:     true,
+			in:      "xdstp://auth/type/id",
+			want:    &Name{Scheme: "xdstp", Authority: "auth", Type: "type", ID: "id"},
+			wantStr: "xdstp://auth/type/id",
+		},
+		{
+			name:    "valid with ctx params",
+			env:     true,
+			in:      "xdstp://auth/type/id?a=1&b=2",
+			want:    &Name{Scheme: "xdstp", Authority: "auth", Type: "type", ID: "id", ContextParams: map[string]string{"a": "1", "b": "2"}},
+			wantStr: "xdstp://auth/type/id?a=1&b=2",
+		},
+		{
+			name:    "valid with ctx params sorted by keys",
+			env:     true,
+			in:      "xdstp://auth/type/id?b=2&a=1",
+			want:    &Name{Scheme: "xdstp", Authority: "auth", Type: "type", ID: "id", ContextParams: map[string]string{"a": "1", "b": "2"}},
+			wantStr: "xdstp://auth/type/id?a=1&b=2",
 		},
 	}
 	for _, tt := range tests {
@@ -77,9 +99,13 @@ func TestParseName(t *testing.T) {
 					return func() { envconfig.XDSFederation = oldEnv }
 				}()()
 			}
-			if got := ParseName(tt.in); !reflect.DeepEqual(got, tt.want) {
+			got := ParseName(tt.in)
+			if !cmp.Equal(got, tt.want, cmpopts.IgnoreFields(Name{}, "processingDirective")) {
 				t.Errorf("ParseName() = %#v, want %#v", got, tt.want)
 			}
+			if gotStr := got.String(); gotStr != tt.wantStr {
+				t.Errorf("Name.String() = %s, want %s", gotStr, tt.wantStr)
+			}
 		})
 	}
 }