Skip to content
This repository has been archived by the owner on Mar 26, 2021. It is now read-only.

Remove cluster role and binding for clusters #207

Merged
merged 1 commit into from
Apr 6, 2018

Conversation

font
Copy link
Contributor

@font font commented Feb 27, 2018

Remove unnecessary cluster role and cluster role binding objects as I don't think we'll need it. This is mainly used to provide permissions for controllers within the cluster to perform specific cluster operations e.g. an admission controller to perform specific cluster resource operations. This cleans up and simplifies the code a bit. We can always add it back later if needed.

The k8s docs on this are a little misleading and suggest this is needed.

/sig multicluster

@k8s-ci-robot k8s-ci-robot added sig/multicluster Categorizes an issue or PR as relevant to SIG Multicluster. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 27, 2018
@mlowery
Copy link
Contributor

mlowery commented Feb 27, 2018

@font: Would it be better to do the following:

  1. Rename clusterregistry.k8s.io:apiserver clusterrole to clusterregistry.k8s.io:admin.
  2. Rename clusterregistry.k8s.io:apiserver clusterrolebinding to clusterregistry.k8s.io:admin.
  3. Modify clusterregistry.k8s.io:admin clusterrolebinding to reference clusterregistry.k8s.io:adminclusterrole.
  4. Modify clusterregistry.k8s.io:admin clusterrolebinding to bind to only user AdminCN (remove service account)?
  5. State in the doc that these steps are performed and are simply an example. Edits to either or both of clusterrole and clusterrolebinding will be necessary to allow other users to perform CRUD operations on cluster objects.

@font
Copy link
Contributor Author

font commented Feb 27, 2018

@mlowery We need to avoid creating unnecessary RBAC objects, especially if they are just an example, unless they are explicitly required by the cluster registry. The admin that deploys the cluster registry will have access to the clusters resource. They are then able to create more fine-grained ACLs if they wish. I don't think we want the crinit tool to assume a particular cluster registry security deployment model.

Copy link
Contributor

@perotinus perotinus left a comment

Choose a reason for hiding this comment

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

Happy to see less code!

// Create a Kubernetes cluster role binding from the default service account
// in our namespace to the cluster role we just created.
glog.V(4).Infof("Creating cluster role bindings %v and %v", apiServerCRBName, authDelegatorCRBName)
// in our namespace to the system:auth-delegator cluster role.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this documented to exist in all clusters that support delegated auth? I expect that it would, but I would be slightly concerned that this is a pattern that not all clusters follow, and that this will break some usages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The system:auth-delegator cluster role is part of the default RBAC roles described here. A quick look through the code shows that these default RBAC cluster roles are created by the kube-apiserver as part of the rbac/bootstrap-roles PostStartHook for the RBAC REST storage provider.

Based on that code and the doc linked above, unless somebody tampers with the defaults, I think this is expected to exist in all clusters that support and enable RBAC (--authorization-mode=RBAC). I think we can assume or at least expect that RBAC will be enabled. Otherwise, the entire cluster registry aggregated deployment will fail. But it probably wouldn't hurt to add a check for RBAC enablement with a graceful error if it's disabled.

@font
Copy link
Contributor Author

font commented Apr 6, 2018

@perotinus PTAL.

@perotinus
Copy link
Contributor

/lgtm

Apologies, this fell off my radar in the midst of discussions about CRDs.

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: font, perotinus

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 09150e6 into kubernetes-retired:master Apr 6, 2018
@font font deleted the craggregation branch July 11, 2018 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm Indicates that a PR is ready to be merged. sig/multicluster Categorizes an issue or PR as relevant to SIG Multicluster. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants