From 3d45f553f804446fc65e97406b6c56035ea06719 Mon Sep 17 00:00:00 2001 From: sas Date: Fri, 8 Nov 2024 10:32:34 +0000 Subject: [PATCH 01/39] feat(coderd/database): allow filtering provisioner daemons by tags --- coderd/database/migrations/000274_check_tags.down.sql | 1 + coderd/database/migrations/000274_check_tags.up.sql | 10 ++++++++++ coderd/database/queries/provisionerdaemons.sql | 6 +++++- coderd/database/queries/provisionerjobs.sql | 10 ++-------- 4 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 coderd/database/migrations/000274_check_tags.down.sql create mode 100644 coderd/database/migrations/000274_check_tags.up.sql diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql new file mode 100644 index 0000000000000..5b1db4c8facea --- /dev/null +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -0,0 +1 @@ +DROP FUNCTION IF EXISTS tags_compatible(jsonb, jsonb); diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql new file mode 100644 index 0000000000000..6c4bb6a575440 --- /dev/null +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -0,0 +1,10 @@ +CREATE OR REPLACE FUNCTION tags_compatible(provisioner_tags jsonb, required_tags jsonb) +RETURNS boolean as $$ +BEGIN + RETURN CASE + WHEN provisioner_tags = '{"scope": "organization", "owner": ""}' :: jsonb + THEN provisioner_tags = required_tags + ELSE required_tags <@ provisioner_tags + END; +END; +$$ LANGUAGE plpgsql; diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index bee1c6e92ff4b..6f46b12ff86e6 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -10,7 +10,11 @@ SELECT FROM provisioner_daemons WHERE - organization_id = @organization_id; + -- If organization_id is provided, filter by it; otherwise, allow all. + (@organization_id IS NULL OR organization_id = @organization_id) + AND + -- If tags are provided, check compatibility; otherwise, skip tags check. + (@tags IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 95a84fcd3c824..9ab61dc3476d7 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -22,13 +22,7 @@ WHERE AND nested.organization_id = @organization_id -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY(@types :: provisioner_type [ ]) - AND CASE - -- Special case for untagged provisioners: only match untagged jobs. - WHEN nested.tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb - THEN nested.tags :: jsonb = @tags :: jsonb - -- Ensure the caller satisfies all job tags. - ELSE nested.tags :: jsonb <@ @tags :: jsonb - END + AND tags_compatible(nested.tags, @tags) ORDER BY nested.created_at FOR UPDATE @@ -160,4 +154,4 @@ RETURNING *; -- name: GetProvisionerJobTimingsByJobID :many SELECT * FROM provisioner_job_timings WHERE job_id = $1 -ORDER BY started_at ASC; \ No newline at end of file +ORDER BY started_at ASC; From 92b9d251372b5f2b4db1258392393c36407b3fe4 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 8 Nov 2024 14:08:12 +0000 Subject: [PATCH 02/39] feat(coderd/database): allow filtering provisioner daemons by tags --- coderd/database/dbauthz/dbauthz.go | 2 +- coderd/database/dbauthz/dbauthz_test.go | 2 +- coderd/database/dbmem/dbmem.go | 5 +++-- coderd/database/dbmetrics/querymetrics.go | 4 ++-- coderd/database/dbmock/dbmock.go | 2 +- coderd/database/dump.sql | 12 ++++++++++++ coderd/database/querier.go | 2 +- coderd/database/queries.sql.go | 23 +++++++++++++---------- enterprise/coderd/provisionerdaemons.go | 2 +- enterprise/coderd/provisionerkeys.go | 2 +- 10 files changed, 36 insertions(+), 20 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go index c855d5a1984df..35a052586fbaf 100644 --- a/coderd/database/dbauthz/dbauthz.go +++ b/coderd/database/dbauthz/dbauthz.go @@ -1888,7 +1888,7 @@ func (q *querier) GetProvisionerDaemons(ctx context.Context) ([]database.Provisi return fetchWithPostFilter(q.auth, policy.ActionRead, fetch)(ctx, nil) } -func (q *querier) GetProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]database.ProvisionerDaemon, error) { +func (q *querier) GetProvisionerDaemonsByOrganization(ctx context.Context, organizationID database.GetProvisionerDaemonsByOrganizationParams) ([]database.ProvisionerDaemon, error) { return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetProvisionerDaemonsByOrganization)(ctx, organizationID) } diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index b610efe0349f5..7bf420a65a119 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2066,7 +2066,7 @@ func (s *MethodTestSuite) TestExtraMethods() { }), }) s.NoError(err, "insert provisioner daemon") - ds, err := db.GetProvisionerDaemonsByOrganization(context.Background(), org.ID) + ds, err := db.GetProvisionerDaemonsByOrganization(context.Background(), database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: org.ID}) s.NoError(err, "get provisioner daemon by org") check.Args(org.ID).Asserts(d, policy.ActionRead).Returns(ds) })) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 9a306db09785e..56ed00ba3a206 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3625,13 +3625,14 @@ func (q *FakeQuerier) GetProvisionerDaemons(_ context.Context) ([]database.Provi return out, nil } -func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, organizationID uuid.UUID) ([]database.ProvisionerDaemon, error) { +func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg database.GetProvisionerDaemonsByOrganizationParams) ([]database.ProvisionerDaemon, error) { q.mutex.RLock() defer q.mutex.RUnlock() daemons := make([]database.ProvisionerDaemon, 0) for _, daemon := range q.provisionerDaemons { - if daemon.OrganizationID == organizationID { + // TODO (sas): filter by tags here + if daemon.OrganizationID == arg.OrganizationID { daemon.Tags = maps.Clone(daemon.Tags) daemons = append(daemons, daemon) } diff --git a/coderd/database/dbmetrics/querymetrics.go b/coderd/database/dbmetrics/querymetrics.go index cee25e482bbaa..4592100992352 100644 --- a/coderd/database/dbmetrics/querymetrics.go +++ b/coderd/database/dbmetrics/querymetrics.go @@ -959,9 +959,9 @@ func (m queryMetricsStore) GetProvisionerDaemons(ctx context.Context) ([]databas return daemons, err } -func (m queryMetricsStore) GetProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]database.ProvisionerDaemon, error) { +func (m queryMetricsStore) GetProvisionerDaemonsByOrganization(ctx context.Context, arg database.GetProvisionerDaemonsByOrganizationParams) ([]database.ProvisionerDaemon, error) { start := time.Now() - r0, r1 := m.s.GetProvisionerDaemonsByOrganization(ctx, organizationID) + r0, r1 := m.s.GetProvisionerDaemonsByOrganization(ctx, arg) m.queryLatencies.WithLabelValues("GetProvisionerDaemonsByOrganization").Observe(time.Since(start).Seconds()) return r0, r1 } diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go index d8721f56d3f4e..1b13c8c25a1e1 100644 --- a/coderd/database/dbmock/dbmock.go +++ b/coderd/database/dbmock/dbmock.go @@ -1973,7 +1973,7 @@ func (mr *MockStoreMockRecorder) GetProvisionerDaemons(arg0 any) *gomock.Call { } // GetProvisionerDaemonsByOrganization mocks base method. -func (m *MockStore) GetProvisionerDaemonsByOrganization(arg0 context.Context, arg1 uuid.UUID) ([]database.ProvisionerDaemon, error) { +func (m *MockStore) GetProvisionerDaemonsByOrganization(arg0 context.Context, arg1 database.GetProvisionerDaemonsByOrganizationParams) ([]database.ProvisionerDaemon, error) { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetProvisionerDaemonsByOrganization", arg0, arg1) ret0, _ := ret[0].([]database.ProvisionerDaemon) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 557b5c2dd9325..79e427f158d24 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -396,6 +396,18 @@ BEGIN END; $$; +CREATE FUNCTION tags_compatible(provisioner_tags jsonb, required_tags jsonb) RETURNS boolean + LANGUAGE plpgsql + AS $$ +BEGIN + RETURN CASE + WHEN provisioner_tags = '{"scope": "organization", "owner": ""}' :: jsonb + THEN provisioner_tags = required_tags + ELSE required_tags <@ provisioner_tags + END; +END; +$$; + CREATE FUNCTION tailnet_notify_agent_change() RETURNS trigger LANGUAGE plpgsql AS $$ diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 46d1b1ae5b322..c7bf9e837c7b8 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -196,7 +196,7 @@ type sqlcQuerier interface { GetParameterSchemasByJobID(ctx context.Context, jobID uuid.UUID) ([]ParameterSchema, error) GetPreviousTemplateVersion(ctx context.Context, arg GetPreviousTemplateVersionParams) (TemplateVersion, error) GetProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, error) - GetProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]ProvisionerDaemon, error) + GetProvisionerDaemonsByOrganization(ctx context.Context, arg GetProvisionerDaemonsByOrganizationParams) ([]ProvisionerDaemon, error) GetProvisionerJobByID(ctx context.Context, id uuid.UUID) (ProvisionerJob, error) GetProvisionerJobTimingsByJobID(ctx context.Context, jobID uuid.UUID) ([]ProvisionerJobTiming, error) GetProvisionerJobsByIDs(ctx context.Context, ids []uuid.UUID) ([]ProvisionerJob, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 87d3c17f5400f..a41777f0d6536 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5269,11 +5269,20 @@ SELECT FROM provisioner_daemons WHERE - organization_id = $1 + -- If organization_id is provided, filter by it; otherwise, allow all. + ($1 IS NULL OR organization_id = $1) + AND + -- If tags are provided, check compatibility; otherwise, skip tags check. + ($2 IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) ` -func (q *sqlQuerier) GetProvisionerDaemonsByOrganization(ctx context.Context, organizationID uuid.UUID) ([]ProvisionerDaemon, error) { - rows, err := q.db.QueryContext(ctx, getProvisionerDaemonsByOrganization, organizationID) +type GetProvisionerDaemonsByOrganizationParams struct { + OrganizationID interface{} `db:"organization_id" json:"organization_id"` + Tags interface{} `db:"tags" json:"tags"` +} + +func (q *sqlQuerier) GetProvisionerDaemonsByOrganization(ctx context.Context, arg GetProvisionerDaemonsByOrganizationParams) ([]ProvisionerDaemon, error) { + rows, err := q.db.QueryContext(ctx, getProvisionerDaemonsByOrganization, arg.OrganizationID, arg.Tags) if err != nil { return nil, err } @@ -5529,13 +5538,7 @@ WHERE AND nested.organization_id = $3 -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY($4 :: provisioner_type [ ]) - AND CASE - -- Special case for untagged provisioners: only match untagged jobs. - WHEN nested.tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb - THEN nested.tags :: jsonb = $5 :: jsonb - -- Ensure the caller satisfies all job tags. - ELSE nested.tags :: jsonb <@ $5 :: jsonb - END + AND tags_compatible(nested.tags, $5) ORDER BY nested.created_at FOR UPDATE diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 6f8cd1a3ec0b6..667342ec0f782 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -62,7 +62,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - daemons, err := api.Database.GetProvisionerDaemonsByOrganization(ctx, org.ID) + daemons, err := api.Database.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: org.ID}) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching provisioner daemons.", diff --git a/enterprise/coderd/provisionerkeys.go b/enterprise/coderd/provisionerkeys.go index 0d153ffef1791..0d715c707b779 100644 --- a/enterprise/coderd/provisionerkeys.go +++ b/enterprise/coderd/provisionerkeys.go @@ -137,7 +137,7 @@ func (api *API) provisionerKeyDaemons(rw http.ResponseWriter, r *http.Request) { } sdkKeys := convertProvisionerKeys(pks) - daemons, err := api.Database.GetProvisionerDaemonsByOrganization(ctx, organization.ID) + daemons, err := api.Database.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: organization.ID}) if err != nil { httpapi.InternalServerError(rw, err) return From e68a8fcaa5e252ff5175d2fcbaca6014b793124b Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 8 Nov 2024 14:15:38 +0000 Subject: [PATCH 03/39] add a type hint for sqlc --- coderd/database/queries/provisionerdaemons.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 6f46b12ff86e6..180dc19cde92f 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE (@organization_id IS NULL OR organization_id = @organization_id) AND -- If tags are provided, check compatibility; otherwise, skip tags check. - (@tags IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); + (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago From 93058e670ef84315db2f3d7d258ff7f3d6ae277f Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Fri, 8 Nov 2024 14:33:00 +0000 Subject: [PATCH 04/39] ensure BC with GetProvisionerDaemonsByOrganization --- coderd/database/queries.sql.go | 14 +++++++++----- coderd/database/queries/provisionerdaemons.sql | 10 +++++++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index a41777f0d6536..bb0b2c768026d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5269,16 +5269,20 @@ SELECT FROM provisioner_daemons WHERE - -- If organization_id is provided, filter by it; otherwise, allow all. + -- This is the original search criteria: ($1 IS NULL OR organization_id = $1) AND - -- If tags are provided, check compatibility; otherwise, skip tags check. - ($2 IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) + -- adding support for searching by tags: + ($2 :: jsonb IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) + AND + -- Because we're adding @tags as a second search parameter, we need to do this check to + -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: + ($1 IS NOT NULL OR $2 IS NOT NULL) ` type GetProvisionerDaemonsByOrganizationParams struct { - OrganizationID interface{} `db:"organization_id" json:"organization_id"` - Tags interface{} `db:"tags" json:"tags"` + OrganizationID interface{} `db:"organization_id" json:"organization_id"` + Tags json.RawMessage `db:"tags" json:"tags"` } func (q *sqlQuerier) GetProvisionerDaemonsByOrganization(ctx context.Context, arg GetProvisionerDaemonsByOrganizationParams) ([]ProvisionerDaemon, error) { diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 180dc19cde92f..68d0c8098f2aa 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -10,11 +10,15 @@ SELECT FROM provisioner_daemons WHERE - -- If organization_id is provided, filter by it; otherwise, allow all. + -- This is the original search criteria: (@organization_id IS NULL OR organization_id = @organization_id) AND - -- If tags are provided, check compatibility; otherwise, skip tags check. - (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); + -- adding support for searching by tags: + (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)) + AND + -- Because we're adding @tags as a second search parameter, we need to do this check to + -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: + (@organization_id IS NOT NULL OR @tags IS NOT NULL); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago From a2bfae31a2cfe5697a2510634580b0318a9b9d7b Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 08:24:52 +0000 Subject: [PATCH 05/39] add a type hint for sqlc --- coderd/database/queries.sql.go | 6 +++--- coderd/database/queries/provisionerdaemons.sql | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index bb0b2c768026d..d4801ef39aaa9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5270,18 +5270,18 @@ FROM provisioner_daemons WHERE -- This is the original search criteria: - ($1 IS NULL OR organization_id = $1) + ($1 :: uuid IS NULL OR organization_id = $1 :: uuid) AND -- adding support for searching by tags: ($2 :: jsonb IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) AND -- Because we're adding @tags as a second search parameter, we need to do this check to -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: - ($1 IS NOT NULL OR $2 IS NOT NULL) + ($1 :: uuid IS NOT NULL OR $2 IS NOT NULL) ` type GetProvisionerDaemonsByOrganizationParams struct { - OrganizationID interface{} `db:"organization_id" json:"organization_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` Tags json.RawMessage `db:"tags" json:"tags"` } diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 68d0c8098f2aa..952b871450269 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -11,14 +11,14 @@ FROM provisioner_daemons WHERE -- This is the original search criteria: - (@organization_id IS NULL OR organization_id = @organization_id) + (@organization_id :: uuid IS NULL OR organization_id = @organization_id :: uuid) AND -- adding support for searching by tags: (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)) AND -- Because we're adding @tags as a second search parameter, we need to do this check to -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: - (@organization_id IS NOT NULL OR @tags IS NOT NULL); + (@organization_id :: uuid IS NOT NULL OR @tags IS NOT NULL); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago From 39ab8a0355ce8a7a554226538cac4c10d5a87c54 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 08:57:16 +0000 Subject: [PATCH 06/39] fix unit test --- coderd/database/dbauthz/dbauthz_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 7bf420a65a119..e7492b3c96765 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2068,7 +2068,7 @@ func (s *MethodTestSuite) TestExtraMethods() { s.NoError(err, "insert provisioner daemon") ds, err := db.GetProvisionerDaemonsByOrganization(context.Background(), database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: org.ID}) s.NoError(err, "get provisioner daemon by org") - check.Args(org.ID).Asserts(d, policy.ActionRead).Returns(ds) + check.Args(database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: org.ID}).Asserts(d, policy.ActionRead).Returns(ds) })) s.Run("DeleteOldProvisionerDaemons", s.Subtest(func(db database.Store, check *expects) { _, err := db.UpsertProvisionerDaemon(context.Background(), database.UpsertProvisionerDaemonParams{ From 8bf79c8e2432530b071c71c1a4cc136a999fc6fb Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 12:56:13 +0000 Subject: [PATCH 07/39] Add tags param to provisioner daemons endpoint --- coderd/apidoc/docs.go | 9 ++++++ coderd/apidoc/swagger.json | 9 ++++++ coderd/database/queries.sql.go | 6 +--- .../database/queries/provisionerdaemons.sql | 8 ++--- codersdk/organizations.go | 21 +++++++++---- docs/reference/api/enterprise.md | 7 +++-- enterprise/cli/provisionerdaemonstart_test.go | 6 ++-- enterprise/coderd/provisionerdaemons.go | 30 ++++++++++++++++++- enterprise/coderd/provisionerdaemons_test.go | 2 +- 9 files changed, 74 insertions(+), 24 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 6c770c18232ac..302f5fcdee762 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2941,6 +2941,15 @@ const docTemplate = `{ "name": "organization", "in": "path", "required": true + }, + { + "type": "array", + "items": { + "type": "string" + }, + "description": "Provisioner tags to filter by", + "name": "tags", + "in": "query" } ], "responses": { diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 4f5ca444f703e..20e4ddc8b3817 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2579,6 +2579,15 @@ "name": "organization", "in": "path", "required": true + }, + { + "type": "array", + "items": { + "type": "string" + }, + "description": "Provisioner tags to filter by", + "name": "tags", + "in": "query" } ], "responses": { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d4801ef39aaa9..c89d514b02434 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5270,14 +5270,10 @@ FROM provisioner_daemons WHERE -- This is the original search criteria: - ($1 :: uuid IS NULL OR organization_id = $1 :: uuid) + organization_id = $1 :: uuid AND -- adding support for searching by tags: ($2 :: jsonb IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) - AND - -- Because we're adding @tags as a second search parameter, we need to do this check to - -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: - ($1 :: uuid IS NOT NULL OR $2 IS NOT NULL) ` type GetProvisionerDaemonsByOrganizationParams struct { diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 952b871450269..e70da31c96d4d 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -11,14 +11,10 @@ FROM provisioner_daemons WHERE -- This is the original search criteria: - (@organization_id :: uuid IS NULL OR organization_id = @organization_id :: uuid) + organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)) - AND - -- Because we're adding @tags as a second search parameter, we need to do this check to - -- ensure that the first parameter's behavior remains unchanged when no second parameter is provided: - (@organization_id :: uuid IS NOT NULL OR @tags IS NOT NULL); + (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 77e24a2be3e10..0952c7a935ca7 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" "strings" "time" @@ -314,11 +315,21 @@ func (c *Client) ProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, e return daemons, json.NewDecoder(res.Body).Decode(&daemons) } -func (c *Client) OrganizationProvisionerDaemons(ctx context.Context, organizationID uuid.UUID) ([]ProvisionerDaemon, error) { - res, err := c.Request(ctx, http.MethodGet, - fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons", organizationID.String()), - nil, - ) +func (c *Client) OrganizationProvisionerDaemons(ctx context.Context, organizationID uuid.UUID, tags []string) ([]ProvisionerDaemon, error) { + baseURL := fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons", organizationID.String()) + + queryParams := url.Values{} + if len(tags) > 0 { + for _, tag := range tags { + queryParams.Add("tags", tag) + } + } + + if len(queryParams) > 0 { + baseURL = fmt.Sprintf("%s?%s", baseURL, queryParams.Encode()) + } + + res, err := c.Request(ctx, http.MethodGet, baseURL, nil) if err != nil { return nil, xerrors.Errorf("execute request: %w", err) } diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index 57ffa5260edde..1aca8d6f69ec7 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -1480,9 +1480,10 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/provisi ### Parameters -| Name | In | Type | Required | Description | -| -------------- | ---- | ------------ | -------- | --------------- | -| `organization` | path | string(uuid) | true | Organization ID | +| Name | In | Type | Required | Description | +| -------------- | ----- | ------------- | -------- | ----------------------------- | +| `organization` | path | string(uuid) | true | Organization ID | +| `tags` | query | array[string] | false | Provisioner tags to filter by | ### Example responses diff --git a/enterprise/cli/provisionerdaemonstart_test.go b/enterprise/cli/provisionerdaemonstart_test.go index 3132e80a4c68e..763ac49b92996 100644 --- a/enterprise/cli/provisionerdaemonstart_test.go +++ b/enterprise/cli/provisionerdaemonstart_test.go @@ -236,7 +236,7 @@ func TestProvisionerDaemon_SessionToken(t *testing.T) { var daemons []codersdk.ProvisionerDaemon var err error require.Eventually(t, func() bool { - daemons, err = client.OrganizationProvisionerDaemons(ctx, anotherOrg.ID) + daemons, err = client.OrganizationProvisionerDaemons(ctx, anotherOrg.ID, nil) if err != nil { return false } @@ -282,7 +282,7 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { var daemons []codersdk.ProvisionerDaemon require.Eventually(t, func() bool { - daemons, err = client.OrganizationProvisionerDaemons(ctx, user.OrganizationID) + daemons, err = client.OrganizationProvisionerDaemons(ctx, user.OrganizationID, nil) if err != nil { return false } @@ -376,7 +376,7 @@ func TestProvisionerDaemon_ProvisionerKey(t *testing.T) { var daemons []codersdk.ProvisionerDaemon require.Eventually(t, func() bool { - daemons, err = client.OrganizationProvisionerDaemons(ctx, anotherOrg.ID) + daemons, err = client.OrganizationProvisionerDaemons(ctx, anotherOrg.ID, nil) if err != nil { return false } diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 667342ec0f782..8a029d44b0781 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -3,6 +3,7 @@ package coderd import ( "context" "database/sql" + "encoding/json" "fmt" "io" "net/http" @@ -56,13 +57,22 @@ func (api *API) provisionerDaemonsEnabledMW(next http.Handler) http.Handler { // @Produce json // @Tags Enterprise // @Param organization path string true "Organization ID" format(uuid) +// @Param tags query []string false "Provisioner tags to filter by" // @Success 200 {array} codersdk.ProvisionerDaemon // @Router /organizations/{organization}/provisionerdaemons [get] func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - daemons, err := api.Database.GetProvisionerDaemonsByOrganization(ctx, database.GetProvisionerDaemonsByOrganizationParams{OrganizationID: org.ID}) + tags := provisionerTags(r) + + daemons, err := api.Database.GetProvisionerDaemonsByOrganization( + ctx, + database.GetProvisionerDaemonsByOrganizationParams{ + OrganizationID: org.ID, + Tags: tags, + }, + ) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error fetching provisioner daemons.", @@ -74,6 +84,24 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) } +func provisionerTags(r *http.Request) json.RawMessage { + tags := r.URL.Query()["tags"] + if len(tags) == 0 { + return json.RawMessage("{}") + } + + var pairs []string + for _, tag := range tags { + parts := strings.SplitN(tag, "=", 2) + if len(parts) == 2 { + pairs = append(pairs, fmt.Sprintf(`%q:%q`, parts[0], parts[1])) + } + } + + jsonString := fmt.Sprintf("{%s}", strings.Join(pairs, ",")) + return json.RawMessage(jsonString) +} + type provisiionerDaemonAuthResponse struct { keyID uuid.UUID orgID uuid.UUID diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index 09a4de5806e88..f5d253ce0b50f 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -768,7 +768,7 @@ func TestGetProvisionerDaemons(t *testing.T) { require.NoError(t, err) srv.DRPCConn().Close() - daemons, err := orgAdmin.OrganizationProvisionerDaemons(ctx, org.ID) + daemons, err := orgAdmin.OrganizationProvisionerDaemons(ctx, org.ID, nil) require.NoError(t, err) require.Len(t, daemons, 1) From 86f7dab01b079bc70d305da9d4f85860d259ca83 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 14:46:55 +0000 Subject: [PATCH 08/39] fix provisioner tag filtering --- coderd/database/dump.sql | 8 ++++---- .../database/migrations/000274_check_tags.up.sql | 8 ++++---- coderd/database/queries.sql.go | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- enterprise/coderd/provisionerdaemons.go | 15 +++++++-------- 5 files changed, 17 insertions(+), 18 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 79e427f158d24..2d3d3013e3d15 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -396,14 +396,14 @@ BEGIN END; $$; -CREATE FUNCTION tags_compatible(provisioner_tags jsonb, required_tags jsonb) RETURNS boolean +CREATE FUNCTION tags_compatible(subset_tags jsonb, superset_tags jsonb) RETURNS boolean LANGUAGE plpgsql AS $$ BEGIN RETURN CASE - WHEN provisioner_tags = '{"scope": "organization", "owner": ""}' :: jsonb - THEN provisioner_tags = required_tags - ELSE required_tags <@ provisioner_tags + WHEN superset_tags = '{"scope": "organization", "owner": ""}' :: jsonb + THEN subset_tags = superset_tags + ELSE subset_tags <@ superset_tags END; END; $$; diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 6c4bb6a575440..983e8c6fdf712 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -1,10 +1,10 @@ -CREATE OR REPLACE FUNCTION tags_compatible(provisioner_tags jsonb, required_tags jsonb) +CREATE OR REPLACE FUNCTION tags_compatible(subset_tags jsonb, superset_tags jsonb) RETURNS boolean as $$ BEGIN RETURN CASE - WHEN provisioner_tags = '{"scope": "organization", "owner": ""}' :: jsonb - THEN provisioner_tags = required_tags - ELSE required_tags <@ provisioner_tags + WHEN superset_tags = '{"scope": "organization", "owner": ""}' :: jsonb + THEN subset_tags = superset_tags + ELSE subset_tags <@ superset_tags END; END; $$ LANGUAGE plpgsql; diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index c89d514b02434..d365faa40fa80 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5538,7 +5538,7 @@ WHERE AND nested.organization_id = $3 -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY($4 :: provisioner_type [ ]) - AND tags_compatible(nested.tags, $5) + AND tags_compatible($5, nested.tags) ORDER BY nested.created_at FOR UPDATE diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 9ab61dc3476d7..9a47d9a5e1c6d 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -22,7 +22,7 @@ WHERE AND nested.organization_id = @organization_id -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY(@types :: provisioner_type [ ]) - AND tags_compatible(nested.tags, @tags) + AND tags_compatible(@tags, nested.tags) ORDER BY nested.created_at FOR UPDATE diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 8a029d44b0781..a066629683d72 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -65,12 +65,12 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { org := httpmw.OrganizationParam(r) tags := provisionerTags(r) - + tagsJSON, err := json.Marshal(tags) daemons, err := api.Database.GetProvisionerDaemonsByOrganization( ctx, database.GetProvisionerDaemonsByOrganizationParams{ OrganizationID: org.ID, - Tags: tags, + Tags: tagsJSON, }, ) if err != nil { @@ -84,22 +84,21 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) } -func provisionerTags(r *http.Request) json.RawMessage { +func provisionerTags(r *http.Request) map[string]string { tags := r.URL.Query()["tags"] if len(tags) == 0 { - return json.RawMessage("{}") + return nil } - var pairs []string + var tagMap map[string]string for _, tag := range tags { parts := strings.SplitN(tag, "=", 2) if len(parts) == 2 { - pairs = append(pairs, fmt.Sprintf(`%q:%q`, parts[0], parts[1])) + tagMap[parts[0]] = parts[1] } } - jsonString := fmt.Sprintf("{%s}", strings.Join(pairs, ",")) - return json.RawMessage(jsonString) + return tagMap } type provisiionerDaemonAuthResponse struct { From bd6f3ed4249108266c2b721f23ab98b4a7fb1c7e Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 15:40:49 +0000 Subject: [PATCH 09/39] handle tag reading error --- enterprise/coderd/provisionerdaemons.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index a066629683d72..4e0282dd0076b 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -66,6 +66,14 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { tags := provisionerTags(r) tagsJSON, err := json.Marshal(tags) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error reading tags.", + Detail: err.Error(), + }) + return + } + daemons, err := api.Database.GetProvisionerDaemonsByOrganization( ctx, database.GetProvisionerDaemonsByOrganizationParams{ From a2476dc5b02a94b703eb54cda4e420de1322d161 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 15:46:45 +0000 Subject: [PATCH 10/39] remove assignment to a nil map --- enterprise/coderd/provisionerdaemons.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 4e0282dd0076b..582bbe0b3daea 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -98,10 +98,11 @@ func provisionerTags(r *http.Request) map[string]string { return nil } - var tagMap map[string]string + tagMap := map[string]string{} for _, tag := range tags { parts := strings.SplitN(tag, "=", 2) if len(parts) == 2 { + // TODO(sas): seems like the kind of place where nilpointers would pop up. tagMap[parts[0]] = parts[1] } } From e835326a099e055885cf3857041463f6fefe8a5f Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 16:19:17 +0000 Subject: [PATCH 11/39] filter by tags in dbmem --- coderd/database/dbmem/dbmem.go | 20 ++++++++++++++++---- enterprise/coderd/provisionerdaemons.go | 11 +++++------ 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 56ed00ba3a206..cdcb6bddc6c30 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3631,11 +3631,23 @@ func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg daemons := make([]database.ProvisionerDaemon, 0) for _, daemon := range q.provisionerDaemons { - // TODO (sas): filter by tags here - if daemon.OrganizationID == arg.OrganizationID { - daemon.Tags = maps.Clone(daemon.Tags) - daemons = append(daemons, daemon) + if daemon.OrganizationID != arg.OrganizationID { + continue + } + if arg.Tags != nil { + var argTags map[string]string + err := json.Unmarshal(arg.Tags, &argTags) + if err != nil { + continue + } + for k, v := range argTags { + if t, found := daemon.Tags[k]; !found || t != v { + continue + } + } } + daemon.Tags = maps.Clone(daemon.Tags) + daemons = append(daemons, daemon) } return daemons, nil diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 582bbe0b3daea..0edb3d2532793 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -64,8 +64,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - tags := provisionerTags(r) - tagsJSON, err := json.Marshal(tags) + tags, err := provisionerTags(r) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Internal error reading tags.", @@ -78,7 +77,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx, database.GetProvisionerDaemonsByOrganizationParams{ OrganizationID: org.ID, - Tags: tagsJSON, + Tags: tags, }, ) if err != nil { @@ -92,10 +91,10 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) } -func provisionerTags(r *http.Request) map[string]string { +func provisionerTags(r *http.Request) ([]byte, error) { tags := r.URL.Query()["tags"] if len(tags) == 0 { - return nil + return nil, nil } tagMap := map[string]string{} @@ -107,7 +106,7 @@ func provisionerTags(r *http.Request) map[string]string { } } - return tagMap + return json.Marshal(tagMap) } type provisiionerDaemonAuthResponse struct { From 23bd23f185d8b71000175759dc5b0b5ee34a9e66 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 16:32:59 +0000 Subject: [PATCH 12/39] correctly set the default value when no tags are provided --- enterprise/coderd/provisionerdaemons.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 0edb3d2532793..e5a4bc556c16b 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -94,7 +94,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { func provisionerTags(r *http.Request) ([]byte, error) { tags := r.URL.Query()["tags"] if len(tags) == 0 { - return nil, nil + return []byte("{}"), nil } tagMap := map[string]string{} From 9985ef6e428dcbf9fc609c27f6546ae2705fde16 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Mon, 11 Nov 2024 16:59:58 +0000 Subject: [PATCH 13/39] support the zero value tag in sql --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/provisionerdaemons.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index d365faa40fa80..3f6bb88c0da81 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,7 +5273,7 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: jsonb IS NULL OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) + ($2 :: jsonb = '{}' :: jsonb OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) ` type GetProvisionerDaemonsByOrganizationParams struct { diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index e70da31c96d4d..e5568bc12d63c 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: jsonb IS NULL OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); + (@tags :: jsonb = '{}' :: jsonb OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago From a173bfbb6b90f86a1ce027d11c3fcdb51fb5e56b Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 08:13:38 +0000 Subject: [PATCH 14/39] update the default value for when no tags are provided --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/provisionerdaemons.sql | 2 +- enterprise/coderd/provisionerdaemons.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3f6bb88c0da81..ecb5e76d6c271 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,7 +5273,7 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: jsonb = '{}' :: jsonb OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) + ($2 :: jsonb = '' :: jsonb OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) ` type GetProvisionerDaemonsByOrganizationParams struct { diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index e5568bc12d63c..131ea2be6c22f 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: jsonb = '{}' :: jsonb OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); + (@tags :: jsonb = '' :: jsonb OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index e5a4bc556c16b..b0e04e7740156 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -94,7 +94,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { func provisionerTags(r *http.Request) ([]byte, error) { tags := r.URL.Query()["tags"] if len(tags) == 0 { - return []byte("{}"), nil + return []byte{}, nil } tagMap := map[string]string{} From 70c7ea90d83743de685c230d505f35b493496581 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 09:03:21 +0000 Subject: [PATCH 15/39] use a sql domain as a type alias so that we can override it as a StringMap in sqlc --- coderd/database/dbmem/dbmem.go | 7 +------ coderd/database/dump.sql | 8 +++++--- .../migrations/000274_check_tags.up.sql | 8 +++++--- coderd/database/queries.sql.go | 8 ++++---- coderd/database/queries/provisionerdaemons.sql | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- coderd/database/sqlc.yaml | 3 +++ enterprise/coderd/provisionerdaemons.go | 17 ++++------------- 8 files changed, 24 insertions(+), 31 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index cdcb6bddc6c30..f5f37daa9633d 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3635,12 +3635,7 @@ func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg continue } if arg.Tags != nil { - var argTags map[string]string - err := json.Unmarshal(arg.Tags, &argTags) - if err != nil { - continue - } - for k, v := range argTags { + for k, v := range arg.Tags { if t, found := daemon.Tags[k]; !found || t != v { continue } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 2d3d3013e3d15..0a816900a281c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -198,6 +198,8 @@ CREATE TYPE startup_script_behavior AS ENUM ( 'non-blocking' ); +CREATE DOMAIN tags AS jsonb; + CREATE TYPE tailnet_status AS ENUM ( 'ok', 'lost' @@ -396,14 +398,14 @@ BEGIN END; $$; -CREATE FUNCTION tags_compatible(subset_tags jsonb, superset_tags jsonb) RETURNS boolean +CREATE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) RETURNS boolean LANGUAGE plpgsql AS $$ BEGIN RETURN CASE - WHEN superset_tags = '{"scope": "organization", "owner": ""}' :: jsonb + WHEN superset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb THEN subset_tags = superset_tags - ELSE subset_tags <@ superset_tags + ELSE subset_tags :: jsonb <@ superset_tags :: jsonb END; END; $$; diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 983e8c6fdf712..6886c9b6bf92e 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -1,10 +1,12 @@ -CREATE OR REPLACE FUNCTION tags_compatible(subset_tags jsonb, superset_tags jsonb) +CREATE DOMAIN tags AS jsonb; + +CREATE OR REPLACE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) RETURNS boolean as $$ BEGIN RETURN CASE - WHEN superset_tags = '{"scope": "organization", "owner": ""}' :: jsonb + WHEN superset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb THEN subset_tags = superset_tags - ELSE subset_tags <@ superset_tags + ELSE subset_tags :: jsonb <@ superset_tags :: jsonb END; END; $$ LANGUAGE plpgsql; diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index ecb5e76d6c271..de41d76cb07a9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,12 +5273,12 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: jsonb = '' :: jsonb OR tags_compatible($2 :: jsonb, provisioner_daemons.tags :: jsonb)) + ($2 :: tags = '{}' :: tags OR tags_compatible($2::tags, provisioner_daemons.tags::tags)) ` type GetProvisionerDaemonsByOrganizationParams struct { - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - Tags json.RawMessage `db:"tags" json:"tags"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + Tags StringMap `db:"tags" json:"tags"` } func (q *sqlQuerier) GetProvisionerDaemonsByOrganization(ctx context.Context, arg GetProvisionerDaemonsByOrganizationParams) ([]ProvisionerDaemon, error) { @@ -5538,7 +5538,7 @@ WHERE AND nested.organization_id = $3 -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY($4 :: provisioner_type [ ]) - AND tags_compatible($5, nested.tags) + AND tags_compatible($5 :: jsonb, nested.tags) ORDER BY nested.created_at FOR UPDATE diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 131ea2be6c22f..e1a691372d74b 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: jsonb = '' :: jsonb OR tags_compatible(@tags :: jsonb, provisioner_daemons.tags :: jsonb)); + (@tags :: tags = '{}' :: tags OR tags_compatible(@tags::tags, provisioner_daemons.tags::tags)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 9a47d9a5e1c6d..3993c078acdd1 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -22,7 +22,7 @@ WHERE AND nested.organization_id = @organization_id -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY(@types :: provisioner_type [ ]) - AND tags_compatible(@tags, nested.tags) + AND tags_compatible(@tags :: jsonb, nested.tags) ORDER BY nested.created_at FOR UPDATE diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 2161feb47e1c3..2fa96d587a932 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -35,6 +35,9 @@ sql: - db_type: "name_organization_pair" go_type: type: "NameOrganizationPair" + - db_type: "tags" + go_type: + type: "StringMap" - column: "custom_roles.site_permissions" go_type: type: "CustomRolePermissions" diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index b0e04e7740156..aff76a78653be 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -3,7 +3,6 @@ package coderd import ( "context" "database/sql" - "encoding/json" "fmt" "io" "net/http" @@ -64,15 +63,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - tags, err := provisionerTags(r) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error reading tags.", - Detail: err.Error(), - }) - return - } - + tags := provisionerTags(r) daemons, err := api.Database.GetProvisionerDaemonsByOrganization( ctx, database.GetProvisionerDaemonsByOrganizationParams{ @@ -91,10 +82,10 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) } -func provisionerTags(r *http.Request) ([]byte, error) { +func provisionerTags(r *http.Request) map[string]string { tags := r.URL.Query()["tags"] if len(tags) == 0 { - return []byte{}, nil + return nil } tagMap := map[string]string{} @@ -106,7 +97,7 @@ func provisionerTags(r *http.Request) ([]byte, error) { } } - return json.Marshal(tagMap) + return tagMap } type provisiionerDaemonAuthResponse struct { From 1c57bd29bb245f0a8d1ea4851e3bc282804108dd Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 09:12:05 +0000 Subject: [PATCH 16/39] complete down migration --- coderd/database/migrations/000274_check_tags.down.sql | 2 ++ 1 file changed, 2 insertions(+) diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql index 5b1db4c8facea..de810214e6b2e 100644 --- a/coderd/database/migrations/000274_check_tags.down.sql +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -1 +1,3 @@ DROP FUNCTION IF EXISTS tags_compatible(jsonb, jsonb); + +DROP DOMAIN IF EXISTS tags; From ebb716db58cc46a2099c1d85fc779f13a9dc1fa5 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 09:22:11 +0000 Subject: [PATCH 17/39] complete down migration --- coderd/database/migrations/000274_check_tags.down.sql | 2 +- coderd/database/migrations/000274_check_tags.up.sql | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql index de810214e6b2e..fccb6be93f824 100644 --- a/coderd/database/migrations/000274_check_tags.down.sql +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -1,3 +1,3 @@ -DROP FUNCTION IF EXISTS tags_compatible(jsonb, jsonb); +DROP FUNCTION IF EXISTS tags_compatible(tags, tags); DROP DOMAIN IF EXISTS tags; diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 6886c9b6bf92e..d13677924fe54 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -1,3 +1,6 @@ +-- We need this as a type alias to ensure that we can override it in sqlc. +-- We need to override it in sqlc so that it can be recognised as a StringMap. +-- Without this zero values for other inferred types cause json syntax errors. CREATE DOMAIN tags AS jsonb; CREATE OR REPLACE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) From 96b64f4f4c8c6551f063d2efcba52813fe8cda13 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 09:53:58 +0000 Subject: [PATCH 18/39] update the default value for when no tags are provided --- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/provisionerdaemons.sql | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index de41d76cb07a9..f333ddcf2144d 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,7 +5273,7 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: tags = '{}' :: tags OR tags_compatible($2::tags, provisioner_daemons.tags::tags)) + ($2 :: tags = 'null' :: tags OR tags_compatible($2::tags, provisioner_daemons.tags::tags)) ` type GetProvisionerDaemonsByOrganizationParams struct { @@ -5538,7 +5538,7 @@ WHERE AND nested.organization_id = $3 -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY($4 :: provisioner_type [ ]) - AND tags_compatible($5 :: jsonb, nested.tags) + AND tags_compatible(nested.tags, $5 :: jsonb) ORDER BY nested.created_at FOR UPDATE diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index e1a691372d74b..7d9f5583e8437 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: tags = '{}' :: tags OR tags_compatible(@tags::tags, provisioner_daemons.tags::tags)); + (@tags :: tags = 'null' :: tags OR tags_compatible(@tags::tags, provisioner_daemons.tags::tags)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 3993c078acdd1..cc2e4407648f3 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -22,7 +22,7 @@ WHERE AND nested.organization_id = @organization_id -- Ensure the caller has the correct provisioner. AND nested.provisioner = ANY(@types :: provisioner_type [ ]) - AND tags_compatible(@tags :: jsonb, nested.tags) + AND tags_compatible(nested.tags, @tags :: jsonb) ORDER BY nested.created_at FOR UPDATE From 5941dc3203c9b12907412343f809717d989406ee Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 10:51:44 +0000 Subject: [PATCH 19/39] fix job acquisition for untagged provisioners --- coderd/database/migrations/000274_check_tags.up.sql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index d13677924fe54..62ebc749f3484 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -7,7 +7,8 @@ CREATE OR REPLACE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) RETURNS boolean as $$ BEGIN RETURN CASE - WHEN superset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb + -- Special case for untagged provisioners + WHEN subset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb THEN subset_tags = superset_tags ELSE subset_tags :: jsonb <@ superset_tags :: jsonb END; From ff75f5e04ea10b5e7b8d95bf81873ecf035f5e0b Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 11:02:48 +0000 Subject: [PATCH 20/39] make gen --- coderd/database/dump.sql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 0a816900a281c..c24324d398863 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -403,7 +403,8 @@ CREATE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) RETURNS bo AS $$ BEGIN RETURN CASE - WHEN superset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb + -- Special case for untagged provisioners + WHEN subset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb THEN subset_tags = superset_tags ELSE subset_tags :: jsonb <@ superset_tags :: jsonb END; From 85b1ef409dff6ce375bcdb13a3fa68f093cad392 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 18:58:20 +0000 Subject: [PATCH 21/39] review notes --- coderd/database/dbauthz/dbauthz_test.go | 2 +- coderd/database/dbfake/dbfake.go | 4 +-- coderd/database/dbgen/dbgen.go | 10 +++--- coderd/database/dbmem/dbmem.go | 12 +++---- coderd/database/dump.sql | 19 ++++++----- .../migrations/000274_check_tags.down.sql | 2 +- .../migrations/000274_check_tags.up.sql | 22 ++++++------- coderd/database/querier_test.go | 2 +- coderd/database/queries.sql.go | 32 ++++++++++--------- .../database/queries/provisionerdaemons.sql | 2 +- coderd/database/queries/provisionerjobs.sql | 14 ++++---- coderd/database/sqlc.yaml | 2 +- coderd/provisionerdserver/acquirer.go | 4 +-- enterprise/coderd/schedule/template_test.go | 8 ++--- 14 files changed, 70 insertions(+), 65 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index e7492b3c96765..413d79a84a400 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -2560,7 +2560,7 @@ func (s *MethodTestSuite) TestSystemFunctions() { j := dbgen.ProvisionerJob(s.T(), db, nil, database.ProvisionerJob{ StartedAt: sql.NullTime{Valid: false}, }) - check.Args(database.AcquireProvisionerJobParams{OrganizationID: j.OrganizationID, Types: []database.ProvisionerType{j.Provisioner}, Tags: must(json.Marshal(j.Tags))}). + check.Args(database.AcquireProvisionerJobParams{OrganizationID: j.OrganizationID, Types: []database.ProvisionerType{j.Provisioner}, ProvisionerTags: must(json.Marshal(j.Tags))}). Asserts( /*rbac.ResourceSystem, policy.ActionUpdate*/ ) })) s.Run("UpdateProvisionerJobWithCompleteByID", s.Subtest(func(db database.Store, check *expects) { diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go index ca514479cab6a..9c5a09f40ff65 100644 --- a/coderd/database/dbfake/dbfake.go +++ b/coderd/database/dbfake/dbfake.go @@ -194,8 +194,8 @@ func (b WorkspaceBuildBuilder) Do() WorkspaceResponse { UUID: uuid.New(), Valid: true, }, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - Tags: []byte(`{"scope": "organization"}`), + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: []byte(`{"scope": "organization"}`), }) require.NoError(b.t, err, "acquire starting job") if j.ID == job.ID { diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go index 5e83125a93b84..793795af7762a 100644 --- a/coderd/database/dbgen/dbgen.go +++ b/coderd/database/dbgen/dbgen.go @@ -531,11 +531,11 @@ func ProvisionerJob(t testing.TB, db database.Store, ps pubsub.Pubsub, orig data } if !orig.StartedAt.Time.IsZero() { job, err = db.AcquireProvisionerJob(genCtx, database.AcquireProvisionerJobParams{ - StartedAt: orig.StartedAt, - OrganizationID: job.OrganizationID, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - Tags: must(json.Marshal(orig.Tags)), - WorkerID: uuid.NullUUID{}, + StartedAt: orig.StartedAt, + OrganizationID: job.OrganizationID, + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: must(json.Marshal(orig.Tags)), + WorkerID: uuid.NullUUID{}, }) require.NoError(t, err) // There is no easy way to make sure we acquire the correct job. diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index f5f37daa9633d..fbfb9fd0547f4 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1194,8 +1194,8 @@ func (q *FakeQuerier) AcquireProvisionerJob(_ context.Context, arg database.Acqu continue } tags := map[string]string{} - if arg.Tags != nil { - err := json.Unmarshal(arg.Tags, &tags) + if arg.ProvisionerTags != nil { + err := json.Unmarshal(arg.ProvisionerTags, &tags) if err != nil { return provisionerJob, xerrors.Errorf("unmarshal: %w", err) } @@ -3634,11 +3634,9 @@ func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg if daemon.OrganizationID != arg.OrganizationID { continue } - if arg.Tags != nil { - for k, v := range arg.Tags { - if t, found := daemon.Tags[k]; !found || t != v { - continue - } + for k, v := range arg.WantTags { + if t, found := daemon.Tags[k]; !found || t != v { + continue } } daemon.Tags = maps.Clone(daemon.Tags) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index c24324d398863..f428093fcf146 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -198,7 +198,9 @@ CREATE TYPE startup_script_behavior AS ENUM ( 'non-blocking' ); -CREATE DOMAIN tags AS jsonb; +CREATE DOMAIN tagset AS jsonb; + +COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; CREATE TYPE tailnet_status AS ENUM ( 'ok', @@ -398,19 +400,20 @@ BEGIN END; $$; -CREATE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) RETURNS boolean +CREATE FUNCTION tagset_contains(superset tagset, subset tagset) RETURNS boolean LANGUAGE plpgsql AS $$ BEGIN - RETURN CASE - -- Special case for untagged provisioners - WHEN subset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb - THEN subset_tags = superset_tags - ELSE subset_tags :: jsonb <@ superset_tags :: jsonb - END; + RETURN + -- Special case for untagged provisioners, where only an exact match should count + (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + -- General case + OR subset <@ superset; END; $$; +COMMENT ON FUNCTION tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; + CREATE FUNCTION tailnet_notify_agent_change() RETURNS trigger LANGUAGE plpgsql AS $$ diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql index fccb6be93f824..dc4a79a4d48ce 100644 --- a/coderd/database/migrations/000274_check_tags.down.sql +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -1,3 +1,3 @@ -DROP FUNCTION IF EXISTS tags_compatible(tags, tags); +DROP FUNCTION IF EXISTS tagset_contains(tags, tags); DROP DOMAIN IF EXISTS tags; diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 62ebc749f3484..b95cb05f4d765 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -1,16 +1,16 @@ --- We need this as a type alias to ensure that we can override it in sqlc. --- We need to override it in sqlc so that it can be recognised as a StringMap. --- Without this zero values for other inferred types cause json syntax errors. -CREATE DOMAIN tags AS jsonb; +CREATE DOMAIN tagset AS jsonb; -CREATE OR REPLACE FUNCTION tags_compatible(subset_tags tags, superset_tags tags) +COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; + +CREATE OR REPLACE FUNCTION tagset_contains(superset tagset, subset tagset) RETURNS boolean as $$ BEGIN - RETURN CASE - -- Special case for untagged provisioners - WHEN subset_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb - THEN subset_tags = superset_tags - ELSE subset_tags :: jsonb <@ superset_tags :: jsonb - END; + RETURN + -- Special case for untagged provisioners, where only an exact match should count + (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + -- General case + OR subset <@ superset; END; $$ LANGUAGE plpgsql; + +COMMENT ON FUNCTION tagset_contains(tagset, tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index b3fde5a558e6b..619e9868b612f 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1020,7 +1020,7 @@ func TestQueuePosition(t *testing.T) { UUID: uuid.New(), Valid: true, }, - Tags: json.RawMessage("{}"), + ProvisionerTags: json.RawMessage("{}"), }) require.NoError(t, err) require.Equal(t, jobs[0].ID, job.ID) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f333ddcf2144d..671dfd3cf88fe 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,16 +5273,16 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: tags = 'null' :: tags OR tags_compatible($2::tags, provisioner_daemons.tags::tags)) + ($2 :: tagset = 'null' :: tagset OR tagset_contains(provisioner_daemons.tags::tagset, $2::tagset)) ` type GetProvisionerDaemonsByOrganizationParams struct { OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - Tags StringMap `db:"tags" json:"tags"` + WantTags StringMap `db:"want_tags" json:"want_tags"` } func (q *sqlQuerier) GetProvisionerDaemonsByOrganization(ctx context.Context, arg GetProvisionerDaemonsByOrganizationParams) ([]ProvisionerDaemon, error) { - rows, err := q.db.QueryContext(ctx, getProvisionerDaemonsByOrganization, arg.OrganizationID, arg.Tags) + rows, err := q.db.QueryContext(ctx, getProvisionerDaemonsByOrganization, arg.OrganizationID, arg.WantTags) if err != nil { return nil, err } @@ -5532,15 +5532,17 @@ WHERE SELECT id FROM - provisioner_jobs AS nested + provisioner_jobs AS potential_job WHERE - nested.started_at IS NULL - AND nested.organization_id = $3 + potential_job.started_at IS NULL + AND potential_job.organization_id = $3 -- Ensure the caller has the correct provisioner. - AND nested.provisioner = ANY($4 :: provisioner_type [ ]) - AND tags_compatible(nested.tags, $5 :: jsonb) + AND potential_job.provisioner = ANY($4 :: provisioner_type [ ]) + -- elsewhere, whe use the tagset type, but here we use jsonb for backward compatibility + -- they are aliases and the code that calls this query already relies on a different type + AND tagset_contains($5 :: jsonb, potential_job.tags) ORDER BY - nested.created_at + potential_job.created_at FOR UPDATE SKIP LOCKED LIMIT @@ -5549,11 +5551,11 @@ WHERE ` type AcquireProvisionerJobParams struct { - StartedAt sql.NullTime `db:"started_at" json:"started_at"` - WorkerID uuid.NullUUID `db:"worker_id" json:"worker_id"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` - Types []ProvisionerType `db:"types" json:"types"` - Tags json.RawMessage `db:"tags" json:"tags"` + StartedAt sql.NullTime `db:"started_at" json:"started_at"` + WorkerID uuid.NullUUID `db:"worker_id" json:"worker_id"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + Types []ProvisionerType `db:"types" json:"types"` + ProvisionerTags json.RawMessage `db:"provisioner_tags" json:"provisioner_tags"` } // Acquires the lock for a single job that isn't started, completed, @@ -5568,7 +5570,7 @@ func (q *sqlQuerier) AcquireProvisionerJob(ctx context.Context, arg AcquireProvi arg.WorkerID, arg.OrganizationID, pq.Array(arg.Types), - arg.Tags, + arg.ProvisionerTags, ) var i ProvisionerJob err := row.Scan( diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 7d9f5583e8437..157de0da2604d 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@tags :: tags = 'null' :: tags OR tags_compatible(@tags::tags, provisioner_daemons.tags::tags)); + (@want_tags :: tagset = 'null' :: tagset OR tagset_contains(provisioner_daemons.tags::tagset, @want_tags::tagset)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index cc2e4407648f3..70175a26d4627 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -16,15 +16,17 @@ WHERE SELECT id FROM - provisioner_jobs AS nested + provisioner_jobs AS potential_job WHERE - nested.started_at IS NULL - AND nested.organization_id = @organization_id + potential_job.started_at IS NULL + AND potential_job.organization_id = @organization_id -- Ensure the caller has the correct provisioner. - AND nested.provisioner = ANY(@types :: provisioner_type [ ]) - AND tags_compatible(nested.tags, @tags :: jsonb) + AND potential_job.provisioner = ANY(@types :: provisioner_type [ ]) + -- elsewhere, whe use the tagset type, but here we use jsonb for backward compatibility + -- they are aliases and the code that calls this query already relies on a different type + AND tagset_contains(@provisioner_tags :: jsonb, potential_job.tags) ORDER BY - nested.created_at + potential_job.created_at FOR UPDATE SKIP LOCKED LIMIT diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index 2fa96d587a932..6a9a66ee45a9b 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -35,7 +35,7 @@ sql: - db_type: "name_organization_pair" go_type: type: "NameOrganizationPair" - - db_type: "tags" + - db_type: "tagset" go_type: type: "StringMap" - column: "custom_roles.site_permissions" diff --git a/coderd/provisionerdserver/acquirer.go b/coderd/provisionerdserver/acquirer.go index 36e0d51df44f8..4c2fe6b1d49a9 100644 --- a/coderd/provisionerdserver/acquirer.go +++ b/coderd/provisionerdserver/acquirer.go @@ -130,8 +130,8 @@ func (a *Acquirer) AcquireJob( UUID: worker, Valid: true, }, - Types: pt, - Tags: dbTags, + Types: pt, + ProvisionerTags: dbTags, }) if xerrors.Is(err, sql.ErrNoRows) { logger.Debug(ctx, "no job available") diff --git a/enterprise/coderd/schedule/template_test.go b/enterprise/coderd/schedule/template_test.go index c85c2c6ea1b0e..f2871928c0ac0 100644 --- a/enterprise/coderd/schedule/template_test.go +++ b/enterprise/coderd/schedule/template_test.go @@ -248,8 +248,8 @@ func TestTemplateUpdateBuildDeadlines(t *testing.T) { UUID: uuid.New(), Valid: true, }, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - Tags: json.RawMessage(fmt.Sprintf(`{%q: "yeah"}`, c.name)), + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: json.RawMessage(fmt.Sprintf(`{%q: "yeah"}`, c.name)), }) require.NoError(t, err) require.Equal(t, job.ID, acquiredJob.ID) @@ -532,8 +532,8 @@ func TestTemplateUpdateBuildDeadlinesSkip(t *testing.T) { UUID: uuid.New(), Valid: true, }, - Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, - Tags: json.RawMessage(fmt.Sprintf(`{%q: "yeah"}`, wsID)), + Types: []database.ProvisionerType{database.ProvisionerTypeEcho}, + ProvisionerTags: json.RawMessage(fmt.Sprintf(`{%q: "yeah"}`, wsID)), }) require.NoError(t, err) require.Equal(t, job.ID, acquiredJob.ID) From 071fdc465b5302db84367f2eb51b6b43d87c2031 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 19:05:25 +0000 Subject: [PATCH 22/39] fix down migration --- coderd/database/migrations/000274_check_tags.down.sql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql index dc4a79a4d48ce..c829417885661 100644 --- a/coderd/database/migrations/000274_check_tags.down.sql +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -1,3 +1,3 @@ -DROP FUNCTION IF EXISTS tagset_contains(tags, tags); +DROP FUNCTION IF EXISTS tagset_contains(tagset, tagset); -DROP DOMAIN IF EXISTS tags; +DROP DOMAIN IF EXISTS tagset; From 78abf67f43cb26b7a71874a339a56a8e8ea0829a Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 19:14:45 +0000 Subject: [PATCH 23/39] use the correct name for provisionerdaemons' tag field --- enterprise/coderd/provisionerdaemons.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index aff76a78653be..113bb0f2a5bc0 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -68,7 +68,7 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx, database.GetProvisionerDaemonsByOrganizationParams{ OrganizationID: org.ID, - Tags: tags, + WantTags: tags, }, ) if err != nil { From d1c7d3e4d8a561d1585321aa1025b9fea1f7d003 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 19:30:38 +0000 Subject: [PATCH 24/39] typo --- coderd/database/queries.sql.go | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 671dfd3cf88fe..bf727be92e907 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5538,7 +5538,7 @@ WHERE AND potential_job.organization_id = $3 -- Ensure the caller has the correct provisioner. AND potential_job.provisioner = ANY($4 :: provisioner_type [ ]) - -- elsewhere, whe use the tagset type, but here we use jsonb for backward compatibility + -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type AND tagset_contains($5 :: jsonb, potential_job.tags) ORDER BY diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 70175a26d4627..91c5dafb21adf 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -22,7 +22,7 @@ WHERE AND potential_job.organization_id = @organization_id -- Ensure the caller has the correct provisioner. AND potential_job.provisioner = ANY(@types :: provisioner_type [ ]) - -- elsewhere, whe use the tagset type, but here we use jsonb for backward compatibility + -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type AND tagset_contains(@provisioner_tags :: jsonb, potential_job.tags) ORDER BY From 38d77cf8d9b0fa88e496106b0d2a2e28afe5a4c3 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 19:34:51 +0000 Subject: [PATCH 25/39] use the updated tag name --- coderd/provisionerdserver/acquirer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/provisionerdserver/acquirer_test.go b/coderd/provisionerdserver/acquirer_test.go index a916cb68fba1f..6f0face1ebb4c 100644 --- a/coderd/provisionerdserver/acquirer_test.go +++ b/coderd/provisionerdserver/acquirer_test.go @@ -649,7 +649,7 @@ func (s *fakeTaggedStore) AcquireProvisionerJob( ) { defer func() { s.params <- params }() var tags provisionerdserver.Tags - err := json.Unmarshal(params.Tags, &tags) + err := json.Unmarshal(params.ProvisionerTags, &tags) if !assert.NoError(s.t, err) { return database.ProvisionerJob{}, err } From 1c643539bf9226075e15c9d9a21829292c0ccf33 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Tue, 12 Nov 2024 19:44:10 +0000 Subject: [PATCH 26/39] fix special case of tagset_contains --- coderd/database/migrations/000274_check_tags.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index b95cb05f4d765..f84e123ba98b7 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -7,7 +7,7 @@ RETURNS boolean as $$ BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + (superset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) -- General case OR subset <@ superset; END; From 8508cd8144d6a6a306f718836dca27c79947c8f8 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 07:57:50 +0000 Subject: [PATCH 27/39] Use a stringmap instead of a stringslice for provisioner tags --- coderd/apidoc/docs.go | 7 ++--- coderd/apidoc/swagger.json | 7 ++--- coderd/database/dump.sql | 2 +- codersdk/organizations.go | 10 +++---- docs/reference/api/enterprise.md | 8 +++--- enterprise/coderd/provisionerdaemons.go | 36 +++++++++++-------------- 6 files changed, 30 insertions(+), 40 deletions(-) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 302f5fcdee762..d03c8b9be989d 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -2943,11 +2943,8 @@ const docTemplate = `{ "required": true }, { - "type": "array", - "items": { - "type": "string" - }, - "description": "Provisioner tags to filter by", + "type": "object", + "description": "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})", "name": "tags", "in": "query" } diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 20e4ddc8b3817..b740ea82e8abb 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -2581,11 +2581,8 @@ "required": true }, { - "type": "array", - "items": { - "type": "string" - }, - "description": "Provisioner tags to filter by", + "type": "object", + "description": "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})", "name": "tags", "in": "query" } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index f428093fcf146..e6ff68d30f8fa 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -406,7 +406,7 @@ CREATE FUNCTION tagset_contains(superset tagset, subset tagset) RETURNS boolean BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + (superset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) -- General case OR subset <@ superset; END; diff --git a/codersdk/organizations.go b/codersdk/organizations.go index 0952c7a935ca7..4966b7a41809c 100644 --- a/codersdk/organizations.go +++ b/codersdk/organizations.go @@ -315,16 +315,16 @@ func (c *Client) ProvisionerDaemons(ctx context.Context) ([]ProvisionerDaemon, e return daemons, json.NewDecoder(res.Body).Decode(&daemons) } -func (c *Client) OrganizationProvisionerDaemons(ctx context.Context, organizationID uuid.UUID, tags []string) ([]ProvisionerDaemon, error) { +func (c *Client) OrganizationProvisionerDaemons(ctx context.Context, organizationID uuid.UUID, tags map[string]string) ([]ProvisionerDaemon, error) { baseURL := fmt.Sprintf("/api/v2/organizations/%s/provisionerdaemons", organizationID.String()) queryParams := url.Values{} - if len(tags) > 0 { - for _, tag := range tags { - queryParams.Add("tags", tag) - } + tagsJSON, err := json.Marshal(tags) + if err != nil { + return nil, xerrors.Errorf("marshal tags: %w", err) } + queryParams.Add("tags", string(tagsJSON)) if len(queryParams) > 0 { baseURL = fmt.Sprintf("%s?%s", baseURL, queryParams.Encode()) } diff --git a/docs/reference/api/enterprise.md b/docs/reference/api/enterprise.md index 1aca8d6f69ec7..b8764fd89b449 100644 --- a/docs/reference/api/enterprise.md +++ b/docs/reference/api/enterprise.md @@ -1480,10 +1480,10 @@ curl -X GET http://coder-server:8080/api/v2/organizations/{organization}/provisi ### Parameters -| Name | In | Type | Required | Description | -| -------------- | ----- | ------------- | -------- | ----------------------------- | -| `organization` | path | string(uuid) | true | Organization ID | -| `tags` | query | array[string] | false | Provisioner tags to filter by | +| Name | In | Type | Required | Description | +| -------------- | ----- | ------------ | -------- | ---------------------------------------------------------------------------------- | +| `organization` | path | string(uuid) | true | Organization ID | +| `tags` | query | object | false | Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'}) | ### Example responses diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 113bb0f2a5bc0..9f889ad43d092 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -3,6 +3,7 @@ package coderd import ( "context" "database/sql" + "encoding/json" "fmt" "io" "net/http" @@ -56,14 +57,27 @@ func (api *API) provisionerDaemonsEnabledMW(next http.Handler) http.Handler { // @Produce json // @Tags Enterprise // @Param organization path string true "Organization ID" format(uuid) -// @Param tags query []string false "Provisioner tags to filter by" +// @Param tags query coderd.provisionerDaemons.tagsQuery false "Provisioner tags to filter by" +// @Param tags query object false "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})" // @Success 200 {array} codersdk.ProvisionerDaemon // @Router /organizations/{organization}/provisionerdaemons [get] func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - tags := provisionerTags(r) + // For documentation purposes above: + type tagsQuery = map[string]string + + var tags tagsQuery + err := json.Unmarshal([]byte(r.URL.Query().Get("tags")), &tags) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid tags query parameter", + Detail: err.Error(), + }) + return + } + daemons, err := api.Database.GetProvisionerDaemonsByOrganization( ctx, database.GetProvisionerDaemonsByOrganizationParams{ @@ -82,24 +96,6 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusOK, db2sdk.List(daemons, db2sdk.ProvisionerDaemon)) } -func provisionerTags(r *http.Request) map[string]string { - tags := r.URL.Query()["tags"] - if len(tags) == 0 { - return nil - } - - tagMap := map[string]string{} - for _, tag := range tags { - parts := strings.SplitN(tag, "=", 2) - if len(parts) == 2 { - // TODO(sas): seems like the kind of place where nilpointers would pop up. - tagMap[parts[0]] = parts[1] - } - } - - return tagMap -} - type provisiionerDaemonAuthResponse struct { keyID uuid.UUID orgID uuid.UUID From 9dcbb83a1ea6b455f7461d8a86cf904060a125c7 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 09:13:16 +0000 Subject: [PATCH 28/39] update tags param parsing in provisionerDaemons --- enterprise/coderd/provisionerdaemons.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 9f889ad43d092..3ba48858a3c9a 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -3,7 +3,6 @@ package coderd import ( "context" "database/sql" - "encoding/json" "fmt" "io" "net/http" @@ -57,7 +56,6 @@ func (api *API) provisionerDaemonsEnabledMW(next http.Handler) http.Handler { // @Produce json // @Tags Enterprise // @Param organization path string true "Organization ID" format(uuid) -// @Param tags query coderd.provisionerDaemons.tagsQuery false "Provisioner tags to filter by" // @Param tags query object false "Provisioner tags to filter by (JSON of the form {'tag1':'value1','tag2':'value2'})" // @Success 200 {array} codersdk.ProvisionerDaemon // @Router /organizations/{organization}/provisionerdaemons [get] @@ -65,17 +63,17 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() org := httpmw.OrganizationParam(r) - // For documentation purposes above: - type tagsQuery = map[string]string - - var tags tagsQuery - err := json.Unmarshal([]byte(r.URL.Query().Get("tags")), &tags) - if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid tags query parameter", - Detail: err.Error(), - }) - return + tags := database.StringMap{} + tagParam := r.URL.Query()["tags"] + if len(tagParam) > 0 { + err := tags.Scan([]byte(tagParam[0])) + if err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid tags query parameter", + Detail: err.Error(), + }) + return + } } daemons, err := api.Database.GetProvisionerDaemonsByOrganization( From 4c1fc0de87c6f233b3e2ecde171e6a495b495e70 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 09:26:27 +0000 Subject: [PATCH 29/39] update tags param parsing in provisionerDaemons --- enterprise/coderd/provisionerdaemons.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 3ba48858a3c9a..b29787ed1d3a3 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -64,9 +64,8 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { org := httpmw.OrganizationParam(r) tags := database.StringMap{} - tagParam := r.URL.Query()["tags"] - if len(tagParam) > 0 { - err := tags.Scan([]byte(tagParam[0])) + if tagParam := r.URL.Query().Get("tags"); tagParam != "" { + err := tags.Scan([]byte(tagParam)) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid tags query parameter", From fbd70f31d12021bb9b9267d749039d3a76c523e1 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 09:30:45 +0000 Subject: [PATCH 30/39] fix special case of tagset_contains --- coderd/database/migrations/000274_check_tags.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index f84e123ba98b7..b95cb05f4d765 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -7,7 +7,7 @@ RETURNS boolean as $$ BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (superset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) -- General case OR subset <@ superset; END; From 69126f472df5374cbb7f2a7c1361dcc9e60c50c0 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 09:38:34 +0000 Subject: [PATCH 31/39] rename tagset_contains function --- coderd/database/dump.sql | 4 ++-- coderd/database/migrations/000274_check_tags.down.sql | 2 +- coderd/database/migrations/000274_check_tags.up.sql | 4 ++-- coderd/database/queries.sql.go | 4 ++-- coderd/database/queries/provisionerdaemons.sql | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index e6ff68d30f8fa..3617abe25b488 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -400,7 +400,7 @@ BEGIN END; $$; -CREATE FUNCTION tagset_contains(superset tagset, subset tagset) RETURNS boolean +CREATE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) RETURNS boolean LANGUAGE plpgsql AS $$ BEGIN @@ -412,7 +412,7 @@ BEGIN END; $$; -COMMENT ON FUNCTION tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; +COMMENT ON FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; CREATE FUNCTION tailnet_notify_agent_change() RETURNS trigger LANGUAGE plpgsql diff --git a/coderd/database/migrations/000274_check_tags.down.sql b/coderd/database/migrations/000274_check_tags.down.sql index c829417885661..623a3e9dac6e5 100644 --- a/coderd/database/migrations/000274_check_tags.down.sql +++ b/coderd/database/migrations/000274_check_tags.down.sql @@ -1,3 +1,3 @@ -DROP FUNCTION IF EXISTS tagset_contains(tagset, tagset); +DROP FUNCTION IF EXISTS provisioner_tagset_contains(tagset, tagset); DROP DOMAIN IF EXISTS tagset; diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index b95cb05f4d765..3e47a2d89ffa6 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -2,7 +2,7 @@ CREATE DOMAIN tagset AS jsonb; COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; -CREATE OR REPLACE FUNCTION tagset_contains(superset tagset, subset tagset) +CREATE OR REPLACE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) RETURNS boolean as $$ BEGIN RETURN @@ -13,4 +13,4 @@ BEGIN END; $$ LANGUAGE plpgsql; -COMMENT ON FUNCTION tagset_contains(tagset, tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; +COMMENT ON FUNCTION provisioner_tagset_contains(tagset, tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index bf727be92e907..52559313f2298 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5273,7 +5273,7 @@ WHERE organization_id = $1 :: uuid AND -- adding support for searching by tags: - ($2 :: tagset = 'null' :: tagset OR tagset_contains(provisioner_daemons.tags::tagset, $2::tagset)) + ($2 :: tagset = 'null' :: tagset OR provisioner_tagset_contains(provisioner_daemons.tags::tagset, $2::tagset)) ` type GetProvisionerDaemonsByOrganizationParams struct { @@ -5540,7 +5540,7 @@ WHERE AND potential_job.provisioner = ANY($4 :: provisioner_type [ ]) -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type - AND tagset_contains($5 :: jsonb, potential_job.tags) + AND provisioner_tagset_contains($5 :: jsonb, potential_job.tags) ORDER BY potential_job.created_at FOR UPDATE diff --git a/coderd/database/queries/provisionerdaemons.sql b/coderd/database/queries/provisionerdaemons.sql index 157de0da2604d..a6633c91158a9 100644 --- a/coderd/database/queries/provisionerdaemons.sql +++ b/coderd/database/queries/provisionerdaemons.sql @@ -14,7 +14,7 @@ WHERE organization_id = @organization_id :: uuid AND -- adding support for searching by tags: - (@want_tags :: tagset = 'null' :: tagset OR tagset_contains(provisioner_daemons.tags::tagset, @want_tags::tagset)); + (@want_tags :: tagset = 'null' :: tagset OR provisioner_tagset_contains(provisioner_daemons.tags::tagset, @want_tags::tagset)); -- name: DeleteOldProvisionerDaemons :exec -- Delete provisioner daemons that have been created at least a week ago diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 91c5dafb21adf..8b74376a99b05 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -24,7 +24,7 @@ WHERE AND potential_job.provisioner = ANY(@types :: provisioner_type [ ]) -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type - AND tagset_contains(@provisioner_tags :: jsonb, potential_job.tags) + AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags) ORDER BY potential_job.created_at FOR UPDATE From e109caf6d8aae51ac5df030d396fef6b03539216 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 13:08:45 +0000 Subject: [PATCH 32/39] attempt to fix the special case for provisioner_tagset_contains --- coderd/database/dbmem/dbmem.go | 14 +++++++--- coderd/database/dump.sql | 28 +++++++++---------- .../migrations/000274_check_tags.up.sql | 4 +-- 3 files changed, 26 insertions(+), 20 deletions(-) diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index fbfb9fd0547f4..002170da508c8 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -3634,10 +3634,16 @@ func (q *FakeQuerier) GetProvisionerDaemonsByOrganization(_ context.Context, arg if daemon.OrganizationID != arg.OrganizationID { continue } - for k, v := range arg.WantTags { - if t, found := daemon.Tags[k]; !found || t != v { - continue - } + // Special case for untagged provisioners: only match untagged jobs. + // Ref: coderd/database/queries/provisionerjobs.sql:24-30 + // CASE WHEN nested.tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb + // THEN nested.tags :: jsonb = @tags :: jsonb + if tagsEqual(arg.WantTags, tagsUntagged) && !tagsEqual(arg.WantTags, daemon.Tags) { + continue + } + // ELSE nested.tags :: jsonb <@ @tags :: jsonb + if !tagsSubset(arg.WantTags, daemon.Tags) { + continue } daemon.Tags = maps.Clone(daemon.Tags) daemons = append(daemons, daemon) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 3617abe25b488..2d52969a53ca6 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -380,6 +380,20 @@ BEGIN END; $$; +CREATE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) RETURNS boolean + LANGUAGE plpgsql + AS $$ +BEGIN + RETURN + -- Special case for untagged provisioners, where only an exact match should count + (subset = '{"scope": "organization", "owner": ""}' :: jsonb AND subset :: jsonb = superset :: jsonb) + -- General case + OR subset :: jsonb <@ superset :: jsonb; +END; +$$; + +COMMENT ON FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; + CREATE FUNCTION remove_organization_member_role() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -400,20 +414,6 @@ BEGIN END; $$; -CREATE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) RETURNS boolean - LANGUAGE plpgsql - AS $$ -BEGIN - RETURN - -- Special case for untagged provisioners, where only an exact match should count - (superset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) - -- General case - OR subset <@ superset; -END; -$$; - -COMMENT ON FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; - CREATE FUNCTION tailnet_notify_agent_change() RETURNS trigger LANGUAGE plpgsql AS $$ diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 3e47a2d89ffa6..2e37b8d1fea0f 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -7,9 +7,9 @@ RETURNS boolean as $$ BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (subset = '{"scope": "organization", "owner": ""}' :: tagset AND subset = superset) + (subset = '{"scope": "organization", "owner": ""}' :: jsonb AND subset :: jsonb = superset :: jsonb) -- General case - OR subset <@ superset; + OR subset :: jsonb <@ superset :: jsonb; END; $$ LANGUAGE plpgsql; From f10561e91df7fb82f2980ec142480ac5de0897e4 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 13:21:38 +0000 Subject: [PATCH 33/39] attempt to fix the special case for provisioner_tagset_contains --- coderd/database/migrations/000274_check_tags.up.sql | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 2e37b8d1fea0f..94a7246e11ff8 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -2,15 +2,15 @@ CREATE DOMAIN tagset AS jsonb; COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; -CREATE OR REPLACE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) +CREATE OR REPLACE FUNCTION provisioner_tagset_contains(provisioner_tags tagset, job_tags tagset) RETURNS boolean as $$ BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (subset = '{"scope": "organization", "owner": ""}' :: jsonb AND subset :: jsonb = superset :: jsonb) + (job_tags = '{"scope": "organization", "owner": ""}' :: jsonb AND job_tags :: jsonb = provisioner_tags :: jsonb) -- General case - OR subset :: jsonb <@ superset :: jsonb; + OR job_tags :: jsonb <@ provisioner_tags :: jsonb; END; $$ LANGUAGE plpgsql; -COMMENT ON FUNCTION provisioner_tagset_contains(tagset, tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; +COMMENT ON FUNCTION provisioner_tagset_contains(tagset, tagset) IS 'Returns true if the provisioner_tags contains the job_tags, or if the job_tags represents an untagged provisioner and the superset is exactly equal to the subset.'; From a56b130f6df5b35fd3986c66a81bc41ff44181d9 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 13:23:15 +0000 Subject: [PATCH 34/39] attempt to fix the special case for provisioner_tagset_contains --- coderd/database/migrations/000274_check_tags.up.sql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 94a7246e11ff8..9d3352bc33fbe 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -7,7 +7,7 @@ RETURNS boolean as $$ BEGIN RETURN -- Special case for untagged provisioners, where only an exact match should count - (job_tags = '{"scope": "organization", "owner": ""}' :: jsonb AND job_tags :: jsonb = provisioner_tags :: jsonb) + (job_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb AND job_tags :: jsonb = provisioner_tags :: jsonb) -- General case OR job_tags :: jsonb <@ provisioner_tags :: jsonb; END; From 2bce007c789bfbccbae1061268010ccf05e635cc Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Wed, 13 Nov 2024 16:57:21 +0000 Subject: [PATCH 35/39] fix provisioner_tagset_contains --- coderd/database/dump.sql | 11 ++++++----- coderd/database/migrations/000274_check_tags.up.sql | 9 +++++---- coderd/database/queries.sql.go | 2 +- coderd/database/queries/provisionerjobs.sql | 2 +- coderd/provisionerdserver/acquirer_test.go | 2 +- 5 files changed, 14 insertions(+), 12 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 2d52969a53ca6..fa0c4b6845230 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -380,19 +380,20 @@ BEGIN END; $$; -CREATE FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) RETURNS boolean +CREATE FUNCTION provisioner_tagset_contains(provisioner_tags tagset, job_tags tagset) RETURNS boolean LANGUAGE plpgsql AS $$ BEGIN - RETURN + RETURN CASE -- Special case for untagged provisioners, where only an exact match should count - (subset = '{"scope": "organization", "owner": ""}' :: jsonb AND subset :: jsonb = superset :: jsonb) + WHEN job_tags::jsonb = '{"scope": "organization", "owner": ""}'::jsonb THEN job_tags::jsonb = provisioner_tags::jsonb -- General case - OR subset :: jsonb <@ superset :: jsonb; + ELSE job_tags::jsonb <@ provisioner_tags::jsonb + END; END; $$; -COMMENT ON FUNCTION provisioner_tagset_contains(superset tagset, subset tagset) IS 'Returns true if the superset contains the subset, or if the subset represents an untagged provisioner and the superset is exactly equal to the subset.'; +COMMENT ON FUNCTION provisioner_tagset_contains(provisioner_tags tagset, job_tags tagset) IS 'Returns true if the provisioner_tags contains the job_tags, or if the job_tags represents an untagged provisioner and the superset is exactly equal to the subset.'; CREATE FUNCTION remove_organization_member_role() RETURNS trigger LANGUAGE plpgsql diff --git a/coderd/database/migrations/000274_check_tags.up.sql b/coderd/database/migrations/000274_check_tags.up.sql index 9d3352bc33fbe..b897e5e8ea124 100644 --- a/coderd/database/migrations/000274_check_tags.up.sql +++ b/coderd/database/migrations/000274_check_tags.up.sql @@ -3,13 +3,14 @@ CREATE DOMAIN tagset AS jsonb; COMMENT ON DOMAIN tagset IS 'A set of tags that match provisioner daemons to provisioner jobs, which can originate from workspaces or templates. tagset is a narrowed type over jsonb. It is expected to be the JSON representation of map[string]string. That is, {"key1": "value1", "key2": "value2"}. We need the narrowed type instead of just using jsonb so that we can give sqlc a type hint, otherwise it defaults to json.RawMessage. json.RawMessage is a suboptimal type to use in the context that we need tagset for.'; CREATE OR REPLACE FUNCTION provisioner_tagset_contains(provisioner_tags tagset, job_tags tagset) -RETURNS boolean as $$ +RETURNS boolean AS $$ BEGIN - RETURN + RETURN CASE -- Special case for untagged provisioners, where only an exact match should count - (job_tags :: jsonb = '{"scope": "organization", "owner": ""}' :: jsonb AND job_tags :: jsonb = provisioner_tags :: jsonb) + WHEN job_tags::jsonb = '{"scope": "organization", "owner": ""}'::jsonb THEN job_tags::jsonb = provisioner_tags::jsonb -- General case - OR job_tags :: jsonb <@ provisioner_tags :: jsonb; + ELSE job_tags::jsonb <@ provisioner_tags::jsonb + END; END; $$ LANGUAGE plpgsql; diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 52559313f2298..564246e32638c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5540,7 +5540,7 @@ WHERE AND potential_job.provisioner = ANY($4 :: provisioner_type [ ]) -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type - AND provisioner_tagset_contains($5 :: jsonb, potential_job.tags) + AND provisioner_tagset_contains($5 :: jsonb, potential_job.tags :: jsonb) ORDER BY potential_job.created_at FOR UPDATE diff --git a/coderd/database/queries/provisionerjobs.sql b/coderd/database/queries/provisionerjobs.sql index 8b74376a99b05..95e8a88b84e6d 100644 --- a/coderd/database/queries/provisionerjobs.sql +++ b/coderd/database/queries/provisionerjobs.sql @@ -24,7 +24,7 @@ WHERE AND potential_job.provisioner = ANY(@types :: provisioner_type [ ]) -- elsewhere, we use the tagset type, but here we use jsonb for backward compatibility -- they are aliases and the code that calls this query already relies on a different type - AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags) + AND provisioner_tagset_contains(@provisioner_tags :: jsonb, potential_job.tags :: jsonb) ORDER BY potential_job.created_at FOR UPDATE diff --git a/coderd/provisionerdserver/acquirer_test.go b/coderd/provisionerdserver/acquirer_test.go index 6f0face1ebb4c..03238ca9a5b2f 100644 --- a/coderd/provisionerdserver/acquirer_test.go +++ b/coderd/provisionerdserver/acquirer_test.go @@ -473,7 +473,7 @@ func TestAcquirer_MatchTags(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitShort) + ctx := testutil.Context(t, testutil.WaitLong) // NOTE: explicitly not using fake store for this test. db, ps := dbtestutil.NewDB(t) log := slogtest.Make(t, nil).Leveled(slog.LevelDebug) From 2860f5ded4f62d1c463efb61394b4ec7ddc6f5d4 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 14 Nov 2024 07:34:16 +0000 Subject: [PATCH 36/39] remove defunct code --- enterprise/coderd/provisionerdaemons.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index b29787ed1d3a3..85f27168bdb95 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -64,15 +64,13 @@ func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { org := httpmw.OrganizationParam(r) tags := database.StringMap{} - if tagParam := r.URL.Query().Get("tags"); tagParam != "" { - err := tags.Scan([]byte(tagParam)) - if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Invalid tags query parameter", - Detail: err.Error(), - }) - return - } + tagParam := r.URL.Query().Get("tags") + if err := tags.Scan([]byte(tagParam)); err != nil { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Invalid tags query parameter", + Detail: err.Error(), + }) + return } daemons, err := api.Database.GetProvisionerDaemonsByOrganization( From d6f26df2ef1654d37c30e75dd84e4158e3cb8e88 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 14 Nov 2024 09:28:05 +0000 Subject: [PATCH 37/39] Add tag filtering tests for GetProvisionerDaemons --- coderd/provisionerdserver/acquirer_test.go | 2 +- enterprise/coderd/provisionerdaemons.go | 13 +- enterprise/coderd/provisionerdaemons_test.go | 191 +++++++++++++++++++ 3 files changed, 200 insertions(+), 6 deletions(-) diff --git a/coderd/provisionerdserver/acquirer_test.go b/coderd/provisionerdserver/acquirer_test.go index 03238ca9a5b2f..6f0face1ebb4c 100644 --- a/coderd/provisionerdserver/acquirer_test.go +++ b/coderd/provisionerdserver/acquirer_test.go @@ -473,7 +473,7 @@ func TestAcquirer_MatchTags(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - ctx := testutil.Context(t, testutil.WaitLong) + ctx := testutil.Context(t, testutil.WaitShort) // NOTE: explicitly not using fake store for this test. db, ps := dbtestutil.NewDB(t) log := slogtest.Make(t, nil).Leveled(slog.LevelDebug) diff --git a/enterprise/coderd/provisionerdaemons.go b/enterprise/coderd/provisionerdaemons.go index 85f27168bdb95..0eb3a51db57dd 100644 --- a/enterprise/coderd/provisionerdaemons.go +++ b/enterprise/coderd/provisionerdaemons.go @@ -60,12 +60,15 @@ func (api *API) provisionerDaemonsEnabledMW(next http.Handler) http.Handler { // @Success 200 {array} codersdk.ProvisionerDaemon // @Router /organizations/{organization}/provisionerdaemons [get] func (api *API) provisionerDaemons(rw http.ResponseWriter, r *http.Request) { - ctx := r.Context() - org := httpmw.OrganizationParam(r) + var ( + ctx = r.Context() + org = httpmw.OrganizationParam(r) + tagParam = r.URL.Query().Get("tags") + tags = database.StringMap{} + err = tags.Scan([]byte(tagParam)) + ) - tags := database.StringMap{} - tagParam := r.URL.Query().Get("tags") - if err := tags.Scan([]byte(tagParam)); err != nil { + if tagParam != "" && err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid tags query parameter", Detail: err.Error(), diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index f5d253ce0b50f..d61a4f833f732 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -3,10 +3,12 @@ package coderd_test import ( "bytes" "context" + "database/sql" "fmt" "io" "net/http" "testing" + "time" "github.com/google/uuid" "github.com/stretchr/testify/assert" @@ -18,6 +20,7 @@ import ( "github.com/coder/coder/v2/buildinfo" "github.com/coder/coder/v2/coderd/coderdtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/provisionerkey" "github.com/coder/coder/v2/coderd/rbac" @@ -794,4 +797,192 @@ func TestGetProvisionerDaemons(t *testing.T) { _, err = outsideOrg.ListProvisionerKeyDaemons(ctx, org.ID) require.Error(t, err) }) + + t.Run("filtered by tags", func(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + tagsToFilterBy map[string]string + provisionerDaemonTags map[string]string + expectToGetDaemon bool + }{ + { + name: "only an empty tagset finds an untagged provisioner", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": ""}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": ""}, + expectToGetDaemon: true, + }, + { + name: "an exact match with a single optional tag finds a provisioner daemon", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + expectToGetDaemon: true, + }, + { + name: "a subset of filter tags finds a daemon with a superset of tags", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "chicago"}, + expectToGetDaemon: true, + }, + { + name: "an exact match with two additional tags finds a provisioner daemon", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "chicago"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "chicago"}, + expectToGetDaemon: true, + }, + { + name: "a user scoped filter tag set finds a user scoped provisioner daemon", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa"}, + expectToGetDaemon: true, + }, + { + name: "a user scoped filter tag set finds a user scoped provisioner daemon with an additional tag", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + expectToGetDaemon: true, + }, + { + name: "user-scoped provisioner with tags and user-scoped filter with tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + expectToGetDaemon: true, + }, + { + name: "user-scoped provisioner with multiple tags and user-scoped filter with a subset of tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "chicago"}, + expectToGetDaemon: true, + }, + { + name: "user-scoped provisioner with multiple tags and user-scoped filter with multiple tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "chicago"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "chicago"}, + expectToGetDaemon: true, + }, + { + name: "untagged provisioner and tagged filter", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": ""}, + expectToGetDaemon: false, + }, + { + name: "tagged provisioner and untagged filter", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": ""}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + expectToGetDaemon: false, + }, + { + name: "tagged provisioner and double-tagged filter", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "chicago"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem"}, + expectToGetDaemon: false, + }, + { + name: "double-tagged provisioner and double-tagged filter with differing tags", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "chicago"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": "", "environment": "on-prem", "datacenter": "new_york"}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner and untagged filter", + tagsToFilterBy: map[string]string{"scope": "organization", "owner": ""}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa"}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner and different user-scoped filter", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "bbb"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa"}, + expectToGetDaemon: false, + }, + { + name: "org-scoped provisioner and user-scoped filter", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": ""}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner and org-scoped filter with tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "organization", "owner": ""}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner and user-scoped filter with tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa"}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner with tags and user-scoped filter with multiple tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "chicago"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem"}, + expectToGetDaemon: false, + }, + { + name: "user-scoped provisioner with tags and user-scoped filter with differing tags", + tagsToFilterBy: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "new_york"}, + provisionerDaemonTags: map[string]string{"scope": "user", "owner": "aaa", "environment": "on-prem", "datacenter": "chicago"}, + expectToGetDaemon: false, + }, + } + for _, tt := range testCases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + dv := coderdtest.DeploymentValues(t) + client, db, _ := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + DeploymentValues: dv, + }, + ProvisionerDaemonPSK: "provisionersftw", + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureExternalProvisionerDaemons: 1, + codersdk.FeatureMultipleOrganizations: 1, + }, + }, + }) + ctx := testutil.Context(t, testutil.WaitShort) + + org := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{ + IncludeProvisionerDaemon: false, + }) + orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgMember(org.ID)) + + daemonCreatedAt := time.Now() + pd, err := db.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(ctx), database.UpsertProvisionerDaemonParams{ + CreatedAt: daemonCreatedAt, + Name: "Test Provisioner Daemon", + Provisioners: []database.ProvisionerType{}, + Tags: tt.provisionerDaemonTags, + LastSeenAt: sql.NullTime{ + Time: daemonCreatedAt, + Valid: true, + }, + Version: "", + OrganizationID: org.ID, + APIVersion: "", + }) + require.NoError(t, err, "should be able to create provisioner daemon") + daemonAsCreated := db2sdk.ProvisionerDaemon(pd) + + allDaemons, err := orgAdmin.OrganizationProvisionerDaemons(ctx, org.ID, nil) + require.NoError(t, err) + require.Len(t, allDaemons, 1) + + daemonsAsFound, err := orgAdmin.OrganizationProvisionerDaemons(ctx, org.ID, tt.tagsToFilterBy) + if tt.expectToGetDaemon { + require.NoError(t, err) + require.Len(t, daemonsAsFound, 1) + require.Equal(t, daemonAsCreated.Tags, daemonsAsFound[0].Tags, "found daemon should have the same tags as created daemon") + } else { + require.NoError(t, err) + assert.Empty(t, daemonsAsFound, "should not have found daemon") + } + }) + } + }) } From 1ce410f33b10c615fbabd271d84b7402908af894 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 14 Nov 2024 09:39:55 +0000 Subject: [PATCH 38/39] fix foreign key constraint in tests --- enterprise/coderd/provisionerdaemons_test.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index d61a4f833f732..d2039d7d0894b 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -953,6 +953,17 @@ func TestGetProvisionerDaemons(t *testing.T) { orgAdmin, _ := coderdtest.CreateAnotherUser(t, client, org.ID, rbac.ScopedRoleOrgMember(org.ID)) daemonCreatedAt := time.Now() + + provisionerKey, err := db.InsertProvisionerKey(dbauthz.AsSystemRestricted(ctx), database.InsertProvisionerKeyParams{ + Name: "Test Provisioner Key", + ID: uuid.New(), + CreatedAt: daemonCreatedAt, + OrganizationID: org.ID, + HashedSecret: []byte{}, + Tags: tt.provisionerDaemonTags, + }) + require.NoError(t, err, "should be able to create a provisioner key") + pd, err := db.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(ctx), database.UpsertProvisionerDaemonParams{ CreatedAt: daemonCreatedAt, Name: "Test Provisioner Daemon", @@ -965,6 +976,7 @@ func TestGetProvisionerDaemons(t *testing.T) { Version: "", OrganizationID: org.ID, APIVersion: "", + KeyID: provisionerKey.ID, }) require.NoError(t, err, "should be able to create provisioner daemon") daemonAsCreated := db2sdk.ProvisionerDaemon(pd) @@ -978,6 +990,7 @@ func TestGetProvisionerDaemons(t *testing.T) { require.NoError(t, err) require.Len(t, daemonsAsFound, 1) require.Equal(t, daemonAsCreated.Tags, daemonsAsFound[0].Tags, "found daemon should have the same tags as created daemon") + require.Equal(t, daemonsAsFound[0].KeyID, provisionerKey.ID) } else { require.NoError(t, err) assert.Empty(t, daemonsAsFound, "should not have found daemon") From c065742e65e53490464b39e1a4c5aa6392065f83 Mon Sep 17 00:00:00 2001 From: Sas Swart Date: Thu, 14 Nov 2024 09:46:48 +0000 Subject: [PATCH 39/39] Add linting --- enterprise/coderd/provisionerdaemons_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/enterprise/coderd/provisionerdaemons_test.go b/enterprise/coderd/provisionerdaemons_test.go index d2039d7d0894b..d8d770097c156 100644 --- a/enterprise/coderd/provisionerdaemons_test.go +++ b/enterprise/coderd/provisionerdaemons_test.go @@ -954,6 +954,7 @@ func TestGetProvisionerDaemons(t *testing.T) { daemonCreatedAt := time.Now() + //nolint:gocritic // We're not testing auth on the following in this test provisionerKey, err := db.InsertProvisionerKey(dbauthz.AsSystemRestricted(ctx), database.InsertProvisionerKeyParams{ Name: "Test Provisioner Key", ID: uuid.New(), @@ -964,6 +965,7 @@ func TestGetProvisionerDaemons(t *testing.T) { }) require.NoError(t, err, "should be able to create a provisioner key") + //nolint:gocritic // We're not testing auth on the following in this test pd, err := db.UpsertProvisionerDaemon(dbauthz.AsSystemRestricted(ctx), database.UpsertProvisionerDaemonParams{ CreatedAt: daemonCreatedAt, Name: "Test Provisioner Daemon",