Repository: nats-io/nats-server
Pull request #5078 (2024-02-14 01:06:38)

Change Account.TraceDest to private and add getter/setter
Update the tests accordingly.

This change is ok since the message tracing feature is targeted for v2.11.0 which has not been released yet.

Related to #5057

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>


Patch:
diff --git a/server/accounts.go b/server/accounts.go
index 56795ae3..ddcb87f7 100644
--- a/server/accounts.go
+++ b/server/accounts.go
@@ -54,7 +54,6 @@ type Account struct {
 	Name         string
 	Nkey         string
 	Issuer       string
-	TraceDest    string
 	claimJWT     string
 	updated      time.Time
 	mu           sync.RWMutex
@@ -97,6 +96,7 @@ type Account struct {
 	nameTag      string
 	lastLimErr   int64
 	routePoolIdx int
+	traceDest    string
 }
 
 const (
@@ -256,12 +256,28 @@ func (a *Account) String() string {
 	return a.Name
 }
 
+func (a *Account) setTraceDest(dest string) {
+	a.mu.Lock()
+	a.traceDest = dest
+	a.mu.Unlock()
+}
+
+func (a *Account) getTraceDest() string {
+	a.mu.RLock()
+	dest := a.traceDest
+	a.mu.RUnlock()
+	return dest
+}
+
 // Used to create shallow copies of accounts for transfer
 // from opts to real accounts in server struct.
+// Account `na` write lock is expected to be held on entry
+// while account `a` is the one from the Options struct
+// being loaded/reloaded and do not need locking.
 func (a *Account) shallowCopy(na *Account) {
 	na.Nkey = a.Nkey
 	na.Issuer = a.Issuer
-	na.TraceDest = a.TraceDest
+	na.traceDest = a.traceDest
 
 	if a.imports.streams != nil {
 		na.imports.streams = make([]*streamImport, 0, len(a.imports.streams))
diff --git a/server/msgtrace.go b/server/msgtrace.go
index 12358c2f..94a27ace 100644
--- a/server/msgtrace.go
+++ b/server/msgtrace.go
@@ -436,9 +436,7 @@ func (c *client) initMsgTrace() *msgTrace {
 	// otherwise, we are not enabling tracing.
 	if external {
 		if acc != nil {
-			acc.mu.RLock()
-			dest = acc.TraceDest
-			acc.mu.RUnlock()
+			dest = acc.getTraceDest()
 		}
 		if dest == _EMPTY_ {
 			// No account destination, no tracing for external trace headers.
diff --git a/server/opts.go b/server/opts.go
index 22bd507c..c9b147e9 100644
--- a/server/opts.go
+++ b/server/opts.go
@@ -3032,7 +3032,7 @@ func parseAccounts(v interface{}, opts *Options, errors *[]error, warnings *[]er
 						*errors = append(*errors, err)
 						continue
 					}
-					acc.TraceDest = td
+					acc.traceDest = td
 				default:
 					if !tk.IsUsedVariable() {
 						err := &unknownConfigFieldErr{
diff --git a/server/server.go b/server/server.go
index 15c67c5b..f437cbce 100644
--- a/server/server.go
+++ b/server/server.go
@@ -1046,15 +1046,6 @@ func validateOptions(o *Options) error {
 		return fmt.Errorf("max_payload (%v) cannot be higher than max_pending (%v)",
 			o.MaxPayload, o.MaxPending)
 	}
-	// Check that account's trace_dest is a valid subject.
-	for _, acc := range o.Accounts {
-		if acc.TraceDest == _EMPTY_ {
-			continue
-		}
-		if !IsValidPublishSubject(acc.TraceDest) {
-			return fmt.Errorf("trace_dest %q of account %q is not valid", acc.TraceDest, acc.Name)
-		}
-	}
 	// Check that the trust configuration is correct.
 	if err := validateTrustedOperators(o); err != nil {
 		return err


Test patch:
diff --git a/server/accounts_test.go b/server/accounts_test.go
index f586af0f..0f8c403e 100644
--- a/server/accounts_test.go
+++ b/server/accounts_test.go
@@ -550,7 +550,7 @@ func TestAccountParseConfig(t *testing.T) {
 	if natsAcc == nil {
 		t.Fatalf("Error retrieving account for 'nats.io'")
 	}
-	require_Equal[string](t, natsAcc.TraceDest, traceDest)
+	require_Equal[string](t, natsAcc.getTraceDest(), traceDest)
 
 	for _, u := range opts.Users {
 		if u.Username == "derek" {
diff --git a/server/msgtrace_test.go b/server/msgtrace_test.go
index 37d79dae..b6ced07e 100644
--- a/server/msgtrace_test.go
+++ b/server/msgtrace_test.go
@@ -3529,9 +3529,7 @@ func TestMsgTraceJetStreamWithSuperCluster(t *testing.T) {
 		for _, s := range cl.servers {
 			acc, err := s.LookupAccount(globalAccountName)
 			require_NoError(t, err)
-			acc.mu.Lock()
-			acc.TraceDest = traceDest
-			acc.mu.Unlock()
+			acc.setTraceDest(traceDest)
 		}
 	}
 
@@ -4598,32 +4596,4 @@ func TestMsgTraceTriggeredByExternalHeader(t *testing.T) {
 			}
 		})
 	}
-
-	nc1.Close()
-	nc2.Close()
-	s2.Shutdown()
-	s1.Shutdown()
-	s1 = nil
-
-	o1.Accounts = o1.Accounts[:1]
-	accName := o1.Accounts[0].Name
-	o1.Port, o1.Cluster.Port = -1, -1
-
-	for _, test := range []struct {
-		name      string
-		traceDest string
-	}{
-		{"account trace invalid subject", "invalid..subject"},
-		{"account trace invalid publish subject", "invalid.publish.*.subject"},
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			o1.Accounts[0].TraceDest = test.traceDest
-			s1, err := NewServer(o1)
-			if err == nil || !strings.Contains(err.Error(), fmt.Sprintf("trace_dest %q of account %q is not valid", test.traceDest, accName)) {
-				if s1 != nil {
-					s1.Shutdown()
-				}
-			}
-		})
-	}
 }