Merge pull request kubernetes#90307 from liggitt/node-labels

Restrict node labels on Node create

Author: k8s-ci-robot

plugin/pkg/admission/noderestriction/BUILD

@@ -32,7 +32,6 @@ go_library(
         "//staging/src/k8s.io/client-go/informers:go_default_library",
         "//staging/src/k8s.io/client-go/listers/core/v1:go_default_library",
         "//staging/src/k8s.io/component-base/featuregate:go_default_library",
-        "//vendor/k8s.io/klog:go_default_library",
     ],
 )
 

plugin/pkg/admission/noderestriction/admission.go

@@ -34,7 +34,6 @@ import (
 	"k8s.io/client-go/informers"
 	corev1lister "k8s.io/client-go/listers/core/v1"
 	"k8s.io/component-base/featuregate"
-	"k8s.io/klog"
 	podutil "k8s.io/kubernetes/pkg/api/pod"
 	authenticationapi "k8s.io/kubernetes/pkg/apis/authentication"
 	coordapi "k8s.io/kubernetes/pkg/apis/coordination"
@@ -406,14 +405,9 @@ func (p *Plugin) admitNode(nodeName string, a admission.Attributes) error {
 		// Don't allow a node to register with labels outside the allowed set.
 		// This would allow a node to add or modify its labels in a way that would let it steer privileged workloads to itself.
 		modifiedLabels := getModifiedLabels(node.Labels, nil)
-		if forbiddenLabels := p.getForbiddenCreateLabels(modifiedLabels); len(forbiddenLabels) > 0 {
+		if forbiddenLabels := p.getForbiddenLabels(modifiedLabels); len(forbiddenLabels) > 0 {
 			return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to set the following labels: %s", nodeName, strings.Join(forbiddenLabels.List(), ", ")))
 		}
-		// check and warn if nodes set labels on create that would have been forbidden on update
-		// TODO(liggitt): in 1.19, expand getForbiddenCreateLabels to match getForbiddenUpdateLabels and drop this
-		if forbiddenUpdateLabels := p.getForbiddenUpdateLabels(modifiedLabels); len(forbiddenUpdateLabels) > 0 {
-			klog.Warningf("node %q added disallowed labels on node creation: %s", nodeName, strings.Join(forbiddenUpdateLabels.List(), ", "))
-		}
 	}
 	if requestedName != nodeName {
 		return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to modify node %q", nodeName, requestedName))
@@ -445,7 +439,7 @@ func (p *Plugin) admitNode(nodeName string, a admission.Attributes) error {
 		// Don't allow a node to update labels outside the allowed set.
 		// This would allow a node to add or modify its labels in a way that would let it steer privileged workloads to itself.
 		modifiedLabels := getModifiedLabels(node.Labels, oldNode.Labels)
-		if forbiddenUpdateLabels := p.getForbiddenUpdateLabels(modifiedLabels); len(forbiddenUpdateLabels) > 0 {
+		if forbiddenUpdateLabels := p.getForbiddenLabels(modifiedLabels); len(forbiddenUpdateLabels) > 0 {
 			return admission.NewForbidden(a, fmt.Errorf("is not allowed to modify labels: %s", strings.Join(forbiddenUpdateLabels.List(), ", ")))
 		}
 	}
@@ -487,26 +481,8 @@ func getLabelNamespace(key string) string {
 	return ""
 }
 
-// getForbiddenCreateLabels returns the set of labels that may not be set by the node.
-// TODO(liggitt): in 1.19, expand to match getForbiddenUpdateLabels()
-func (p *Plugin) getForbiddenCreateLabels(modifiedLabels sets.String) sets.String {
-	if len(modifiedLabels) == 0 {
-		return nil
-	}
-
-	forbiddenLabels := sets.NewString()
-	for label := range modifiedLabels {
-		namespace := getLabelNamespace(label)
-		// forbid kubelets from setting node-restriction labels
-		if namespace == v1.LabelNamespaceNodeRestriction || strings.HasSuffix(namespace, "."+v1.LabelNamespaceNodeRestriction) {
-			forbiddenLabels.Insert(label)
-		}
-	}
-	return forbiddenLabels
-}
-
-// getForbiddenLabels returns the set of labels that may not be set by the node on update.
-func (p *Plugin) getForbiddenUpdateLabels(modifiedLabels sets.String) sets.String {
+// getForbiddenLabels returns the set of labels that may not be added, removed, or modified by the node on create or update.
+func (p *Plugin) getForbiddenLabels(modifiedLabels sets.String) sets.String {
 	if len(modifiedLabels) == 0 {
 		return nil
 	}

plugin/pkg/admission/noderestriction/admission_test.go

@@ -146,9 +146,6 @@ func setAllLabels(node *api.Node, value string) *api.Node {
 
 func setAllowedCreateLabels(node *api.Node, value string) *api.Node {
 	node = setAllowedUpdateLabels(node, value)
-	// also allow other kubernetes labels on create until 1.17 (TODO: remove this in 1.17)
-	node.Labels["other.kubernetes.io/foo"] = value
-	node.Labels["other.k8s.io/foo"] = value
 	return node
 }
 
@@ -206,9 +203,8 @@ func setForbiddenCreateLabels(node *api.Node, value string) *api.Node {
 	// node restriction labels are forbidden
 	node.Labels["node-restriction.kubernetes.io/foo"] = value
 	node.Labels["foo.node-restriction.kubernetes.io/foo"] = value
-	// TODO: in 1.17, forbid arbitrary kubernetes labels on create
-	// node.Labels["other.kubernetes.io/foo"] = value
-	// node.Labels["other.k8s.io/foo"] = value
+	node.Labels["other.kubernetes.io/foo"] = value
+	node.Labels["other.k8s.io/foo"] = value
 	return node
 }
 
@@ -925,7 +921,7 @@ func Test_nodePlugin_Admit(t *testing.T) {
 			name:       "forbid create of my node with forbidden labels",
 			podsGetter: noExistingPods,
 			attributes: admission.NewAttributesRecord(setForbiddenCreateLabels(mynodeObj, ""), nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode),
-			err:        `is not allowed to set the following labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo`,
+			err:        `is not allowed to set the following labels: foo.node-restriction.kubernetes.io/foo, node-restriction.kubernetes.io/foo, other.k8s.io/foo, other.kubernetes.io/foo`,
 		},
 		{
 			name:       "allow update of my node",