Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Fix leader election with RBAC in tests #70

Merged
merged 2 commits into from
Oct 31, 2017

Conversation

munnerz
Copy link
Contributor

@munnerz munnerz commented Oct 31, 2017

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

NONE

@jetstack-bot
Copy link
Collaborator

@munnerz: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

This PR also disables scoping Navigator to a single namespace as part of the standard helm chart deployment

@munnerz munnerz assigned wallrj and unassigned simonswine Oct 31, 2017
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @munnerz

I built your branch and ran e2e tests locally.

  • Tests pass, but
  • leader election still seems to be failing.

minikube start --vm-driver kvm --extra-config=apiserver.Authorization.Mode=RBAC

I'll attach the logs.

I suggest we:

- apiGroups: [""]
resources: ["endpoints"]
verbs: ["*"]
resourceNames: ["navigator-controller"]
Copy link
Member

Choose a reason for hiding this comment

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

❓ What does this do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows navigator-controller to modify Endpoints resources with the name navigator-controller. This is the resource used for leader election 😄

@wallrj
Copy link
Member

wallrj commented Oct 31, 2017

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

So after looking into this some more, the actual controller won't be able to create the initial Endpoint resource used for leader election as we have explicitly named the resource in the RBAC policy, which consequently then rejects POST requests to the API endpoint (as the endpoint does not include the resource name at this point). This is a limitation with RBAC at the moment.

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

Updated & tested working. Review as and when you've got time. Don't want to hold you up!

Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @munnerz

So you're having helm create an Endpoint object that would normally be created automatically here:

Fine.

But ideally I'd suggest that we add a note to this file linking to the RBAC shortcoming that you mentioned above and which makes this necessary. And maybe a navigator issue to remove this file when it is no longer needed.

I can't see an equivalent file in the service-catalog project, but it looks like they may just disable the leader election in their tests:

I also repeat that I think you ought to add an extra e2e test that waits for an ElasticSearch services to be created. That way we know that the controller is doing something at least.

kubectl get services --namespace navigator-e2e-database1
NAME                TYPE        CLUSTER-IP   EXTERNAL-IP   PORT(S)             AGE
es-demo             ClusterIP   10.0.0.151   <none>        9300/TCP,9200/TCP   26m
es-demo-discovery   ClusterIP   10.0.0.3     <none>        9300/TCP            26m

Merge when you're satisfied with everything.

@wallrj
Copy link
Member

wallrj commented Oct 31, 2017

/lgtm

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

@wallrj - I'm well aware the e2e tests need improving. Currently we are blocked by needing to supply some e2e testing infrastructure via Prow, so that we can actually get VMs big enough to run Elasticsearch (it is a massive resource hog).

This will I'll be working on some e2e testing, so will get the actual test cases written up then so we can start to see how they perform 😄

@munnerz
Copy link
Contributor Author

munnerz commented Oct 31, 2017

/approve

@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, wallrj

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@jetstack-bot
Copy link
Collaborator

Automatic merge from submit-queue.

@jetstack-bot jetstack-bot merged commit 0e2c5a4 into jetstack:master Oct 31, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants