From 650e48624df0d5f7dcc9dabf088709f87c2390a9 Mon Sep 17 00:00:00 2001 From: freddygv Date: Fri, 26 Aug 2022 10:52:47 -0600 Subject: [PATCH] Allow terminated peerings to be deleted Peerings are terminated when a peer decides to delete the peering from their end. Deleting a peering sends a termination message to the peer and triggers them to mark the peering as terminated but does NOT delete the peering itself. This is to prevent peerings from disappearing from both sides just because one side deleted them. Previously the Delete endpoint was skipping the deletion if the peering was not marked as active. However, terminated peerings are also inactive. This PR makes some updates so that peerings marked as terminated can be deleted by users. --- agent/consul/state/peering.go | 23 ++- agent/consul/state/peering_test.go | 228 +++++++++++++++++++++++++---- agent/rpc/peering/service.go | 3 +- agent/rpc/peering/service_test.go | 64 ++++---- 4 files changed, 258 insertions(+), 60 deletions(-) diff --git a/agent/consul/state/peering.go b/agent/consul/state/peering.go index 287e822919..9457dd811a 100644 --- a/agent/consul/state/peering.go +++ b/agent/consul/state/peering.go @@ -549,8 +549,25 @@ func (s *Store) PeeringWrite(idx uint64, req *pbpeering.PeeringWriteRequest) err if req.Peering.ID != existing.ID { return fmt.Errorf("A peering already exists with the name %q and a different ID %q", req.Peering.Name, existing.ID) } + + // Nothing to do if our peer wants to terminate the peering but the peering is already marked for deletion. + if existing.State == pbpeering.PeeringState_DELETING && req.Peering.State == pbpeering.PeeringState_TERMINATED { + return nil + } + + // No-op deletion + if existing.State == pbpeering.PeeringState_DELETING && req.Peering.State == pbpeering.PeeringState_DELETING { + return nil + } + + // No-op termination + if existing.State == pbpeering.PeeringState_TERMINATED && req.Peering.State == pbpeering.PeeringState_TERMINATED { + return nil + } + // Prevent modifications to Peering marked for deletion. - if !existing.IsActive() { + // This blocks generating new peering tokens or re-establishing the peering until the peering is done deleting. + if existing.State == pbpeering.PeeringState_DELETING { return fmt.Errorf("cannot write to peering that is marked for deletion") } @@ -582,8 +599,8 @@ func (s *Store) PeeringWrite(idx uint64, req *pbpeering.PeeringWriteRequest) err req.Peering.ModifyIndex = idx } - // Ensure associated secrets are cleaned up when a peering is marked for deletion. - if req.Peering.State == pbpeering.PeeringState_DELETING { + // Ensure associated secrets are cleaned up when a peering is marked for deletion or terminated. + if !req.Peering.IsActive() { 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 bfce75295c..1dc2446fe1 100644 --- a/agent/consul/state/peering_test.go +++ b/agent/consul/state/peering_test.go @@ -1112,16 +1112,22 @@ func TestStore_PeeringWrite(t *testing.T) { // Each case depends on the previous. s := NewStateStore(nil) + testTime := time.Now() + + type expectations struct { + peering *pbpeering.Peering + secrets *pbpeering.PeeringSecrets + err string + } type testcase struct { - name string - input *pbpeering.PeeringWriteRequest - expectSecrets *pbpeering.PeeringSecrets - expectErr string + name string + input *pbpeering.PeeringWriteRequest + expect expectations } run := func(t *testing.T, tc testcase) { err := s.PeeringWrite(10, tc.input) - if tc.expectErr != "" { - testutil.RequireErrorContains(t, err, tc.expectErr) + if tc.expect.err != "" { + testutil.RequireErrorContains(t, err, tc.expect.err) return } require.NoError(t, err) @@ -1133,57 +1139,185 @@ func TestStore_PeeringWrite(t *testing.T) { _, p, err := s.PeeringRead(nil, q) require.NoError(t, err) require.NotNil(t, p) - require.Equal(t, tc.input.Peering.State, p.State) - require.Equal(t, tc.input.Peering.Name, p.Name) + require.Equal(t, tc.expect.peering.State, p.State) + require.Equal(t, tc.expect.peering.Name, p.Name) + require.Equal(t, tc.expect.peering.Meta, p.Meta) + if tc.expect.peering.DeletedAt != nil { + require.Equal(t, tc.expect.peering.DeletedAt, p.DeletedAt) + } secrets, err := s.PeeringSecretsRead(nil, tc.input.Peering.ID) require.NoError(t, err) - prototest.AssertDeepEqual(t, tc.expectSecrets, secrets) + prototest.AssertDeepEqual(t, tc.expect.secrets, secrets) } tcs := []testcase{ { name: "create baz", input: &pbpeering.PeeringWriteRequest{ Peering: &pbpeering.Peering{ - ID: testBazPeerID, - Name: "baz", - Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_ESTABLISHING, + PeerServerAddresses: []string{"localhost:8502"}, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), }, SecretsRequest: &pbpeering.SecretsWriteRequest{ PeerID: testBazPeerID, - Request: &pbpeering.SecretsWriteRequest_GenerateToken{ - GenerateToken: &pbpeering.SecretsWriteRequest_GenerateTokenRequest{ - EstablishmentSecret: testBazSecretID, + Request: &pbpeering.SecretsWriteRequest_Establish{ + Establish: &pbpeering.SecretsWriteRequest_EstablishRequest{ + ActiveStreamSecret: testBazSecretID, }, }, }, }, - expectSecrets: &pbpeering.PeeringSecrets{ - PeerID: testBazPeerID, - Establishment: &pbpeering.PeeringSecrets_Establishment{ - SecretID: testBazSecretID, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_ESTABLISHING, }, + secrets: &pbpeering.PeeringSecrets{ + PeerID: testBazPeerID, + Stream: &pbpeering.PeeringSecrets_Stream{ + ActiveSecretID: testBazSecretID, + }, + }, + }, + }, + { + name: "cannot change ID for baz", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: "123", + Name: "baz", + State: pbpeering.PeeringState_FAILING, + PeerServerAddresses: []string{"localhost:8502"}, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + err: `A peering already exists with the name "baz" and a different ID`, }, }, { name: "update baz", input: &pbpeering.PeeringWriteRequest{ Peering: &pbpeering.Peering{ - ID: testBazPeerID, - Name: "baz", - State: pbpeering.PeeringState_FAILING, + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_FAILING, + PeerServerAddresses: []string{"localhost:8502"}, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_FAILING, + }, + secrets: &pbpeering.PeeringSecrets{ + PeerID: testBazPeerID, + Stream: &pbpeering.PeeringSecrets_Stream{ + ActiveSecretID: testBazSecretID, + }, + }, + }, + }, + { + name: "if no state was included in request it is inherited from existing", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + // Send undefined state. + // State: pbpeering.PeeringState_FAILING, + PeerServerAddresses: []string{"localhost:8502"}, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + // Previous failing state is picked up. + State: pbpeering.PeeringState_FAILING, + }, + secrets: &pbpeering.PeeringSecrets{ + PeerID: testBazPeerID, + Stream: &pbpeering.PeeringSecrets_Stream{ + ActiveSecretID: testBazSecretID, + }, + }, + }, + }, + { + name: "mark baz as terminated", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_TERMINATED, + PeerServerAddresses: []string{"localhost:8502"}, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_TERMINATED, + }, + // Secrets for baz should have been deleted + secrets: nil, + }, + }, + { + name: "cannot edit data during no-op termination", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_TERMINATED, + // Attempt to modify the addresses + Meta: map[string]string{"foo": "bar"}, Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), }, }, - expectSecrets: &pbpeering.PeeringSecrets{ - PeerID: testBazPeerID, - Establishment: &pbpeering.PeeringSecrets_Establishment{ - SecretID: testBazSecretID, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_TERMINATED, + // Meta should be unchanged. + Meta: nil, }, }, }, { name: "mark baz for deletion", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_DELETING, + PeerServerAddresses: []string{"localhost:8502"}, + DeletedAt: structs.TimeToProto(testTime), + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_DELETING, + DeletedAt: structs.TimeToProto(testTime), + }, + secrets: nil, + }, + }, + { + name: "deleting a deleted peering is a no-op", input: &pbpeering.PeeringWriteRequest{ Peering: &pbpeering.Peering{ ID: testBazPeerID, @@ -1193,8 +1327,38 @@ func TestStore_PeeringWrite(t *testing.T) { Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), }, }, - // Secrets for baz should have been deleted - expectSecrets: nil, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + // Still marked as deleting at the original testTime + State: pbpeering.PeeringState_DELETING, + DeletedAt: structs.TimeToProto(testTime), + }, + // Secrets for baz should have been deleted + secrets: nil, + }, + }, + { + name: "terminating a peering marked for deletion is a no-op", + input: &pbpeering.PeeringWriteRequest{ + Peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + State: pbpeering.PeeringState_TERMINATED, + Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), + }, + }, + expect: expectations{ + peering: &pbpeering.Peering{ + ID: testBazPeerID, + Name: "baz", + // Still marked as deleting + State: pbpeering.PeeringState_DELETING, + }, + // Secrets for baz should have been deleted + secrets: nil, + }, }, { name: "cannot update peering marked for deletion", @@ -1209,7 +1373,9 @@ func TestStore_PeeringWrite(t *testing.T) { Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), }, }, - expectErr: "cannot write to peering that is marked for deletion", + expect: expectations{ + err: "cannot write to peering that is marked for deletion", + }, }, { name: "cannot create peering marked for deletion", @@ -1221,7 +1387,9 @@ func TestStore_PeeringWrite(t *testing.T) { Partition: structs.NodeEnterpriseMetaInDefaultPartition().PartitionOrEmpty(), }, }, - expectErr: "cannot create a new peering marked for deletion", + expect: expectations{ + err: "cannot create a new peering marked for deletion", + }, }, } for _, tc := range tcs { diff --git a/agent/rpc/peering/service.go b/agent/rpc/peering/service.go index 20bbafc1cf..6c0950d9e9 100644 --- a/agent/rpc/peering/service.go +++ b/agent/rpc/peering/service.go @@ -726,11 +726,12 @@ func (s *Server) PeeringDelete(ctx context.Context, req *pbpeering.PeeringDelete return nil, err } - if !existing.IsActive() { + if existing == nil || existing.State == pbpeering.PeeringState_DELETING { // Return early when the Peering doesn't exist or is already marked for deletion. // We don't return nil because the pb will fail to marshal. return &pbpeering.PeeringDeleteResponse{}, nil } + // We are using a write request due to needing to perform a deferred deletion. // The peering gets marked for deletion by setting the DeletedAt field, // and a leader routine will handle deleting the peering. diff --git a/agent/rpc/peering/service_test.go b/agent/rpc/peering/service_test.go index 54770d6a61..6b26db7d04 100644 --- a/agent/rpc/peering/service_test.go +++ b/agent/rpc/peering/service_test.go @@ -621,38 +621,50 @@ func TestPeeringService_Read_ACLEnforcement(t *testing.T) { } func TestPeeringService_Delete(t *testing.T) { - // TODO(peering): see note on newTestServer, refactor to not use this - s := newTestServer(t, nil) - - p := &pbpeering.Peering{ - ID: testUUID(t), - Name: "foo", - State: pbpeering.PeeringState_ESTABLISHING, - PeerCAPems: nil, - PeerServerName: "test", - PeerServerAddresses: []string{"addr1"}, + tt := map[string]pbpeering.PeeringState{ + "active peering": pbpeering.PeeringState_ACTIVE, + "terminated peering": pbpeering.PeeringState_TERMINATED, } - err := s.Server.FSM().State().PeeringWrite(10, &pbpeering.PeeringWriteRequest{Peering: p}) - require.NoError(t, err) - require.Nil(t, p.DeletedAt) - require.True(t, p.IsActive()) - client := pbpeering.NewPeeringServiceClient(s.ClientConn(t)) + for name, overrideState := range tt { + t.Run(name, func(t *testing.T) { + // TODO(peering): see note on newTestServer, refactor to not use this + s := newTestServer(t, nil) - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) - t.Cleanup(cancel) + // A pointer is kept for the following peering so that we can modify the object without another PeeringWrite. + p := &pbpeering.Peering{ + ID: testUUID(t), + Name: "foo", + PeerCAPems: nil, + PeerServerName: "test", + PeerServerAddresses: []string{"addr1"}, + } + err := s.Server.FSM().State().PeeringWrite(10, &pbpeering.PeeringWriteRequest{Peering: p}) + require.NoError(t, err) + require.Nil(t, p.DeletedAt) + require.True(t, p.IsActive()) - _, err = client.PeeringDelete(ctx, &pbpeering.PeeringDeleteRequest{Name: "foo"}) - require.NoError(t, err) + // Overwrite the peering state to simulate deleting from a non-initial state. + p.State = overrideState - retry.Run(t, func(r *retry.R) { - _, resp, err := s.Server.FSM().State().PeeringRead(nil, state.Query{Value: "foo"}) - require.NoError(r, err) + client := pbpeering.NewPeeringServiceClient(s.ClientConn(t)) - // Initially the peering will be marked for deletion but eventually the leader - // routine will clean it up. - require.Nil(r, resp) - }) + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + t.Cleanup(cancel) + + _, err = client.PeeringDelete(ctx, &pbpeering.PeeringDeleteRequest{Name: "foo"}) + require.NoError(t, err) + + retry.Run(t, func(r *retry.R) { + _, resp, err := s.Server.FSM().State().PeeringRead(nil, state.Query{Value: "foo"}) + require.NoError(r, err) + + // Initially the peering will be marked for deletion but eventually the leader + // routine will clean it up. + require.Nil(r, resp) + }) + }) + } } func TestPeeringService_Delete_ACLEnforcement(t *testing.T) {