diff --git a/agent/consul/state/peering.go b/agent/consul/state/peering.go index b3e230a70..4a5c84e52 100644 --- a/agent/consul/state/peering.go +++ b/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) } } diff --git a/agent/consul/state/peering_test.go b/agent/consul/state/peering_test.go index 73fee261f..1e7a2ccf1 100644 --- a/agent/consul/state/peering_test.go +++ b/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) + }) } } diff --git a/agent/rpc/peering/service.go b/agent/rpc/peering/service.go index 79ae815e1..4ccf4c206 100644 --- a/agent/rpc/peering/service.go +++ b/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(),