From d741034829df9367728bdf54727509be70f6d82e Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Wed, 25 May 2022 16:20:14 +0000 Subject: [PATCH] perf(session): Change list behavior around terminated sessions It was determined that it is a more common use case for users to request non-terminated sessions (i.e. sessions in `pending`, `active`, or `canceling` when requesting sessions. Notably, end users would need non-terminated sessions to: - Establish a new connection to a session. - Terminate a session So now by default a list request to `/v1/sessions` will *not* include terminated sessions. To still enable use cases where terminated sessions are needed, this adds a new query parameter to include the terminated session: `/v1/sessions?include_terminated=true`. Note that this query parameter is considered experimental and subject to change. --- .../handlers/sessions/session_service.go | 2 +- .../handlers/sessions/session_service_test.go | 59 ++++- internal/session/options.go | 8 + internal/session/query.go | 11 +- internal/session/repository_session.go | 41 ++-- internal/session/repository_session_test.go | 229 ++++++++++++++++++ .../session/service_list_for_authz_check.go | 23 ++ 7 files changed, 348 insertions(+), 25 deletions(-) create mode 100644 internal/session/service_list_for_authz_check.go diff --git a/internal/daemon/controller/handlers/sessions/session_service.go b/internal/daemon/controller/handlers/sessions/session_service.go index 4fa4d01c6..3a0388544 100644 --- a/internal/daemon/controller/handlers/sessions/session_service.go +++ b/internal/daemon/controller/handlers/sessions/session_service.go @@ -152,7 +152,7 @@ func (s Service) ListSessions(ctx context.Context, req *pbs.ListSessionsRequest) RootScopeId: req.GetScopeId(), Type: resource.Session, Recursive: req.GetRecursive(), - AuthzProtectedEntityProvider: repo, + AuthzProtectedEntityProvider: session.ListForAuthzCheck(repo, session.WithTerminated(req.IncludeTerminated)), ActionSet: IdActions, }, ) diff --git a/internal/daemon/controller/handlers/sessions/session_service_test.go b/internal/daemon/controller/handlers/sessions/session_service_test.go index d18499c4f..cbd864502 100644 --- a/internal/daemon/controller/handlers/sessions/session_service_test.go +++ b/internal/daemon/controller/handlers/sessions/session_service_test.go @@ -232,6 +232,7 @@ func TestList(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") wrap := db.TestWrapper(t) kms := kms.TestKms(t, conn, wrap) + ctx := context.Background() iamRepo := iam.TestRepo(t, conn, wrap) @@ -260,16 +261,17 @@ func TestList(t *testing.T) { 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(context.Background(), t, conn, pWithSessions.GetPublicId(), "test", target.WithHostSources([]string{hs.GetPublicId()})) + tar := tcp.TestTarget(ctx, t, conn, pWithSessions.GetPublicId(), "test", target.WithHostSources([]string{hs.GetPublicId()})) hcOther := static.TestCatalogs(t, conn, pWithOtherSessions.GetPublicId(), 1)[0] hsOther := static.TestSets(t, conn, hcOther.GetPublicId(), 1)[0] hOther := static.TestHosts(t, conn, hcOther.GetPublicId(), 1)[0] static.TestSetMembers(t, conn, hsOther.GetPublicId(), []*static.Host{hOther}) - tarOther := tcp.TestTarget(context.Background(), t, conn, pWithOtherSessions.GetPublicId(), "test", target.WithHostSources([]string{hsOther.GetPublicId()})) + tarOther := tcp.TestTarget(ctx, t, conn, pWithOtherSessions.GetPublicId(), "test", target.WithHostSources([]string{hsOther.GetPublicId()})) var wantSession []*pb.Session var totalSession []*pb.Session + var wantIncludeTerminatedSessions []*pb.Session for i := 0; i < 10; i++ { sess := session.TestSession(t, conn, wrap, session.ComposedOf{ UserId: uId, @@ -308,6 +310,7 @@ func TestList(t *testing.T) { }) totalSession = append(totalSession, wantSession[i]) + wantIncludeTerminatedSessions = append(wantIncludeTerminatedSessions, wantSession[i]) sess = session.TestSession(t, conn, wrap, session.ComposedOf{ UserId: uIdOther, @@ -346,6 +349,53 @@ func TestList(t *testing.T) { }) } + { + sess := session.TestSession(t, conn, wrap, session.ComposedOf{ + UserId: uId, + HostId: h.GetPublicId(), + TargetId: tar.GetPublicId(), + HostSetId: hs.GetPublicId(), + AuthTokenId: at.GetPublicId(), + ScopeId: pWithSessions.GetPublicId(), + Endpoint: "tcp://127.0.0.1:22", + }) + + sess, err := sessRepo.CancelSession(ctx, sess.PublicId, sess.Version) + require.NoError(t, err) + terminated, err := sessRepo.TerminateCompletedSessions(ctx) + require.NoError(t, err) + require.Equal(t, 1, terminated) + + sess, _, err = sessRepo.LookupSession(ctx, sess.PublicId) + require.NoError(t, err) + status, states := convertStates(sess.States) + + expected := &pb.Session{ + Id: sess.GetPublicId(), + ScopeId: pWithSessions.GetPublicId(), + AuthTokenId: at.GetPublicId(), + UserId: at.GetIamUserId(), + TargetId: sess.TargetId, + Endpoint: sess.Endpoint, + HostSetId: sess.HostSetId, + HostId: sess.HostId, + Version: sess.Version, + UpdatedTime: sess.UpdateTime.GetTimestamp(), + CreatedTime: sess.CreateTime.GetTimestamp(), + ExpirationTime: sess.ExpirationTime.GetTimestamp(), + Scope: &scopes.ScopeInfo{Id: pWithSessions.GetPublicId(), Type: scope.Project.String(), ParentScopeId: o.GetPublicId()}, + Status: status, + States: states, + Certificate: sess.Certificate, + TerminationReason: sess.TerminationReason, + Type: tcp.Subtype.String(), + AuthorizedActions: testAuthorizedActions, + Connections: []*pb.Connection{}, // connections should not be returned for list + } + + wantIncludeTerminatedSessions = append(wantIncludeTerminatedSessions, expected) + } + cases := []struct { name string req *pbs.ListSessionsRequest @@ -357,6 +407,11 @@ func TestList(t *testing.T) { req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId()}, res: &pbs.ListSessionsResponse{Items: wantSession}, }, + { + name: "List Many Include Terminated", + req: &pbs.ListSessionsRequest{ScopeId: pWithSessions.GetPublicId(), IncludeTerminated: true}, + res: &pbs.ListSessionsResponse{Items: wantIncludeTerminatedSessions}, + }, { name: "List No Sessions", req: &pbs.ListSessionsRequest{ScopeId: pNoSessions.GetPublicId()}, diff --git a/internal/session/options.go b/internal/session/options.go index a4e5a335b..e69ec2c7d 100644 --- a/internal/session/options.go +++ b/internal/session/options.go @@ -32,6 +32,7 @@ type options struct { withServerId string withDbOpts []db.Option withWorkerStateDelay time.Duration + withTerminated bool } func getDefaultOptions() options { @@ -121,3 +122,10 @@ func WithWorkerStateDelay(d time.Duration) Option { o.withWorkerStateDelay = d } } + +// WithTerminated is used to include terminated sessions in a list request. +func WithTerminated(withTerminated bool) Option { + return func(o *options) { + o.withTerminated = withTerminated + } +} diff --git a/internal/session/query.go b/internal/session/query.go index 97a8c57f8..5242ff9a1 100644 --- a/internal/session/query.go +++ b/internal/session/query.go @@ -117,11 +117,20 @@ session_connection_limit(expiration_time, connection_limit) as ( select expiration_time, connection_limit, current_connection_count from session_connection_limit, session_connection_count; +` + nonTerminatedSessionPublicIdList = ` +select public_id, scope_id, user_id from session +where + session.termination_reason is null +and + scope_id = any(@scope_ids) +; ` sessionPublicIdList = ` select public_id, scope_id, user_id from session -%s +where + scope_id = any(@scope_ids) ; ` diff --git a/internal/session/repository_session.go b/internal/session/repository_session.go index a6acb6fce..07cfd76fb 100644 --- a/internal/session/repository_session.go +++ b/internal/session/repository_session.go @@ -207,32 +207,31 @@ func (r *Repository) LookupSession(ctx context.Context, sessionId string, _ ...O return &session, authzSummary, nil } -// FetchAuthzProtectedEntitiesByScope implements boundary.AuthzProtectedEntityProvider -func (r *Repository) FetchAuthzProtectedEntitiesByScope(ctx context.Context, scopeIds []string) (map[string][]boundary.AuthzProtectedEntity, error) { - const op = "session.(Repository).FetchAuthzProtectedEntityInfo" +// fetchAuthzProtectedSessionsByScope fetches sessions for the given scopes. +// Note that the sessions are not fully populated, and only contain the +// necessary information to implement the boundary.AuthzProtectedEntity +// interface. Supports the WithTerminated option. +func (r *Repository) fetchAuthzProtectedSessionsByScope( + ctx context.Context, scopeIds []string, opt ...Option, +) (map[string][]boundary.AuthzProtectedEntity, error) { + const op = "session.(Repository).fetchAuthzProtectedSessionsByScope" - var where string - var args []interface{} + opts := getOpts(opt...) - inClauseCnt := 0 - - switch len(scopeIds) { - case 0: + if len(scopeIds) == 0 { return nil, errors.New(ctx, errors.InvalidParameter, op, "no scopes given") - case 1: - inClauseCnt += 1 - where, args = fmt.Sprintf("where scope_id = @%d", inClauseCnt), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), scopeIds[0])) - default: - idsInClause := make([]string, 0, len(scopeIds)) - for _, id := range scopeIds { - inClauseCnt += 1 - idsInClause, args = append(idsInClause, fmt.Sprintf("@%d", inClauseCnt)), append(args, sql.Named(fmt.Sprintf("%d", inClauseCnt), id)) - } - where = fmt.Sprintf("where scope_id in (%s)", strings.Join(idsInClause, ",")) } - q := sessionPublicIdList - query := fmt.Sprintf(q, where) + args := []interface{}{ + sql.Named("scope_ids", "{"+strings.Join(scopeIds, ",")+"}"), + } + + var query string + if opts.withTerminated { + query = sessionPublicIdList + } else { + query = nonTerminatedSessionPublicIdList + } rows, err := r.reader.Query(ctx, query, args) if err != nil { diff --git a/internal/session/repository_session_test.go b/internal/session/repository_session_test.go index acf9801c0..45309cd20 100644 --- a/internal/session/repository_session_test.go +++ b/internal/session/repository_session_test.go @@ -1592,3 +1592,232 @@ func TestRepository_deleteTerminated(t *testing.T) { }) } } + +func TestFetchAuthzProtectedSessionsByScopes(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + ctx := context.Background() + wrapper := db.TestWrapper(t) + iamRepo := iam.TestRepo(t, conn, wrapper) + rw := db.New(conn) + kms := kms.TestKms(t, conn, wrapper) + repo, err := NewRepository(rw, rw, kms) + require.NoError(t, err) + composedOf := TestSessionParams(t, conn, wrapper, iamRepo) + + _, pWithOtherSessions := iam.TestScopes(t, iamRepo) + + hcOther := static.TestCatalogs(t, conn, pWithOtherSessions.GetPublicId(), 1)[0] + hsOther := static.TestSets(t, conn, hcOther.GetPublicId(), 1)[0] + hOther := static.TestHosts(t, conn, hcOther.GetPublicId(), 1)[0] + static.TestSetMembers(t, conn, hsOther.GetPublicId(), []*static.Host{hOther}) + tarOther := tcp.TestTarget(ctx, t, conn, pWithOtherSessions.GetPublicId(), "test", target.WithHostSources([]string{hsOther.GetPublicId()})) + + composedOfOther := ComposedOf{ + UserId: composedOf.UserId, + HostId: hOther.GetPublicId(), + TargetId: tarOther.GetPublicId(), + HostSetId: hsOther.GetPublicId(), + AuthTokenId: composedOf.AuthTokenId, + ScopeId: pWithOtherSessions.GetPublicId(), + Endpoint: "tcp://127.0.0.1:22", + } + + type testCase struct { + name string + createCnt int + terminateCnt int + otherCnt int + otherTerminateCnt int + opts []Option + reqScopes []string + wantCnt int + wantOtherCnt int + wantErr bool + } + + cases := []testCase{ + { + name: "NonTerminated/none", + createCnt: 0, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 0, + wantErr: false, + }, + { + name: "NonTerminated/one", + createCnt: 1, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 1, + wantErr: false, + }, + { + name: "NonTerminated/many", + createCnt: 5, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 5, + wantErr: false, + }, + { + name: "NonTerminated/many one terminated", + createCnt: 5, + terminateCnt: 1, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 4, + wantErr: false, + }, + { + name: "NonTerminated/many terminated", + createCnt: 5, + terminateCnt: 3, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 2, + wantErr: false, + }, + { + name: "NonTerminated/many multiple scopes", + createCnt: 5, + terminateCnt: 3, + reqScopes: []string{composedOf.ScopeId, composedOfOther.ScopeId}, + wantCnt: 2, + wantErr: false, + }, + { + name: "NonTerminated/many multiple scopes", + createCnt: 5, + terminateCnt: 3, + otherCnt: 3, + otherTerminateCnt: 1, + reqScopes: []string{composedOf.ScopeId, composedOfOther.ScopeId}, + wantCnt: 2, + wantOtherCnt: 2, + wantErr: false, + }, + { + name: "NonTerminated/no scopes", + createCnt: 2, + terminateCnt: 1, + otherCnt: 2, + otherTerminateCnt: 1, + reqScopes: []string{}, + wantErr: true, + }, + { + name: "none", + opts: []Option{WithTerminated(true)}, + createCnt: 0, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 0, + wantErr: false, + }, + { + name: "one", + opts: []Option{WithTerminated(true)}, + createCnt: 1, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 1, + wantErr: false, + }, + { + name: "many", + opts: []Option{WithTerminated(true)}, + createCnt: 5, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 5, + wantErr: false, + }, + { + name: "many one terminated", + opts: []Option{WithTerminated(true)}, + createCnt: 5, + terminateCnt: 1, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 5, + wantErr: false, + }, + { + name: "many terminated", + opts: []Option{WithTerminated(true)}, + createCnt: 5, + terminateCnt: 3, + reqScopes: []string{composedOf.ScopeId}, + wantCnt: 5, + wantErr: false, + }, + { + name: "many multiple scopes", + opts: []Option{WithTerminated(true)}, + createCnt: 5, + terminateCnt: 3, + reqScopes: []string{composedOf.ScopeId, composedOfOther.ScopeId}, + wantCnt: 5, + wantErr: false, + }, + { + name: "many multiple scopes", + opts: []Option{WithTerminated(true)}, + createCnt: 5, + terminateCnt: 3, + otherCnt: 3, + otherTerminateCnt: 1, + reqScopes: []string{composedOf.ScopeId, composedOfOther.ScopeId}, + wantCnt: 5, + wantOtherCnt: 3, + wantErr: false, + }, + { + name: "no scopes", + opts: []Option{WithTerminated(true)}, + createCnt: 2, + terminateCnt: 1, + otherCnt: 2, + otherTerminateCnt: 1, + reqScopes: []string{}, + wantErr: true, + }, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + db.TestDeleteWhere(t, conn, func() interface{} { i := AllocSession(); return &i }(), "1=1") + + testSessions := []*Session{} + for i := 0; i < tt.createCnt; i++ { + s := TestSession(t, conn, wrapper, composedOf) + _ = TestState(t, conn, s.PublicId, StatusActive) + testSessions = append(testSessions, s) + } + for i := 0; i < tt.terminateCnt; i++ { + _, err := repo.CancelSession(ctx, testSessions[i].PublicId, testSessions[i].Version) + require.NoError(err) + } + terminated, err := repo.TerminateCompletedSessions(ctx) + require.NoError(err) + require.Equal(tt.terminateCnt, terminated) + + otherTestSessions := []*Session{} + for i := 0; i < tt.otherCnt; i++ { + s := TestSession(t, conn, wrapper, composedOfOther) + _ = TestState(t, conn, s.PublicId, StatusActive) + otherTestSessions = append(otherTestSessions, s) + } + for i := 0; i < tt.otherTerminateCnt; i++ { + _, err := repo.CancelSession(ctx, otherTestSessions[i].PublicId, otherTestSessions[i].Version) + require.NoError(err) + } + terminated, err = repo.TerminateCompletedSessions(ctx) + require.NoError(err) + require.Equal(tt.otherTerminateCnt, terminated) + + assert.Equal(tt.otherCnt, len(otherTestSessions)) + + got, err := repo.fetchAuthzProtectedSessionsByScope(ctx, tt.reqScopes, tt.opts...) + if tt.wantErr { + require.Error(err) + return + } + require.NoError(err) + assert.Equal(tt.wantCnt, len(got[composedOf.ScopeId])) + assert.Equal(tt.wantOtherCnt, len(got[composedOfOther.ScopeId])) + }) + } +} diff --git a/internal/session/service_list_for_authz_check.go b/internal/session/service_list_for_authz_check.go new file mode 100644 index 000000000..c1c2f0508 --- /dev/null +++ b/internal/session/service_list_for_authz_check.go @@ -0,0 +1,23 @@ +package session + +import ( + "context" + + "github.com/hashicorp/boundary/internal/boundary" +) + +type listForAuthzCheckFunc func(ctx context.Context, scopeIds []string) (map[string][]boundary.AuthzProtectedEntity, error) + +func (a listForAuthzCheckFunc) FetchAuthzProtectedEntitiesByScope(ctx context.Context, scopeIds []string) (map[string][]boundary.AuthzProtectedEntity, error) { + return a(ctx, scopeIds) +} + +// ListForAuthzCheck returns a functions that fetches sessions for the given +// scopes. Note that the sessions are not fully populated, and only contain the +// necessary information to implement the boundary.AuthzProtectedEntity +// interface. Supports the WithTerminated option. +func ListForAuthzCheck(repo *Repository, opt ...Option) listForAuthzCheckFunc { + return func(ctx context.Context, scopeIds []string) (map[string][]boundary.AuthzProtectedEntity, error) { + return repo.fetchAuthzProtectedSessionsByScope(ctx, scopeIds, opt...) + } +}