Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit notifications when creating User Task for a given Integration #47556

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marcoandredinis
Copy link
Contributor

@marcoandredinis marcoandredinis commented Oct 14, 2024

User Tasks is a new resource that contains issues about an integration.
For now, User Tasks are only created to register issues related to auto
enrolling EC2 instances.
Other types of issues will be registered in the future.

Users must proactively list User Tasks in order to understand existing
issues.
For now, this can only be done using tctl get user_task.

An UI for this new resource will be created, but users still need to
navigate to that UI to see what issues exist.

This PR creates a new Notification type which is used to explicitly
notify the user that the Integration has pending User Tasks.

When are users notified about pending User Tasks?
When a User Task is created or updated, if its state is OPEN, then a
notification is created.

Given that we can have dozens of UserTasks for a single integration, we
had to ensure only one Notification exists per Integration.
To do that, the current Notification ID is stored in the Integration's
Status field.
This way, when we try to create a new Notification, we ensure the
existing one is deleted to prevent showing two (or more) Notifications
saying "Your integration needs attention".
All of this is protected by a semaphore lock. The DiscoveryService is
highly async and multiple UserTasks can be created in parallel, and the
locks prevents it.

Before merging:

  • (WebUI) Create User Tasks UI
  • (WebUI) Create Pending User Tasks Integration in Notifications Factory in UI

@marcoandredinis marcoandredinis added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v16 labels Oct 14, 2024
@marcoandredinis marcoandredinis force-pushed the marco/discovery_emit_discoverec2_tasks branch from f7ac023 to 7868348 Compare October 15, 2024 08:22
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 2 times, most recently from aeb8da5 to d452cdd Compare October 15, 2024 09:38
@marcoandredinis marcoandredinis force-pushed the marco/discovery_emit_discoverec2_tasks branch from 7868348 to 1ee8889 Compare October 15, 2024 16:13
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch from d452cdd to 5c59d0f Compare October 15, 2024 17:28
@marcoandredinis marcoandredinis force-pushed the marco/discovery_emit_discoverec2_tasks branch from 1ee8889 to c2bae80 Compare October 15, 2024 17:29
Base automatically changed from marco/discovery_emit_discoverec2_tasks to master October 16, 2024 10:33
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 17 times, most recently from 8048d2e to 8688572 Compare October 23, 2024 09:48
@marcoandredinis marcoandredinis marked this pull request as ready for review October 23, 2024 10:02
Copy link
Contributor

@bernardjkim bernardjkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not entirely familiar with the notification system. But does the global notification not need to be deleted when the user task is deleted? Do we just want to wait for it to expire instead?

api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved
lib/auth/usertasks/usertasksv1/service.go Outdated Show resolved Hide resolved
@marcoandredinis
Copy link
Contributor Author

Not entirely familiar with the notification system. But does the global notification not need to be deleted when the user task is deleted? Do we just want to wait for it to expire instead?

It should but I'm not too worried about it because UserTasks should expire after 10 minutes (twice the PollInterval of teleport.yaml/discovery_service.poll_interval, which defaults to 5 minutes) - thus, the Notifications will also expire after 10 minutes.
We also give no way for users to delete User Tasks from the UI, they can only mark it as Resolved.

I'm hesitant in adding the deletion of the Notification when a given UserTask is deleted, because it would add some complexity and make the system slower.
This complexity comes from deleting the Notification only when there's no other pending UserTask, and the need to update the Integration.Status.PendingUserTaskNotificationExpiration when the Notification we are about to delete was the longest one.

So, we would need to iterate over all UserTasks to check whether we must delete the Integration or update the expiration date.
All of this under the existing Lock for integration: integration_name key.

I would rather not do it for now, but please let me know what you think.

@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 2 times, most recently from bca87f4 to 1dbb17a Compare October 25, 2024 10:25
@marcoandredinis marcoandredinis added backport/branch/v17 discover Issues related to Teleport Discover labels Oct 28, 2024
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch from 1dbb17a to 6a81ac9 Compare November 11, 2024 11:01
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 4 times, most recently from f1788bb to 147f187 Compare November 28, 2024 18:33
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 2 times, most recently from 2da8ed9 to 2c6948c Compare December 6, 2024 11:06
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch 2 times, most recently from 279ec57 to f3058a9 Compare December 10, 2024 11:35
User Tasks is a new resource that contains issues about an integration.
For now, User Tasks are only created to register issues related to auto
enrolling EC2 instances.
Other types of issues will be registered in the future.

Users must proactively list User Tasks in order to understand existing
issues.
For now, this can only be done using `tctl get user_task`.

An UI for this new resource will be created, but users still need to
navigate to that UI to see what issues exist.

This PR creates a new Notification type which is used to explicitly
notify the user that the Integration has pending User Tasks.

When are users notified about pending User Tasks?
When a User Task is created or updated, if its state is OPEN, then a
notification is created.

Given that we can have dozens of UserTasks for a single integration, we
had to ensure only one Notification exists per Integration.
To do that, the current Notification ID is stored in the Integration's
Status field.
This way, when we try to create a new Notification, we ensure the
existing one is deleted to prevent showing two (or more) Notifications
saying "Your integration needs attention".
All of this is protected by a semaphore lock. The DiscoveryService is
highly async and multiple UserTasks can be created in parallel, and the
locks prevents it.
@marcoandredinis marcoandredinis force-pushed the marco/notify-on-discoverec2-tasks branch from f3058a9 to c25372a Compare December 16, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 discover Issues related to Teleport Discover discovery do-not-merge no-changelog Indicates that a PR does not require a changelog entry size/md
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants