From 4ce23c7d1bff012e40e3f2bc9ed83fd7bb400b60 Mon Sep 17 00:00:00 2001 From: Michal Nicpon <39177923+michalnicp@users.noreply.github.com> Date: Mon, 21 Feb 2022 09:18:58 -0700 Subject: [PATCH] Add team targets to pack spec (#4272) * skip flaky tests --- .../issue-3031-add-team-target-to-pack-spec | 1 + cmd/fleetctl/apply.go | 16 +-- cmd/fleetctl/convert_test.go | 2 +- cmd/fleetctl/get_test.go | 46 +++++- cmd/fleetctl/testdata/convert_output.yml | 1 + .../07-API-for-contributors.md | 15 +- server/datastore/mysql/hosts_test.go | 2 + server/datastore/mysql/locks_test.go | 7 +- server/datastore/mysql/packs.go | 132 +++++++++++------- server/datastore/mysql/packs_test.go | 21 +++ server/fleet/packs.go | 1 + server/fleet/packs_test.go | 9 +- 12 files changed, 173 insertions(+), 80 deletions(-) create mode 100644 changes/issue-3031-add-team-target-to-pack-spec diff --git a/changes/issue-3031-add-team-target-to-pack-spec b/changes/issue-3031-add-team-target-to-pack-spec new file mode 100644 index 0000000000..6972e08ec5 --- /dev/null +++ b/changes/issue-3031-add-team-target-to-pack-spec @@ -0,0 +1 @@ +- Add support for teams to pack spec endpoints and fleetctl diff --git a/cmd/fleetctl/apply.go b/cmd/fleetctl/apply.go index 356fb80c15..4a83a55bec 100644 --- a/cmd/fleetctl/apply.go +++ b/cmd/fleetctl/apply.go @@ -64,28 +64,28 @@ func specGroupFromBytes(b []byte) (*specGroup, error) { case fleet.QueryKind: var querySpec *fleet.QuerySpec if err := yaml.Unmarshal(s.Spec, &querySpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.Queries = append(specs.Queries, querySpec) case fleet.PackKind: var packSpec *fleet.PackSpec if err := yaml.Unmarshal(s.Spec, &packSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.Packs = append(specs.Packs, packSpec) case fleet.LabelKind: var labelSpec *fleet.LabelSpec if err := yaml.Unmarshal(s.Spec, &labelSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.Labels = append(specs.Labels, labelSpec) case fleet.PolicyKind: var policySpec *fleet.PolicySpec if err := yaml.Unmarshal(s.Spec, &policySpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.Policies = append(specs.Policies, policySpec) @@ -96,7 +96,7 @@ func specGroupFromBytes(b []byte) (*specGroup, error) { var appConfigSpec interface{} if err := yaml.Unmarshal(s.Spec, &appConfigSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.AppConfig = appConfigSpec @@ -107,21 +107,21 @@ func specGroupFromBytes(b []byte) (*specGroup, error) { var enrollSecretSpec *fleet.EnrollSecretSpec if err := yaml.Unmarshal(s.Spec, &enrollSecretSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.EnrollSecret = enrollSecretSpec case fleet.UserRolesKind: var userRoleSpec *fleet.UsersRoleSpec if err := yaml.Unmarshal(s.Spec, &userRoleSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.UsersRoles = userRoleSpec case fleet.TeamKind: var teamSpec TeamSpec if err := yaml.Unmarshal(s.Spec, &teamSpec); err != nil { - return nil, fmt.Errorf("unmarshaling "+kind+" spec: %w", err) + return nil, fmt.Errorf("unmarshaling %s spec: %w", kind, err) } specs.Teams = append(specs.Teams, teamSpec.Team) diff --git a/cmd/fleetctl/convert_test.go b/cmd/fleetctl/convert_test.go index 3b838aae1e..6903d24221 100644 --- a/cmd/fleetctl/convert_test.go +++ b/cmd/fleetctl/convert_test.go @@ -36,7 +36,7 @@ func TestConvertFileOutput(t *testing.T) { // convert command ran and wrote converted file to output destination got, err := ioutil.ReadFile(file.Name()) require.NoError(t, err) - require.Equal(t, expected, got) + require.YAMLEq(t, string(expected), string(got)) } func TestConvertFileStdout(t *testing.T) { diff --git a/cmd/fleetctl/get_test.go b/cmd/fleetctl/get_test.go index d0d7f4a7e3..b8db34dd6e 100644 --- a/cmd/fleetctl/get_test.go +++ b/cmd/fleetctl/get_test.go @@ -826,13 +826,29 @@ spec: platform: darwin targets: labels: null + teams: null ` - expectedJson := `{"kind":"pack","apiVersion":"v1","spec":{"id":7,"name":"pack1","description":"some desc","platform":"darwin","disabled":false,"targets":{"labels":null}}} + expectedJson := ` +{ + "kind": "pack", + "apiVersion": "v1", + "spec": { + "id": 7, + "name": "pack1", + "description": "some desc", + "platform": "darwin", + "disabled": false, + "targets": { + "labels": null, + "teams": null + } + } +} ` assert.Equal(t, expected, runAppForTest(t, []string{"get", "packs"})) - assert.Equal(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "--yaml"})) - assert.Equal(t, expectedJson, runAppForTest(t, []string{"get", "packs", "--json"})) + assert.YAMLEq(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "--yaml"})) + assert.JSONEq(t, expectedJson, runAppForTest(t, []string{"get", "packs", "--json"})) } func TestGetPack(t *testing.T) { @@ -874,13 +890,29 @@ spec: platform: darwin targets: labels: null + teams: null ` - expectedJson := `{"kind":"pack","apiVersion":"v1","spec":{"id":7,"name":"pack1","description":"some desc","platform":"darwin","disabled":false,"targets":{"labels":null}}} + expectedJson := ` +{ + "kind": "pack", + "apiVersion": "v1", + "spec": { + "id": 7, + "name": "pack1", + "description": "some desc", + "platform": "darwin", + "disabled": false, + "targets": { + "labels": null, + "teams": null + } + } +} ` - assert.Equal(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "pack1"})) - assert.Equal(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "--yaml", "pack1"})) - assert.Equal(t, expectedJson, runAppForTest(t, []string{"get", "packs", "--json", "pack1"})) + assert.YAMLEq(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "pack1"})) + assert.YAMLEq(t, expectedYaml, runAppForTest(t, []string{"get", "packs", "--yaml", "pack1"})) + assert.JSONEq(t, expectedJson, runAppForTest(t, []string{"get", "packs", "--json", "pack1"})) } func TestGetQueries(t *testing.T) { diff --git a/cmd/fleetctl/testdata/convert_output.yml b/cmd/fleetctl/testdata/convert_output.yml index a76016b011..233584961a 100644 --- a/cmd/fleetctl/testdata/convert_output.yml +++ b/cmd/fleetctl/testdata/convert_output.yml @@ -39,6 +39,7 @@ spec: version: 1.4.7 targets: labels: null + teams: null --- apiVersion: v1 kind: query diff --git a/docs/03-Contributing/07-API-for-contributors.md b/docs/03-Contributing/07-API-for-contributors.md index c6c2c011a2..0004ef9442 100644 --- a/docs/03-Contributing/07-API-for-contributors.md +++ b/docs/03-Contributing/07-API-for-contributors.md @@ -150,7 +150,8 @@ Returns the specs for all packs in the Fleet instance. "description": "Description", "disabled": false, "targets": { - "labels": ["All Hosts"] + "labels": ["All Hosts"], + "teams": null }, "queries": [ { @@ -220,7 +221,8 @@ Returns the specs for all packs in the Fleet instance. "name": "pack_2", "disabled": false, "targets": { - "labels": null + "labels": null, + "teams": null }, "queries": [ { @@ -268,7 +270,8 @@ Returns the specs for all packs in the Fleet instance. "description": "Description", "disabled": false, "targets": { - "labels": ["All Hosts"] + "labels": ["All Hosts"], + "teams": null }, "queries": [ { @@ -326,7 +329,8 @@ Returns the specs for all packs in the Fleet instance. "name": "pack_2", "disabled": false, "targets": { - "labels": null + "labels": null, + "teams": null }, "queries": [ { @@ -376,7 +380,8 @@ Returns the spec for the specified pack by pack name. "description": "Description", "disabled": false, "targets": { - "labels": ["All Hosts"] + "labels": ["All Hosts"], + "teams": null }, "queries": [ { diff --git a/server/datastore/mysql/hosts_test.go b/server/datastore/mysql/hosts_test.go index 644c0dc734..c265e5e5ef 100644 --- a/server/datastore/mysql/hosts_test.go +++ b/server/datastore/mysql/hosts_test.go @@ -1977,6 +1977,8 @@ func getReads(t *testing.T, ds *Datastore) int { } func testHostsReadsLessRows(t *testing.T, ds *Datastore) { + t.Skip("flaky: https://github.com/fleetdm/fleet/issues/4270") + user1 := test.NewUser(t, ds, "alice", "alice-123@example.com", true) var hosts []*fleet.Host for i := 0; i < 10; i++ { diff --git a/server/datastore/mysql/locks_test.go b/server/datastore/mysql/locks_test.go index e36eb8efd4..9fafa9c5d8 100644 --- a/server/datastore/mysql/locks_test.go +++ b/server/datastore/mysql/locks_test.go @@ -20,8 +20,8 @@ func TestLocks(t *testing.T) { name string fn func(t *testing.T, ds *Datastore) }{ - {"LockUnlock", func(t *testing.T, ds *Datastore) { testLocksLockUnlock(t, ds) }}, - {"DBLocks", func(t *testing.T, ds *Datastore) { testLocksDBLocks(t, ds) }}, + {"LockUnlock", testLocksLockUnlock}, + {"DBLocks", testLocksDBLocks}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { @@ -86,6 +86,7 @@ const ( mariaDB_10_6 mysqlServer = 3 ) +//nolint:unused // used in skipped tests func getMySQLServer(t *testing.T, r dbReader) mysqlServer { row := r.QueryRowxContext(context.Background(), "SELECT VERSION()") var version string @@ -104,6 +105,8 @@ func getMySQLServer(t *testing.T, r dbReader) mysqlServer { } func testLocksDBLocks(t *testing.T, ds *Datastore) { + t.Skip("flaky: https://github.com/fleetdm/fleet/issues/4270") + if srv := getMySQLServer(t, ds.reader); srv == mysql8 { t.Skip("#3626: DBLocks is not supported for mysql 8 yet.") } diff --git a/server/datastore/mysql/packs.go b/server/datastore/mysql/packs.go index deb59b05cc..4f201034ab 100644 --- a/server/datastore/mysql/packs.go +++ b/server/datastore/mysql/packs.go @@ -29,6 +29,7 @@ func applyPackSpecDB(ctx context.Context, tx sqlx.ExtContext, spec *fleet.PackSp if spec.Name == "" { return ctxerr.New(ctx, "pack name must not be empty") } + // Insert/update pack query := ` INSERT INTO packs (name, description, platform, disabled) @@ -59,21 +60,21 @@ func applyPackSpecDB(ctx context.Context, tx sqlx.ExtContext, spec *fleet.PackSp } // Insert new scheduled queries for pack + query = ` + INSERT INTO scheduled_queries ( + pack_id, query_name, name, description, ` + "`interval`" + `, + snapshot, removed, shard, platform, version, denylist + ) + VALUES ( + ?, ?, ?, ?, ?, + ?, ?, ?, ?, ?, ? + ) + ` for _, q := range spec.Queries { // Default to query name if scheduled query name is not specified. if q.Name == "" { q.Name = q.QueryName } - query = ` - INSERT INTO scheduled_queries ( - pack_id, query_name, name, description, ` + "`interval`" + `, - snapshot, removed, shard, platform, version, denylist - ) - VALUES ( - ?, ?, ?, ?, ?, - ?, ?, ?, ?, ?, ? - ) - ` _, err := tx.ExecContext(ctx, query, packID, q.QueryName, q.Name, q.Description, q.Interval, q.Snapshot, q.Removed, q.Shard, q.Platform, q.Version, q.Denylist, @@ -92,22 +93,32 @@ func applyPackSpecDB(ctx context.Context, tx sqlx.ExtContext, spec *fleet.PackSp return ctxerr.Wrap(ctx, err, "delete existing targets") } - // Insert targets + query = ` + INSERT INTO pack_targets (pack_id, type, target_id) + VALUES (?, ?, (SELECT id FROM labels WHERE name = ?)) + ` for _, l := range spec.Targets.Labels { - query = ` - INSERT INTO pack_targets (pack_id, type, target_id) - VALUES (?, ?, (SELECT id FROM labels WHERE name = ?)) - ` if _, err := tx.ExecContext(ctx, query, packID, fleet.TargetLabel, l); err != nil { return ctxerr.Wrap(ctx, err, "adding label to pack") } } + query = ` + INSERT INTO pack_targets (pack_id, type, target_id) + VALUES (?, ?, (SELECT id FROM teams WHERE name = ?)) + ` + for _, t := range spec.Targets.Teams { + if _, err := tx.ExecContext(ctx, query, packID, fleet.TargetTeam, t); err != nil { + return ctxerr.Wrap(ctx, err, "adding team to pack") + } + } + return nil } -func (ds *Datastore) GetPackSpecs(ctx context.Context) (specs []*fleet.PackSpec, err error) { - err = ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { +func (ds *Datastore) GetPackSpecs(ctx context.Context) ([]*fleet.PackSpec, error) { + var specs []*fleet.PackSpec + err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { // Get basic specs query := "SELECT id, name, description, platform, disabled FROM packs WHERE pack_type IS NULL OR pack_type = ''" if err := sqlx.SelectContext(ctx, tx, &specs, query); err != nil { @@ -116,13 +127,25 @@ func (ds *Datastore) GetPackSpecs(ctx context.Context) (specs []*fleet.PackSpec, // Load targets for _, spec := range specs { + + // Load labels query = ` SELECT l.name FROM labels l JOIN pack_targets pt WHERE pack_id = ? AND pt.type = ? AND pt.target_id = l.id ` if err := sqlx.SelectContext(ctx, tx, &spec.Targets.Labels, query, spec.ID, fleet.TargetLabel); err != nil { - return ctxerr.Wrap(ctx, err, "get pack targets") + return ctxerr.Wrap(ctx, err, "get pack label targets") + } + + // Load teams + query = ` +SELECT t.name +FROM teams t JOIN pack_targets pt +WHERE pack_id = ? AND pt.type = ? AND pt.target_id = t.id +` + if err := sqlx.SelectContext(ctx, tx, &spec.Targets.Teams, query, spec.ID, fleet.TargetTeam); err != nil { + return ctxerr.Wrap(ctx, err, "get pack team targets") } } @@ -142,7 +165,6 @@ WHERE pack_id = ? return nil }) - if err != nil { return nil, err } @@ -150,31 +172,36 @@ WHERE pack_id = ? return specs, nil } -func (ds *Datastore) GetPackSpec(ctx context.Context, name string) (spec *fleet.PackSpec, err error) { - err = ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { +func (ds *Datastore) GetPackSpec(ctx context.Context, name string) (*fleet.PackSpec, error) { + spec := &fleet.PackSpec{} + err := ds.withRetryTxx(ctx, func(tx sqlx.ExtContext) error { // Get basic spec - var specs []*fleet.PackSpec query := "SELECT id, name, description, platform, disabled FROM packs WHERE name = ?" - if err := sqlx.SelectContext(ctx, tx, &specs, query, name); err != nil { + if err := sqlx.GetContext(ctx, tx, spec, query, name); err != nil { + if err == sql.ErrNoRows { + return ctxerr.Wrap(ctx, notFound("Pack").WithName(name)) + } return ctxerr.Wrap(ctx, err, "get packs") } - if len(specs) == 0 { - return ctxerr.Wrap(ctx, notFound("Pack").WithName(name)) - } - if len(specs) > 1 { - return ctxerr.Errorf(ctx, "expected 1 pack row, got %d", len(specs)) - } - spec = specs[0] - - // Load targets + // Load label targets query = ` SELECT l.name FROM labels l JOIN pack_targets pt WHERE pack_id = ? AND pt.type = ? AND pt.target_id = l.id ` if err := sqlx.SelectContext(ctx, tx, &spec.Targets.Labels, query, spec.ID, fleet.TargetLabel); err != nil { - return ctxerr.Wrap(ctx, err, "get pack targets") + return ctxerr.Wrap(ctx, err, "get pack label targets") + } + + // Load team targets + query = ` +SELECT t.name +FROM teams t JOIN pack_targets pt +WHERE pack_id = ? AND pt.type = ? AND pt.target_id = t.id +` + if err := sqlx.SelectContext(ctx, tx, &spec.Targets.Teams, query, spec.ID, fleet.TargetTeam); err != nil { + return ctxerr.Wrap(ctx, err, "get pack team targets") } // Load queries @@ -191,7 +218,6 @@ WHERE pack_id = ? return nil }) - if err != nil { return nil, err } @@ -254,25 +280,6 @@ func replacePackTargetsDB(ctx context.Context, tx sqlx.ExecerContext, pack *flee return ctxerr.Wrap(ctx, err, "delete pack targets") } - // Insert hosts - if len(pack.HostIDs) > 0 { - var args []interface{} - for _, id := range pack.HostIDs { - args = append(args, pack.ID, fleet.TargetHost, id) - } - values := strings.TrimSuffix( - strings.Repeat("(?,?,?),", len(pack.HostIDs)), - ",", - ) - sql = fmt.Sprintf(` - INSERT INTO pack_targets (pack_id, type, target_id) - VALUES %s - `, values) - if _, err := tx.ExecContext(ctx, sql, args...); err != nil { - return ctxerr.Wrap(ctx, err, "insert host targets") - } - } - // Insert labels if len(pack.LabelIDs) > 0 { var args []interface{} @@ -292,6 +299,25 @@ func replacePackTargetsDB(ctx context.Context, tx sqlx.ExecerContext, pack *flee } } + // Insert hosts + if len(pack.HostIDs) > 0 { + var args []interface{} + for _, id := range pack.HostIDs { + args = append(args, pack.ID, fleet.TargetHost, id) + } + values := strings.TrimSuffix( + strings.Repeat("(?,?,?),", len(pack.HostIDs)), + ",", + ) + sql = fmt.Sprintf(` + INSERT INTO pack_targets (pack_id, type, target_id) + VALUES %s + `, values) + if _, err := tx.ExecContext(ctx, sql, args...); err != nil { + return ctxerr.Wrap(ctx, err, "insert host targets") + } + } + // Insert teams if len(pack.TeamIDs) > 0 { var args []interface{} diff --git a/server/datastore/mysql/packs_test.go b/server/datastore/mysql/packs_test.go index 59743410db..fd46836680 100644 --- a/server/datastore/mysql/packs_test.go +++ b/server/datastore/mysql/packs_test.go @@ -169,6 +169,23 @@ func setupPackSpecsTest(t *testing.T, ds fleet.Datastore) []*fleet.PackSpec { err = ds.ApplyLabelSpecs(context.Background(), labels) require.Nil(t, err) + // create some teams + teams := []*fleet.Team{ + { + Name: "team1", + }, + { + Name: "team2", + }, + { + Name: "team3", + }, + } + for _, team := range teams { + _, err := ds.NewTeam(context.Background(), team) + require.NoError(t, err) + } + expectedSpecs := []*fleet.PackSpec{ { ID: 1, @@ -179,6 +196,10 @@ func setupPackSpecsTest(t *testing.T, ds fleet.Datastore) []*fleet.PackSpec { "bar", "bing", }, + Teams: []string{ + "team1", + "team2", + }, }, Queries: []fleet.PackSpecQuery{ { diff --git a/server/fleet/packs.go b/server/fleet/packs.go index 21e6f010f1..014d53f873 100644 --- a/server/fleet/packs.go +++ b/server/fleet/packs.go @@ -93,6 +93,7 @@ func (p *PackSpec) Verify() error { type PackSpecTargets struct { Labels []string `json:"labels"` + Teams []string `json:"teams"` } type PackSpecQuery struct { diff --git a/server/fleet/packs_test.go b/server/fleet/packs_test.go index 38bd3bdc01..0ebf6387e3 100644 --- a/server/fleet/packs_test.go +++ b/server/fleet/packs_test.go @@ -1,11 +1,11 @@ package fleet import ( - "bytes" "encoding/json" "testing" "github.com/fleetdm/fleet/v4/server/ptr" + "github.com/stretchr/testify/require" ) func TestPack_EditablePackType(t *testing.T) { @@ -102,7 +102,8 @@ func TestPack_Marshal(t *testing.T) { if err != nil { t.Fatal(err) } - if !bytes.Contains(b, []byte("\"disabled\":false")) { - t.Fatalf("marshalled pack does not contain disabled field: %s", string(b)) - } + var m map[string]interface{} + err = json.Unmarshal(b, &m) + require.NoError(t, err) + require.NotNil(t, m["disabled"], "marshalled pack does not contain disabled field: %s", string(b)) }