From 08a2dc73cdcfebc9963d759718ad2da8341d0090 Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Thu, 21 Jun 2018 17:06:44 -0700 Subject: [PATCH] Fix bugs with packs created in UI (#1843) - Delete duplicate queries in packs created by the UI (because the duplicates were causing undefined behavior). Now it is not possible to schedule duplicates in the UI (but is in fleetctl). - Fix bug in which packs created in UI could not be loaded by fleetctl. --- server/datastore/datastore_packs_test.go | 2 + server/datastore/datastore_queries_test.go | 3 + .../20180620175054_ScheduledQueryNameKeys.go | 81 +++++++++++++++++++ server/datastore/mysql/scheduled_queries.go | 13 +-- server/kolide/scheduled_queries.go | 6 +- server/service/endpoint_scheduled_queries.go | 20 ++--- server/service/service_scheduled_queries.go | 10 +++ 7 files changed, 117 insertions(+), 18 deletions(-) create mode 100644 server/datastore/mysql/migrations/tables/20180620175054_ScheduledQueryNameKeys.go diff --git a/server/datastore/datastore_packs_test.go b/server/datastore/datastore_packs_test.go index dca7652c27..38f889b910 100644 --- a/server/datastore/datastore_packs_test.go +++ b/server/datastore/datastore_packs_test.go @@ -194,6 +194,7 @@ func setupPackSpecsTest(t *testing.T, ds kolide.Datastore) []*kolide.PackSpec { Queries: []kolide.PackSpecQuery{ kolide.PackSpecQuery{ QueryName: queries[0].Name, + Name: "q0", Description: "test_foo", Interval: 42, }, @@ -204,6 +205,7 @@ func setupPackSpecsTest(t *testing.T, ds kolide.Datastore) []*kolide.PackSpec { Snapshot: boolPtr(true), }, kolide.PackSpecQuery{ + Name: "q2", QueryName: queries[1].Name, Interval: 600, Removed: boolPtr(false), diff --git a/server/datastore/datastore_queries_test.go b/server/datastore/datastore_queries_test.go index f4a081c1e5..a0e241c35c 100644 --- a/server/datastore/datastore_queries_test.go +++ b/server/datastore/datastore_queries_test.go @@ -227,6 +227,7 @@ func testLoadPacksForQueries(t *testing.T, ds kolide.Datastore) { Name: "p2", Queries: []kolide.PackSpecQuery{ kolide.PackSpecQuery{ + Name: "q0", QueryName: queries[0].Name, Interval: 60, }, @@ -288,10 +289,12 @@ func testLoadPacksForQueries(t *testing.T, ds kolide.Datastore) { Name: "p3", Queries: []kolide.PackSpecQuery{ kolide.PackSpecQuery{ + Name: "q0", QueryName: queries[0].Name, Interval: 60, }, kolide.PackSpecQuery{ + Name: "q1", QueryName: queries[1].Name, Interval: 60, }, diff --git a/server/datastore/mysql/migrations/tables/20180620175054_ScheduledQueryNameKeys.go b/server/datastore/mysql/migrations/tables/20180620175054_ScheduledQueryNameKeys.go new file mode 100644 index 0000000000..10c2de46cd --- /dev/null +++ b/server/datastore/mysql/migrations/tables/20180620175054_ScheduledQueryNameKeys.go @@ -0,0 +1,81 @@ +package tables + +import ( + "database/sql" + + "github.com/jmoiron/sqlx" + "github.com/jmoiron/sqlx/reflectx" + "github.com/pkg/errors" +) + +func init() { + MigrationClient.AddMigration(Up_20180620175054, Down_20180620175054) +} + +func Up_20180620175054(tx *sql.Tx) error { + // Make sure all names are non-empty + query := ` + UPDATE scheduled_queries + SET name = COALESCE(name, query_name), + description = COALESCE(description, ''), + platform = COALESCE(platform, ''), + version = COALESCE(version, '') + ` + if _, err := tx.Exec(query); err != nil { + return errors.Wrap(err, "set name for all queries") + } + + // Dedupe (name, pack_id) + query = ` + SELECT name, pack_id + FROM scheduled_queries + GROUP BY name, pack_id + HAVING count(pack_id) > 1 + ` + txx := sqlx.Tx{Tx: tx, Mapper: reflectx.NewMapperFunc("db", sqlx.NameMapper)} + var dupes []struct { + Name string `db:"name"` + PackID uint `db:"pack_id"` + } + if err := txx.Select(&dupes, query); err != nil && err != sql.ErrNoRows { + return errors.Wrap(err, "get duplicate query names") + } + + for _, dupe := range dupes { + // Yes you really need to SELECT id FROM ( SELECT id FROM... + // Otherwise MySQL errors + query = ` + DELETE FROM scheduled_queries + WHERE id IN + ( SELECT id FROM ( + SELECT id from scheduled_queries + WHERE name = ? AND pack_id = ? + ORDER BY id DESC + LIMIT 9999 OFFSET 1 + ) AS t + ) + ` + if _, err := tx.Exec(query, dupe.Name, dupe.PackID); err != nil { + return errors.Wrapf(err, "delete dupe %s", dupe.Name) + } + } + + // Enforce not-null, uniqueness and add column defaults + query = ` + ALTER TABLE scheduled_queries + MODIFY name varchar(255) NOT NULL, + MODIFY description varchar(1023) DEFAULT '', + MODIFY platform varchar(255) DEFAULT '', + MODIFY version varchar(255) DEFAULT '', + ADD UNIQUE KEY unique_names_in_packs (name, pack_id) + ` + if _, err := tx.Exec(query); err != nil { + return errors.Wrapf(err, "altering table") + } + + return nil +} + +func Down_20180620175054(tx *sql.Tx) error { + return nil +} diff --git a/server/datastore/mysql/scheduled_queries.go b/server/datastore/mysql/scheduled_queries.go index def05fce1c..253053ef7a 100644 --- a/server/datastore/mysql/scheduled_queries.go +++ b/server/datastore/mysql/scheduled_queries.go @@ -12,13 +12,13 @@ func (d *Datastore) ListScheduledQueriesInPack(id uint, opts kolide.ListOptions) SELECT sq.id, sq.pack_id, - COALESCE(sq.name, q.name) AS name, + sq.name, sq.query_name, - COALESCE(sq.description, '') AS description, + sq.description, sq.interval, sq.snapshot, sq.removed, - COALESCE(sq.platform, '') AS platform, + sq.platform, sq.version, sq.shard, q.query, @@ -47,6 +47,7 @@ func (d *Datastore) NewScheduledQuery(sq *kolide.ScheduledQuery, opts ...kolide. query := ` INSERT INTO scheduled_queries ( query_name, + name, pack_id, snapshot, removed, @@ -55,11 +56,11 @@ func (d *Datastore) NewScheduledQuery(sq *kolide.ScheduledQuery, opts ...kolide. version, shard ) - SELECT name, ?, ?, ?, ?, ?, ?, ? + SELECT name, ?, ?, ?, ?, ?, ?, ?, ? FROM queries WHERE id = ? ` - result, err := db.Exec(query, sq.PackID, sq.Snapshot, sq.Removed, sq.Interval, sq.Platform, sq.Version, sq.Shard, sq.QueryID) + result, err := db.Exec(query, sq.Name, sq.PackID, sq.Snapshot, sq.Removed, sq.Interval, sq.Platform, sq.Version, sq.Shard, sq.QueryID) if err != nil { return nil, errors.Wrap(err, "inserting scheduled query") } @@ -128,7 +129,7 @@ func (d *Datastore) ScheduledQuery(id uint) (*kolide.ScheduledQuery, error) { sq.version, sq.shard, sq.query_name, - COALESCE(sq.description, '') AS description, + sq.description, q.query, q.name, q.id AS query_id diff --git a/server/kolide/scheduled_queries.go b/server/kolide/scheduled_queries.go index 48c45eb36f..1f91d27839 100644 --- a/server/kolide/scheduled_queries.go +++ b/server/kolide/scheduled_queries.go @@ -29,12 +29,12 @@ type ScheduledQuery struct { QueryID uint `json:"query_id" db:"query_id"` QueryName string `json:"query_name" db:"query_name"` Query string `json:"query"` // populated via a join on queries - Description string `json:"description"` + Description string `json:"description,omitempty"` Interval uint `json:"interval"` Snapshot *bool `json:"snapshot"` Removed *bool `json:"removed"` - Platform *string `json:"platform"` - Version *string `json:"version"` + Platform *string `json:"platform,omitempty"` + Version *string `json:"version,omitempty"` Shard *uint `json:"shard"` } diff --git a/server/service/endpoint_scheduled_queries.go b/server/service/endpoint_scheduled_queries.go index 92ba125a25..13ccd8bd4b 100644 --- a/server/service/endpoint_scheduled_queries.go +++ b/server/service/endpoint_scheduled_queries.go @@ -56,8 +56,8 @@ type getScheduledQueryRequest struct { } type getScheduledQueryResponse struct { - Scheduled scheduledQueryResponse `json:"scheduled,omitempty"` - Err error `json:"error,omitempty"` + Scheduled *scheduledQueryResponse `json:"scheduled,omitempty"` + Err error `json:"error,omitempty"` } func (r getScheduledQueryResponse) error() error { return r.Err } @@ -72,7 +72,7 @@ func makeGetScheduledQueryEndpoint(svc kolide.Service) endpoint.Endpoint { } return getScheduledQueryResponse{ - Scheduled: scheduledQueryResponse{ + Scheduled: &scheduledQueryResponse{ ScheduledQuery: *sq, }, }, nil @@ -95,10 +95,12 @@ type scheduleQueryRequest struct { } type scheduleQueryResponse struct { - Scheduled scheduledQueryResponse `json:"scheduled"` - Err error `json:"error,omitempty"` + Scheduled *scheduledQueryResponse `json:"scheduled,omitempty"` + Err error `json:"error,omitempty"` } +func (r scheduleQueryResponse) error() error { return r.Err } + func makeScheduleQueryEndpoint(svc kolide.Service) endpoint.Endpoint { return func(ctx context.Context, request interface{}) (interface{}, error) { req := request.(scheduleQueryRequest) @@ -116,7 +118,7 @@ func makeScheduleQueryEndpoint(svc kolide.Service) endpoint.Endpoint { if err != nil { return scheduleQueryResponse{Err: err}, nil } - return scheduleQueryResponse{Scheduled: scheduledQueryResponse{ + return scheduleQueryResponse{Scheduled: &scheduledQueryResponse{ ScheduledQuery: *scheduled, }}, nil } @@ -132,8 +134,8 @@ type modifyScheduledQueryRequest struct { } type modifyScheduledQueryResponse struct { - Scheduled scheduledQueryResponse `json:"scheduled,omitempty"` - Err error `json:"error,omitempty"` + Scheduled *scheduledQueryResponse `json:"scheduled,omitempty"` + Err error `json:"error,omitempty"` } func (r modifyScheduledQueryResponse) error() error { return r.Err } @@ -148,7 +150,7 @@ func makeModifyScheduledQueryEndpoint(svc kolide.Service) endpoint.Endpoint { } return modifyScheduledQueryResponse{ - Scheduled: scheduledQueryResponse{ + Scheduled: &scheduledQueryResponse{ ScheduledQuery: *sq, }, }, nil diff --git a/server/service/service_scheduled_queries.go b/server/service/service_scheduled_queries.go index 71842ac508..2272c29783 100644 --- a/server/service/service_scheduled_queries.go +++ b/server/service/service_scheduled_queries.go @@ -16,6 +16,16 @@ func (svc service) GetScheduledQuery(ctx context.Context, id uint) (*kolide.Sche } func (svc service) ScheduleQuery(ctx context.Context, sq *kolide.ScheduledQuery) (*kolide.ScheduledQuery, error) { + // Fill in the name with query name if it is unset (because the UI + // doesn't provide a way to set it) + if sq.Name == "" { + query, err := svc.ds.Query(sq.QueryID) + if err != nil { + return nil, errors.Wrap(err, "lookup name for query") + } + sq.Name = query.Name + sq.QueryName = query.Name + } return svc.ds.NewScheduledQuery(sq) }