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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions pkg/apis/core/validation/validation.go
Expand Up @@ -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
}

Expand All @@ -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) == "" {
Copy link
Member

Choose a reason for hiding this comment

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

why uid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 {
Expand Down
3 changes: 2 additions & 1 deletion pkg/kubeapiserver/authorizer/config.go
Expand Up @@ -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(),
Expand Down
7 changes: 6 additions & 1 deletion plugin/pkg/auth/authorizer/node/BUILD
Expand Up @@ -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",
Expand Down
66 changes: 66 additions & 0 deletions plugin/pkg/auth/authorizer/node/graph.go
Expand Up @@ -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):
//
Expand Down Expand Up @@ -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))
}

}
61 changes: 61 additions & 0 deletions plugin/pkg/auth/authorizer/node/graph_populator.go
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package node

import (
"fmt"
"github.com/golang/glog"

storagev1beta1 "k8s.io/api/storage/v1beta1"
Expand All @@ -34,6 +35,7 @@ type graphPopulator struct {

func AddGraphEventHandlers(
graph *Graph,
nodes coreinformers.NodeInformer,
pods coreinformers.PodInformer,
pvs coreinformers.PersistentVolumeInformer,
attachments storageinformers.VolumeAttachmentInformer,
Expand All @@ -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,
Expand All @@ -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)
}
Expand Down