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 sets up access rules for dynamic config #60100

Merged
merged 1 commit into from Mar 30, 2018

Conversation

@mtaufen
Contributor

mtaufen commented Feb 20, 2018

This PR makes the node authorizer automatically set up access rules for
dynamic Kubelet config.

I also added some validation to the node strategy, which I discovered we
were missing while writing this.

This PR is based on another WIP from @liggitt.

The node authorizer now automatically sets up rules for Node.Spec.ConfigSource when the DynamicKubeletConfig feature gate is enabled.

@mtaufen mtaufen changed the title from (WIP) node authorizer sets up access rules for dynamic config to node authorizer sets up access rules for dynamic config Feb 22, 2018

@mtaufen mtaufen modified the milestones: next-candidate, v1.10 Feb 26, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Feb 26, 2018

I sent pings trying to get this approved for 1.10 milestone, if it doesn't, so be it.

@mtaufen mtaufen referenced this pull request Feb 27, 2018

Open

Dynamic Kubelet Configuration #281

27 of 27 tasks complete
@thockin

This comment has been minimized.

Member

thockin commented Mar 5, 2018

This change is Reviewable

@mtaufen mtaufen modified the milestones: v1.10, v1.11 Mar 6, 2018

@k8s-merge-robot k8s-merge-robot removed this from the v1.11 milestone Mar 20, 2018

@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Mar 21, 2018

/status approved-for-milestone

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 21, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@liggitt @mtaufen

Pull Request Labels
  • sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/feature: New functionality.
Help
if ref := source.ConfigMapRef; ref != nil {
count++
// name, namespace, and UID must all be non-empty for ConfigMapRef
if ref.Name == "" || ref.Namespace == "" || string(ref.UID) == "" {

This comment has been minimized.

@liggitt

liggitt Mar 22, 2018

Member

why uid?

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

This has been a requirement of dynamic Kubelet config since the original proposal, to require users to be completely explicit (time and space) about which ConfigMap to use. If the UID in the config source doesn't match the UID of the ConfigMap, the Kubelet currently rejects it.

IIRC there were concerns along the lines of ConfigMaps being deleted and recreated with the same name but different content... though this is less of an issue today now that we have the --append-hash option in kubectl (obviously the potential for in-place ConfigMap mutation still exists, but if users are using --append-hash I wouldn't expect them to mutate the ConfigMap after creation).

The Kubelet also checkpoints by UID, so it was convenient to already have this specified in the object ref, though I guess if we relaxed this requirement it could just resolve the UID and stick it in the local copy of the config source when it first downloads the ConfigMap.

I can't remember other reasons to require UID, if any.

We should probably discuss this in parallel to this PR; if we relax the UID requirement there is other code that needs to change too, and I'd like to make all those changes in a single, isolated PR. I'm glad you brought this up; having this discussion prior to DKC beta was on my todo list.

This comment has been minimized.

@liggitt

liggitt Mar 23, 2018

Member

fine to leave it, I just missed any other use of it in this PR and wondered

This comment has been minimized.

@mtaufen
func (g *graphPopulator) deleteNode(obj interface{}) {
node := obj.(*api.Node)
g.graph.DeleteNode(node.Name)

This comment has been minimized.

@liggitt

liggitt Mar 22, 2018

Member

can we just reuse SetNodeConfigMap(node.Name, "", "")?

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

Hmm, since we don't actually delete the Node from the graph anyway, that sounds like it'd work.

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

defer g.lock.Unlock()
// nothing to do if we don't have a node
if len(nodeName) > 0 {

This comment has been minimized.

@liggitt

liggitt Mar 22, 2018

Member

calling this with an empty node name is an error... shouldn't need to check this

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

I guess I wouldn't expect us to get a node with an empty name, but would we catch such a case earlier than this?

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

There's a similar len(nodeName) check in this file in AddVolumeAttachment, is there a reason for this one?

// must be called under write lock
// deletes edges from a given vertex type to a given vertex type
// will also delete the "from" nodes if they are orphaned, but will not delete the "to" node
func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toName string) {

This comment has been minimized.

@liggitt

liggitt Mar 22, 2018

Member

this seems specialized enough that I'm unsure of it being a generic graph method... needs tests, no matter where it is

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

agree wrt tests, will add
pretty much indifferent on whether we make this a generic graph method

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

added some test cases

path := "nil"
if node.Spec.ConfigSource != nil {
path = fmt.Sprintf("/api/v1/namespaces/%s/configmaps/%s", namespace, name)

This comment has been minimized.

@liggitt

liggitt Mar 22, 2018

Member

just log %s/%s namespace/name, no need to make the API path

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

will do

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 22, 2018

add a positive and negative test case to the auth/node integration test as well

if ref := source.ConfigMapRef; ref != nil {
count++
// name, namespace, and UID must all be non-empty for ConfigMapRef
if ref.Name == "" || ref.Namespace == "" || string(ref.UID) == "" {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

This has been a requirement of dynamic Kubelet config since the original proposal, to require users to be completely explicit (time and space) about which ConfigMap to use. If the UID in the config source doesn't match the UID of the ConfigMap, the Kubelet currently rejects it.

IIRC there were concerns along the lines of ConfigMaps being deleted and recreated with the same name but different content... though this is less of an issue today now that we have the --append-hash option in kubectl (obviously the potential for in-place ConfigMap mutation still exists, but if users are using --append-hash I wouldn't expect them to mutate the ConfigMap after creation).

The Kubelet also checkpoints by UID, so it was convenient to already have this specified in the object ref, though I guess if we relaxed this requirement it could just resolve the UID and stick it in the local copy of the config source when it first downloads the ConfigMap.

I can't remember other reasons to require UID, if any.

We should probably discuss this in parallel to this PR; if we relax the UID requirement there is other code that needs to change too, and I'd like to make all those changes in a single, isolated PR. I'm glad you brought this up; having this discussion prior to DKC beta was on my todo list.

path := "nil"
if node.Spec.ConfigSource != nil {
path = fmt.Sprintf("/api/v1/namespaces/%s/configmaps/%s", namespace, name)

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

will do

defer g.lock.Unlock()
// nothing to do if we don't have a node
if len(nodeName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

I guess I wouldn't expect us to get a node with an empty name, but would we catch such a case earlier than this?

// must be called under write lock
// deletes edges from a given vertex type to a given vertex type
// will also delete the "from" nodes if they are orphaned, but will not delete the "to" node
func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toName string) {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

agree wrt tests, will add
pretty much indifferent on whether we make this a generic graph method

g.deleteEdges_locked(configMapVertexType, nodeVertexType, "", nodeName)
// establish new edges if we have a real ConfigMap to reference
if len(configMapName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

note to self, also handle empty namespace?

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done, now we also skip if the namespace is an empty string

func (g *Graph) DeleteNode(nodeName string) {
g.lock.Lock()
defer g.lock.Unlock()
if len(nodeName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

this check probably also unnecessary wrt @liggitt's comment above

This comment has been minimized.

@liggitt

liggitt Mar 23, 2018

Member

I think this whole function is not needed (and was weird anyway, since it didn't actually delete the node)

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

func (g *graphPopulator) deleteNode(obj interface{}) {
node := obj.(*api.Node)
g.graph.DeleteNode(node.Name)

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

Hmm, since we don't actually delete the Node from the graph anyway, that sounds like it'd work.

@mtaufen

Updated, still have to add the integration test cases.

path := "nil"
if node.Spec.ConfigSource != nil {
path = fmt.Sprintf("/api/v1/namespaces/%s/configmaps/%s", namespace, name)

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

defer g.lock.Unlock()
// nothing to do if we don't have a node
if len(nodeName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

There's a similar len(nodeName) check in this file in AddVolumeAttachment, is there a reason for this one?

g.deleteEdges_locked(configMapVertexType, nodeVertexType, "", nodeName)
// establish new edges if we have a real ConfigMap to reference
if len(configMapName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done, now we also skip if the namespace is an empty string

func (g *Graph) DeleteNode(nodeName string) {
g.lock.Lock()
defer g.lock.Unlock()
if len(nodeName) > 0 {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

func (g *graphPopulator) deleteNode(obj interface{}) {
node := obj.(*api.Node)
g.graph.DeleteNode(node.Name)

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

done

// must be called under write lock
// deletes edges from a given vertex type to a given vertex type
// will also delete the "from" nodes if they are orphaned, but will not delete the "to" node
func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toName string) {

This comment has been minimized.

@mtaufen

mtaufen Mar 23, 2018

Contributor

added some test cases

node authorizer sets up access rules for dynamic config
This PR makes the node authorizer automatically set up access rules for
dynamic Kubelet config.

I also added some validation to the node strategy, which I discovered we
were missing while writing this.
@mtaufen

This comment has been minimized.

Contributor

mtaufen commented Mar 29, 2018

/retest

Name: "myconfigmapconfigsource",
// validation just requires UID to be non-empty and it isn't necessary for GET,
// so we just use a bogus one for the test
UID: "uid",

This comment has been minimized.

@liggitt

liggitt Mar 29, 2018

Member

if you have valid data from creating the configmap above, go ahead and set it here

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 29, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 29, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mtaufen

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-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 30, 2018

Automatic merge from submit-queue (batch tested with PRs 61829, 61908, 61307, 61872, 60100). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 5ae7bba into kubernetes:master Mar 30, 2018

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment