Use non-redacted URLs when computing remote identity

Also, during configuration parsing, the duplicate detection will
work but possibly before non normalized URLs (say without default
leafnode port). But NewServer() will call validateOptions which
will re-check for duplicates (after normalization).

These changes have removed the caching of the remote identity
in the RemoteLeafOpts object.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>

Author: kozlovic

server/leafnode.go

@@ -218,7 +218,7 @@ func validateLeafNode(o *Options) error {
 			}
 			rn := r.name()
 			if _, dup := names[rn]; dup {
-				return fmt.Errorf("duplicate remote %s", rn)
+				return fmt.Errorf("duplicate remote %s", r.safeName())
 			}
 			names[rn] = struct{}{}
 		}

server/opts.go

@@ -312,35 +312,36 @@ type RemoteLeafOpts struct {
 	// existing connection will be closed and not solicited again (until it is changed
 	// to `false` again.
 	Disabled bool `json:"-"`
-
-	// Internal fields
-
-	// This is a string representation of the remote, containing the URLs (with
-	// password redacted in case it is used in logging), the account (or "$G" if
-	// LocalAccount is empty) and possibly the Credentials file name.
-	// This should not be accessed directly and instead through name() since
-	// the value will originally be empty and be set the first time name()
-	// is invoked. Also, if in the future we add a public `Name` field, that field
-	// could be returned instead of the internal representation.
-	iname string
 }
 
 // Returns a string representation of this `RemoteLeafOpts` object, containing
-// the URLs (redacted), the account (or "$G" if none is specified) and, if present,
+// the URLs (unredacted), the account (or "$G" if none is specified) and, if present,
 // the credentials filename.
 func (r *RemoteLeafOpts) name() string {
-	if r.iname == _EMPTY_ {
-		acc := r.LocalAccount
-		if acc == _EMPTY_ {
-			acc = globalAccountName
-		}
-		var creds string
-		if c := r.Credentials; c != _EMPTY_ {
-			creds = fmt.Sprintf(", credentials=%q", c)
-		}
-		r.iname = fmt.Sprintf("urls=%q, account=%q%s", redactURLList(r.URLs), acc, creds)
+	return generateRemoteLeafOptsName(r, false)
+}
+
+// Same than RemoteLeafOpts.name() but uses redacted URLs. This is to be used for logging.
+func (r *RemoteLeafOpts) safeName() string {
+	return generateRemoteLeafOptsName(r, true)
+}
+
+func generateRemoteLeafOptsName(r *RemoteLeafOpts, redacted bool) string {
+	acc := r.LocalAccount
+	if acc == _EMPTY_ {
+		acc = globalAccountName
+	}
+	var creds string
+	if c := r.Credentials; c != _EMPTY_ {
+		creds = fmt.Sprintf(", credentials=%q", c)
+	}
+	var urls []*url.URL
+	if redacted {
+		urls = redactURLList(r.URLs)
+	} else {
+		urls = r.URLs
 	}
-	return r.iname
+	return fmt.Sprintf("urls=%q, account=%q%s", urls, acc, creds)
 }
 
 // JSLimitOpts are active limits for the meta cluster
@@ -3196,7 +3197,7 @@ func parseRemoteLeafNodes(v any, errors *[]error, warnings *[]error) ([]*RemoteL
 		}
 		rn := remote.name()
 		if _, dup := names[rn]; dup {
-			*errors = append(*errors, &configErr{tk, fmt.Sprintf("duplicate remote %s", rn)})
+			*errors = append(*errors, &configErr{tk, fmt.Sprintf("duplicate remote %s", remote.safeName())})
 			continue
 		}
 		names[rn] = struct{}{}

server/opts_test.go

@@ -2425,8 +2425,6 @@ func TestParsingLeafNodeRemotes(t *testing.T) {
 		}
 		u, _ := url.Parse("nats-leaf://127.0.0.1:2222")
 		expected.URLs = append(expected.URLs, u)
-		// Force creation of the name for `expected`
-		expected.name()
 		if !reflect.DeepEqual(opts.LeafNode.Remotes[0], expected) {
 			t.Fatalf("Expected %v, got %v", expected, opts.LeafNode.Remotes[0])
 		}
@@ -2463,8 +2461,6 @@ func TestParsingLeafNodeRemotes(t *testing.T) {
 		}
 		u, _ := url.Parse("nats-leaf://127.0.0.1:2222")
 		expected.URLs = append(expected.URLs, u)
-		// Force creation of the name for `expected`
-		expected.name()
 		if !reflect.DeepEqual(opts.LeafNode.Remotes[0], expected) {
 			t.Fatalf("Expected %v, got %v", expected, opts.LeafNode.Remotes[0])
 		}
@@ -2608,10 +2604,6 @@ func TestParsingLeafNodeRemotes(t *testing.T) {
 				JetStreamClusterMigrate: false,
 			},
 		}
-		// Force creation of the name for `expected`
-		for _, e := range expected {
-			e.name()
-		}
 		if !reflect.DeepEqual(opts.LeafNode.Remotes, expected) {
 			t.Fatalf("Expected %v, got %v", expected, opts.LeafNode.Remotes)
 		}
@@ -4511,32 +4503,37 @@ func TestOptionsRemoteLeafNodeName(t *testing.T) {
 	require_NoError(t, err)
 	u2, err := url.Parse("nats://user2:secretpwd@127.0.0.1:7222")
 	require_NoError(t, err)
-	// We expect redacted versions of the URLs
-	urls := []string{"nats://user1:xxxxx@127.0.0.1:7222", "nats://user2:xxxxx@127.0.0.1:7222"}
+	// With unredacted versions of the URLs
+	urls := []*url.URL{u1, u2}
+	safeURLs := redactURLList(urls)
 	for _, test := range []struct {
-		name   string
-		input  *RemoteLeafOpts
-		output string
+		name       string
+		input      *RemoteLeafOpts
+		output     string
+		safeOutput string
 	}{
 		{
 			"url only", &RemoteLeafOpts{
 				URLs: []*url.URL{u1, u2},
 			},
 			fmt.Sprintf("urls=%q, account=%q", urls, globalAccountName),
+			fmt.Sprintf("urls=%q, account=%q", safeURLs, globalAccountName),
 		},
 		{
 			"url with account", &RemoteLeafOpts{
 				URLs:         []*url.URL{u1, u2},
 				LocalAccount: "A",
 			},
 			fmt.Sprintf("urls=%q, account=%q", urls, "A"),
+			fmt.Sprintf("urls=%q, account=%q", safeURLs, "A"),
 		},
 		{
 			"url with credentials", &RemoteLeafOpts{
 				URLs:        []*url.URL{u1, u2},
 				Credentials: "credsfile",
 			},
 			fmt.Sprintf("urls=%q, account=%q, credentials=%q", urls, globalAccountName, "credsfile"),
+			fmt.Sprintf("urls=%q, account=%q, credentials=%q", safeURLs, globalAccountName, "credsfile"),
 		},
 		{
 			"url with account and credentials", &RemoteLeafOpts{
@@ -4545,18 +4542,21 @@ func TestOptionsRemoteLeafNodeName(t *testing.T) {
 				Credentials:  "credsfile",
 			},
 			fmt.Sprintf("urls=%q, account=%q, credentials=%q", urls, "A", "credsfile"),
+			fmt.Sprintf("urls=%q, account=%q, credentials=%q", safeURLs, "A", "credsfile"),
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			name := test.input.name()
 			require_Equal(t, name, test.output)
+			name = test.input.safeName()
+			require_Equal(t, name, test.safeOutput)
 		})
 	}
 
 	// Because we use `%q` when building the name, those two will have different
 	// names:
-	// r1=urls=["nats://user1:xxxxx@127.0.0.1:7222"], account="A", credentials="creds"
-	// r2=urls=["nats://user1:xxxxx@127.0.0.1:7222"], account="A\", credentials=\"creds"
+	// r1=urls=["nats://user1:secretpwd@127.0.0.1:7222"], account="A", credentials="creds"
+	// r2=urls=["nats://user1:secretpwd@127.0.0.1:7222"], account="A\", credentials=\"creds"
 	r1 := &RemoteLeafOpts{URLs: []*url.URL{u1}, LocalAccount: "A", Credentials: "creds"}
 	r2 := &RemoteLeafOpts{URLs: []*url.URL{u1}, LocalAccount: `A", credentials="creds`}
 	require_False(t, r1.name() == r2.name())

server/reload.go

@@ -944,7 +944,7 @@ DO_REMOTES:
 		// list. If it is found, that function returns an updated list
 		// with the element removed from it.
 		lrc.RLock()
-		rlo, nlo.added = getRemoteLeafOpts(lrc.RemoteLeafOpts, nlo.added)
+		rlo, nlo.added = getRemoteLeafOpts(lrc.RemoteLeafOpts.name(), nlo.added)
 		if rlo == nil {
 			// Not found, will be removed in leafNodeOption.Apply().
 			removed = true
@@ -964,7 +964,7 @@ DO_REMOTES:
 		if err != nil {
 			lrc.RUnlock()
 			s.mu.RUnlock()
-			return nil, fmt.Errorf(remoteErrFormat, rlo.name(), err)
+			return nil, fmt.Errorf(remoteErrFormat, rlo.safeName(), err)
 		}
 		disabledChanged := lrc.Disabled != rlo.Disabled
 		// If this remote was disabled and is now enabled, we need to make sure
@@ -976,7 +976,7 @@ DO_REMOTES:
 			if failed++; failed < maxAttempts {
 				goto DO_REMOTES
 			}
-			return nil, fmt.Errorf(remoteErrFormat, rlo.name(),
+			return nil, fmt.Errorf(remoteErrFormat, rlo.safeName(),
 				"cannot be enabled at the moment, try again")
 		}
 		// Since we will use the new `rlo.TLSConfig` later on, consider all
@@ -1005,7 +1005,7 @@ DO_REMOTES:
 				if failed++; failed < maxAttempts {
 					goto DO_REMOTES
 				}
-				return nil, fmt.Errorf(remoteErrFormat, rlo.name(),
+				return nil, fmt.Errorf(remoteErrFormat, rlo.safeName(),
 					"cannot be added at the moment, try again")
 			}
 		}
@@ -1050,12 +1050,12 @@ func usersHaveChanged(ousers, nusers []*User) bool {
 	return false
 }
 
-// Given the `search` remote leafnode options, search for a match in the `list`.
+// Given the `search` remote leafnode options name, searches for a match in the `list`.
 // If found, returns the `*RemoteLeafOpts` from the list, and the updated list
 // without the element in it. If not found, returns `nil` and the unmodified list.
-func getRemoteLeafOpts(search *RemoteLeafOpts, list []*RemoteLeafOpts) (*RemoteLeafOpts, []*RemoteLeafOpts) {
+func getRemoteLeafOpts(search string, list []*RemoteLeafOpts) (*RemoteLeafOpts, []*RemoteLeafOpts) {
 	for i, rlo := range list {
-		if search.name() == rlo.name() {
+		if search == rlo.name() {
 			lastIdx := len(list) - 1
 			if lastIdx == 0 {
 				return rlo, nil
@@ -1098,7 +1098,7 @@ func (l *leafNodeOption) Apply(s *Server) {
 			}
 			s.rmLeafRemoteCfgs[lrc.name()] = lrc
 			lrc.markAsRemoved()
-			s.Noticef("Reloaded: LeafNode Remote %s removed", lrc.RemoteLeafOpts.name())
+			s.Noticef("Reloaded: LeafNode Remote %s removed", lrc.RemoteLeafOpts.safeName())
 			// We will close the existing connection in the next for-loop.
 			continue
 		}
@@ -1109,12 +1109,12 @@ func (l *leafNodeOption) Apply(s *Server) {
 		if rlo.tlsFirstChanged {
 			lrc.TLSHandshakeFirst = rlo.opts.TLSHandshakeFirst
 			s.Noticef("Reloaded: LeafNode Remote %s TLS HandshakeFirst value is: %v",
-				lrc.RemoteLeafOpts.name(), rlo.opts.TLSHandshakeFirst)
+				lrc.RemoteLeafOpts.safeName(), rlo.opts.TLSHandshakeFirst)
 		}
 		if rlo.compressionChanged {
 			lrc.Compression = rlo.opts.Compression
 			s.Noticef("Reloaded: LeafNode Remote %s Compression value is: %v",
-				lrc.RemoteLeafOpts.name(), rlo.opts.Compression)
+				lrc.RemoteLeafOpts.safeName(), rlo.opts.Compression)
 		}
 		if rlo.disabledChanged {
 			// Change to new value.
@@ -1125,7 +1125,7 @@ func (l *leafNodeOption) Apply(s *Server) {
 				enable = append(enable, lrc)
 			}
 			s.Noticef("Reloaded: LeafNode Remote %s Disabled value is: %v",
-				lrc.RemoteLeafOpts.name(), rlo.opts.Disabled)
+				lrc.RemoteLeafOpts.safeName(), rlo.opts.Disabled)
 		}
 		lrc.Unlock()
 	}

server/reload_test.go

@@ -6494,7 +6494,7 @@ func TestConfigReloadGetRemoteLeafOpts(t *testing.T) {
 		{"list of three and found at pos 3", rlo3, []*RemoteLeafOpts{rlo1, rlo2, rlo3}, rlo3, []*RemoteLeafOpts{rlo1, rlo2}},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			rlo, list := getRemoteLeafOpts(test.search, test.listIn)
+			rlo, list := getRemoteLeafOpts(test.search.name(), test.listIn)
 			require_True(t, reflect.DeepEqual(list, test.listOut))
 			require_Equal(t, rlo, test.result)
 		})
@@ -6796,7 +6796,7 @@ func TestConfigReloadGetLeafNodeOptionsChanges(t *testing.T) {
 				return ts, old, new
 			},
 			nil,
-			fmt.Sprintf("remote %s: field %q: old=false, new=true", remote.name(), "Hub"),
+			fmt.Sprintf("remote %s: field %q: old=false, new=true", remote.safeName(), "Hub"),
 		},
 		{
 			"remote tlsfirst changed",
@@ -6967,7 +6967,7 @@ func TestConfigReloadLeafNodeUnsupportedChangesFail(t *testing.T) {
 
 	remote := o2.LeafNode.Remotes[0]
 	require_NotNil(t, remote)
-	remoteName := remote.name()
+	remoteName := remote.safeName()
 	require_NotEqual(t, remoteName, _EMPTY_)
 
 	checkConfigReload := func(content []byte, errTxt string) {