Skip to content

[v18] AccessListService: Fix updating status.owner_of for removed owners #62979

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.

Sign up for GitHub

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

Merged
kopiczko merged 1 commit into from
Jan 20, 2026
Merged

[v18] AccessListService: Fix updating status.owner_of for removed owners #62979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 57 additions & 23 deletions lib/services/local/access_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,27 +250,14 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
return accesslists.ValidateAccessListWithMembers(ctx, existingAccessList, accessList, listMembers, &accessListAndMembersGetter{a.service, a.memberService})
}

updateAccessList := func() error {
var err error
upserted, err = opFn(ctx, accessList)
return trace.Wrap(err)
}

reconcileOwners := func() error {
reconcileOldOwners := func() error {
currentOwnersMap := make(map[string]struct{})
for _, owner := range accessList.Spec.Owners {
if owner.MembershipKind == accesslist.MembershipKindList {
currentOwnersMap[owner.Name] = struct{}{}
}
}

// update references for new owners
for ownerName := range currentOwnersMap {
if err := a.updateAccessListOwnerOf(ctx, accessList.GetName(), ownerName, true); err != nil {
return trace.Wrap(err)
}
}

// update references for old owners
if existingAccessList != nil {
for _, owner := range existingAccessList.Spec.Owners {
Expand All @@ -290,6 +277,23 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
return nil
}

updateAccessList := func() error {
var err error
upserted, err = opFn(ctx, accessList)
return trace.Wrap(err)
}

reconcileNewOwners := func() error {
for _, owner := range accessList.Spec.Owners {
if owner.MembershipKind == accesslist.MembershipKindList {
if err := a.updateAccessListOwnerOf(ctx, accessList.GetName(), owner.Name, true); err != nil {
return trace.Wrap(err)
}
}
}
return nil
}

var actions []func() error

// If IGS is not enabled for this cluster we need to wrap the whole
Expand All @@ -300,7 +304,12 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
actions = append(actions, func() error { return a.VerifyAccessListCreateLimit(ctx, accessList.GetName()) })
}

actions = append(actions, validateAccessList, updateAccessList, reconcileOwners)
// Note we need to reconcile the old owners (clean status.owner_of for the owner lists
// which are removed with this request) first, then update the access list and then
// reconcile the new owners (set status.owner_of of the owner lists that are added with
// this request). This is to make sure the operation doesn't escalate privileges if
// interrupted as we user status.owner_of to calculate hierarchy.
actions = append(actions, validateAccessList, reconcileOldOwners, updateAccessList, reconcileNewOwners)

err := a.service.RunWhileLocked(ctx, []string{accessListResourceLockName}, accessListLockTTL,
func(ctx context.Context, _ backend.Backend) error {
Expand Down Expand Up @@ -703,8 +712,11 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
}
}

var existingAccessList *accesslist.AccessList

validateAccessList := func() error {
existingAccessList, err := a.service.GetResource(ctx, accessList.GetName())
var err error
existingAccessList, err = a.service.GetResource(ctx, accessList.GetName())
if err != nil {
// a not found error is totally legal for an upsert operation, but
// fatal for an update.
Expand Down Expand Up @@ -798,12 +810,18 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
return nil
}

reconcileOwners := func() error {
// update references for new owners
for _, owner := range accessList.Spec.Owners {
if owner.MembershipKind == accesslist.MembershipKindList {
if err := a.updateAccessListOwnerOf(ctx, accessList.GetName(), owner.Name, true); err != nil {
return trace.Wrap(err)
reconcileOldOwners := func() error {
if existingAccessList == nil {
return nil
}
for _, existingOwner := range existingAccessList.Spec.Owners {
if existingOwner.MembershipKind == accesslist.MembershipKindList {
if !slices.ContainsFunc(accessList.Spec.Owners, func(owner accesslist.Owner) bool {
return owner.Name == existingOwner.Name
}) {
if err := a.updateAccessListOwnerOf(ctx, existingAccessList.GetName(), existingOwner.Name, false); err != nil {
return trace.Wrap(err)
}
}
}
}
Expand All @@ -816,6 +834,17 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
return trace.Wrap(err)
}

reconcileNewOwners := func() error {
for _, owner := range accessList.Spec.Owners {
if owner.MembershipKind == accesslist.MembershipKindList {
if err := a.updateAccessListOwnerOf(ctx, accessList.GetName(), owner.Name, true); err != nil {
return trace.Wrap(err)
}
}
}
return nil
}

var actions []func() error

// If IGS is not enabled for this cluster we need to wrap the whole update and
Expand All @@ -826,7 +855,12 @@ func (a *AccessListService) writeAccessListWithMembers(ctx context.Context, acce
actions = append(actions, func() error { return a.VerifyAccessListCreateLimit(ctx, accessList.GetName()) })
}

actions = append(actions, validateAccessList, reconcileMembers, writeAccessList, reconcileOwners)
// Note we need to reconcile the old owners (clean status.owner_of for the owner lists
// which are removed with this request) first, then update the access list and then
// reconcile the new owners (set status.owner_of of the owner lists that are added with
// this request). This is to make sure the operation doesn't escalate privileges if
// interrupted as we use status.owner_of to calculate hierarchy.
actions = append(actions, validateAccessList, reconcileMembers, reconcileOldOwners, writeAccessList, reconcileNewOwners)

if err := a.service.RunWhileLocked(ctx, []string{accessListResourceLockName}, 2*accessListLockTTL, func(ctx context.Context, _ backend.Backend) error {
return a.service.RunWhileLocked(ctx, lockName(accessList.GetName()), 2*accessListLockTTL, func(ctx context.Context, _ backend.Backend) error {
Expand Down
Loading
Loading