diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 5b54d6fe642c..3bff0af6edd2 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4098,6 +4098,9 @@ func ValidateNodeUpdate(node, oldNode *core.Node) field.ErrorList { // Allow updates to Node.Spec.ConfigSource if DynamicKubeletConfig feature gate is enabled if utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) { + if node.Spec.ConfigSource != nil { + allErrs = append(allErrs, validateNodeConfigSource(node.Spec.ConfigSource, field.NewPath("spec", "configSource"))...) + } oldNode.Spec.ConfigSource = node.Spec.ConfigSource } @@ -4111,6 +4114,25 @@ func ValidateNodeUpdate(node, oldNode *core.Node) field.ErrorList { return allErrs } +func validateNodeConfigSource(source *core.NodeConfigSource, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + count := int(0) + 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) == "" { + allErrs = append(allErrs, field.Invalid(fldPath.Child("configMapRef"), ref, "name, namespace, and UID must all be non-empty")) + } + } + // add more subfields here in the future as they are added to NodeConfigSource + + // exactly one reference subfield must be non-nil + if count != 1 { + allErrs = append(allErrs, field.Invalid(fldPath, source, "exactly one reference subfield must be non-nil")) + } + return allErrs +} + // Validate compute resource typename. // Refer to docs/design/resources.md for more details. func validateResourceName(value string, fldPath *field.Path) field.ErrorList { diff --git a/pkg/kubeapiserver/authorizer/config.go b/pkg/kubeapiserver/authorizer/config.go index 35e61489cf69..7646f80bc1bd 100644 --- a/pkg/kubeapiserver/authorizer/config.go +++ b/pkg/kubeapiserver/authorizer/config.go @@ -68,12 +68,13 @@ func (config AuthorizationConfig) New() (authorizer.Authorizer, authorizer.RuleR ) for _, authorizationMode := range config.AuthorizationModes { - // Keep cases in sync with constant list above. + // Keep cases in sync with constant list in k8s.io/kubernetes/pkg/kubeapiserver/authorizer/modes/modes.go. switch authorizationMode { case modes.ModeNode: graph := node.NewGraph() node.AddGraphEventHandlers( graph, + config.InformerFactory.Core().InternalVersion().Nodes(), config.InformerFactory.Core().InternalVersion().Pods(), config.InformerFactory.Core().InternalVersion().PersistentVolumes(), config.VersionedInformerFactory.Storage().V1beta1().VolumeAttachments(), diff --git a/plugin/pkg/auth/authorizer/node/BUILD b/plugin/pkg/auth/authorizer/node/BUILD index 37d0db0b0d44..47c3b30c788f 100644 --- a/plugin/pkg/auth/authorizer/node/BUILD +++ b/plugin/pkg/auth/authorizer/node/BUILD @@ -8,15 +8,20 @@ load( go_test( name = "go_default_test", - srcs = ["node_authorizer_test.go"], + srcs = [ + "graph_test.go", + "node_authorizer_test.go", + ], embed = [":go_default_library"], deps = [ "//pkg/apis/core:go_default_library", "//pkg/auth/nodeidentifier:go_default_library", "//pkg/features:go_default_library", "//plugin/pkg/auth/authorizer/rbac/bootstrappolicy:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/k8s.io/api/storage/v1beta1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", "//vendor/k8s.io/apiserver/pkg/authentication/user:go_default_library", "//vendor/k8s.io/apiserver/pkg/authorization/authorizer:go_default_library", "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library", diff --git a/plugin/pkg/auth/authorizer/node/graph.go b/plugin/pkg/auth/authorizer/node/graph.go index 38fea6d0323c..7a1b1b97e08d 100644 --- a/plugin/pkg/auth/authorizer/node/graph.go +++ b/plugin/pkg/auth/authorizer/node/graph.go @@ -198,6 +198,50 @@ func (g *Graph) deleteVertex_locked(vertexType vertexType, namespace, name strin } } +// must be called under write lock +// deletes edges from a given vertex type to a specific vertex +// will delete each orphaned "from" vertex, but will never delete the "to" vertex +func (g *Graph) deleteEdges_locked(fromType, toType vertexType, toNamespace, toName string) { + // get the "to" side + toVert, exists := g.getVertex_rlocked(toType, toNamespace, toName) + if !exists { + return + } + + // get potential "from" verts that match fromType + namespaces, exists := g.vertices[fromType] + if !exists { + return + } + + // delete all edges between vertices of fromType and toVert + removeVerts := []*namedVertex{} + for _, vertexMapping := range namespaces { + for _, fromVert := range vertexMapping { + if g.graph.HasEdgeBetween(fromVert, toVert) { + // remove the edge (no-op if edge doesn't exist) + g.graph.RemoveEdge(newDestinationEdge(fromVert, toVert, nil)) + // remember to clean up the fromVert if we orphaned it + if g.graph.Degree(fromVert) == 0 { + removeVerts = append(removeVerts, fromVert) + } + } + } + } + + // clean up orphaned verts + for _, v := range removeVerts { + g.graph.RemoveNode(v) + delete(g.vertices[v.vertexType][v.namespace], v.name) + if len(g.vertices[v.vertexType][v.namespace]) == 0 { + delete(g.vertices[v.vertexType], v.namespace) + } + if len(g.vertices[v.vertexType]) == 0 { + delete(g.vertices, v.vertexType) + } + } +} + // AddPod should only be called once spec.NodeName is populated. // It sets up edges for the following relationships (which are immutable for a pod once bound to a node): // @@ -301,3 +345,25 @@ func (g *Graph) DeleteVolumeAttachment(name string) { defer g.lock.Unlock() g.deleteVertex_locked(vaVertexType, "", name) } + +// SetNodeConfigMap sets up edges for the Node.Spec.ConfigSource.ConfigMapRef relationship: +// +// configmap -> node +func (g *Graph) SetNodeConfigMap(nodeName, configMapName, configMapNamespace string) { + g.lock.Lock() + defer g.lock.Unlock() + + // TODO(mtaufen): ensure len(nodeName) > 0 in all cases (would sure be nice to have a dependently-typed language here...) + + // clear edges configmaps -> node where the destination is the current node *only* + // at present, a node can only have one *direct* configmap reference at a time + g.deleteEdges_locked(configMapVertexType, nodeVertexType, "", nodeName) + + // establish new edges if we have a real ConfigMap to reference + if len(configMapName) > 0 && len(configMapNamespace) > 0 { + configmapVertex := g.getOrCreateVertex_locked(configMapVertexType, configMapNamespace, configMapName) + nodeVertex := g.getOrCreateVertex_locked(nodeVertexType, "", nodeName) + g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex, nodeVertex)) + } + +} diff --git a/plugin/pkg/auth/authorizer/node/graph_populator.go b/plugin/pkg/auth/authorizer/node/graph_populator.go index 8c1c36a2d242..0c1b1d25fb5b 100644 --- a/plugin/pkg/auth/authorizer/node/graph_populator.go +++ b/plugin/pkg/auth/authorizer/node/graph_populator.go @@ -17,6 +17,7 @@ limitations under the License. package node import ( + "fmt" "github.com/golang/glog" storagev1beta1 "k8s.io/api/storage/v1beta1" @@ -34,6 +35,7 @@ type graphPopulator struct { func AddGraphEventHandlers( graph *Graph, + nodes coreinformers.NodeInformer, pods coreinformers.PodInformer, pvs coreinformers.PersistentVolumeInformer, attachments storageinformers.VolumeAttachmentInformer, @@ -42,6 +44,14 @@ func AddGraphEventHandlers( graph: graph, } + if utilfeature.DefaultFeatureGate.Enabled(features.DynamicKubeletConfig) { + nodes.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ + AddFunc: g.addNode, + UpdateFunc: g.updateNode, + DeleteFunc: g.deleteNode, + }) + } + pods.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: g.addPod, UpdateFunc: g.updatePod, @@ -63,6 +73,57 @@ func AddGraphEventHandlers( } } +func (g *graphPopulator) addNode(obj interface{}) { + g.updateNode(nil, obj) +} + +func (g *graphPopulator) updateNode(oldObj, obj interface{}) { + node := obj.(*api.Node) + var oldNode *api.Node + if oldObj != nil { + oldNode = oldObj.(*api.Node) + } + + // we only set up rules for ConfigMapRef today, because that is the only reference type + + var name, namespace string + if source := node.Spec.ConfigSource; source != nil && source.ConfigMapRef != nil { + name = source.ConfigMapRef.Name + namespace = source.ConfigMapRef.Namespace + } + + var oldName, oldNamespace string + if oldNode != nil { + if oldSource := oldNode.Spec.ConfigSource; oldSource != nil && oldSource.ConfigMapRef != nil { + oldName = oldSource.ConfigMapRef.Name + oldNamespace = oldSource.ConfigMapRef.Namespace + } + } + + // if Node.Spec.ConfigSource wasn't updated, nothing for us to do + if name == oldName && namespace == oldNamespace { + return + } + + path := "nil" + if node.Spec.ConfigSource != nil { + path = fmt.Sprintf("%s/%s", namespace, name) + } + glog.V(4).Infof("updateNode configSource reference to %s for node %s", path, node.Name) + g.graph.SetNodeConfigMap(node.Name, name, namespace) +} + +func (g *graphPopulator) deleteNode(obj interface{}) { + node := obj.(*api.Node) + + // TODO(mtaufen): ensure len(nodeName) > 0 in all cases (would sure be nice to have a dependently-typed language here...) + + // NOTE: We don't remove the node, because if the node is re-created not all pod -> node + // links are re-established (we don't get relevant events because the no mutations need + // to happen in the API; the state is already there). + g.graph.SetNodeConfigMap(node.Name, "", "") +} + func (g *graphPopulator) addPod(obj interface{}) { g.updatePod(nil, obj) } diff --git a/plugin/pkg/auth/authorizer/node/graph_test.go b/plugin/pkg/auth/authorizer/node/graph_test.go new file mode 100644 index 000000000000..3f38b285d35a --- /dev/null +++ b/plugin/pkg/auth/authorizer/node/graph_test.go @@ -0,0 +1,176 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package node + +import ( + "sort" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDeleteEdges_locked(t *testing.T) { + cases := []struct { + desc string + fromType vertexType + toType vertexType + toNamespace string + toName string + start *Graph + expect *Graph + }{ + { + // single edge from a configmap to a node, will delete edge and orphaned configmap + desc: "edges and source orphans are deleted, destination orphans are preserved", + fromType: configMapVertexType, + toType: nodeVertexType, + toNamespace: "", + toName: "node1", + start: func() *Graph { + g := NewGraph() + nodeVertex := g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + configmapVertex := g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex, nodeVertex)) + return g + }(), + expect: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + return g + }(), + }, + { + // two edges from the same configmap to distinct nodes, will delete one of the edges + desc: "edges are deleted, non-orphans and destination orphans are preserved", + fromType: configMapVertexType, + toType: nodeVertexType, + toNamespace: "", + toName: "node2", + start: func() *Graph { + g := NewGraph() + nodeVertex1 := g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + nodeVertex2 := g.getOrCreateVertex_locked(nodeVertexType, "", "node2") + configmapVertex := g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex1, nodeVertex1)) + g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex2, nodeVertex2)) + return g + }(), + expect: func() *Graph { + g := NewGraph() + nodeVertex1 := g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + g.getOrCreateVertex_locked(nodeVertexType, "", "node2") + configmapVertex := g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + g.graph.SetEdge(newDestinationEdge(configmapVertex, nodeVertex1, nodeVertex1)) + return g + }(), + }, + { + desc: "no edges to delete", + fromType: configMapVertexType, + toType: nodeVertexType, + toNamespace: "", + toName: "node1", + start: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + return g + }(), + expect: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + return g + }(), + }, + { + desc: "destination vertex does not exist", + fromType: configMapVertexType, + toType: nodeVertexType, + toNamespace: "", + toName: "node1", + start: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + return g + }(), + expect: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(configMapVertexType, "namespace1", "configmap1") + return g + }(), + }, + { + desc: "source vertex type doesn't exist", + fromType: configMapVertexType, + toType: nodeVertexType, + toNamespace: "", + toName: "node1", + start: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + return g + }(), + expect: func() *Graph { + g := NewGraph() + g.getOrCreateVertex_locked(nodeVertexType, "", "node1") + return g + }(), + }, + } + for _, c := range cases { + t.Run(c.desc, func(t *testing.T) { + c.start.deleteEdges_locked(c.fromType, c.toType, c.toNamespace, c.toName) + + // Note: We assert on substructures (graph.Nodes(), graph.Edges()) because the graph tracks + // freed IDs for reuse, which results in an irrelevant inequality between start and expect. + + // sort the nodes by ID + // (the slices we get back are from map iteration, where order is not guaranteed) + expectNodes := c.expect.graph.Nodes() + sort.Slice(expectNodes, func(i, j int) bool { + return expectNodes[i].ID() < expectNodes[j].ID() + }) + startNodes := c.start.graph.Nodes() + sort.Slice(expectNodes, func(i, j int) bool { + return startNodes[i].ID() < startNodes[j].ID() + }) + assert.Equal(t, expectNodes, startNodes) + + // sort the edges by from ID, then to ID + // (the slices we get back are from map iteration, where order is not guaranteed) + expectEdges := c.expect.graph.Edges() + sort.Slice(expectEdges, func(i, j int) bool { + if expectEdges[i].From().ID() == expectEdges[j].From().ID() { + return expectEdges[i].To().ID() < expectEdges[j].To().ID() + } + return expectEdges[i].From().ID() < expectEdges[j].From().ID() + }) + startEdges := c.start.graph.Edges() + sort.Slice(expectEdges, func(i, j int) bool { + if startEdges[i].From().ID() == startEdges[j].From().ID() { + return startEdges[i].To().ID() < startEdges[j].To().ID() + } + return startEdges[i].From().ID() < startEdges[j].From().ID() + }) + assert.Equal(t, expectEdges, startEdges) + + // vertices is a recursive map, no need to sort + assert.Equal(t, c.expect.vertices, c.start.vertices) + }) + } +} diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer.go b/plugin/pkg/auth/authorizer/node/node_authorizer.go index b2078d2c6d9e..601bfbd13a11 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer.go +++ b/plugin/pkg/auth/authorizer/node/node_authorizer.go @@ -38,6 +38,7 @@ import ( // 1. If a request is not from a node (NodeIdentity() returns isNode=false), reject // 2. If a specific node cannot be identified (NodeIdentity() returns nodeName=""), reject // 3. If a request is for a secret, configmap, persistent volume or persistent volume claim, reject unless the verb is get, and the requested object is related to the requesting node: +// node <- configmap // node <- pod // node <- pod <- secret // node <- pod <- configmap diff --git a/plugin/pkg/auth/authorizer/node/node_authorizer_test.go b/plugin/pkg/auth/authorizer/node/node_authorizer_test.go index 19eb32fdfcec..3fa16752ce5b 100644 --- a/plugin/pkg/auth/authorizer/node/node_authorizer_test.go +++ b/plugin/pkg/auth/authorizer/node/node_authorizer_test.go @@ -26,6 +26,7 @@ import ( storagev1beta1 "k8s.io/api/storage/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/authorization/authorizer" utilfeature "k8s.io/apiserver/pkg/util/feature" @@ -72,8 +73,8 @@ func TestAuthorizer(t *testing.T) { sharedPVCsPerPod: 0, uniquePVCsPerPod: 1, } - pods, pvs, attachments := generate(opts) - populate(g, pods, pvs, attachments) + nodes, pods, pvs, attachments := generate(opts) + populate(g, nodes, pods, pvs, attachments) identifier := nodeidentifier.NewDefaultNodeIdentifier() authz := NewAuthorizer(g, identifier, bootstrappolicy.NodeRules()).(*NodeAuthorizer) @@ -86,6 +87,11 @@ func TestAuthorizer(t *testing.T) { expect authorizer.Decision features utilfeature.FeatureGate }{ + { + name: "allowed node configmap", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "node0-configmap", Namespace: "ns0"}, + expect: authorizer.DecisionAllow, + }, { name: "allowed configmap", attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "configmap0-pod0-node0", Namespace: "ns0"}, @@ -117,6 +123,11 @@ func TestAuthorizer(t *testing.T) { expect: authorizer.DecisionAllow, }, + { + name: "disallowed node configmap", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "node1-configmap", Namespace: "ns0"}, + expect: authorizer.DecisionNoOpinion, + }, { name: "disallowed configmap", attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "configmap0-pod0-node1", Namespace: "ns0"}, @@ -247,9 +258,14 @@ func TestAuthorizerSharedResources(t *testing.T) { }, }) + g.SetNodeConfigMap("node1", "shared-configmap", "ns1") + g.SetNodeConfigMap("node2", "shared-configmap", "ns1") + g.SetNodeConfigMap("node3", "configmap", "ns1") + testcases := []struct { User user.Info Secret string + ConfigMap string ExpectAllowed bool }{ {User: node1, ExpectAllowed: true, Secret: "node1-only"}, @@ -263,14 +279,39 @@ func TestAuthorizerSharedResources(t *testing.T) { {User: node3, ExpectAllowed: false, Secret: "node1-only"}, {User: node3, ExpectAllowed: false, Secret: "node1-node2-only"}, {User: node3, ExpectAllowed: true, Secret: "shared-all"}, + + {User: node1, ExpectAllowed: true, ConfigMap: "shared-configmap"}, + {User: node1, ExpectAllowed: false, ConfigMap: "configmap"}, + + {User: node2, ExpectAllowed: true, ConfigMap: "shared-configmap"}, + {User: node2, ExpectAllowed: false, ConfigMap: "configmap"}, + + {User: node3, ExpectAllowed: false, ConfigMap: "shared-configmap"}, + {User: node3, ExpectAllowed: true, ConfigMap: "configmap"}, } for i, tc := range testcases { - decision, _, err := authz.Authorize(authorizer.AttributesRecord{User: tc.User, ResourceRequest: true, Verb: "get", Resource: "secrets", Namespace: "ns1", Name: tc.Secret}) - if err != nil { - t.Errorf("%d: unexpected error: %v", i, err) - continue + var ( + decision authorizer.Decision + err error + ) + + if len(tc.Secret) > 0 { + decision, _, err = authz.Authorize(authorizer.AttributesRecord{User: tc.User, ResourceRequest: true, Verb: "get", Resource: "secrets", Namespace: "ns1", Name: tc.Secret}) + if err != nil { + t.Errorf("%d: unexpected error: %v", i, err) + continue + } + } else if len(tc.ConfigMap) > 0 { + decision, _, err = authz.Authorize(authorizer.AttributesRecord{User: tc.User, ResourceRequest: true, Verb: "get", Resource: "configmaps", Namespace: "ns1", Name: tc.ConfigMap}) + if err != nil { + t.Errorf("%d: unexpected error: %v", i, err) + continue + } + } else { + t.Fatalf("test case must include a request for a Secret or ConfigMap") } + if (decision == authorizer.DecisionAllow) != tc.ExpectAllowed { t.Errorf("%d: expected %v, got %v", i, tc.ExpectAllowed, decision) } @@ -309,12 +350,12 @@ func BenchmarkPopulationAllocation(b *testing.B) { uniquePVCsPerPod: 1, } - pods, pvs, attachments := generate(opts) + nodes, pods, pvs, attachments := generate(opts) b.ResetTimer() for i := 0; i < b.N; i++ { g := NewGraph() - populate(g, pods, pvs, attachments) + populate(g, nodes, pods, pvs, attachments) } } @@ -340,14 +381,14 @@ func BenchmarkPopulationRetention(b *testing.B) { uniquePVCsPerPod: 1, } - pods, pvs, attachments := generate(opts) + nodes, pods, pvs, attachments := generate(opts) // Garbage collect before the first iteration runtime.GC() b.ResetTimer() for i := 0; i < b.N; i++ { g := NewGraph() - populate(g, pods, pvs, attachments) + populate(g, nodes, pods, pvs, attachments) if i == 0 { f, _ := os.Create("BenchmarkPopulationRetention.profile") @@ -375,8 +416,8 @@ func BenchmarkAuthorization(b *testing.B) { sharedPVCsPerPod: 0, uniquePVCsPerPod: 1, } - pods, pvs, attachments := generate(opts) - populate(g, pods, pvs, attachments) + nodes, pods, pvs, attachments := generate(opts) + populate(g, nodes, pods, pvs, attachments) identifier := nodeidentifier.NewDefaultNodeIdentifier() authz := NewAuthorizer(g, identifier, bootstrappolicy.NodeRules()).(*NodeAuthorizer) @@ -389,6 +430,11 @@ func BenchmarkAuthorization(b *testing.B) { expect authorizer.Decision features utilfeature.FeatureGate }{ + { + name: "allowed node configmap", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "node0-configmap", Namespace: "ns0"}, + expect: authorizer.DecisionAllow, + }, { name: "allowed configmap", attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "configmap0-pod0-node0", Namespace: "ns0"}, @@ -404,6 +450,12 @@ func BenchmarkAuthorization(b *testing.B) { attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "secrets", Name: "secret0-shared", Namespace: "ns0"}, expect: authorizer.DecisionAllow, }, + + { + name: "disallowed node configmap", + attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "node1-configmap", Namespace: "ns0"}, + expect: authorizer.DecisionNoOpinion, + }, { name: "disallowed configmap", attrs: authorizer.AttributesRecord{User: node0, ResourceRequest: true, Verb: "get", Resource: "configmaps", Name: "configmap0-pod0-node1", Namespace: "ns0"}, @@ -467,9 +519,12 @@ func BenchmarkAuthorization(b *testing.B) { } } -func populate(graph *Graph, pods []*api.Pod, pvs []*api.PersistentVolume, attachments []*storagev1beta1.VolumeAttachment) { +func populate(graph *Graph, nodes []*api.Node, pods []*api.Pod, pvs []*api.PersistentVolume, attachments []*storagev1beta1.VolumeAttachment) { p := &graphPopulator{} p.graph = graph + for _, node := range nodes { + p.addNode(node) + } for _, pod := range pods { p.addPod(pod) } @@ -485,7 +540,8 @@ func populate(graph *Graph, pods []*api.Pod, pvs []*api.PersistentVolume, attach // the secret/configmap/pvc/node references in the pod and pv objects are named to indicate the connections between the objects. // for example, secret0-pod0-node0 is a secret referenced by pod0 which is bound to node0. // when populated into the graph, the node authorizer should allow node0 to access that secret, but not node1. -func generate(opts sampleDataOpts) ([]*api.Pod, []*api.PersistentVolume, []*storagev1beta1.VolumeAttachment) { +func generate(opts sampleDataOpts) ([]*api.Node, []*api.Pod, []*api.PersistentVolume, []*storagev1beta1.VolumeAttachment) { + nodes := make([]*api.Node, 0, opts.nodes) pods := make([]*api.Pod, 0, opts.nodes*opts.podsPerNode) pvs := make([]*api.PersistentVolume, 0, (opts.nodes*opts.podsPerNode*opts.uniquePVCsPerPod)+(opts.sharedPVCsPerPod*opts.namespaces)) attachments := make([]*storagev1beta1.VolumeAttachment, 0, opts.nodes*opts.attachmentsPerNode) @@ -552,6 +608,20 @@ func generate(opts sampleDataOpts) ([]*api.Pod, []*api.PersistentVolume, []*stor attachment.Spec.NodeName = nodeName attachments = append(attachments, attachment) } + + name := fmt.Sprintf("%s-configmap", nodeName) + nodes = append(nodes, &api.Node{ + ObjectMeta: metav1.ObjectMeta{Name: nodeName}, + Spec: api.NodeSpec{ + ConfigSource: &api.NodeConfigSource{ + ConfigMapRef: &api.ObjectReference{ + Name: name, + Namespace: "ns0", + UID: types.UID(fmt.Sprintf("ns0-%s", name)), + }, + }, + }, + }) } - return pods, pvs, attachments + return nodes, pods, pvs, attachments } diff --git a/test/e2e/auth/node_authz.go b/test/e2e/auth/node_authz.go index a7d8c95f1071..042d5863432a 100644 --- a/test/e2e/auth/node_authz.go +++ b/test/e2e/auth/node_authz.go @@ -74,11 +74,33 @@ var _ = SIGDescribe("[Feature:NodeAuthorizer]", func() { Expect(apierrors.IsForbidden(err)).Should(Equal(true)) }) - It("Getting an existent secret should exit with the Forbidden error", func() { + It("Getting an existing secret should exit with the Forbidden error", func() { _, err := c.CoreV1().Secrets(ns).Get(defaultSaSecret, metav1.GetOptions{}) Expect(apierrors.IsForbidden(err)).Should(Equal(true)) }) + It("Getting a non-existent configmap should exit with the Forbidden error, not a NotFound error", func() { + _, err := c.CoreV1().ConfigMaps(ns).Get("foo", metav1.GetOptions{}) + Expect(apierrors.IsForbidden(err)).Should(Equal(true)) + }) + + It("Getting an existing configmap should exit with the Forbidden error", func() { + By("Create a configmap for testing") + configmap := &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: "node-auth-configmap", + }, + Data: map[string]string{ + "data": "content", + }, + } + _, err := f.ClientSet.CoreV1().ConfigMaps(ns).Create(configmap) + Expect(err).NotTo(HaveOccurred()) + _, err = c.CoreV1().ConfigMaps(ns).Get(configmap.Name, metav1.GetOptions{}) + Expect(apierrors.IsForbidden(err)).Should(Equal(true)) + }) + It("Getting a secret for a workload the node has access to should succeed", func() { By("Create a secret for testing") secret := &v1.Secret{ @@ -138,7 +160,7 @@ var _ = SIGDescribe("[Feature:NodeAuthorizer]", func() { Expect(err).NotTo(HaveOccurred()) }) - It("A node shouldn't be able to create an other node", func() { + It("A node shouldn't be able to create another node", func() { node := &v1.Node{ ObjectMeta: metav1.ObjectMeta{Name: "foo"}, TypeMeta: metav1.TypeMeta{ @@ -151,7 +173,7 @@ var _ = SIGDescribe("[Feature:NodeAuthorizer]", func() { Expect(apierrors.IsForbidden(err)).Should(Equal(true)) }) - It("A node shouldn't be able to delete an other node", func() { + It("A node shouldn't be able to delete another node", func() { By(fmt.Sprintf("Create node foo by user: %v", asUser)) err := c.CoreV1().Nodes().Delete("foo", &metav1.DeleteOptions{}) Expect(apierrors.IsForbidden(err)).Should(Equal(true)) diff --git a/test/e2e_node/dynamic_kubelet_config_test.go b/test/e2e_node/dynamic_kubelet_config_test.go index e852059dc90f..cdc419d10f52 100644 --- a/test/e2e_node/dynamic_kubelet_config_test.go +++ b/test/e2e_node/dynamic_kubelet_config_test.go @@ -19,6 +19,7 @@ package e2e_node import ( "fmt" "reflect" + "strings" "time" "github.com/davecgh/go-spew/spew" @@ -40,6 +41,8 @@ type configState struct { configSource *apiv1.NodeConfigSource expectConfigOk *apiv1.NodeCondition expectConfig *kubeletconfig.KubeletConfiguration + // whether to expect this substring in an error returned from the API server when updating the config source + apierr string // whether the state would cause a config change event as a result of the update to Node.Spec.ConfigSource, // assuming that the current source would have also caused a config change event. // for example, some malformed references may result in a download failure, in which case the Kubelet @@ -132,25 +135,16 @@ var _ = framework.KubeDescribe("DynamicKubeletConfiguration [Feature:DynamicKube // Node.Spec.ConfigSource has all nil subfields {desc: "Node.Spec.ConfigSource has all nil subfields", - configSource: &apiv1.NodeConfigSource{ConfigMapRef: nil}, - expectConfigOk: &apiv1.NodeCondition{Type: apiv1.NodeKubeletConfigOk, Status: apiv1.ConditionFalse, - Message: "", - Reason: fmt.Sprintf(status.FailSyncReasonFmt, status.FailSyncReasonAllNilSubfields)}, - expectConfig: nil, - event: false, + configSource: &apiv1.NodeConfigSource{}, + apierr: "exactly one reference subfield must be non-nil", }, // Node.Spec.ConfigSource.ConfigMapRef is partial {desc: "Node.Spec.ConfigSource.ConfigMapRef is partial", - // TODO(mtaufen): check the other 7 partials in a unit test configSource: &apiv1.NodeConfigSource{ConfigMapRef: &apiv1.ObjectReference{ UID: "foo", Name: "bar"}}, // missing Namespace - expectConfigOk: &apiv1.NodeCondition{Type: apiv1.NodeKubeletConfigOk, Status: apiv1.ConditionFalse, - Message: "", - Reason: fmt.Sprintf(status.FailSyncReasonFmt, status.FailSyncReasonPartialObjectReference)}, - expectConfig: nil, - event: false, + apierr: "name, namespace, and UID must all be non-empty", }, // Node.Spec.ConfigSource's UID does not align with namespace/name @@ -348,10 +342,20 @@ func setAndTestKubeletConfigState(f *framework.Framework, state *configState, ex // set the desired state, retry a few times in case we are competing with other editors Eventually(func() error { if err := setNodeConfigSource(f, state.configSource); err != nil { - return fmt.Errorf("case %s: error setting Node.Spec.ConfigSource", err) + if len(state.apierr) == 0 { + return fmt.Errorf("case %s: expect nil error but got %q", state.desc, err.Error()) + } else if !strings.Contains(err.Error(), state.apierr) { + return fmt.Errorf("case %s: expect error to contain %q but got %q", state.desc, state.apierr, err.Error()) + } + } else if len(state.apierr) > 0 { + return fmt.Errorf("case %s: expect error to contain %q but got nil error", state.desc, state.apierr) } return nil }, time.Minute, time.Second).Should(BeNil()) + // skip further checks if we expected an API error + if len(state.apierr) > 0 { + return + } // check that config source actually got set to what we expect checkNodeConfigSource(f, state.desc, state.configSource) // check condition @@ -391,7 +395,6 @@ func checkConfigOkCondition(f *framework.Framework, desc string, expect *apiv1.N timeout = time.Minute interval = time.Second ) - Eventually(func() error { node, err := f.ClientSet.CoreV1().Nodes().Get(framework.TestContext.NodeName, metav1.GetOptions{}) if err != nil { diff --git a/test/integration/auth/node_test.go b/test/integration/auth/node_test.go index 929f8828d84f..5c6ecbeb6836 100644 --- a/test/integration/auth/node_test.go +++ b/test/integration/auth/node_test.go @@ -81,6 +81,9 @@ func TestNodeAuthorizer(t *testing.T) { // Enabled CSIPersistentVolume feature at startup so volumeattachments get watched defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.CSIPersistentVolume, true)() + // Enable DynamicKubeletConfig feature so that Node.Spec.ConfigSource can be set + defer utilfeaturetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.DynamicKubeletConfig, true)() + // Set up Node+RBAC authorizer authorizerConfig := &authorizer.AuthorizationConfig{ AuthorizationModes: []string{"Node", "RBAC"}, @@ -135,6 +138,9 @@ func TestNodeAuthorizer(t *testing.T) { if _, err := superuserClient.Core().ConfigMaps("ns").Create(&api.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "myconfigmap"}}); err != nil { t.Fatal(err) } + if _, err := superuserClient.Core().ConfigMaps("ns").Create(&api.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "myconfigmapconfigsource"}}); err != nil { + t.Fatal(err) + } pvName := "mypv" if _, err := superuserClientExternal.StorageV1beta1().VolumeAttachments().Create(&storagev1beta1.VolumeAttachment{ ObjectMeta: metav1.ObjectMeta{Name: "myattachment"}, @@ -186,6 +192,12 @@ func TestNodeAuthorizer(t *testing.T) { return err } } + getConfigMapConfigSource := func(client clientset.Interface) func() error { + return func() error { + _, err := client.Core().ConfigMaps("ns").Get("myconfigmapconfigsource", metav1.GetOptions{}) + return err + } + } getPVC := func(client clientset.Interface) func() error { return func() error { _, err := client.Core().PersistentVolumeClaims("ns").Get("mypvc", metav1.GetOptions{}) @@ -267,6 +279,36 @@ func TestNodeAuthorizer(t *testing.T) { return err } } + setNode2ConfigSource := func(client clientset.Interface) func() error { + return func() error { + node2, err := client.Core().Nodes().Get("node2", metav1.GetOptions{}) + if err != nil { + return err + } + node2.Spec.ConfigSource = &api.NodeConfigSource{ + ConfigMapRef: &api.ObjectReference{ + Namespace: "ns", + 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", + }, + } + _, err = client.Core().Nodes().Update(node2) + return err + } + } + unsetNode2ConfigSource := func(client clientset.Interface) func() error { + return func() error { + node2, err := client.Core().Nodes().Get("node2", metav1.GetOptions{}) + if err != nil { + return err + } + node2.Spec.ConfigSource = nil + _, err = client.Core().Nodes().Update(node2) + return err + } + } updateNode2Status := func(client clientset.Interface) func() error { return func() error { _, err := client.Core().Nodes().UpdateStatus(&api.Node{ @@ -420,6 +462,7 @@ func TestNodeAuthorizer(t *testing.T) { expectAllowed(t, deleteNode2NormalPod(node2Client)) expectAllowed(t, createNode2MirrorPod(node2Client)) expectAllowed(t, deleteNode2MirrorPod(node2Client)) + // recreate as an admin to test eviction expectAllowed(t, createNode2NormalPod(superuserClient)) expectAllowed(t, createNode2MirrorPod(superuserClient)) @@ -449,6 +492,25 @@ func TestNodeAuthorizer(t *testing.T) { expectForbidden(t, getVolumeAttachment(node1ClientExternal)) expectAllowed(t, getVolumeAttachment(node2ClientExternal)) + // create node2 again + expectAllowed(t, createNode2(node2Client)) + // node2 can not set its own config source + expectForbidden(t, setNode2ConfigSource(node2Client)) + // node2 can not access the configmap config source yet + expectForbidden(t, getConfigMapConfigSource(node2Client)) + // superuser can access the configmap config source + expectAllowed(t, getConfigMapConfigSource(superuserClient)) + // superuser can set node2's config source + expectAllowed(t, setNode2ConfigSource(superuserClient)) + // node2 can now get the configmap assigned as its config source + expectAllowed(t, getConfigMapConfigSource(node2Client)) + // superuser can unset node2's config source + expectAllowed(t, unsetNode2ConfigSource(superuserClient)) + // node2 can no longer get the configmap after it is unassigned as its config source + expectForbidden(t, getConfigMapConfigSource(node2Client)) + // clean up node2 + expectAllowed(t, deleteNode2(node2Client)) + //TODO(mikedanese): integration test node restriction of TokenRequest }