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

Update lease access check to account for lease name and multiple rules #2456

Merged
merged 2 commits into from
Jul 2, 2024

Conversation

MikeEdgar
Copy link
Contributor

Allow the verbs required for lease access to be split between multiple rules and exclude rules that name a non-matching lease name. This allows rules such as those below to work as expected.

rules:
  - apiGroups:
      - coordination.k8s.io
    resources:
      # The "create" verb cannot be used with "resourceNames"
      - leases
    verbs:
      - create
  - apiGroups:
      - coordination.k8s.io
    resources:
      - leases
    resourceNames:
      - my-lease
    verbs:
      - get
      - list
      - watch
      - delete
      - patch
      - update

@openshift-ci openshift-ci bot requested review from adam-sandor and csviri June 26, 2024 00:23
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class LeaderElectionManagerTest {

private LeaderElectionManager leaderElectionManager() {
private LeaderElectionManager leaderElectionManager(Optional<Object> selfSubjectReview) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to not use Optional here but pass the object directly and use Optional.ofNullable when resolving the kubernetes client instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated how Optional is used for the mock client - let me know if it's not what you had in mind.

@csviri
Copy link
Collaborator

csviri commented Jun 26, 2024

Hi @MikeEdgar thx for the PR,

pls run mvn clean install to format the code.

@csviri
Copy link
Collaborator

csviri commented Jun 26, 2024

The integration test: LeaderElectionChangeNamespaceIT is also failing, could you pls take a look why? later also I can take a look

@MikeEdgar
Copy link
Contributor Author

The integration test: LeaderElectionChangeNamespaceIT is also failing, could you pls take a look why? later also I can take a look

Thanks, I'll take a look today.

@MikeEdgar
Copy link
Contributor Author

The integration test: LeaderElectionChangeNamespaceIT is also failing, could you pls take a look why? later also I can take a look

Thanks, I'll take a look today.

The rule filter predicate on main has incorrect grouping that I didn't notice before, that was fixed with the changes. The filter is picking any rule that has the universal/wildcard verb. I'll fix the test in this PR to deploy/remove a role/binding.

var foundRule = reviewResult.getStatus().getResourceRules().stream()
.filter(rule -> rule.getApiGroups().contains(COORDINATION_GROUP)
&& rule.getResources().contains(LEASES_RESOURCE)
&& (rule.getVerbs().containsAll(verbs)) || rule.getVerbs().contains(UNIVERSAL_VERB))
.findAny();

Signed-off-by: Michael Edgar <medgar@redhat.com>
@MikeEdgar MikeEdgar force-pushed the fix-lease-rule-check branch from 6b6b9a1 to 109e831 Compare June 26, 2024 14:57
@MikeEdgar
Copy link
Contributor Author

The permission check now tests for wildcard * for the API group and resource kinds.

@MikeEdgar MikeEdgar requested a review from metacosm June 27, 2024 11:43
Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

LGMT!

Thank you!

Signed-off-by: Chris Laprun <claprun@redhat.com>
@metacosm metacosm merged commit 0efdf85 into operator-framework:main Jul 2, 2024
20 checks passed
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.

3 participants