Skip to content

[v18] Revert setOwnersEligibility setMemberEligibility functionality #60575

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
smallinsky merged 1 commit into from
Oct 24, 2025
Merged

[v18] Revert setOwnersEligibility setMemberEligibility functionality #60575

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
56 changes: 0 additions & 56 deletions lib/services/local/access_list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,8 +236,6 @@ func (a *AccessListService) runOpWithLock(ctx context.Context, accessList *acces
return trace.Wrap(err)
}
preserveAccessListFields(existingAccessList, accessList)
setOwnersEligibility(accessList)

listMembers, err := a.memberService.WithPrefix(accessList.GetName()).GetResources(ctx)
if err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -537,7 +535,6 @@ func (a *AccessListService) UpsertAccessListMember(ctx context.Context, member *
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)
setMemberEligibility(memberList, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -585,7 +582,6 @@ func (a *AccessListService) UpdateAccessListMember(ctx context.Context, member *
return trace.Wrap(err)
}
keepAWSIdentityCenterLabels(existingMember, member)
setMemberEligibility(memberList, member)

if err := accesslists.ValidateAccessListMember(ctx, memberList, member, &accessListAndMembersGetter{a.service, a.memberService}); err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -685,7 +681,6 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc
if err := accessList.CheckAndSetDefaults(); err != nil {
return nil, nil, trace.Wrap(err)
}
setOwnersEligibility(accessList)

for _, m := range membersIn {
if err := m.CheckAndSetDefaults(); err != nil {
Expand Down Expand Up @@ -773,11 +768,6 @@ func (a *AccessListService) UpsertAccessListWithMembers(ctx context.Context, acc

// Add any remaining members to the access list.
for _, member := range membersMap {
// Set Eligibility status for new members if this is possible
// to avoid updating the user object by access list eligibility reconciler
// and save backend operations.
setMemberEligibility(accessList, member)

upserted, err := a.memberService.WithPrefix(accessList.GetName()).UpsertResource(ctx, member)
if err != nil {
return trace.Wrap(err)
Expand Down Expand Up @@ -1089,49 +1079,3 @@ func keepAWSIdentityCenterLabels(old, new *accesslist.AccessListMember) {
new.Metadata.Labels = old.GetAllLabels()
}
}

// setMemberEligibility sets the member eligibility status to eligible if possible to avoid
// unnecessary updates via access list eligibility reconciler.
func setMemberEligibility(acl *accesslist.AccessList, member *accesslist.AccessListMember) {
if isEligibilityStatusKnown(member.Spec.IneligibleStatus) {
// If the status was explicit set don't change it.
return
}
if !member.Spec.Expires.IsZero() || !acl.Spec.MembershipRequires.IsEmpty() {
// If the member has an expiration date or the Access List Requirements are not empty
// we cant assume the eligibility status. That needs to be calculated
// by the eligibility reconsider based on the user object.
return
}
member.Spec.IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String()
}

// setOwnersEligibility sets the owners eligibility status to eligible if possible to avoid
// unnecessary updated via access list eligibility reconsider.
func setOwnersEligibility(accessList *accesslist.AccessList) {
if !accessList.Spec.OwnershipRequires.IsEmpty() {
// Owners eligibility needs to be calculated based on the user object.
// This is done by the eligibility reconsider.
return
}

for i := range accessList.Spec.Owners {
if isEligibilityStatusKnown(accessList.Spec.Owners[i].IneligibleStatus) {
// If the status was explicit set don't change it.
continue
}
// If the ownership requirements are empty, all owners are eligible.
// There is no owner ineligibility expiration date
accessList.Spec.Owners[i].IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String()
}
}

// isEligibilityStatusKnown returns true if the status is known (not empty and not undefined)
func isEligibilityStatusKnown(status string) bool {
switch status {
case accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_UNSPECIFIED.String(), "":
return false
default:
return true
}
}
93 changes: 0 additions & 93 deletions lib/services/local/access_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import (
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/require"

accesslistv1 "github.com/gravitational/teleport/api/gen/proto/go/teleport/accesslist/v1"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/common"
"github.com/gravitational/teleport/api/types/header"
Expand Down Expand Up @@ -1441,80 +1440,6 @@ func TestAccessListRequiresEqual(t *testing.T) {
}
}

func TestAccessListMemberOwnerEligibility(t *testing.T) {
clock := clockwork.NewFakeClock()
ctx := context.Background()

mem, err := memory.New(memory.Config{
Context: ctx,
Clock: clock,
})
require.NoError(t, err)

service := newAccessListService(t, mem, clock, true /* igsEnabled */)

acl := newAccessList(t, "test-access-list-1", clock,
withOwners([]accesslist.Owner{{Name: "test-owner-1"}}),
withOwnerRequires(accesslist.Requires{}),
withMemberRequires(accesslist.Requires{}),
)

aclR := newAccessList(t, "test-access-list-2", clock,
withOwners([]accesslist.Owner{{Name: "test-owner-1"}}),
withOwnerRequires(accesslist.Requires{Roles: []string{"role1"}}),
withMemberRequires(accesslist.Requires{Roles: []string{"role2"}}),
)

_, err = service.UpsertAccessList(ctx, acl)
require.NoError(t, err)
item, err := service.GetAccessList(ctx, acl.GetName())
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), item.GetOwners()[0].IneligibleStatus)

_, err = service.UpsertAccessList(ctx, aclR)
require.NoError(t, err)
item2, err := service.GetAccessList(ctx, aclR.GetName())
require.NoError(t, err)
require.Empty(t, item2.GetOwners()[0].IneligibleStatus)

_, err = service.UpsertAccessListMember(ctx, newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{})))
require.NoError(t, err)
m, err := service.GetAccessListMember(ctx, acl.GetName(), "member1")
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), m.Spec.IneligibleStatus)

_, err = service.UpsertAccessListMember(ctx, newAccessListMember(t, aclR.GetName(), "member1"))
require.NoError(t, err)
m, err = service.GetAccessListMember(ctx, aclR.GetName(), "member1")
require.NoError(t, err)
require.Empty(t, m.Spec.IneligibleStatus)

require.NoError(t, service.DeleteAllAccessLists(ctx))

_, _, err = service.UpsertAccessListWithMembers(ctx, acl, []*accesslist.AccessListMember{
newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{})),
newAccessListMember(t, acl.GetName(), "member2", withExpire(time.Now().Add(time.Hour))),
})
require.NoError(t, err)
m1, err := service.GetAccessListMember(ctx, acl.GetName(), "member1")
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_ELIGIBLE.String(), m1.Spec.IneligibleStatus)

m2, err := service.GetAccessListMember(ctx, acl.GetName(), "member2")
require.NoError(t, err)
require.Empty(t, m2.Spec.IneligibleStatus)

require.NoError(t, service.DeleteAllAccessLists(ctx))
member := newAccessListMember(t, acl.GetName(), "member1", withExpire(time.Time{}))
member.Spec.IneligibleStatus = accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_USER_NOT_EXIST.String()
_, _, err = service.UpsertAccessListWithMembers(ctx, acl, []*accesslist.AccessListMember{member})
require.NoError(t, err)

m3, err := service.GetAccessListMember(ctx, acl.GetName(), "member1")
require.NoError(t, err)
require.Equal(t, accesslistv1.IneligibleStatus_INELIGIBLE_STATUS_USER_NOT_EXIST.String(), m3.Spec.IneligibleStatus)
}

type newAccessListOptions struct {
typ accesslist.Type
owners []accesslist.Owner
Expand All @@ -1536,18 +1461,6 @@ func withOwners(owners []accesslist.Owner) newAccessListOpt {
}
}

func withOwnerRequires(req accesslist.Requires) newAccessListOpt {
return func(o *newAccessListOptions) {
o.ownerRequires = req
}
}

func withMemberRequires(req accesslist.Requires) newAccessListOpt {
return func(o *newAccessListOptions) {
o.memberRequires = req
}
}

func newAccessList(t *testing.T, name string, clock clockwork.Clock, opts ...newAccessListOpt) *accesslist.AccessList {
t.Helper()

Expand Down Expand Up @@ -1634,12 +1547,6 @@ func withMembershipKind(membershipKind string) accessListMemberOpt {
}
}

func withExpire(t time.Time) accessListMemberOpt {
return func(o *accessListMemberOptions) {
o.expires = t
}
}

func newAccessListMember(t *testing.T, accessList, name string, opts ...accessListMemberOpt) *accesslist.AccessListMember {
t.Helper()

Expand Down
Loading