From eee539cccc099f26780797bfdfc4f1aa2eca10e9 Mon Sep 17 00:00:00 2001 From: Tomas Touceda Date: Wed, 19 Jan 2022 10:28:08 -0300 Subject: [PATCH] Issue 3707 clean targets on delete (#3739) * wip * Delete targets when deleting teams, hosts, and labels * Add changes file * Fix error message * Remove unused teamsTable * Cleanup new pack * Clean new packs at end of test --- changes/issue-3707-clean-targets-on-delete | 1 + server/datastore/mysql/hosts.go | 17 ++++++++++++++++ server/datastore/mysql/hosts_test.go | 12 +++++++++++ server/datastore/mysql/labels.go | 6 ++++++ server/datastore/mysql/labels_test.go | 23 ++++++++++++++++++++++ server/datastore/mysql/mysql.go | 1 - server/datastore/mysql/teams.go | 17 ++++++++++++---- server/datastore/mysql/teams_test.go | 12 +++++++++++ server/service/integration_core_test.go | 16 +++++++++++++++ 9 files changed, 100 insertions(+), 5 deletions(-) create mode 100644 changes/issue-3707-clean-targets-on-delete diff --git a/changes/issue-3707-clean-targets-on-delete b/changes/issue-3707-clean-targets-on-delete new file mode 100644 index 0000000000..71dee4f3c2 --- /dev/null +++ b/changes/issue-3707-clean-targets-on-delete @@ -0,0 +1 @@ +* Cleanup pack targets when deleting hosts, labels, and teams diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index df15f7a94e..f9c9c76767 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -343,6 +343,12 @@ func (d *Datastore) DeleteHost(ctx context.Context, hid uint) error { return err } } + + _, err = tx.ExecContext(ctx, `DELETE FROM pack_targets WHERE type=? AND target_id=?`, fleet.TargetHost, hid) + if err != nil { + return ctxerr.Wrapf(ctx, err, "deleting pack_targets for host %d", hid) + } + return nil }) } @@ -984,6 +990,17 @@ func (d *Datastore) DeleteHosts(ctx context.Context, ids []uint) error { if err != nil { return ctxerr.Wrap(ctx, err, "deleting host emails") } + + query, args, err = sqlx.In(`DELETE FROM pack_targets WHERE type=? AND target_id in (?)`, fleet.TargetHost, ids) + if err != nil { + return ctxerr.Wrapf(ctx, err, "building delete pack_targets query") + } + + _, err = d.writer.ExecContext(ctx, query, args...) + if err != nil { + return ctxerr.Wrapf(ctx, err, "deleting pack_targets for hosts") + } + return nil } diff --git a/server/datastore/mysql/hosts_test.go b/server/datastore/mysql/hosts_test.go index 75f5fea793..5f6d1f6e10 100644 --- a/server/datastore/mysql/hosts_test.go +++ b/server/datastore/mysql/hosts_test.go @@ -170,12 +170,24 @@ func testSaveHost(t *testing.T, ds *Datastore, saveHostFunc func(context.Context require.NoError(t, err) require.NotNil(t, host) + p, err := ds.NewPack(context.Background(), &fleet.Pack{ + Name: t.Name(), + HostIDs: []uint{host.ID}, + }) + require.NoError(t, err) + err = ds.DeleteHost(context.Background(), host.ID) assert.Nil(t, err) + newP, err := ds.Pack(context.Background(), p.ID) + require.NoError(t, err) + require.Empty(t, newP.Hosts) + host, err = ds.Host(context.Background(), host.ID, false) assert.NotNil(t, err) assert.Nil(t, host) + + require.NoError(t, ds.DeletePack(context.Background(), newP.Name)) } func testHostsDeleteWithSoftware(t *testing.T, ds *Datastore) { diff --git a/server/datastore/mysql/labels.go b/server/datastore/mysql/labels.go index fea1634ce7..b8e87f8b1b 100644 --- a/server/datastore/mysql/labels.go +++ b/server/datastore/mysql/labels.go @@ -245,6 +245,12 @@ func (d *Datastore) DeleteLabel(ctx context.Context, name string) error { if err != nil { return ctxerr.Wrapf(ctx, err, "delete label_membership") } + + _, err = tx.ExecContext(ctx, `DELETE FROM pack_targets WHERE type=? AND target_id=?`, fleet.TargetLabel, labelID) + if err != nil { + return ctxerr.Wrapf(ctx, err, "deleting pack_targets for label %d", labelID) + } + return nil }) } diff --git a/server/datastore/mysql/labels_test.go b/server/datastore/mysql/labels_test.go index 45787131f5..988bc93988 100644 --- a/server/datastore/mysql/labels_test.go +++ b/server/datastore/mysql/labels_test.go @@ -59,6 +59,7 @@ func TestLabels(t *testing.T) { {"Save", testLabelsSave}, {"QueriesForCentOSHost", testLabelsQueriesForCentOSHost}, {"RecordNonExistentQueryLabelExecution", testLabelsRecordNonexistentQueryLabelExecution}, + {"DeleteLabel", testDeleteLabel}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -753,3 +754,25 @@ func testLabelsRecordNonexistentQueryLabelExecution(t *testing.T, db *Datastore) require.NoError(t, db.RecordLabelQueryExecutions(context.Background(), h1, map[uint]*bool{99999: ptr.Bool(true)}, time.Now(), false)) } + +func testDeleteLabel(t *testing.T, db *Datastore) { + l, err := db.NewLabel(context.Background(), &fleet.Label{ + Name: t.Name(), + Query: "query1", + }) + require.NoError(t, err) + + p, err := db.NewPack(context.Background(), &fleet.Pack{ + Name: t.Name(), + LabelIDs: []uint{l.ID}, + }) + require.NoError(t, err) + + require.NoError(t, db.DeleteLabel(context.Background(), l.Name)) + + newP, err := db.Pack(context.Background(), p.ID) + require.NoError(t, err) + require.Empty(t, newP.Labels) + + require.NoError(t, db.DeletePack(context.Background(), newP.Name)) +} diff --git a/server/datastore/mysql/mysql.go b/server/datastore/mysql/mysql.go index 8c48eb6566..d95df87aee 100644 --- a/server/datastore/mysql/mysql.go +++ b/server/datastore/mysql/mysql.go @@ -74,7 +74,6 @@ var ( packsTable = entity{"packs"} queriesTable = entity{"queries"} sessionsTable = entity{"sessions"} - teamsTable = entity{"teams"} usersTable = entity{"users"} ) diff --git a/server/datastore/mysql/teams.go b/server/datastore/mysql/teams.go index 691bfdbe6a..b5137b316c 100644 --- a/server/datastore/mysql/teams.go +++ b/server/datastore/mysql/teams.go @@ -80,10 +80,19 @@ func saveTeamSecretsDB(ctx context.Context, exec sqlx.ExecerContext, team *fleet } func (d *Datastore) DeleteTeam(ctx context.Context, tid uint) error { - if err := d.deleteEntity(ctx, teamsTable, tid); err != nil { - return ctxerr.Wrapf(ctx, err, "delete team id %d", tid) - } - return nil + return d.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { + _, err := tx.ExecContext(ctx, `DELETE FROM teams WHERE id = ?`, tid) + if err != nil { + return ctxerr.Wrapf(ctx, err, "delete team %d", tid) + } + + _, err = tx.ExecContext(ctx, `DELETE FROM pack_targets WHERE type=? AND target_id=?`, fleet.TargetTeam, tid) + if err != nil { + return ctxerr.Wrapf(ctx, err, "deleting pack_targets for team %d", tid) + } + + return nil + }) } func (d *Datastore) TeamByName(ctx context.Context, name string) (*fleet.Team, error) { diff --git a/server/datastore/mysql/teams_test.go b/server/datastore/mysql/teams_test.go index 8427f3cd3e..ed3e488910 100644 --- a/server/datastore/mysql/teams_test.go +++ b/server/datastore/mysql/teams_test.go @@ -64,11 +64,23 @@ func testTeamsGetSetDelete(t *testing.T, ds *Datastore) { assert.Equal(t, tt.name, team.Name) assert.Equal(t, tt.description, team.Description) + p, err := ds.NewPack(context.Background(), &fleet.Pack{ + Name: tt.name, + TeamIDs: []uint{team.ID}, + }) + require.NoError(t, err) + err = ds.DeleteTeam(context.Background(), team.ID) require.NoError(t, err) + newP, err := ds.Pack(context.Background(), p.ID) + require.NoError(t, err) + require.Empty(t, newP.Teams) + team, err = ds.TeamByName(context.Background(), tt.name) require.Error(t, err) + + require.NoError(t, ds.DeletePack(context.Background(), newP.Name)) }) } } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index ed84846058..601c06a2e6 100644 --- a/server/service/integration_core_test.go +++ b/server/service/integration_core_test.go @@ -511,6 +511,17 @@ func (s *integrationTestSuite) TestBulkDeleteHostsFromTeam() { team1, err := s.ds.NewTeam(context.Background(), &fleet.Team{Name: t.Name() + "team1"}) require.NoError(t, err) + p, err := s.ds.NewPack(context.Background(), &fleet.Pack{ + Name: t.Name(), + Hosts: []fleet.Target{ + { + Type: fleet.TargetHost, + TargetID: hosts[0].ID, + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.ds.AddHostsToTeam(context.Background(), &team1.ID, []uint{hosts[0].ID})) req := deleteHostsRequest{ @@ -533,6 +544,11 @@ func (s *integrationTestSuite) TestBulkDeleteHostsFromTeam() { err = s.ds.DeleteHosts(context.Background(), []uint{hosts[1].ID, hosts[2].ID}) require.NoError(t, err) + + newP, err := s.ds.Pack(context.Background(), p.ID) + require.NoError(t, err) + require.Empty(t, newP.Hosts) + require.NoError(t, s.ds.DeletePack(context.Background(), newP.Name)) } func (s *integrationTestSuite) TestBulkDeleteHostsInLabel() {