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

Node authorizer #46076

Merged
merged 9 commits into from
May 31, 2017
Merged

Node authorizer #46076

merged 9 commits into from
May 31, 2017

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented May 19, 2017

This PR implements the authorization portion of https://github.com/kubernetes/community/blob/master/contributors/design-proposals/kubelet-authorizer.md and kubernetes/enhancements#279:

  • Adds a new authorization mode (Node) that authorizes requests from nodes based on a graph of related pods,secrets,configmaps,pvcs, and pvs:
    • Watches pods, adds edges (secret -> pod, configmap -> pod, pvc -> pod, pod -> node)
    • Watches pvs, adds edges (secret -> pv, pv -> pvc)
    • When both Node and RBAC authorization modes are enabled, the default RBAC binding that grants the system:node role to the system:nodes group is not automatically created.
  • Tightens the NodeRestriction admission plugin to require identifiable nodes for requests from users in the system:nodes group.

This authorization mode is intended to be used in combination with the NodeRestriction admission plugin, which limits the pods and nodes a node may modify. To enable in combination with RBAC authorization and the NodeRestriction admission plugin:

  • start the API server with --authorization-mode=Node,RBAC --admission-control=...,NodeRestriction,...
  • start kubelets with TLS boostrapping or with client credentials that place them in the system:nodes group with a username of system:node:<nodeName>
kube-apiserver: a new authorization mode (`--authorization-mode=Node`) authorizes nodes to access secrets, configmaps, persistent volume claims and persistent volumes related to their pods.
* Nodes must use client credentials that place them in the `system:nodes` group with a username of `system:node:<nodeName>` in order to be authorized by the node authorizer (the credentials obtained by the kubelet via TLS bootstrapping satisfy these requirements)
* When used in combination with the `RBAC` authorization mode (`--authorization-mode=Node,RBAC`), the `system:node` role is no longer automatically granted to the `system:nodes` group.
RBAC: the automatic binding of the `system:node` role to the `system:nodes` group is deprecated and will not be created in future releases. It is recommended that nodes be authorized using the new `Node` authorization mode instead. Installations that wish to continue giving all members of the `system:nodes` group the `system:node` role (which grants broad read access, including all secrets and configmaps) must create an installation-specific ClusterRoleBinding.

Follow-up:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2017
@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 19, 2017

const (
ConfigMapNode nodeType = iota
NodeNode nodeType = iota
Copy link
Member

Choose a reason for hiding this comment

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

iota copypasta?

type GraphPopulator struct {
graph *Graph

podInformer cache.SharedIndexInformer
Copy link
Member

Choose a reason for hiding this comment

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

We generally haven't been storing refs to the underlying cache.SharedIndexInformer objects. If you're using a shared informer, it's generally safe to assume that something else (e.g. kube-controller-manager) will start it.

}

func (g *GraphPopulator) Run(stopCh <-chan struct{}) {
g.podInformer.GetController().Run(stopCh)
Copy link
Member

Choose a reason for hiding this comment

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

These 2 lines are likely no-ops (see my comment above,

func (s *sharedIndexInformer) GetController() Controller {
return &dummyController{informer: s}
}
, and
func (v *dummyController) Run(stopCh <-chan struct{}) {
).

}
if oldObj != nil && pod.Spec.NodeName == oldObj.(*api.Pod).Spec.NodeName {
// Node is unchanged
// TODO: do we need to check for uid changes?
Copy link
Member

Choose a reason for hiding this comment

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

I would say yes

@liggitt liggitt force-pushed the node-authorizer branch 3 times, most recently from 7d81814 to 9f152be Compare May 20, 2017 03:28
@liggitt liggitt changed the title WIP - Node authorizer Node authorizer May 20, 2017
@liggitt
Copy link
Member Author

liggitt commented May 20, 2017

Comments addressed. Still working on tests, and I'll have some follow-ups for performance/memory related improvements to the graph, but the authorizer and graph population code is ready for review

@@ -0,0 +1 @@
Forked from gonum/graph@50b27dea7ebbfb052dfaf91681afc6fde28d8796 to support memory-use improvements to the simple graph
Copy link
Contributor

Choose a reason for hiding this comment

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

Any plans to upstream the changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Absolutely, I just don't want to block on the upstream PRs being accepted and vendorable prior to code freeze

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 22, 2017
@liggitt liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 23, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 23, 2017
@liggitt liggitt force-pushed the node-authorizer branch 2 times, most recently from fc2aa23 to 90da41d Compare May 23, 2017 05:17
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2017
@liggitt
Copy link
Member Author

liggitt commented May 30, 2017

rebased, comments addressed. will look more into graph space optimizations in 1.8 timeframe.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2017
@smarterclayton
Copy link
Contributor

No other comments from me

/approve

Pending other reviewers sign off

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2017
@timstclair
Copy link

LGTM, but I'll avoid adding the label to let other reviewers respond. Feel free to self apply once they have.

@ericchiang
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ericchiang, liggitt, smarterclayton

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

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017
@liggitt
Copy link
Member Author

liggitt commented May 30, 2017

fixed lint issue, retagging

@liggitt liggitt added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017
@liggitt
Copy link
Member Author

liggitt commented May 30, 2017

@k8s-bot pull-kubernetes-e2e-kops-aws test this

@liggitt liggitt added this to the v1.7 milestone May 31, 2017
@liggitt
Copy link
Member Author

liggitt commented May 31, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5995690 into kubernetes:master May 31, 2017
@k8s-ci-robot
Copy link
Contributor

@liggitt: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce fc8e915 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@liggitt liggitt deleted the node-authorizer branch June 2, 2017 02:34
k8s-github-robot pushed a commit that referenced this pull request Jul 28, 2017
Automatic merge from submit-queue (batch tested with PRs 49619, 49598, 47267, 49597, 49638)

Remove default binding of system:node role to system:nodes group

part of kubernetes/enhancements#279

deprecation of this automatic binding announced in 1.7 in #46076

```release-note
RBAC: the `system:node` role is no longer automatically granted to the `system:nodes` group in new clusters. It is recommended that nodes be authorized using the `Node` authorization mode instead. Installations that wish to continue giving all members of the `system:nodes` group the `system:node` role (which grants broad read access, including all secrets and configmaps) must create an installation-specific `ClusterRoleBinding`.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.