Avoid deleting peering secret UUIDs at dialers

Dialers do not keep track of peering secret UUIDs, so they should not
attempt to clean up data from that table when their peering is deleted.

We also now keep peer server addresses when marking peerings for
deletion. Peer server addresses are used by the ShouldDial() helper
when determining whether the peering is for a dialer or an acceptor.
We need to keep this data so that peering secrets can be cleaned up
accordingly.

agent/consul/state/peering.go

@@ -267,7 +267,7 @@ func (s *Store) peeringSecretsWriteTxn(tx WriteTxn, secret *pbpeering.PeeringSec
 		// Old establishment secret ID are always cleaned up when they don't match.
 		// They will either be replaced by a new one or deleted in the secret exchange RPC.
 		existingEstablishment := existing.GetEstablishment().GetSecretID()
-		if existingEstablishment != "" && existingEstablishment != secret.GetEstablishment().GetSecretID() {
+		if existingEstablishment != "" && secret.GetEstablishment().GetSecretID() != "" && existingEstablishment != secret.GetEstablishment().GetSecretID() {
 			toDelete = append(toDelete, existingEstablishment)
 		}
 
@@ -304,17 +304,17 @@ func (s *Store) peeringSecretsWriteTxn(tx WriteTxn, secret *pbpeering.PeeringSec
 	return nil
 }
 
-func (s *Store) PeeringSecretsDelete(idx uint64, peerID string) error {
+func (s *Store) PeeringSecretsDelete(idx uint64, peerID string, dialer bool) error {
 	tx := s.db.WriteTxn(idx)
 	defer tx.Abort()
 
-	if err := peeringSecretsDeleteTxn(tx, peerID); err != nil {
+	if err := peeringSecretsDeleteTxn(tx, peerID, dialer); err != nil {
 		return fmt.Errorf("failed to write peering secret: %w", err)
 	}
 	return tx.Commit()
 }
 
-func peeringSecretsDeleteTxn(tx WriteTxn, peerID string) error {
+func peeringSecretsDeleteTxn(tx WriteTxn, peerID string, dialer bool) error {
 	secretRaw, err := tx.First(tablePeeringSecrets, indexID, peerID)
 	if err != nil {
 		return fmt.Errorf("failed to fetch secret for peering: %w", err)
@@ -326,6 +326,11 @@ func peeringSecretsDeleteTxn(tx WriteTxn, peerID string) error {
 		return fmt.Errorf("failed to delete secret for peering: %w", err)
 	}
 
+	// Dialing peers do not track secrets in tablePeeringSecretUUIDs.
+	if dialer {
+		return nil
+	}
+
 	secrets, ok := secretRaw.(*pbpeering.PeeringSecrets)
 	if !ok {
 		return fmt.Errorf("invalid type %T", secretRaw)
@@ -520,7 +525,7 @@ func (s *Store) PeeringWrite(idx uint64, req *pbpeering.PeeringWriteRequest) err
 
 	// Ensure associated secrets are cleaned up when a peering is marked for deletion.
 	if req.Peering.State == pbpeering.PeeringState_DELETING {
-		if err := peeringSecretsDeleteTxn(tx, req.Peering.ID); err != nil {
+		if err := peeringSecretsDeleteTxn(tx, req.Peering.ID, req.Peering.ShouldDial()); err != nil {
 			return fmt.Errorf("failed to delete peering secrets: %w", err)
 		}
 	}

agent/consul/state/peering_test.go

@@ -58,7 +58,7 @@ func insertTestPeerings(t *testing.T, s *Store) {
 	require.NoError(t, tx.Commit())
 }
 
-func insertTestPeeringSecret(t *testing.T, s *Store, secret *pbpeering.PeeringSecrets) {
+func insertTestPeeringSecret(t *testing.T, s *Store, secret *pbpeering.PeeringSecrets, dialer bool) {
 	t.Helper()
 
 	tx := s.db.WriteTxn(0)
@@ -78,9 +78,12 @@ func insertTestPeeringSecret(t *testing.T, s *Store, secret *pbpeering.PeeringSe
 		uuids = append(uuids, active)
 	}
 
-	for _, id := range uuids {
-		err = tx.Insert(tablePeeringSecretUUIDs, id)
-		require.NoError(t, err)
+	// Dialing peers do not track secret UUIDs because they don't generate them.
+	if !dialer {
+		for _, id := range uuids {
+			err = tx.Insert(tablePeeringSecretUUIDs, id)
+			require.NoError(t, err)
+		}
 	}
 
 	require.NoError(t, tx.Commit())
@@ -182,7 +185,7 @@ func TestStateStore_PeeringSecretsRead(t *testing.T) {
 		Establishment: &pbpeering.PeeringSecrets_Establishment{
 			SecretID: testFooSecretID,
 		},
-	})
+	}, false)
 
 	type testcase struct {
 		name   string
@@ -499,40 +502,67 @@ func TestStore_PeeringSecretsWrite(t *testing.T) {
 }
 
 func TestStore_PeeringSecretsDelete(t *testing.T) {
-	s := NewStateStore(nil)
-	insertTestPeerings(t, s)
-
 	const (
 		establishmentID = "b4b9cbae-4bbd-454b-b7ae-441a5c89c3b9"
 		pendingID       = "0ba06390-bd77-4c52-8397-f88c0867157d"
 		activeID        = "0b8a3817-aca0-4c06-94b6-b0763a5cd013"
 	)
 
-	insertTestPeeringSecret(t, s, &pbpeering.PeeringSecrets{
-		PeerID: testFooPeerID,
-		Establishment: &pbpeering.PeeringSecrets_Establishment{
-			SecretID: establishmentID,
-		},
-		Stream: &pbpeering.PeeringSecrets_Stream{
-			PendingSecretID: pendingID,
-			ActiveSecretID:  activeID,
-		},
-	})
+	type testCase struct {
+		dialer bool
+		secret *pbpeering.PeeringSecrets
+	}
 
-	require.NoError(t, s.PeeringSecretsDelete(12, testFooPeerID))
+	run := func(t *testing.T, tc testCase) {
+		s := NewStateStore(nil)
 
-	// The secrets should be gone
-	secrets, err := s.PeeringSecretsRead(nil, testFooPeerID)
-	require.NoError(t, err)
-	require.Nil(t, secrets)
+		insertTestPeerings(t, s)
+		insertTestPeeringSecret(t, s, tc.secret, tc.dialer)
 
-	// The UUIDs should be free
-	uuids := []string{establishmentID, pendingID, activeID}
+		require.NoError(t, s.PeeringSecretsDelete(12, testFooPeerID, tc.dialer))
 
-	for _, id := range uuids {
-		free, err := s.ValidateProposedPeeringSecretUUID(id)
+		// The secrets should be gone
+		secrets, err := s.PeeringSecretsRead(nil, testFooPeerID)
 		require.NoError(t, err)
-		require.True(t, free)
+		require.Nil(t, secrets)
+
+		uuids := []string{establishmentID, pendingID, activeID}
+		for _, id := range uuids {
+			free, err := s.ValidateProposedPeeringSecretUUID(id)
+			require.NoError(t, err)
+			require.True(t, free)
+		}
+	}
+
+	tt := map[string]testCase{
+		"acceptor": {
+			dialer: false,
+			secret: &pbpeering.PeeringSecrets{
+				PeerID: testFooPeerID,
+				Establishment: &pbpeering.PeeringSecrets_Establishment{
+					SecretID: establishmentID,
+				},
+				Stream: &pbpeering.PeeringSecrets_Stream{
+					PendingSecretID: pendingID,
+					ActiveSecretID:  activeID,
+				},
+			},
+		},
+		"dialer": {
+			dialer: true,
+			secret: &pbpeering.PeeringSecrets{
+				PeerID: testFooPeerID,
+				Stream: &pbpeering.PeeringSecrets_Stream{
+					ActiveSecretID: activeID,
+				},
+			},
+		},
+	}
+
+	for name, tc := range tt {
+		t.Run(name, func(t *testing.T) {
+			run(t, tc)
+		})
 	}
 }
 

agent/rpc/peering/service.go

@@ -729,10 +729,11 @@ func (s *Server) PeeringDelete(ctx context.Context, req *pbpeering.PeeringDelete
 			// We only need to include the name and partition for the peering to be identified.
 			// All other data associated with the peering can be discarded because once marked
 			// for deletion the peering is effectively gone.
-			ID:        existing.ID,
-			Name:      req.Name,
-			State:     pbpeering.PeeringState_DELETING,
-			DeletedAt: structs.TimeToProto(time.Now().UTC()),
+			ID:                  existing.ID,
+			Name:                req.Name,
+			State:               pbpeering.PeeringState_DELETING,
+			PeerServerAddresses: existing.PeerServerAddresses,
+			DeletedAt:           structs.TimeToProto(time.Now().UTC()),
 
 			// PartitionOrEmpty is used to avoid writing "default" in OSS.
 			Partition: entMeta.PartitionOrEmpty(),