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

Add RoleAssignment UUID Generation ADR #3935

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
---
title: '2024-04: RoleAssignments UUID Generation'
---

## Context

In ASO, we made several improvements to make RoleAssignment user-friendly by auto-generating their AzureName (which must be a UUID) using UUIDv5 with a seed string based on group, kind, namespace, and name.
The aim for above approach was to:
- include the namespace and name to ensure no two RoleAssignments in the same cluster can end up with the same UUID.
- include the group and kind to ensure that different kinds of resources get different UUIDs. This isn't entirely required by Azure, but it makes sense to avoid collisions between two resources of different types even if they have the same namespace and name.
- include the owner group, kind, and name to avoid collisions between resources with the same name in different clusters that actually point to different Azure resources.

However, the case where users have multiple ASO instances in multiple clusters with resources in the same namespace, in each cluster having the same name and pointing to different Azure resource is not supported without the user manually giving each RoleAssignment its own UUID.
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this sentence.

The above issue can be avoided by defaulting the AzureName property to a random UUID, however it will cause issues if the RoleAssignment is ever deleted and recreated (or migrated between clusters) as the old UUID will be orphaned.

Issue: [RoleAssignment UUID clashes](https://github.com/Azure/azure-service-operator/issues/3637)

### Option 1: Adding a new property

Add a new property on RoleAssignment spec to control this behaviour.

To use random uuid:
```yaml
uuidGeneration: random
```

To use deterministic uuid(default)
```yaml
uuidGeneration: stable
```

**Pro**: Backward compatible
**Pro**: Ease of use

**Con**: Need to implement infrastructure to add a new property
**Con**: Users will have to manage the AzureName by themselves while exporting/importing

### Option 2: Using annotations

We can use annotation below to control the name generation behaviour.
In this case, If user does not specify any annotation and does not specify AzureName, ASO sets the default annotation below and follows the default behaviour of stable UUID generation.

To use random uuid:
```yaml
serviceoperator.azure.com/uuid-generation: random
matthchr marked this conversation as resolved.
Show resolved Hide resolved
```

To use deterministic uuid(default)
```yaml
serviceoperator.azure.com/uuid-generation: stable
```
Copy link
Member

Choose a reason for hiding this comment

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

I think you should talk here about how we actually do defaulting too:

There are two options:

  • If not specified, automatically set serviceoperator.azure.com/uuid-generation: Default when a resource is created when defaulting its name.
  • If not specified, don't set any annotation (but still generate a UUID as per the Default pattern.

I think it's OK to default-in the uuid-generation annotation if the user hasn't set it (and also hasn't set the name?). I guess if the name is set by the user, then we wouldn't add the annotation at all because we actualyl didn't do any generation?

and if the user sets it, we'll honor what they set (though it'll not do anything if they set it and the name)?


**Pro**: Backward compatible
**Pro**: Ease of use

**Con**: Users will have to manage the AzureName by themselves while exporting/importing
super-harsh marked this conversation as resolved.
Show resolved Hide resolved
**Con**: Pushes a crucial part of the resource definition into an annotation; the spec is no longer a complete definition of the resource.
**Con**: Annotations are far more easily modified by other tooling, which may have unexpected flow on effects.

### Option 3: Using subscription ID as seed

Adding subscriptionId as a seed string would make the resource names distinct across subscriptions

**Pro**: User does not have to do anything

**Con**: Moving resource with same name between older to newer ASO version would require a workaround
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change when someone upgrades the version of ASO in their cluster, as any automated deployments will suddenly experience different behaviour.

**Con**: Webhooks don't have information about subscriptionID
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this automatically disqualify Option 3 from consideration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Kinda, however worth mentioning it here so we know why we didn't consider this option. Just in case if someone stumbles upon this in future


matthchr marked this conversation as resolved.
Show resolved Hide resolved
## Decision

Recommendation: Option 2 - Using annotations

We retain how the resource shape looks like and suggest using annotation for the users running into the edge case.
Comment on lines +71 to +73
Copy link
Member

Choose a reason for hiding this comment

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

Ergh. Do not like.


## Status

Proposed.

## Consequences

TBC

## Experience Report

TBC

## References

None