Skip to content

Keystone: RoleAssignment Controller#774

Merged
mandre merged 13 commits into
k-orc:mainfrom
dlaw4608:role_assignment
Jul 2, 2026
Merged

Keystone: RoleAssignment Controller#774
mandre merged 13 commits into
k-orc:mainfrom
dlaw4608:role_assignment

Conversation

@dlaw4608

@dlaw4608 dlaw4608 commented May 1, 2026

Copy link
Copy Markdown
Contributor

Closes #669
Implement RoleAssignment controller treating role assignments as relationships
rather than resources, since OpenStack doesn't assign IDs to role assignments.

Key changes:

  • Framework extension: Split ORCStatusApplyConfig into base/extended interfaces to support NoResourceID resources (enables reuse for endpoint policies, aggregate hosts)
  • Added NoResourceID flag to resource generator (removes import.id and status.id from CRD)
  • Custom reconciler with tuple-based identification: stores (roleID, userID/groupID,
    projectID/domainID) in status.resource instead of single ID
  • Integrates with generic framework components (status.UpdateStatus, ResourceStatusWriter)
  • Immutable spec: matches OpenStack API constraints (assignments can't be updated)
  • Deletion guards: All dependencies (Role, User/Group, Project/Domain) protected from deletion

E2E test coverage (7 suites, 56 test files):

  • All actor-scope combinations (user-project, user-domain, group-project, group-domain)
  • Import by filter (happy path, multiple-match error, dependency lifecycle)
  • Dependency guards and lifecycle
  • Unit tests for adoption edge cases

Implementation:

  • reconciler.go: Custom reconcile loop with import/adoption/create phases
  • actuator.go: GetResourceByComponents() queries by tuple instead of ID
  • status.go: ResourceStatusWriter for framework integration

Co.Authored By @mandre
API Ref: https://docs.openstack.org/api-ref/identity/v3/index.html#roles

@github-actions github-actions Bot added the semver:major Breaking change label May 1, 2026
…t Controller

Signed-off-by: Daniel Lawton <dlawton@redhat.com>
@dlaw4608 dlaw4608 force-pushed the role_assignment branch 3 times, most recently from a79310e to 1235e66 Compare May 19, 2026 16:48
@dlaw4608 dlaw4608 marked this pull request as ready for review May 19, 2026 16:48
@dlaw4608 dlaw4608 marked this pull request as draft May 20, 2026 09:31
…ationships

  rather than resources, since OpenStack doesn't assign IDs to role assignments.

  Key changes:
  - Custom reconciler: Ignores generic framework since role assignments lack
    OpenStack resource IDs
  - Component-based identification: Uses tuple (roleID, userID/groupID,
    projectID/domainID) stored in Status.Resource instead of UIDs
  - Status.ID intentionally nil: Components serve as natural identifiers
  - Immutable spec: Role assignments can't be modified after creation
    (matching Kubernetes RBAC behavior)
  - Deletion guards: All dependencies (Role, User/Group, Project/Domain)
    protected from deletion while in use

  E2E tests cover four actor-scope combinations:
  - roleassignment-create-user-project
  - roleassignment-create-user-domain
  - roleassignment-create-group-project
  - roleassignment-create-group-domain

  Plus roleassignment-dependency test verifying deletion guard behavior.

  Implementation details:
  - reconciler.go: Custom reconcile loop handling create/delete lifecycle
  - actuator.go: GetResourceByComponents() replaces GetOSResourceByID()

Signed-off-by: Daniel Lawton <dlawton@redhat.com>
@dlaw4608 dlaw4608 marked this pull request as ready for review May 20, 2026 11:36
@berendt

berendt commented Jun 10, 2026

Copy link
Copy Markdown

@mandre Can you have a look please? We need this one soonish.

@mandre

mandre commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator

@mandre Can you have a look please? We need this one soonish.

Will do. We can review this new controller and merge it soon, but I can't promise this will be in a release shortly (we've switched the main branch to v3 development and I'm a bit reluctant about backporting controllers to v2 as). We'll see.

Comment thread cmd/resource-generator/main.go Outdated
},
{
Name: "RoleAssignment",
IsNotNamed: true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think we can use the resource-generator for RoleAssignment today, because this is a relationship resource and a lot of things the generator creates for us wouldn't apply here (id for sure, possibly import).

The closest we have right now in the repo is the RouterInterface, that was completely handwritten. We should either:

  • update the generator to account for this relationship resources that lack ID
  • move the RoleAssignment to be a specialResource like RouterInterface

@mandre mandre Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It seems to me the easiest would be to add a new NoResourceID option to the generator that would omit:

  • import.id from the Import type
  • status.ID from the Status type
  • the ID print column

@winiciusallan

Copy link
Copy Markdown
Contributor

I see a direction for this PR where we follow what Kubernetes API does for the RoleBinding resource. There, we can pass a list of subjects , thus this could be a list of users or groups in our case.

I wonder if, from the user perspective, it wouldn't be interest to be able to change the list of users/group that have a specific role assigned.

mandre added 11 commits July 2, 2026 10:18
OpenStack role assignments are relationship resources without an
OpenStack-assigned ID. Add NoResourceID support to the resource generator
templates and set it for RoleAssignment, removing import.id, status.id,
and the ID print column from the CRD.
The roleassignment reconciler was missing the import path entirely.
Add it to reconcileNormal, between the existing
status check and creation phases. The implementation mirrors the generic
reconciler's GetOrCreateOSResource import-by-filter logic:

- Resolves filter dependencies via ListOSResourcesForImport
- Validates at most one result (terminal error on multiple matches)
- Polls with 15s interval when no match is found yet

Add KUTTL E2E tests.
Split ORCStatusApplyConfig into a base interface (WithConditions only)
and ORCStatusApplyConfigWithID (adds WithID) so that resources without
an OpenStack-assigned ID can use ResourceStatusWriter and
status.UpdateStatus without requiring a WithID method on their status
apply configuration.

The generic Controller and SetStatusID use ORCStatusApplyConfigWithID,
so existing controllers are unaffected. RoleAssignment's custom
reconciler now delegates status writing to status.UpdateStatus via a
ResourceStatusWriter implementation, replacing the 40-line inline
updateStatus method and aligning with the pattern used by all other
controllers.
Add adoption logic to the roleassignment custom reconciler, matching the
pattern used by the generic reconciler framework for ID-based resources.

In reconcileNormal, between the import phase and the create phase:
- Add an unmanaged policy guard that returns a terminal error
- Call ListOSResourcesForAdoption to find and adopt an existing OpenStack
  resource before creating a new one

In reconcileDelete, add orphan cleanup:
- When Status.Resource was never populated, call ListOSResourcesForAdoption
  to find orphaned resources that were created but not recorded in status
- Restructure actuator creation out of the Status.Resource block so it is
  shared by both the status-based fetch and the adoption-based orphan check

Add unit tests for ListOSResourcesForAdoption covering nil resource spec,
user+project scope, group+domain scope, empty OS results, and OS client
error propagation.
Replace the naive 'return first result' loop with a call to atMostOne,
which verifies the iterator yields at most one element. If OpenStack
returns multiple results for the same (role, actor, scope) tuple, the
controller now returns a terminal error instead of silently ignoring
the extras.
Remove local finalizer variable assignments in reconcileNormal and
reconcileDelete that shadowed the package-level finalizer variable
defined in zz_generated.controller.go. The local computation was
redundant and could diverge if controllerName were ever changed in
only one location.
GetResourceByComponents uses a LIST query, which returns (nil, nil)
for empty results rather than a 404 error. The IsNotFound(err) branch
was therefore dead code: it could never trigger when a role assignment
was deleted from OpenStack.

The actual deletion case — status fully populated but list returns no
results — fell through to the import/adoption/create phases, silently
re-creating the resource instead of reporting a terminal error.

Fix by removing the dead IsNotFound branch and returning a terminal
error when osResource is nil after a successful query with all status
components populated, matching the generic reconciler's behavior for
ID-based resources.
…pts construction

The same roles.ListAssignmentsOpts building pattern — conditionally
setting RoleID, UserID/GroupID, ScopeProjectID/ScopeDomainID from
component IDs — was repeated in GetResourceByComponents,
ListOSResourcesForAdoption, ListOSResourcesForImport, and the
post-create verification in CreateResource.

Extract a buildListOpts helper that all four call sites now use.

Also apply the atMostOne duplicate check to CreateResource's
post-create verification for consistency with GetResourceByComponents.
…cy errors

FetchDependency always returns a non-nil pointer (new(T)), so the
nil checks on the returned object were dead code. When a dependency
was not found or not ready, the code proceeded with empty-string IDs,
causing buildListOpts to produce a partially-filtered or unfiltered
OpenStack query that could match unrelated role assignments.

Check ReconcileStatus.NeedsReschedule() instead, which correctly
detects not-found, not-ready, and error states. This also fixes the
reconcileDelete path which calls ListOSResourcesForAdoption to find
orphaned resources.

Add unit tests for role-not-found, user-not-ready, and
project-not-found cases to prevent regression.
Run `make generate-bundle` which is now required in CI.
Remove the name parameter from helper functions availableRole,
availableUser, availableGroup, availableProject, and availableDomain
since each was always called with the same value. The names are now
defined as constants and used directly in the helpers.

Also vary statusID values in the 'no matches from OS' test case to
avoid secondary unparam warnings for availableUser and availableProject.
@mandre

mandre commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

@dlaw4608 I added a few commits to your PR. Could you have a look and let me know if they make sense to you?

@dlaw4608

dlaw4608 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

@mandre LGTM I updated the PR to reflect the changes you made

@mandre mandre added this pull request to the merge queue Jul 2, 2026
}
}

// Phase 3: Create actuator

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cosmetic Nit: is this phase a duplicate or meant to be the same as whats in line 127?

Merged via the queue into k-orc:main with commit b5a0e33 Jul 2, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver:major Breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keystone Role Assignment Controller

4 participants