Add unique notification identifier resource and CRUD methods

[rudream]

Purpose

This PR adds a new "unique notification identifier" resource to the backend as well as CRUD methods for it. This resource will be used as a reference to prevent duplicate notifications.

For context, one of the shortcomings of the notifications system as it is now is that it is not possible to keep track of whether a notification for a particular event has already been created. This means that if we want to create a notification as a result of repetitive logic (such as a periodic check), it would keep creating a new duplicate notification every time it is run.

With this new resource, notifications that fall into this category can be tied to a unique notification identifier resource. The first time the notification is created, it will create a unique identifier resource tied to it via some type of metadata as the identifier, for example, a 30 day access list review reminder notification would create an identifier with a key like unique_notification_identifier/access_list_30d_reminder/<access-list-id>, then every time we detect that an access list is due for review in less than 30 days, we can first check for the existence of this unique identifier resource in order to know whether to avoid creating a duplicate notification.

[marcoandredinis]

Adding myself as reviewer because I'm also interested (see #47556)

[fspmarshall]

CRUD logic here all seems reasonable. From a concurrency/correctness standpoint, this kind of scheme is a fairly weak guarantee and may result in missed and/or duplicate notifications under some conditions.

An AtomicWrite based system would be more robust, but implementing an AtomicWrite based system that supports the usecase of creating many identifiers that map to a single notification would be non-trivial.

I suggest adding a note to the documentation about this feature to call out the fact that this scheme is likely not suitable for any notification that we deem security critical due to weak concurrency/correctness guarantees (idk that we currently have any of those, but it would be good for future reference).

[espadolini]

LGTM as long as we keep in mind that it's only an optimization to reduce the amount of duplicate notifications and not at all a guarantee that they won't be created (because of concurrent operations and/or potential crashes halfway through RPCs).

[espadolini]

this scheme is likely not suitable for any notification that we deem security critical

Is there a scenario where problems can lead to missing a notification rather than emitting a spurious extra one?

[marcoandredinis]

How's it going to work?

We check whether a given prefix exists (ListUniqueNotificationIdentifiersForPrefix)

• if it returns 0 results, we create the notification and the uniq notification prefix (CreateUniqueNotificationIdentifier
• if it returns at least 1 result, we skip the notification creation

Is the above correct?

[rudream]

Is there a scenario where problems can lead to missing a notification rather than emitting a spurious extra one?

@espadolini Depending on the order of operations, it's possible that we create the identifier and then the server crashes before we create the notification, or the notification creation fails for some other reason. This would result in the identifier being created but no notification, and the next time the logic runs the notification wouldn't be created because the identifier exists already. This can be mitigated though by ensuring we create the notification before the identifier, so that in the worst case we just have a duplicate notification.

How's it going to work?

We check whether a given prefix exists (ListUniqueNotificationIdentifiersForPrefix)

if it returns 0 results, we create the notification and the uniq notification prefix (CreateUniqueNotificationIdentifier
if it returns at least 1 result, we skip the notification creation
Is the above correct?

@marcoandredinis That's correct, however we would be checking more than just the prefix but also the identifier itself (the prefix is just a way to group identifiers).

[marcoandredinis]

I wonder if we should create a single operation that creates a notification and a unique notification identifier for it.
Either with a semaphore lock or some other concurrency primitive.

I'm saying this because in my use case we might have concurrent calls to create notifications that should be represented as a single one. So, it's high likely that this will generate duplicates.

[rudream]

I wonder if we should create a single operation that creates a notification and a unique notification identifier for it.
Either with a semaphore lock or some other concurrency primitive.

I'm saying this because in my use case we might have concurrent calls to create notifications that should be represented as a single one. So, it's high likely that this will generate duplicates.

We can create an adapter for this for use cases like yours, however there are cases where a notification would map to multiple identifiers (like if we're batching access list review reminders together), so this wouldn't always work.

[espadolini]

I wonder if we should create a single operation that creates a notification and a unique notification identifier for it. Either with a semaphore lock or some other concurrency primitive.

You can use backend.AtomicWrite but on some backends (dynamodb 😢) transactional writes cost twice as much, so we should be somewhat careful about using them.

[public-teleport-github-review-bot]

@rudream See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Create PR