Merge pull request #19772 from enj/enj/i/pod_node_constraints_mirror_pod/1579605

Ignore node created self targeting mirror pods in PodNodeConstraints

Author: openshift-merge-robot

pkg/scheduler/admission/podnodeconstraints/admission.go

@@ -15,39 +15,43 @@ import (
 	"k8s.io/apiserver/pkg/authorization/authorizer"
 	kapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/extensions"
+	"k8s.io/kubernetes/pkg/auth/nodeidentifier"
 
 	"github.com/openshift/origin/pkg/api/meta"
 	configlatest "github.com/openshift/origin/pkg/cmd/server/apis/config/latest"
 	"github.com/openshift/origin/pkg/scheduler/admission/apis/podnodeconstraints"
 )
 
+const PluginName = "PodNodeConstraints"
+
 // kindsToIgnore is a list of kinds that contain a PodSpec that
 // we choose not to handle in this plugin
 var kindsToIgnore = []schema.GroupKind{
 	extensions.Kind("DaemonSet"),
 }
 
 func Register(plugins *admission.Plugins) {
-	plugins.Register("PodNodeConstraints",
+	plugins.Register(PluginName,
 		func(config io.Reader) (admission.Interface, error) {
 			pluginConfig, err := readConfig(config)
 			if err != nil {
 				return nil, err
 			}
 			if pluginConfig == nil {
-				glog.Infof("Admission plugin %q is not configured so it will be disabled.", "PodNodeConstraints")
+				glog.Infof("Admission plugin %q is not configured so it will be disabled.", PluginName)
 				return nil, nil
 			}
-			return NewPodNodeConstraints(pluginConfig), nil
+			return NewPodNodeConstraints(pluginConfig, nodeidentifier.NewDefaultNodeIdentifier()), nil
 		})
 }
 
 // NewPodNodeConstraints creates a new admission plugin to prevent objects that contain pod templates
 // from containing node bindings by name or selector based on role permissions.
-func NewPodNodeConstraints(config *podnodeconstraints.PodNodeConstraintsConfig) admission.Interface {
+func NewPodNodeConstraints(config *podnodeconstraints.PodNodeConstraintsConfig, nodeIdentifier nodeidentifier.NodeIdentifier) admission.Interface {
 	plugin := podNodeConstraints{
-		config:  config,
-		Handler: admission.NewHandler(admission.Create, admission.Update),
+		config:         config,
+		Handler:        admission.NewHandler(admission.Create, admission.Update),
+		nodeIdentifier: nodeIdentifier,
 	}
 	if config != nil {
 		plugin.selectorLabelBlacklist = sets.NewString(config.NodeSelectorLabelBlacklist...)
@@ -61,6 +65,7 @@ type podNodeConstraints struct {
 	selectorLabelBlacklist sets.String
 	config                 *podnodeconstraints.PodNodeConstraintsConfig
 	authorizer             authorizer.Authorizer
+	nodeIdentifier         nodeidentifier.NodeIdentifier
 }
 
 func shouldCheckResource(resource schema.GroupResource, kind schema.GroupKind) (bool, error) {
@@ -135,6 +140,12 @@ func (o *podNodeConstraints) getPodSpec(attr admission.Attributes) (kapi.PodSpec
 
 // validate PodSpec if NodeName or NodeSelector are specified
 func (o *podNodeConstraints) admitPodSpec(attr admission.Attributes, ps kapi.PodSpec) error {
+	// a node creating a mirror pod that targets itself is allowed
+	// see the NodeRestriction plugin for further details
+	if o.isNodeSelfTargetWithMirrorPod(attr, ps.NodeName) {
+		return nil
+	}
+
 	matchingLabels := []string{}
 	// nodeSelector blacklist filter
 	for nodeSelectorLabel := range ps.NodeSelector {
@@ -168,7 +179,10 @@ func (o *podNodeConstraints) SetAuthorizer(a authorizer.Authorizer) {
 
 func (o *podNodeConstraints) ValidateInitialization() error {
 	if o.authorizer == nil {
-		return fmt.Errorf("PodNodeConstraints needs an Openshift Authorizer")
+		return fmt.Errorf("%s requires an authorizer", PluginName)
+	}
+	if o.nodeIdentifier == nil {
+		return fmt.Errorf("%s requires a node identifier", PluginName)
 	}
 	return nil
 }
@@ -190,3 +204,25 @@ func (o *podNodeConstraints) checkPodsBindAccess(attr admission.Attributes) (boo
 	authorized, _, err := o.authorizer.Authorize(authzAttr)
 	return authorized == authorizer.DecisionAllow, err
 }
+
+func (o *podNodeConstraints) isNodeSelfTargetWithMirrorPod(attr admission.Attributes, nodeName string) bool {
+	// make sure we are actually trying to target a node
+	if len(nodeName) == 0 {
+		return false
+	}
+	// this check specifically requires the object to be pod (unlike the other checks where we want any pod spec)
+	pod, ok := attr.GetObject().(*kapi.Pod)
+	if !ok {
+		return false
+	}
+	// note that anyone can create a mirror pod, but they are not privileged in any way
+	// they are actually highly constrained since they cannot reference secrets
+	// nodes can only create and delete them, and they will delete any "orphaned" mirror pods
+	if _, isMirrorPod := pod.Annotations[kapi.MirrorPodAnnotationKey]; !isMirrorPod {
+		return false
+	}
+	// we are targeting a node with a mirror pod
+	// confirm the user is a node that is targeting itself
+	actualNodeName, isNode := o.nodeIdentifier.NodeIdentity(attr.GetUserInfo())
+	return isNode && actualNodeName == nodeName
+}

pkg/scheduler/admission/podnodeconstraints/admission_test.go

@@ -15,6 +15,7 @@ import (
 	"k8s.io/kubernetes/pkg/apis/batch"
 	kapi "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/apis/extensions"
+	"k8s.io/kubernetes/pkg/auth/nodeidentifier"
 	"k8s.io/kubernetes/pkg/serviceaccount"
 
 	_ "github.com/openshift/origin/pkg/api/install"
@@ -99,11 +100,39 @@ func TestPodNodeConstraints(t *testing.T) {
 			expectedResource: "pods/binding",
 			expectedErrorMsg: "",
 		},
+		// 7: expect nodeName to succeed with node user self targeting mirror pod
+		{
+			config:           testConfig(),
+			resource:         nodeNameMirrorPod(),
+			userinfo:         &user.DefaultInfo{Name: "system:node:frank", Groups: []string{user.NodesGroup}},
+			expectedErrorMsg: "",
+		},
+		// 8: expect nodeName to fail with node user self targeting non-mirror pod
+		{
+			config:           testConfig(),
+			resource:         nodeNamePod(),
+			userinfo:         &user.DefaultInfo{Name: "system:node:frank", Groups: []string{user.NodesGroup}},
+			expectedErrorMsg: "node selection by nodeName is prohibited by policy for your role",
+		},
+		// 9: expect nodeName to fail with node user non-self targeting mirror pod
+		{
+			config:           testConfig(),
+			resource:         nodeNameMirrorPod(),
+			userinfo:         &user.DefaultInfo{Name: "system:node:bob", Groups: []string{user.NodesGroup}},
+			expectedErrorMsg: "node selection by nodeName is prohibited by policy for your role",
+		},
+		// 10: expect nodeName to fail with node user non-self targeting non-mirror pod
+		{
+			config:           testConfig(),
+			resource:         nodeNamePod(),
+			userinfo:         &user.DefaultInfo{Name: "system:node:bob", Groups: []string{user.NodesGroup}},
+			expectedErrorMsg: "node selection by nodeName is prohibited by policy for your role",
+		},
 	}
 	for i, tc := range tests {
 		var expectedError error
 		errPrefix := fmt.Sprintf("%d", i)
-		prc := NewPodNodeConstraints(tc.config)
+		prc := NewPodNodeConstraints(tc.config, nodeidentifier.NewDefaultNodeIdentifier())
 		prc.(initializer.WantsAuthorizer).SetAuthorizer(fakeAuthorizer(t))
 		err := prc.(admission.InitializationValidator).ValidateInitialization()
 		if err != nil {
@@ -123,7 +152,7 @@ func TestPodNodeConstraintsPodUpdate(t *testing.T) {
 	ns := metav1.NamespaceDefault
 	var expectedError error
 	errPrefix := "PodUpdate"
-	prc := NewPodNodeConstraints(testConfig())
+	prc := NewPodNodeConstraints(testConfig(), nodeidentifier.NewDefaultNodeIdentifier())
 	prc.(initializer.WantsAuthorizer).SetAuthorizer(fakeAuthorizer(t))
 	err := prc.(admission.InitializationValidator).ValidateInitialization()
 	if err != nil {
@@ -139,7 +168,7 @@ func TestPodNodeConstraintsNonHandledResources(t *testing.T) {
 	ns := metav1.NamespaceDefault
 	errPrefix := "ResourceQuotaTest"
 	var expectedError error
-	prc := NewPodNodeConstraints(testConfig())
+	prc := NewPodNodeConstraints(testConfig(), nodeidentifier.NewDefaultNodeIdentifier())
 	prc.(initializer.WantsAuthorizer).SetAuthorizer(fakeAuthorizer(t))
 	err := prc.(admission.InitializationValidator).ValidateInitialization()
 	if err != nil {
@@ -257,7 +286,7 @@ func TestPodNodeConstraintsResources(t *testing.T) {
 				for _, top := range testops {
 					var expectedError error
 					errPrefix := fmt.Sprintf("%s; %s; %s", tr.prefix, tp.prefix, top.operation)
-					prc := NewPodNodeConstraints(tc.config)
+					prc := NewPodNodeConstraints(tc.config, nodeidentifier.NewDefaultNodeIdentifier())
 					prc.(initializer.WantsAuthorizer).SetAuthorizer(fakeAuthorizer(t))
 					err := prc.(admission.InitializationValidator).ValidateInitialization()
 					if err != nil {
@@ -312,6 +341,13 @@ func nodeNamePod() *kapi.Pod {
 	return pod
 }
 
+func nodeNameMirrorPod() *kapi.Pod {
+	pod := &kapi.Pod{}
+	pod.Annotations = map[string]string{kapi.MirrorPodAnnotationKey: "true"}
+	pod.Spec.NodeName = "frank"
+	return pod
+}
+
 func nodeSelectorPod() *kapi.Pod {
 	pod := &kapi.Pod{}
 	pod.Spec.NodeSelector = map[string]string{"bogus": "frank"}