From 16dd2ac096064b330de3d9e5d85131e4d724d107 Mon Sep 17 00:00:00 2001 From: Marco Dinis Date: Wed, 8 Jan 2025 09:45:31 +0000 Subject: [PATCH 1/3] Fix UserTask Status not being updated The Status field for UserTasks was not being correctly updated when the Spec.State was not changed. --- lib/auth/usertasks/usertasksv1/service.go | 24 +++++++++----- .../usertasks/usertasksv1/service_test.go | 33 ++++++++++++++++++- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/lib/auth/usertasks/usertasksv1/service.go b/lib/auth/usertasks/usertasksv1/service.go index a383e55a70135..1853a04964b3e 100644 --- a/lib/auth/usertasks/usertasksv1/service.go +++ b/lib/auth/usertasks/usertasksv1/service.go @@ -131,7 +131,7 @@ func (s *Service) CreateUserTask(ctx context.Context, req *usertasksv1.CreateUse return nil, trace.Wrap(err) } - s.updateStatus(req.UserTask) + s.updateStatus(req.UserTask, nil /* existing user task */) rsp, err := s.backend.CreateUserTask(ctx, req.UserTask) s.emitCreateAuditEvent(ctx, rsp, authCtx, err) @@ -264,10 +264,7 @@ func (s *Service) UpdateUserTask(ctx context.Context, req *usertasksv1.UpdateUse } stateChanged := existingUserTask.GetSpec().GetState() != req.GetUserTask().GetSpec().GetState() - - if stateChanged { - s.updateStatus(req.UserTask) - } + s.updateStatus(req.UserTask, existingUserTask) rsp, err := s.backend.UpdateUserTask(ctx, req.UserTask) s.emitUpdateAuditEvent(ctx, existingUserTask, req.GetUserTask(), authCtx, err) @@ -333,9 +330,7 @@ func (s *Service) UpsertUserTask(ctx context.Context, req *usertasksv1.UpsertUse stateChanged = existingUserTask.GetSpec().GetState() != req.GetUserTask().GetSpec().GetState() } - if stateChanged { - s.updateStatus(req.UserTask) - } + s.updateStatus(req.UserTask, existingUserTask) rsp, err := s.backend.UpsertUserTask(ctx, req.UserTask) s.emitUpsertAuditEvent(ctx, existingUserTask, req.GetUserTask(), authCtx, err) @@ -350,10 +345,21 @@ func (s *Service) UpsertUserTask(ctx context.Context, req *usertasksv1.UpsertUse return rsp, nil } -func (s *Service) updateStatus(ut *usertasksv1.UserTask) { +func (s *Service) updateStatus(ut *usertasksv1.UserTask, existing *usertasksv1.UserTask) { + // Default status for UserTask. ut.Status = &usertasksv1.UserTaskStatus{ LastStateChange: timestamppb.New(s.clock.Now()), } + + if existing != nil { + // Inherit everything from existing UserTask. + ut.Status = existing.Status + + // Update specific values. + if existing.GetSpec().GetState() != ut.GetSpec().GetState() { + ut.Status.LastStateChange = timestamppb.New(s.clock.Now()) + } + } } func (s *Service) emitUpsertAuditEvent(ctx context.Context, old, new *usertasksv1.UserTask, authCtx *authz.Context, err error) { diff --git a/lib/auth/usertasks/usertasksv1/service_test.go b/lib/auth/usertasks/usertasksv1/service_test.go index d40b3740af591..1a909c278bdd8 100644 --- a/lib/auth/usertasks/usertasksv1/service_test.go +++ b/lib/auth/usertasks/usertasksv1/service_test.go @@ -153,6 +153,7 @@ func TestEvents(t *testing.T) { // LastStateChange is updated. require.Equal(t, timestamppb.New(fakeClock.Now()), createUserTaskResp.Status.LastStateChange) + expectedLastStateChange := createUserTaskResp.Status.LastStateChange ut1.Spec.DiscoverEc2.Instances["i-345"] = &usertasksv1.DiscoverEC2Instance{ InstanceId: "i-345", DiscoveryConfig: "dc01", @@ -165,7 +166,7 @@ func TestEvents(t *testing.T) { require.Len(t, testReporter.emittedEvents, 1) consumeAssertEvent(t, auditEventsSink.C(), auditEventFor(userTaskName, "update", "OPEN", "OPEN")) // LastStateChange is not updated. - require.Equal(t, createUserTaskResp.Status.LastStateChange, upsertUserTaskResp.Status.LastStateChange) + require.Equal(t, expectedLastStateChange.AsTime(), upsertUserTaskResp.Status.LastStateChange.AsTime()) ut1.Spec.State = "RESOLVED" fakeClock.Advance(1 * time.Minute) @@ -177,6 +178,36 @@ func TestEvents(t *testing.T) { // LastStateChange was updated because the state changed. require.Equal(t, timestamppb.New(fakeClock.Now()), updateUserTaskResp.Status.LastStateChange) + // Updating one of the instances. + expectedLastStateChange = updateUserTaskResp.Status.GetLastStateChange() + fakeClock.Advance(1 * time.Minute) + ut1.Spec.DiscoverEc2.Instances["i-345"] = &usertasksv1.DiscoverEC2Instance{ + InstanceId: "i-345", + DiscoveryConfig: "dc01", + DiscoveryGroup: "dg01", + SyncTime: timestamppb.New(fakeClock.Now()), + } + updateUserTaskResp, err = service.UpdateUserTask(ctx, &usertasksv1.UpdateUserTaskRequest{UserTask: ut1}) + require.NoError(t, err) + // Does not change the LastStateChange + require.Equal(t, expectedLastStateChange.AsTime(), updateUserTaskResp.Status.LastStateChange.AsTime()) + consumeAssertEvent(t, auditEventsSink.C(), auditEventFor(userTaskName, "update", "RESOLVED", "RESOLVED")) + + // Upserting one of the instances. + expectedLastStateChange = updateUserTaskResp.Status.GetLastStateChange() + fakeClock.Advance(1 * time.Minute) + ut1.Spec.DiscoverEc2.Instances["i-345"] = &usertasksv1.DiscoverEC2Instance{ + InstanceId: "i-345", + DiscoveryConfig: "dc01", + DiscoveryGroup: "dg01", + SyncTime: timestamppb.New(fakeClock.Now()), + } + upsertUserTaskResp, err = service.UpsertUserTask(ctx, &usertasksv1.UpsertUserTaskRequest{UserTask: ut1}) + require.NoError(t, err) + // Does not change the LastStateChange + require.Equal(t, expectedLastStateChange.AsTime(), upsertUserTaskResp.Status.LastStateChange.AsTime()) + consumeAssertEvent(t, auditEventsSink.C(), auditEventFor(userTaskName, "update", "RESOLVED", "RESOLVED")) + _, err = service.DeleteUserTask(ctx, &usertasksv1.DeleteUserTaskRequest{Name: userTaskName}) require.NoError(t, err) // No usage report for deleted resources. From 61bb2ec06132c56be0cf0a1a160d905e5f58c960 Mon Sep 17 00:00:00 2001 From: Marco Dinis Date: Thu, 9 Jan 2025 18:13:28 +0000 Subject: [PATCH 2/3] copy the status field --- lib/auth/usertasks/usertasksv1/service.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/auth/usertasks/usertasksv1/service.go b/lib/auth/usertasks/usertasksv1/service.go index 1853a04964b3e..dcea679c76e2a 100644 --- a/lib/auth/usertasks/usertasksv1/service.go +++ b/lib/auth/usertasks/usertasksv1/service.go @@ -19,6 +19,7 @@ package usertasksv1 import ( + "cmp" "context" "log/slog" "time" @@ -353,7 +354,7 @@ func (s *Service) updateStatus(ut *usertasksv1.UserTask, existing *usertasksv1.U if existing != nil { // Inherit everything from existing UserTask. - ut.Status = existing.Status + ut.Status.LastStateChange = cmp.Or(existing.Status.LastStateChange, ut.Status.LastStateChange) // Update specific values. if existing.GetSpec().GetState() != ut.GetSpec().GetState() { From 3d93237a7805f8bcabb3d720cffeff06a0325c15 Mon Sep 17 00:00:00 2001 From: Marco Dinis Date: Fri, 10 Jan 2025 10:26:00 +0000 Subject: [PATCH 3/3] use admin client instead of backend directly --- lib/auth/usertasks/usertasksv1/service.go | 2 +- lib/web/usertasks_test.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/auth/usertasks/usertasksv1/service.go b/lib/auth/usertasks/usertasksv1/service.go index dcea679c76e2a..74223f258369c 100644 --- a/lib/auth/usertasks/usertasksv1/service.go +++ b/lib/auth/usertasks/usertasksv1/service.go @@ -354,7 +354,7 @@ func (s *Service) updateStatus(ut *usertasksv1.UserTask, existing *usertasksv1.U if existing != nil { // Inherit everything from existing UserTask. - ut.Status.LastStateChange = cmp.Or(existing.Status.LastStateChange, ut.Status.LastStateChange) + ut.Status.LastStateChange = cmp.Or(existing.GetStatus().GetLastStateChange(), ut.Status.LastStateChange) // Update specific values. if existing.GetSpec().GetState() != ut.GetSpec().GetState() { diff --git a/lib/web/usertasks_test.go b/lib/web/usertasks_test.go index 0bb2dbb9a9f9a..13e9723458090 100644 --- a/lib/web/usertasks_test.go +++ b/lib/web/usertasks_test.go @@ -31,6 +31,7 @@ import ( usertasksv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/usertasks/v1" "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/api/types/usertasks" + "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/web/ui" ) @@ -53,6 +54,8 @@ func TestUserTask(t *testing.T) { }) require.NoError(t, err) pack := env.proxies[0].authPack(t, userWithRW, []types.Role{roleRWUserTask}) + adminClient, err := env.server.NewClient(auth.TestAdmin()) + require.NoError(t, err) getAllEndpoint := pack.clt.Endpoint("webapi", "sites", clusterName, "usertask") singleItemEndpoint := func(name string) string { @@ -90,7 +93,7 @@ func TestUserTask(t *testing.T) { }) require.NoError(t, err) - _, err = env.proxies[0].auth.Auth().CreateUserTask(ctx, userTask) + _, err = adminClient.UserTasksServiceClient().CreateUserTask(ctx, userTask) require.NoError(t, err) userTaskForTest = userTask }