refactor: addition of rbac needed for instascale controller #304
Conversation
config/rbac/instascale_role.yaml
Outdated
| - apiGroups: | ||
| - machine.openshift.io | ||
| resources: | ||
| - '*' |
There was a problem hiding this comment.
Could wildcard be avoided? It's generally a best practice recommended by SRE.
config/rbac/instascale_role.yaml
Outdated
| - controlplanemachinesets | ||
| - machinehealthchecks |
There was a problem hiding this comment.
Are these two resources really used by InstaScale?
There was a problem hiding this comment.
I should have specified I've pushed this as myself and mark are testing which are actually used and what we can cut away.
There was a problem hiding this comment.
but if you know off hand...that would save us some time?
I was doubtful that we used those TBH
There was a problem hiding this comment.
My understanding is that InstaScale only uses the Machine and MachineSet APIs but better double checking.
There was a problem hiding this comment.
Tested this on an OCP cluster with Antonin's requested changes and got expected InstaScale behaviour.
When testing with OSD I received this error on scale up:
E0925 15:58:54.827990 1 machinepools.go:58] Error creating MachinePool: expected response content type 'application/json' but received '' and content ''
I0925 15:58:54.828009 1 machinepools.go:60] Created MachinePool: <nil>
E0925 15:58:55.263876 1 machinepools.go:58] Error creating MachinePool: expected response content type 'application/json' but received '' and content ''
I0925 15:58:55.263893 1 machinepools.go:60] Created MachinePool: <nil>
Could it be that we are missing permissions on |
config/rbac/instascale_role.yaml
Outdated
| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - machineset | ||
| verbs: | ||
| - create | ||
| - delete | ||
| - get | ||
| - list | ||
| - patch | ||
| - update | ||
| - watch | ||
| - apiGroups: | ||
| - apps | ||
| resources: | ||
| - machineset/status | ||
| verbs: | ||
| - get |
There was a problem hiding this comment.
Are MachineSet APIs really in the apps group? It seems it's a wrong duplicate of the machine.openshift.io one.
There was a problem hiding this comment.
nope, I didn't think they were in the apps group, I thought first class objects like replicasets, deployments etc were.
but I was just copying what was in the original role to try get it working.
I can't really figure out why this is the case, even getting rid of those and allowing the wildcard on the machine.openshift.io group doesn't work and just returns the same error as ye got.
I also can't seem to find a machinepool resource for any api-group in any of the docs.
Any ideas here?
There was a problem hiding this comment.
Actually, I don't think the error is related to the RBAC changes. I suspect there is an issue with the OCM client configuration.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astefanutti The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue link
closes #278
What changes have been made
Addition of a ClusterRole and a ClusterRoleBinding which binds the required permissions to the CFO serviceaccount
Verification steps
Run through demo instascale NBs with clusters leveraging machinepools (OSD) and machinesets (instructions for sample can be found here)
Checks