From 4cacd0a60558adc5c0a56dee94d5e9dad950d276 Mon Sep 17 00:00:00 2001 From: Louis Ruch Date: Wed, 22 Jun 2022 13:20:27 -0700 Subject: [PATCH] bug(scheduler): Fix controller id length check (#2226) * bug(scheduler): Fix controller id length check The job_run table has a foreign key to the server_controller private_id. While this column is called a prviate_id it does not implement the wt_private_id postgres domain type, and therefore does not have a minimum length of 10 characters. --- CHANGELOG.md | 10 +++ .../34/02_worker_controller_tables.up.sql | 2 + .../oss/postgres/35/01_job_migrations.up.sql | 25 ++++++ internal/db/sqltest/Makefile | 3 +- .../tests/controller/controller_id.sql | 33 ++++++++ internal/scheduler/job/repository_run_test.go | 12 +++ internal/scheduler/job/testing.go | 48 ++++++++++- internal/server/repository_controller_test.go | 83 +++++++++++++++++++ 8 files changed, 211 insertions(+), 5 deletions(-) create mode 100644 internal/db/schema/migrations/oss/postgres/35/01_job_migrations.up.sql create mode 100644 internal/db/sqltest/tests/controller/controller_id.sql create mode 100644 internal/server/repository_controller_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 3fa611263..46b696d87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,16 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ## Next +### Bug Fixes + +* scheduler: Removes a Postgres check constraint on the length of the controller id. + The constraint causes an error when running jobs with controllers configured + with a name of less than 10 character: + `controller_id_must_be_at_least_10_characters constraint failed`. + Since a controller's name is used as its private id, Boundary does not enforce + the normal length requirements on the `server_controller` table + ([PR](https://github.com/hashicorp/boundary/pull/2226)). + ## 0.9.0 (2022/06/20) ### New and Improved diff --git a/internal/db/schema/migrations/oss/postgres/34/02_worker_controller_tables.up.sql b/internal/db/schema/migrations/oss/postgres/34/02_worker_controller_tables.up.sql index e67230adb..2e304f742 100644 --- a/internal/db/schema/migrations/oss/postgres/34/02_worker_controller_tables.up.sql +++ b/internal/db/schema/migrations/oss/postgres/34/02_worker_controller_tables.up.sql @@ -3,6 +3,7 @@ begin; -- Split the server table into two new tables: controller and worker create table server_controller ( + -- Column updated in 35/01_job_migrations.up.sql private_id text primary key, description wt_description, address wt_network_address not null, @@ -248,6 +249,7 @@ alter table session_connection -- Finally, neither jobs nor servers are exposed out of boundary so the risk of -- losing data that would be useful later on is diminished. alter table job_run + -- Column updated in 35/01_job_migrations.up.sql add column controller_id text, drop column server_id; alter table job_run diff --git a/internal/db/schema/migrations/oss/postgres/35/01_job_migrations.up.sql b/internal/db/schema/migrations/oss/postgres/35/01_job_migrations.up.sql new file mode 100644 index 000000000..2a594d83a --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/35/01_job_migrations.up.sql @@ -0,0 +1,25 @@ +begin; + +-- The job_run table has a foreign key to the server_controller private_id. +-- While this column is called a prviate_id it does not implement the wt_private_id +-- domain type, and therefore does not have a minimum length of 10 characters. + +create domain wt_controller_id as text + check( + length(trim(value)) > 0 + ); +comment on domain wt_controller_id is +'A text column that can only be null or not empty representing the id of the controller.'; + +-- Drop constraint that requires controller_id to be at least 10 chars +-- Alters controller_id column defined in 34/02_worker_controller_tables.up.sql +alter table job_run + drop constraint controller_id_must_be_at_least_10_characters, + alter column controller_id type wt_controller_id; + +-- Add a not empty contraint to the private_id +-- Updates column private_id defined in 34/02_worker_controller_tables.up.sql +alter table server_controller + alter column private_id type wt_controller_id; + +commit; diff --git a/internal/db/sqltest/Makefile b/internal/db/sqltest/Makefile index 1d28fef9b..ea3099750 100644 --- a/internal/db/sqltest/Makefile +++ b/internal/db/sqltest/Makefile @@ -21,7 +21,8 @@ TESTS ?= tests/setup/*.sql \ tests/wh/*/*.sql \ tests/sentinel/*.sql \ tests/credential/*/*.sql \ - tests/session/*.sql + tests/session/*.sql \ + tests/controller/*.sql POSTGRES_DOCKER_IMAGE_BASE ?= postgres diff --git a/internal/db/sqltest/tests/controller/controller_id.sql b/internal/db/sqltest/tests/controller/controller_id.sql new file mode 100644 index 000000000..c6ab50a8e --- /dev/null +++ b/internal/db/sqltest/tests/controller/controller_id.sql @@ -0,0 +1,33 @@ +-- controller_id tests: +-- validates the wt_controller_id domain + +begin; + select plan(8); + + select has_domain('wt_controller_id'); + + create table controller_id_testing ( + id wt_controller_id + ); + + prepare empty_insert as insert into controller_id_testing (id) values (''); + SELECT throws_like( + 'empty_insert', + '%"wt_controller_id_check"', + 'We should error for empty controller id' + ); + + prepare null_insert as insert into controller_id_testing (id) values (null); + select lives_ok('null_insert'); + select is(count(*), 1::bigint) from controller_id_testing where id is null; + + prepare valid_insert as insert into controller_id_testing (id) values ('test-controller-id'); + select lives_ok('valid_insert'); + select is(count(*), 1::bigint) from controller_id_testing where id = 'test-controller-id'; + + prepare short_insert as insert into controller_id_testing (id) values ('_'); + select lives_ok('short_insert'); + select is(count(*), 1::bigint) from controller_id_testing where id = '_'; + + select * from finish(); +rollback; diff --git a/internal/scheduler/job/repository_run_test.go b/internal/scheduler/job/repository_run_test.go index 73947fc95..669baa8a0 100644 --- a/internal/scheduler/job/repository_run_test.go +++ b/internal/scheduler/job/repository_run_test.go @@ -25,6 +25,7 @@ func TestRepository_RunJobs(t *testing.T) { iam.TestRepo(t, conn, wrapper) server := testController(t, conn, wrapper) + shortName := testController(t, conn, wrapper, withControllerId("not_10")) tests := []struct { name string @@ -59,6 +60,17 @@ func TestRepository_RunJobs(t *testing.T) { }, wantRun: true, }, + { + name: "valid-short", + ControllerId: shortName.PrivateId, + job: &Job{ + Job: &store.Job{ + Name: "valid-short-controller-name", + Description: "description", + }, + }, + wantRun: true, + }, { name: "fake-server-id", ControllerId: "fake-server-id", diff --git a/internal/scheduler/job/testing.go b/internal/scheduler/job/testing.go index b5f7e8281..698790e71 100644 --- a/internal/scheduler/job/testing.go +++ b/internal/scheduler/job/testing.go @@ -90,20 +90,60 @@ func testRunWithUpdateTime(conn *db.DB, pluginId, name, cId string, updateTime t return run, nil } -func testController(t *testing.T, conn *db.DB, wrapper wrapping.Wrapper) *store.Controller { +func testController(t *testing.T, conn *db.DB, wrapper wrapping.Wrapper, opt ...testOption) *store.Controller { t.Helper() rw := db.New(conn) kms := kms.TestKms(t, conn, wrapper) serversRepo, err := server.NewRepository(rw, rw, kms) require.NoError(t, err) - id, err := uuid.GenerateUUID() - require.NoError(t, err) + opts := getTestOpts(t, opt...) + + privateId := opts.controllerId + if privateId == "" { + // generate a unique ID for the test + id, err := uuid.GenerateUUID() + require.NoError(t, err) + privateId = "test-job-server-" + id + } controller := &store.Controller{ - PrivateId: "test-job-server-" + id, + PrivateId: privateId, Address: "127.0.0.1", } _, err = serversRepo.UpsertController(context.Background(), controller) require.NoError(t, err) return controller } + +func getTestOpts(t testing.TB, opt ...testOption) testOptions { + t.Helper() + opts := getDefaultTestOptions(t) + for _, o := range opt { + o(t, &opts) + } + return opts +} + +// testOption - how Options are passed as arguments. +type testOption func(testing.TB, *testOptions) + +// options = how options are represented +type testOptions struct { + controllerId string +} + +func getDefaultTestOptions(t testing.TB) testOptions { + t.Helper() + + return testOptions{ + controllerId: "", + } +} + +// withControllerId sets the controller id +func withControllerId(p string) testOption { + return func(t testing.TB, o *testOptions) { + t.Helper() + o.controllerId = p + } +} diff --git a/internal/server/repository_controller_test.go b/internal/server/repository_controller_test.go new file mode 100644 index 000000000..0b39c483e --- /dev/null +++ b/internal/server/repository_controller_test.go @@ -0,0 +1,83 @@ +package server + +import ( + "context" + "testing" + + "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/server/store" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRepository_UpsertController(t *testing.T) { + ctx := context.Background() + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrapper := db.TestWrapper(t) + testKms := kms.TestKms(t, conn, wrapper) + testRepo, err := NewRepository(rw, rw, testKms) + require.NoError(t, err) + + iamRepo := iam.TestRepo(t, conn, wrapper) + iam.TestScopes(t, iamRepo) + + tests := []struct { + name string + controller *store.Controller + wantCount int + wantErr bool + }{ + { + name: "nil-controller", + wantErr: true, + }, + { + name: "empty-id", + controller: &store.Controller{ + PrivateId: "", + Address: "127.0.0.1", + }, + wantErr: true, + }, + { + name: "empty-address", + controller: &store.Controller{ + PrivateId: "test-controller", + Address: "", + }, + wantErr: true, + }, + { + name: "valid-controller", + controller: &store.Controller{ + PrivateId: "test-controller", + Address: "127.0.0.1", + }, + wantCount: 1, + }, + { + name: "valid-controller-short-name", + controller: &store.Controller{ + PrivateId: "test", + Address: "127.0.0.1", + }, + wantCount: 1, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + got, err := testRepo.UpsertController(ctx, tt.controller) + if tt.wantErr { + require.Error(err) + assert.Equal(0, got) + return + } + require.NoError(err) + assert.Equal(tt.wantCount, got) + }) + } +}