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

fix(accesscontrol): CacheKey could not be stable #244

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

aruiz14
Copy link
Contributor

@aruiz14 aruiz14 commented Jul 18, 2024

While understanding how this code works, we realized (actually it was @bigkevmcd, kudos to him) 2 problems:

  • According to the copy godoc:

    Copy returns the number of elements copied, which will be the minimum of len(src) and len(dst).

    This makes the following code to produce an empty slice, since only the length is taken into account, not the capacity:

    groups := make([]string, 0, len(groupBase))
    copy(groups, groupBase)
    
  • The groups variable is not actually used, since we range over user.GetGroups() (the unused variable warning was probably masked by using sort.Strings).

I have no evidence this could be currently causing any problems though. It depends on the underlying implementation of GetGroups(). For our use case, it does not really matter if the slice is actually sorted, the only requirement would be to be stable.

Unit tests added in follow-up PR containing a small refactoring to make it easier to test:
#245

@aruiz14 aruiz14 requested a review from a team as a code owner July 18, 2024 08:04
@aruiz14 aruiz14 requested a review from bigkevmcd July 18, 2024 08:04
@moio moio merged commit 435e220 into rancher:master Jul 31, 2024
1 check passed
@aruiz14 aruiz14 deleted the cachekey-sorted branch August 19, 2024 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants