Allow nodes to create evictions for its own pods in NodeRestriction admission controller #48707

danielfm · 2017-07-10T14:03:57Z

What this PR does / why we need it: This PR adds support for pods/eviction sub-resource to the NodeRestriction admission controller so it allows a node to evict pods bound to itself.

Which issue this PR fixes: fixes #48666

Special notes for your reviewer: The NodeRestriction already allows nodes to delete pods bound to itself, so allowing nodes to also delete pods via the Eviction API probably makes sense.

The NodeRestriction admission plugin now allows a node to evict pods bound to itself

k8s-ci-robot · 2017-07-10T14:04:05Z

Hi @danielfm. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

danielfm · 2017-07-11T23:01:42Z

Okay, now I think this is ready for review @deads2k @tallclair @liggitt

liggitt · 2017-07-12T16:33:29Z

@kubernetes/sig-auth-pr-reviews @kubernetes/sig-node-pr-reviews

liggitt · 2017-07-12T16:36:50Z

plugin/pkg/admission/noderestriction/admission.go

+			return admission.NewForbidden(a, fmt.Errorf("unexpected type %T", a.GetObject()))
+		}
+		// get the existing pod from the server cache
+		existingPod, err := c.podsGetter.Pods(eviction.Namespace).Get(eviction.Name, v1.GetOptions{ResourceVersion: "0"})


I would expect the namespace to be pulled from the admission attributes, and the pod name to be pulled from the admission attributes as well, if set, rather than from the submitted object

liggitt · 2017-07-12T16:38:12Z

plugin/pkg/admission/noderestriction/admission.go

+			// wasn't found in the server cache, do a live lookup before forbidding
+			existingPod, err = c.podsGetter.Pods(eviction.Namespace).Get(eviction.Name, v1.GetOptions{})
+		}
+		if existingPod == nil {


check err != nil, rather than existingPod == nil

both here and in the delete case, I wonder if we should return a NotFound error instead of a Forbidden error if the err here is a NotFound error...

check err != nil, rather than existingPod == nil

Done.

both here and in the delete case, I wonder if we should return a NotFound error instead of a Forbidden error if the err here is a NotFound error...

What do you propose we do? Should these be addressed here, or in a different PR?

I would change it here

Something like 8f7cd82a84c17bf7a8f8bb78b737116dd133a4b3?

liggitt · 2017-07-12T16:42:02Z

plugin/pkg/admission/noderestriction/admission_test.go

-		podKind     = api.Kind("Pod").WithVersion("v1")
+		podResource  = api.Resource("pods").WithVersion("v1")
+		podKind      = api.Kind("Pod").WithVersion("v1")
+		evictionKind = api.Kind("Eviction").WithVersion("policy/v1beta1")


should be policyapi.Kind("Eviction").WithVersion("v1beta1")

liggitt · 2017-07-12T19:26:28Z

plugin/pkg/admission/noderestriction/admission.go

@@ -161,6 +164,9 @@ func (c *nodePlugin) admitPod(nodeName string, a admission.Attributes) error {
 		if errors.IsNotFound(err) {
 			// wasn't found in the server cache, do a live lookup before forbidding
 			existingPod, err = c.podsGetter.Pods(a.GetNamespace()).Get(a.GetName(), v1.GetOptions{})
+			if errors.IsNotFound(err) {
+				return admission.NewNotFound(a)


would it work to just return the error as-is, with no wrapping?

liggitt · 2017-07-12T19:30:22Z

this looks pretty good to me, go ahead and rebase on latest master and squash

liggitt · 2017-07-12T19:31:02Z

@kubernetes/sig-scheduling-pr-reviews @davidopp wanted to make sure it made sense to allow a node to perform evictions on its own pods... it can already delete them directly

danielfm · 2017-07-12T19:52:01Z

Rebased and squashed.

liggitt · 2017-07-12T20:13:25Z

@k8s-bot ok to test

danielfm · 2017-07-13T13:44:22Z

@liggitt Can you please take another look? I implemented the suggested changes, although I'm not so sure about the change in which we return HTTP 404, particularly in how to document the expected behavior in integration tests (the comment is probably wrong).

I'd be happy to address any other issues you might find.

liggitt · 2017-07-13T13:48:58Z

plugin/pkg/admission/noderestriction/admission_test.go

+			err:        "forbidden: unexpected operation",
+		},
+		{
+			name:       "allow create of unnamed eviction for mirror pod to another",


test name is wrong, this is expecting a forbidden error

liggitt · 2017-07-13T14:03:13Z

test/integration/auth/node_test.go

@@ -230,7 +255,9 @@ func TestNodeAuthorizer(t *testing.T) {
 	expectForbidden(t, getPV(node1Client))
 	expectForbidden(t, createNode2NormalPod(nodeanonClient))
 	expectForbidden(t, createNode2MirrorPod(node1Client))
-	expectForbidden(t, deleteNode2MirrorPod(node1Client))
+	// mirror pods do not exist from the apiserver stand point


I don't think a comment is needed here... the pod doesn't exist from any stand point :)

Hahah got it. :)

danielfm · 2017-07-15T15:13:43Z

Done.

Thanks again for your input, @liggitt.

redbaron · 2017-07-16T08:53:16Z

/test pull-kubernetes-kubemark-e2e-gce

danielfm · 2017-07-18T13:23:09Z

Any updates? /cc @liggitt @davidopp @tallclair

redbaron · 2017-07-20T19:28:07Z

/retest

davidopp · 2017-07-23T08:25:56Z

wanted to make sure it made sense to allow a node to perform evictions on its own pods... it can already delete them directly

Sorry for the delay. Yes, this is fine. Evict is just a delete that can sometimes be rejected by the API server. So since a node can already delete, it's fine for it to be able to use the eviction subresource.

liggitt · 2017-07-23T10:31:22Z

/lgtm

k8s-github-robot · 2017-07-23T10:31:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielfm, liggitt

Associated issue: 48666

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~plugin/pkg/admission/noderestriction/OWNERS~~ [liggitt]
~~plugin/pkg/auth/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-23T11:16:50Z

Automatic merge from submit-queue

redbaron · 2017-07-23T13:30:42Z

Can it be cherry-picked to release-1.7 please?

liggitt · 2017-07-24T12:47:11Z

backports are typically limited to fixes for bugs and regressions

redbaron · 2017-07-31T15:21:16Z

@liggitt , it is kinda bug no? NodeRestriction admission controller can't be used in all cases it was designed to be used. Missing eviction permission is clearly an oversight and was never an intent. Maybe you can consider to including it to 1.7?

liggitt · 2017-07-31T15:25:46Z

Is there code in the kubelet that calls the evict subresource? If so, I would agree this is a bug. If not, I'd consider it an enhancement.

redbaron · 2017-08-02T17:32:57Z

@liggitt , yes there is eviction code in a kubelet. It is invoked when it tried to free resources when under pressure:

kubernetes/pkg/kubelet/eviction/eviction_manager.go

Lines 524 to 545 in 74b1f5b

    
           func (m *managerImpl) evictPod(pod *v1.Pod, resourceName v1.ResourceName, evictMsg string) bool { 
        
           	if utilfeature.DefaultFeatureGate.Enabled(features.ExperimentalCriticalPodAnnotation) && 
        
           		kubelettypes.IsCriticalPod(pod) && kubepod.IsStaticPod(pod) { 
        
           		glog.Errorf("eviction manager: cannot evict a critical pod %s", format.Pod(pod)) 
        
           		return false 
        
           	} 
        
           	status := v1.PodStatus{ 
        
           		Phase:   v1.PodFailed, 
        
           		Message: fmt.Sprintf(message, resourceName), 
        
           		Reason:  reason, 
        
           	} 
        
           	// record that we are evicting the pod 
        
           	m.recorder.Eventf(pod, v1.EventTypeWarning, reason, evictMsg) 
        
           	gracePeriod := int64(0) 
        
           	err := m.killPodFunc(pod, status, &gracePeriod) 
        
           	if err != nil { 
        
           		glog.Errorf("eviction manager: pod %s failed to evict %v", format.Pod(pod), err) 
        
           	} else { 
        
           		glog.Infof("eviction manager: pod %s is evicted successfully", format.Pod(pod)) 
        
           	} 
        
           	return true 
        
           }

liggitt · 2017-08-02T17:37:34Z

I stand corrected, I'll open a cherry-pick

…7-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #48707 Cherry pick of #48707 on release-1.7. #48707: Make NodeRestriction admission allow evictions for bounded

k8s-cherrypick-bot · 2017-08-04T04:16:39Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 10, 2017

k8s-github-robot assigned deads2k and timstclair Jul 10, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 10, 2017

danielfm changed the title ~~Make NodeRestriction admission create evictions for its own pods~~ Allow nodes to create evictions for its own pods in NodeRestriction admission controller Jul 10, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 11, 2017

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Jul 12, 2017

liggitt reviewed Jul 12, 2017

View reviewed changes

liggitt added the area/security label Jul 12, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2017

liggitt reviewed Jul 12, 2017

View reviewed changes

k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jul 12, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 12, 2017

liggitt reviewed Jul 13, 2017

View reviewed changes

liggitt assigned liggitt and davidopp and unassigned deads2k Jul 15, 2017

Make NodeRestriction admission allow evictions for bounded pods

81ba522

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 23, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2017

k8s-github-robot merged commit 4d2a721 into kubernetes:master Jul 23, 2017

danielfm deleted the node-restriction-pod-eviction-subresource branch July 24, 2017 14:21

liggitt added the cherrypick-candidate label Aug 2, 2017

liggitt added this to the v1.7 milestone Aug 2, 2017

liggitt mentioned this pull request Aug 2, 2017

Automated cherry pick of #48707 #50030

Merged

wojtek-t added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Aug 3, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Aug 4, 2017

danielfm mentioned this pull request Aug 22, 2017

Add support for Node Authorizer + Node Restriction admission controller kubernetes-retired/kube-aws#851

Merged

Allow nodes to create evictions for its own pods in NodeRestriction admission controller #48707

Allow nodes to create evictions for its own pods in NodeRestriction admission controller #48707

Conversation

danielfm commented Jul 10, 2017 • edited by liggitt Loading

k8s-ci-robot commented Jul 10, 2017

danielfm commented Jul 11, 2017 • edited Loading

liggitt commented Jul 12, 2017

liggitt Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Jul 12, 2017

liggitt commented Jul 12, 2017

danielfm commented Jul 12, 2017

liggitt commented Jul 12, 2017

danielfm commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielfm commented Jul 15, 2017

redbaron commented Jul 16, 2017

danielfm commented Jul 18, 2017

redbaron commented Jul 20, 2017

davidopp commented Jul 23, 2017

liggitt commented Jul 23, 2017

k8s-github-robot commented Jul 23, 2017

k8s-github-robot commented Jul 23, 2017

redbaron commented Jul 23, 2017

liggitt commented Jul 24, 2017

redbaron commented Jul 31, 2017

liggitt commented Jul 31, 2017

redbaron commented Aug 2, 2017

liggitt commented Aug 2, 2017

k8s-cherrypick-bot commented Aug 4, 2017

danielfm commented Jul 10, 2017 •

edited by liggitt

Loading

danielfm commented Jul 11, 2017 •

edited

Loading

liggitt Jul 12, 2017 •

edited

Loading