Skip to content

Fix AccessListService.UpsertAccessList MemberOf update #56760

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
Jul 18, 2025
Merged

Fix AccessListService.UpsertAccessList MemberOf update #56760

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
2 changes: 1 addition & 1 deletion api/utils/clientutils/resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func ResourcesWithPageSize[T any](ctx context.Context, pageFunc func(context.Con
continue
}

yield(*new(T), err)
yield(*new(T), trace.Wrap(err))
return
}
for _, resource := range page {
Expand Down
55 changes: 29 additions & 26 deletions lib/services/local/access_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
}

var upserted *accesslist.AccessList
var existingList *accesslist.AccessList
var existingAccessList *accesslist.AccessList

opFn := a.service.UpsertResource
if op == opTypeUpdate {
Expand All @@ -214,19 +214,14 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
validateAccessList := func() error {
var err error

if op == opTypeUpdate {
existingList, err = a.service.GetResource(ctx, accessList.GetName())
if err != nil {
return trace.Wrap(err)
}
// Set memberOf / ownerOf to the existing values to prevent them from being updated.
accessList.Status.MemberOf = existingList.Status.MemberOf
accessList.Status.OwnerOf = existingList.Status.OwnerOf
} else {
// In case the MemberOf/OwnerOf fields were manually changed, set to empty.
accessList.Status.MemberOf = []string{}
accessList.Status.OwnerOf = []string{}
existingAccessList, err = a.service.GetResource(ctx, accessList.GetName())
if op == opTypeUpsert && trace.IsNotFound(err) {
// Not having already existing access_list in the backend is ok in case of
// upsert.
} else if err != nil {
return trace.Wrap(err)
}
preserveAccessListFields(existingAccessList, accessList)

listMembers, err := a.memberService.WithPrefix(accessList.GetName()).GetResources(ctx)
if err != nil {
Expand All @@ -245,8 +240,8 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
reconcileOwners := func() error {
// Create map to store owners for efficient lookup
originalOwnersMap := make(map[string]struct{})
if existingList != nil {
for _, owner := range existingList.Spec.Owners {
if existingAccessList != nil {
for _, owner := range existingAccessList.Spec.Owners {
if owner.MembershipKind == accesslist.MembershipKindList {
originalOwnersMap[owner.Name] = struct{}{}
}
Expand Down Expand Up @@ -552,14 +547,17 @@ func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member *
}

upserted, err = a.memberService.WithPrefix(member.Spec.AccessList).UpsertResource(ctx, member)
if err != nil {
return trace.Wrap(err)
}

if err == nil && member.Spec.MembershipKind == accesslist.MembershipKindList {
if member.Spec.MembershipKind == accesslist.MembershipKindList {
if err := a.updateAccessListMemberOf(ctx, member.Spec.AccessList, member.Spec.Name, true); err != nil {
return trace.Wrap(err)
}
}

return trace.Wrap(err)
return nil
}

err := a.service.RunWhileLocked(ctx, []string{accessListResourceLockName}, accessListLockTTL, func(ctx context.Context, _ backend.Backend) error {
Expand Down Expand Up @@ -679,18 +677,11 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc
}

validateAccessList := func() error {
existingList, err := a.service.GetResource(ctx, accessList.GetName())
existingAccessList, err := a.service.GetResource(ctx, accessList.GetName())
if err != nil && !trace.IsNotFound(err) {
return trace.Wrap(err)
}
if existingList != nil {
accessList.Status.MemberOf = existingList.Status.MemberOf
accessList.Status.OwnerOf = existingList.Status.OwnerOf
} else {
// In case the MemberOf/OwnerOf fields were manually changed, set to empty.
accessList.Status.MemberOf = []string{}
accessList.Status.OwnerOf = []string{}
}
preserveAccessListFields(existingAccessList, accessList)

if err := accesslists.ValidateAccessListWithMembers(ctx, accessList, membersIn, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -1045,6 +1036,18 @@ func (a *AccessListService) VerifyAccessListCreateLimit(ctx context.Context, tar
return trace.AccessDenied("%s", limitReachedMessage)
}

func preserveAccessListFields(existingAccessList, accessList *accesslist.AccessList) {
if existingAccessList != nil {
// Set MemberOf/OwnerOf to the existing values to prevent them from being updated.
accessList.Status.MemberOf = existingAccessList.Status.MemberOf
accessList.Status.OwnerOf = existingAccessList.Status.OwnerOf
} else {
// For newly created AccessList make sure MemberOf/OwnerOf are empty.
accessList.Status.MemberOf = []string{}
accessList.Status.OwnerOf = []string{}
}
}
Comment thread
kopiczko marked this conversation as resolved.

// keepAWSIdentityCenterLabels preserves member labels if
// it originated from AWS Identity Center plugin.
// The Web UI does not currently preserve metadata labels so this function should be called
Expand Down
Loading
Loading