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

@@ -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

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

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;

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
 

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;

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",

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
+	}
+}

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)
+		})
+	}
+}