Move rbac up to CVO. Drop admin perms #210

enxebre · 2019-02-14T11:14:58Z

Move rbac up to CVO. Drop admin perms

enxebre · 2019-02-14T11:15:07Z

bison · 2019-02-14T11:32:24Z

install/0000_30_machine-api-operator_08_rbac.yaml

+  name: cluster-api-manager-role
+subjects:
+  - kind: ServiceAccount
+    name: default


Can we create a dedicated service account? It's not much more trouble, and it means random pods started in the namespace don't get elevated permissions.

bison · 2019-02-14T11:33:17Z

install/0000_30_machine-api-operator_08_rbac.yaml

+kind: ClusterRole
+metadata:
+  creationTimestamp: null
+  name: cluster-api-manager-role


This is a nit, but I don't think these names should end in -role etc. You already know that by the type, and it looks weird in the console. I would just call this machine-api or something.

bison · 2019-02-14T11:33:37Z

install/0000_30_machine-api-operator_08_rbac.yaml

- kind: ServiceAccount
-  name: default
-  namespace: openshift-machine-api
+  name: cluster-api-manager-rolebinding


Same nit on the name including the type here.

enxebre · 2019-02-15T14:00:14Z

/retest

enxebre · 2019-02-15T14:29:55Z

Before this merges, we need to make sure we add a validation for draining in this repo and we add the required permissions e.g manipulating daemonSets openshift/cluster-api-provider-aws#158 otherwise this would break draining cc @ingvagabund

enxebre · 2019-02-15T15:23:07Z

/test e2e-aws-operator

enxebre · 2019-02-18T08:10:32Z

/test e2e-aws-operator

enxebre · 2019-02-18T09:14:36Z

Looking at the logs now perms list seems ok. I'll update for:

Using specific sa
Including draining permissions

ingvagabund · 2019-02-18T09:58:45Z

let's wait until I introduce new test for the draining

enxebre · 2019-02-18T10:34:05Z

/test e2e-aws-operator

enxebre · 2019-02-18T14:46:13Z

/test e2e-aws-operator

enxebre · 2019-02-19T08:30:26Z

/test e2e-aws-operator

enxebre · 2019-02-19T09:37:54Z

/test e2e-aws-operator

enxebre · 2019-02-19T11:08:35Z

/test e2e-aws

enxebre · 2019-02-19T15:17:44Z

/retest

enxebre · 2019-02-19T19:51:03Z

/test e2e-aws

enxebre · 2019-02-19T20:46:44Z

/test e2e-aws

enxebre · 2019-02-19T22:45:54Z

/test e2e-aws-operator

enxebre · 2019-02-19T23:31:31Z

/retest

enxebre · 2019-02-20T21:06:17Z

/retest

enxebre · 2019-02-20T21:49:50Z

/test e2e-aws

enxebre · 2019-02-21T08:26:17Z

/test e2e-aws-operator

enxebre · 2019-02-21T09:37:11Z

/retest

enxebre · 2019-02-21T09:41:59Z

/retest

enxebre · 2019-02-21T10:57:55Z

/retest

enxebre · 2019-02-21T11:41:05Z

/retest

enxebre · 2019-02-21T13:28:16Z

/retest

enxebre · 2019-02-22T07:43:26Z

/retest

enxebre · 2019-02-22T09:35:43Z

@ingvagabund @bison @frobware let's get this in. I'll add a specific SA in a follow up

frobware · 2019-02-22T09:40:56Z

The draining test seems orthogonal to the purpose of the PR (given the description). Can we move the test to a separate PR?

enxebre · 2019-02-22T09:43:11Z

@frobware The perms has been made granular including the new draining featured added in aws actuator. This needs to be merged in one go, otherwise there is room for breaking draining. That's why they have a different commit

frobware · 2019-02-22T09:45:05Z

@frobware The perms has been made granular including the new draining featured added in aws actuator. This needs to be merged in one go, otherwise there is room for breaking draining. That's why they have a different commit

OK, thanks for the explanation.

ingvagabund · 2019-02-22T09:45:34Z

/approve

Agree with Andrew that the node draining test should go into separate PR and merged first before this PR is. Though, given the current state of the CI I agree with Alberto it's better to keep it as it is.

openshift-ci-robot · 2019-02-22T09:46:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

frobware · 2019-02-22T09:48:09Z

test/e2e/operator_expectations.go

+		var machineDeletedEvent *corev1.Event
+		for _, eventItem := range eventList.Items {
+			if eventItem.Reason == "Deleted" && eventItem.Message == fmt.Sprintf("Node %q drained", nodeName) {
+				nodeDrainedEvent = &eventItem


We're taking the address of the local loop variable here - is this intentional?

It is not, it should be changed actually. Though, ExpectNodeToBeDrainedBeforeDeletingMachine was superseded by ExpectNodeToBeDrainedBeforeMachineIsDeleted so it can be deleted.

Thanks! Seems ok since this is not storing appended values or similar
This is just bringing @ingvagabund draining tests from aws. To validate the new rbac does not break current coverage

Right after this we can merge #219
and openshift/cluster-api-actuator-pkg#28
And start rewriting the e2e suite there addressing this specific concern and others

frobware · 2019-02-22T09:55:11Z

Agree with Andrew that the node draining test should go into separate PR and merged first before this PR is. Though, given the current state of the CI I agree with Alberto it's better to keep it as it is.

I think the key point is if you ever had to revert this PR would you also want to revert the test. If the answer is "no" then they should be separate commits.

frobware · 2019-02-22T09:56:50Z

test/e2e/operator_expectations.go

+		}
+		for _, nodeItem := range nodeList.Items {
+			if nodeItem.Name == nodeName {
+				node = &nodeItem


This takes the address of a local variable which seems Ok-ish here given we break. But it may be clearer and more future proof to use &nodeList.Items[i].

frobware · 2019-02-22T10:07:11Z

As long as we follow up and address the issues RSN...

/lgtm

paulfantom · 2019-02-22T11:22:33Z

/retest

enxebre · 2019-02-22T12:45:54Z

/retest

Update machine.Spec.ProviderID field

openshift-ci-robot requested review from paulfantom and spangenberg February 14, 2019 11:15

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 14, 2019

bison reviewed Feb 14, 2019

View reviewed changes

enxebre force-pushed the rbac branch 2 times, most recently from 301b5c6 to 3c97b2c Compare February 14, 2019 15:53

enxebre added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 14, 2019

enxebre force-pushed the rbac branch 2 times, most recently from d917e5c to c5dcc98 Compare February 15, 2019 13:45

enxebre force-pushed the rbac branch from c5dcc98 to ea77619 Compare February 15, 2019 16:26

enxebre mentioned this pull request Feb 19, 2019

Move rbac up to CVO. Drop admin perms dup #214

Closed

enxebre force-pushed the rbac branch from ea77619 to db02174 Compare February 19, 2019 18:50

enxebre force-pushed the rbac branch from db02174 to 5e26cc1 Compare February 19, 2019 22:02

enxebre removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2019

Add draining test

87c4882

enxebre force-pushed the rbac branch from 59579ec to 87c4882 Compare February 20, 2019 21:18

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 22, 2019

frobware reviewed Feb 22, 2019

View reviewed changes

frobware requested changes Feb 22, 2019

View reviewed changes

openshift-ci-robot assigned frobware Feb 22, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2019

frobware approved these changes Feb 22, 2019

View reviewed changes

openshift-merge-robot merged commit 2ddecde into openshift:master Feb 22, 2019

ingvagabund pushed a commit to ingvagabund/machine-api-operator that referenced this pull request Jul 11, 2019

Merge pull request openshift#210 from vikaschoudhary16/set-provider-id

461e8d0

Update machine.Spec.ProviderID field

Move rbac up to CVO. Drop admin perms #210

Move rbac up to CVO. Drop admin perms #210

Uh oh!

Conversation

enxebre commented Feb 14, 2019

Uh oh!

enxebre commented Feb 14, 2019

Uh oh!

bison Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

bison Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

bison Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

enxebre commented Feb 15, 2019

Uh oh!

enxebre commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enxebre commented Feb 15, 2019

Uh oh!

enxebre commented Feb 18, 2019

Uh oh!

enxebre commented Feb 18, 2019

Uh oh!

ingvagabund commented Feb 18, 2019

Uh oh!

enxebre commented Feb 18, 2019

Uh oh!

enxebre commented Feb 18, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 19, 2019

Uh oh!

enxebre commented Feb 20, 2019

Uh oh!

enxebre commented Feb 20, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 21, 2019

Uh oh!

enxebre commented Feb 22, 2019

Uh oh!

enxebre commented Feb 22, 2019

Uh oh!

frobware commented Feb 22, 2019

Uh oh!

enxebre commented Feb 22, 2019

Uh oh!

frobware commented Feb 22, 2019

Uh oh!

ingvagabund commented Feb 22, 2019

Uh oh!

openshift-ci-robot commented Feb 22, 2019

Uh oh!

enxebre commented Feb 15, 2019 •

edited

Loading