From 54c30d2cad3c1888e120dc4abad407bf0925f5d6 Mon Sep 17 00:00:00 2001 From: irenarindos Date: Mon, 9 May 2022 10:03:19 -0400 Subject: [PATCH] bug(session): null fkey trigger also checks for session terminated state --- CHANGELOG.md | 2 + .../oss/postgres/0/50_session.up.sql | 1 + .../29/01_cancel_session_null_fkey.up.sql | 37 +++++ .../migrations/oss/postgres_29_01_test.go | 140 ++++++++++++++++++ .../tests/session/delete_session_target.sql | 25 ++++ internal/session/repository_session_test.go | 48 ++++++ 6 files changed, 253 insertions(+) create mode 100644 internal/db/schema/migrations/oss/postgres/29/01_cancel_session_null_fkey.up.sql create mode 100644 internal/db/schema/migrations/oss/postgres_29_01_test.go create mode 100644 internal/db/sqltest/tests/session/delete_session_target.sql diff --git a/CHANGELOG.md b/CHANGELOG.md index d57df1eb19..48341934b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,8 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. * controller: Do not shut down cluster listener when it receives an invalid packet ([Issue](https://github.com/hashicorp/boundary/issues/2072), [PR](https://github.com/hashicorp/boundary/pull/2073)) +* session: update cancel_session() function to check for terminated state ([Issue](https://github.com/hashicorp/boundary/issues/2064), + [PR](https://github.com/hashicorp/boundary/pull/2065)) ## 0.8.0 (2022/05/03) diff --git a/internal/db/schema/migrations/oss/postgres/0/50_session.up.sql b/internal/db/schema/migrations/oss/postgres/0/50_session.up.sql index 1e16244ff0..c297159643 100644 --- a/internal/db/schema/migrations/oss/postgres/0/50_session.up.sql +++ b/internal/db/schema/migrations/oss/postgres/0/50_session.up.sql @@ -290,6 +290,7 @@ begin; for each row execute procedure update_session_state_on_termination_reason(); + -- Updated in 29/01_cancel_session_null_fkey -- cancel_session will insert a cancel state for the session, if there's isn't -- a canceled state already. It's used by cancel_session_with_null_fk. create or replace function diff --git a/internal/db/schema/migrations/oss/postgres/29/01_cancel_session_null_fkey.up.sql b/internal/db/schema/migrations/oss/postgres/29/01_cancel_session_null_fkey.up.sql new file mode 100644 index 0000000000..4625a3df06 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/29/01_cancel_session_null_fkey.up.sql @@ -0,0 +1,37 @@ +begin; + +drop function cancel_session(in sessionId text); +-- Updates cancel_session() from 0/50_session to check if a session is either terminated or canceling +-- Sessions can progress directly to terminated without going through the canceling state +-- cancel_session will insert a cancel state for the session, if there's isn't +-- a canceled or terminated state already. It's used by cancel_session_with_null_fk. +create function + cancel_session(in sessionId text) returns void +as $$ +declare + rows_affected numeric; +begin + insert into session_state(session_id, state) + select + sessionId::text, 'canceling' + from + session s + where + s.public_id = sessionId::text and + s.public_id not in ( + select + session_id + from + session_state + where + session_id = sessionId::text and + state in('canceling','terminated') + ) limit 1; + get diagnostics rows_affected = row_count; + if rows_affected > 1 then + raise exception 'cancel session: more than one row affected: %', rows_affected; + end if; +end; +$$ language plpgsql; + +commit; \ No newline at end of file diff --git a/internal/db/schema/migrations/oss/postgres_29_01_test.go b/internal/db/schema/migrations/oss/postgres_29_01_test.go new file mode 100644 index 0000000000..32206bc046 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres_29_01_test.go @@ -0,0 +1,140 @@ +package oss_test + +import ( + "context" + "testing" + + "github.com/hashicorp/boundary/internal/authtoken" + "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/db/common" + "github.com/hashicorp/boundary/internal/db/schema" + "github.com/hashicorp/boundary/internal/host/static" + "github.com/hashicorp/boundary/internal/iam" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/session" + "github.com/hashicorp/boundary/internal/target" + "github.com/hashicorp/boundary/internal/target/tcp" + "github.com/hashicorp/boundary/internal/types/resource" + "github.com/hashicorp/boundary/testing/dbtest" + "github.com/stretchr/testify/require" +) + +func TestMigrations_SessionFKeyDelete(t *testing.T) { + t.Parallel() + require := require.New(t) + + const ( + priorMigration = 28002 + currentMigration = 29001 + ) + dialect := dbtest.Postgres + ctx := context.Background() + + c, u, _, err := dbtest.StartUsingTemplate(dialect, dbtest.WithTemplate(dbtest.Template1)) + require.NoError(err) + t.Cleanup(func() { + require.NoError(c()) + }) + d, err := common.SqlOpen(dialect, u) + require.NoError(err) + + // migration to the prior migration (before the one we want to test) + m, err := schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": priorMigration}), + )) + require.NoError(err) + + require.NoError(m.ApplyMigrations(ctx)) + state, err := m.CurrentState(ctx) + require.NoError(err) + want := &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: priorMigration, + DatabaseSchemaVersion: priorMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(want, state) + + // Seed the database with test data + dbType, err := db.StringToDbType(dialect) + require.NoError(err) + + conn, err := db.Open(dbType, u) + require.NoError(err) + + rw := db.New(conn) + wrapper := db.TestWrapper(t) + + org, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + require.NotNil(prj) + + hc := static.TestCatalogs(t, conn, prj.GetPublicId(), 1)[0] + hs := static.TestSets(t, conn, hc.GetPublicId(), 1)[0] + h := static.TestHosts(t, conn, hc.GetPublicId(), 1)[0] + static.TestSetMembers(t, conn, hs.GetPublicId(), []*static.Host{h}) + tar := tcp.TestTarget(ctx, t, conn, prj.GetPublicId(), "test", target.WithHostSources([]string{hs.GetPublicId()})) + kmsCache := kms.TestKms(t, conn, wrapper) + targetRepo, err := target.NewRepository(rw, rw, kmsCache) + require.NoError(err) + + serverId := "worker" + tofu := session.TestTofu(t) + session.TestWorker(t, conn, wrapper, session.WithServerId(serverId)) + at := authtoken.TestAuthToken(t, conn, kmsCache, org.GetPublicId()) + uId := at.GetIamUserId() + sess := session.TestSession(t, conn, wrapper, session.ComposedOf{ + UserId: uId, + HostId: h.GetPublicId(), + TargetId: tar.GetPublicId(), + HostSetId: hs.GetPublicId(), + AuthTokenId: at.GetPublicId(), + ScopeId: prj.GetPublicId(), + Endpoint: "tcp://127.0.0.1:22", + ConnectionLimit: 1, + }) + + sessionRepo, err := session.NewRepository(rw, rw, kmsCache) + require.NoError(err) + + // Create and terminate session without canceling + _, _, err = sessionRepo.ActivateSession(ctx, sess.PublicId, sess.Version, serverId, resource.Worker.String(), tofu) + require.NoError(err) + session.TestState(t, conn, sess.PublicId, session.StatusTerminated) + + // Delete target; expect a session_state violation and failure to delete target + rows, err := targetRepo.DeleteTarget(ctx, tar.GetPublicId()) + require.Errorf(err, "target.(Repository).DeleteTarget: db.DoTx: target.(Repository).DeleteTarget: db.Delete: insert or update on table \"session_state\" violates foreign key constraint \"session_valid_state_enm_fkey\": integrity violation: error #1003") + require.Equal(0, rows) + + // now we're ready for the migration we want to test. + m, err = schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": currentMigration}), + )) + require.NoError(err) + + require.NoError(m.ApplyMigrations(ctx)) + state, err = m.CurrentState(ctx) + require.NoError(err) + want = &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: currentMigration, + DatabaseSchemaVersion: currentMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(want, state) + + // Try to delete target again, should succeed without error + rows, err = targetRepo.DeleteTarget(ctx, tar.GetPublicId()) + require.NoError(err) + require.Equal(1, rows) +} diff --git a/internal/db/sqltest/tests/session/delete_session_target.sql b/internal/db/sqltest/tests/session/delete_session_target.sql new file mode 100644 index 0000000000..bfdd3b0acd --- /dev/null +++ b/internal/db/sqltest/tests/session/delete_session_target.sql @@ -0,0 +1,25 @@ +begin; + select plan(9); + + -- Ensure session state table is populated + select is(count(*), 1::bigint) from session_state where session_id = 's1_____clare' and state='pending'; + select is(count(*), 1::bigint) from session_state where session_id = 's1_____cindy' and state='terminated'; + select is(count(*), 1::bigint) from session_state where session_id = 's1_____ciara' and state='canceling'; + select is(count(*), 1::bigint) from session_state where session_id = 's1_____carly' and state='active'; + + -- Check that we have 4 sessions using this target + select is(count(*), 4::bigint) from session where target_id = 't_________cb'; + + -- Delete target, expect no errors + delete from target where public_id='t_________cb'; + select is(count(*), 0::bigint) from target where public_id='t_________cb'; + + -- Ensure we no longer have sessions associated with this target + select is(count(*), 0::bigint) from session where target_id = 't_________cb'; + + -- Ensure sessions that were pending or active are now in canceling state + select is(count(*), 1::bigint) from session_state where state = 'canceling' and session_id = 's1_____clare'; + select is(count(*), 1::bigint) from session_state where state = 'canceling' and session_id = 's1_____carly'; + + select * from finish(); +rollback; \ No newline at end of file diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index cc9a770c65..4086337436 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -1455,3 +1455,51 @@ func testSessionCredentialParams(t *testing.T, conn *db.DB, wrapper wrapping.Wra params.DynamicCredentials = creds return params } + +func TestRepository_deleteTargetFKey(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrapper := db.TestWrapper(t) + iamRepo := iam.TestRepo(t, conn, wrapper) + kms := kms.TestKms(t, conn, wrapper) + repo, err := NewRepository(rw, rw, kms) + targetRepo, err := target.NewRepository(rw, rw, kms) + require.NoError(t, err) + + tests := []struct { + name string + state Status + }{ + { + name: "Delete target for terminated session", + state: StatusTerminated, + }, + { + name: "Delete target for canceling session", + state: StatusCanceling, + }, + { + name: "Delete target for active session", + state: StatusActive, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + + c := TestSessionParams(t, conn, wrapper, iamRepo) + sesh := TestSession(t, conn, wrapper, c) + + s := TestState(t, conn, sesh.PublicId, tt.state) + assert.Equal(tt.state, s.Status) + + // Delete target associated with session; ensure target deletion with no state violations + rows, err := targetRepo.DeleteTarget(context.Background(), c.TargetId) + require.NoError(err) + assert.Equal(1, rows) + foundSession, _, err := repo.LookupSession(context.Background(), sesh.PublicId) + assert.NoError(err) + assert.Empty(foundSession.TargetId) + }) + } +}