diff --git a/plugin/pkg/admission/noderestriction/admission.go b/plugin/pkg/admission/noderestriction/admission.go index 7792048adfb..5488c68c3b5 100644 --- a/plugin/pkg/admission/noderestriction/admission.go +++ b/plugin/pkg/admission/noderestriction/admission.go @@ -70,6 +70,7 @@ type Plugin struct { *admission.Handler nodeIdentifier nodeidentifier.NodeIdentifier podsGetter corev1lister.PodLister + nodesGetter corev1lister.NodeLister tokenRequestEnabled bool csiNodeInfoEnabled bool @@ -92,6 +93,7 @@ func (p *Plugin) InspectFeatureGates(featureGates featuregate.FeatureGate) { // SetExternalKubeInformerFactory registers an informer factory into Plugin func (p *Plugin) SetExternalKubeInformerFactory(f informers.SharedInformerFactory) { p.podsGetter = f.Core().V1().Pods().Lister() + p.nodesGetter = f.Core().V1().Nodes().Lister() } // ValidateInitialization validates the Plugin was initialized properly @@ -102,6 +104,9 @@ func (p *Plugin) ValidateInitialization() error { if p.podsGetter == nil { return fmt.Errorf("%s requires a pod getter", PluginName) } + if p.nodesGetter == nil { + return fmt.Errorf("%s requires a node getter", PluginName) + } return nil } @@ -128,6 +133,8 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission. return admission.NewForbidden(a, fmt.Errorf("could not determine node from user %q", a.GetUserInfo().GetName())) } + // TODO: if node doesn't exist and this isn't a create node request, then reject. + switch a.GetResource().GroupResource() { case podResource: switch a.GetSubresource() { @@ -177,43 +184,7 @@ func (p *Plugin) Admit(ctx context.Context, a admission.Attributes, o admission. func (p *Plugin) admitPod(nodeName string, a admission.Attributes) error { switch a.GetOperation() { case admission.Create: - // require a pod object - pod, ok := a.GetObject().(*api.Pod) - if !ok { - return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject())) - } - - // only allow nodes to create mirror pods - if _, isMirrorPod := pod.Annotations[api.MirrorPodAnnotationKey]; !isMirrorPod { - return admission.NewForbidden(a, fmt.Errorf("pod does not have %q annotation, node %q can only create mirror pods", api.MirrorPodAnnotationKey, nodeName)) - } - - // only allow nodes to create a pod bound to itself - if pod.Spec.NodeName != nodeName { - return admission.NewForbidden(a, fmt.Errorf("node %q can only create pods with spec.nodeName set to itself", nodeName)) - } - - // don't allow a node to create a pod that references any other API objects - if pod.Spec.ServiceAccountName != "" { - return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference a service account", nodeName)) - } - hasSecrets := false - podutil.VisitPodSecretNames(pod, func(name string) (shouldContinue bool) { hasSecrets = true; return false }) - if hasSecrets { - return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference secrets", nodeName)) - } - hasConfigMaps := false - podutil.VisitPodConfigmapNames(pod, func(name string) (shouldContinue bool) { hasConfigMaps = true; return false }) - if hasConfigMaps { - return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference configmaps", nodeName)) - } - for _, v := range pod.Spec.Volumes { - if v.PersistentVolumeClaim != nil { - return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference persistentvolumeclaims", nodeName)) - } - } - - return nil + return p.admitPodCreate(nodeName, a) case admission.Delete: // get the existing pod @@ -235,6 +206,75 @@ func (p *Plugin) admitPod(nodeName string, a admission.Attributes) error { } } +func (p *Plugin) admitPodCreate(nodeName string, a admission.Attributes) error { + // require a pod object + pod, ok := a.GetObject().(*api.Pod) + if !ok { + return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject())) + } + + // only allow nodes to create mirror pods + if _, isMirrorPod := pod.Annotations[api.MirrorPodAnnotationKey]; !isMirrorPod { + return admission.NewForbidden(a, fmt.Errorf("pod does not have %q annotation, node %q can only create mirror pods", api.MirrorPodAnnotationKey, nodeName)) + } + + // only allow nodes to create a pod bound to itself + if pod.Spec.NodeName != nodeName { + return admission.NewForbidden(a, fmt.Errorf("node %q can only create pods with spec.nodeName set to itself", nodeName)) + } + if len(pod.OwnerReferences) > 1 { + return admission.NewForbidden(a, fmt.Errorf("node %q can only create pods with a single owner reference set to itself", nodeName)) + } + if len(pod.OwnerReferences) == 1 { + owner := pod.OwnerReferences[0] + if owner.APIVersion != v1.SchemeGroupVersion.String() || + owner.Kind != "Node" || + owner.Name != nodeName { + return admission.NewForbidden(a, fmt.Errorf("node %q can only create pods with an owner reference set to itself", nodeName)) + } + if owner.Controller == nil || !*owner.Controller { + return admission.NewForbidden(a, fmt.Errorf("node %q can only create pods with a controller owner reference set to itself", nodeName)) + } + if owner.BlockOwnerDeletion != nil && *owner.BlockOwnerDeletion { + return admission.NewForbidden(a, fmt.Errorf("node %q must not set blockOwnerDeletion on an owner reference", nodeName)) + } + + // Verify the node UID. + node, err := p.nodesGetter.Get(nodeName) + if errors.IsNotFound(err) { + return err + } + if err != nil { + return admission.NewForbidden(a, fmt.Errorf("error looking up node %s to verify uid: %v", nodeName, err)) + } + if owner.UID != node.UID { + return admission.NewForbidden(a, fmt.Errorf("node %s UID mismatch: expected %s got %s", nodeName, owner.UID, node.UID)) + } + } + + // don't allow a node to create a pod that references any other API objects + if pod.Spec.ServiceAccountName != "" { + return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference a service account", nodeName)) + } + hasSecrets := false + podutil.VisitPodSecretNames(pod, func(name string) (shouldContinue bool) { hasSecrets = true; return false }) + if hasSecrets { + return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference secrets", nodeName)) + } + hasConfigMaps := false + podutil.VisitPodConfigmapNames(pod, func(name string) (shouldContinue bool) { hasConfigMaps = true; return false }) + if hasConfigMaps { + return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference configmaps", nodeName)) + } + for _, v := range pod.Spec.Volumes { + if v.PersistentVolumeClaim != nil { + return admission.NewForbidden(a, fmt.Errorf("node %q can not create pods that reference persistentvolumeclaims", nodeName)) + } + } + + return nil +} + // admitPodStatus allows to update the status of a pod if it is // assigned to the current node. func (p *Plugin) admitPodStatus(nodeName string, a admission.Attributes) error { diff --git a/plugin/pkg/admission/noderestriction/admission_test.go b/plugin/pkg/admission/noderestriction/admission_test.go index 48126ee7946..805552e0203 100644 --- a/plugin/pkg/admission/noderestriction/admission_test.go +++ b/plugin/pkg/admission/noderestriction/admission_test.go @@ -25,6 +25,7 @@ import ( "time" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" @@ -48,7 +49,6 @@ import ( var ( trEnabledFeature = featuregate.NewFeatureGate() - trDisabledFeature = featuregate.NewFeatureGate() csiNodeInfoEnabledFeature = featuregate.NewFeatureGate() csiNodeInfoDisabledFeature = featuregate.NewFeatureGate() ) @@ -61,7 +61,6 @@ func init() { features.ExpandPersistentVolumes: {Default: false}, } utilruntime.Must(trEnabledFeature.Add(relevantFeatures)) - utilruntime.Must(trDisabledFeature.Add(relevantFeatures)) utilruntime.Must(csiNodeInfoEnabledFeature.Add(relevantFeatures)) utilruntime.Must(csiNodeInfoDisabledFeature.Add(relevantFeatures)) @@ -83,6 +82,18 @@ func makeTestPod(namespace, name, node string, mirror bool) (*api.Pod, *corev1.P if mirror { corePod.Annotations = map[string]string{api.MirrorPodAnnotationKey: "true"} v1Pod.Annotations = map[string]string{api.MirrorPodAnnotationKey: "true"} + + // Insert a valid owner reference by default. + controller := true + owner := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "Node", + Name: node, + UID: types.UID(node + "-uid"), + Controller: &controller, + } + corePod.OwnerReferences = []metav1.OwnerReference{owner} + v1Pod.OwnerReferences = []metav1.OwnerReference{owner} } return corePod, v1Pod } @@ -218,12 +229,40 @@ func setForbiddenUpdateLabels(node *api.Node, value string) *api.Node { return node } +type admitTestCase struct { + name string + podsGetter corev1lister.PodLister + nodesGetter corev1lister.NodeLister + attributes admission.Attributes + features featuregate.FeatureGate + err string +} + +func (a *admitTestCase) run(t *testing.T) { + t.Run(a.name, func(t *testing.T) { + c := NewPlugin(nodeidentifier.NewDefaultNodeIdentifier()) + if a.features != nil { + c.InspectFeatureGates(a.features) + } + c.podsGetter = a.podsGetter + c.nodesGetter = a.nodesGetter + err := c.Admit(context.TODO(), a.attributes, nil) + if (err == nil) != (len(a.err) == 0) { + t.Errorf("nodePlugin.Admit() error = %v, expected %v", err, a.err) + return + } + if len(a.err) > 0 && !strings.Contains(err.Error(), a.err) { + t.Errorf("nodePlugin.Admit() error = %v, expected %v", err, a.err) + } + }) +} + func Test_nodePlugin_Admit(t *testing.T) { var ( mynode = &user.DefaultInfo{Name: "system:node:mynode", Groups: []string{"system:nodes"}} bob = &user.DefaultInfo{Name: "bob"} - mynodeObjMeta = metav1.ObjectMeta{Name: "mynode"} + mynodeObjMeta = metav1.ObjectMeta{Name: "mynode", UID: "mynode-uid"} mynodeObj = &api.Node{ObjectMeta: mynodeObjMeta} mynodeObjConfigA = &api.Node{ObjectMeta: mynodeObjMeta, Spec: api.NodeSpec{ConfigSource: &api.NodeConfigSource{ ConfigMap: &api.ConfigMapNodeConfigSource{ @@ -340,6 +379,9 @@ func Test_nodePlugin_Admit(t *testing.T) { }, } + existingNodesIndex = cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) + existingNodes = corev1lister.NewNodeLister(existingNodesIndex) + noExistingPodsIndex = cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) noExistingPods = corev1lister.NewPodLister(noExistingPodsIndex) @@ -364,6 +406,8 @@ func Test_nodePlugin_Admit(t *testing.T) { existingPodsIndex.Add(v1otherpod) existingPodsIndex.Add(v1unboundpod) + existingNodesIndex.Add(&v1.Node{ObjectMeta: mynodeObjMeta}) + sapod, _ := makeTestPod("ns", "mysapod", "mynode", true) sapod.Spec.ServiceAccountName = "foo" @@ -376,13 +420,7 @@ func Test_nodePlugin_Admit(t *testing.T) { pvcpod, _ := makeTestPod("ns", "mypvcpod", "mynode", true) pvcpod.Spec.Volumes = []api.Volume{{VolumeSource: api.VolumeSource{PersistentVolumeClaim: &api.PersistentVolumeClaimVolumeSource{ClaimName: "foo"}}}} - tests := []struct { - name string - podsGetter corev1lister.PodLister - attributes admission.Attributes - features featuregate.FeatureGate - err string - }{ + tests := []admitTestCase{ // Mirror pods bound to us { name: "allow creating a mirror pod bound to self", @@ -1232,21 +1270,125 @@ func Test_nodePlugin_Admit(t *testing.T) { }, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := NewPlugin(nodeidentifier.NewDefaultNodeIdentifier()) - if tt.features != nil { - c.InspectFeatureGates(tt.features) - } - c.podsGetter = tt.podsGetter - err := c.Admit(context.TODO(), tt.attributes, nil) - if (err == nil) != (len(tt.err) == 0) { - t.Errorf("nodePlugin.Admit() error = %v, expected %v", err, tt.err) - return - } - if len(tt.err) > 0 && !strings.Contains(err.Error(), tt.err) { - t.Errorf("nodePlugin.Admit() error = %v, expected %v", err, tt.err) - } - }) + tt.nodesGetter = existingNodes + tt.run(t) + } +} + +func Test_nodePlugin_Admit_OwnerReference(t *testing.T) { + expectedNodeIndex := cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) + expectedNodeIndex.Add(&v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "mynode", UID: "mynode-uid"}}) + expectedNode := corev1lister.NewNodeLister(expectedNodeIndex) + + unexpectedNodeIndex := cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) + unexpectedNodeIndex.Add(&v1.Node{ObjectMeta: metav1.ObjectMeta{Name: "mynode", UID: "mynode-unexpected-uid"}}) + unexpectedNode := corev1lister.NewNodeLister(unexpectedNodeIndex) + + noNodesIndex := cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) + noNodes := corev1lister.NewNodeLister(noNodesIndex) + + noExistingPodsIndex := cache.NewIndexer(cache.MetaNamespaceKeyFunc, nil) + noExistingPods := corev1lister.NewPodLister(noExistingPodsIndex) + + mynode := &user.DefaultInfo{Name: "system:node:mynode", Groups: []string{"system:nodes"}} + validOwner := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "Node", + Name: "mynode", + UID: "mynode-uid", + Controller: pointer.BoolPtr(true), + } + invalidName := validOwner + invalidName.Name = "other" + invalidKind := validOwner + invalidKind.Kind = "Pod" + invalidAPI := validOwner + invalidAPI.APIVersion = "v2" + invalidControllerNil := validOwner + invalidControllerNil.Controller = nil + invalidControllerFalse := validOwner + invalidControllerFalse.Controller = pointer.BoolPtr(false) + invalidBlockDeletion := validOwner + invalidBlockDeletion.BlockOwnerDeletion = pointer.BoolPtr(true) + + tests := []struct { + name string + owners []metav1.OwnerReference + nodesGetter corev1lister.NodeLister + expectErr string + }{ + { + name: "no owner", + owners: nil, + }, + { + name: "valid owner", + owners: []metav1.OwnerReference{validOwner}, + }, + { + name: "duplicate owner", + owners: []metav1.OwnerReference{validOwner, validOwner}, + expectErr: "can only create pods with a single owner reference set to itself", + }, + { + name: "invalid name", + owners: []metav1.OwnerReference{invalidName}, + expectErr: "can only create pods with an owner reference set to itself", + }, + { + name: "invalid UID", + owners: []metav1.OwnerReference{validOwner}, + nodesGetter: unexpectedNode, + expectErr: "UID mismatch", + }, + { + name: "node not found", + owners: []metav1.OwnerReference{validOwner}, + nodesGetter: noNodes, + expectErr: "not found", + }, + { + name: "invalid API version", + owners: []metav1.OwnerReference{invalidAPI}, + expectErr: "can only create pods with an owner reference set to itself", + }, + { + name: "invalid kind", + owners: []metav1.OwnerReference{invalidKind}, + expectErr: "can only create pods with an owner reference set to itself", + }, + { + name: "nil controller", + owners: []metav1.OwnerReference{invalidControllerNil}, + expectErr: "can only create pods with a controller owner reference set to itself", + }, + { + name: "false controller", + owners: []metav1.OwnerReference{invalidControllerFalse}, + expectErr: "can only create pods with a controller owner reference set to itself", + }, + { + name: "invalid blockOwnerDeletion", + owners: []metav1.OwnerReference{invalidBlockDeletion}, + expectErr: "must not set blockOwnerDeletion on an owner reference", + }, + } + + for _, test := range tests { + if test.nodesGetter == nil { + test.nodesGetter = expectedNode + } + + pod, _ := makeTestPod("ns", "test", "mynode", true) + pod.OwnerReferences = test.owners + a := &admitTestCase{ + name: test.name, + podsGetter: noExistingPods, + nodesGetter: test.nodesGetter, + attributes: createPodAttributes(pod, mynode), + err: test.expectErr, + } + a.run(t) } } @@ -1326,3 +1468,9 @@ func Test_getModifiedLabels(t *testing.T) { }) } } + +func createPodAttributes(pod *api.Pod, user user.Info) admission.Attributes { + podResource := api.Resource("pods").WithVersion("v1") + podKind := api.Kind("Pod").WithVersion("v1") + return admission.NewAttributesRecord(pod, nil, podKind, pod.Namespace, pod.Name, podResource, "", admission.Create, &metav1.CreateOptions{}, false, user) +}