From b28f62c8ad88b45d9d2323166673b595918849e0 Mon Sep 17 00:00:00 2001 From: Di Xu Date: Sun, 30 Jun 2019 20:37:45 +0800 Subject: [PATCH] check pod name with that in pod eviction object --- pkg/registry/core/pod/storage/eviction.go | 13 +- .../core/pod/storage/eviction_test.go | 118 ++++++++---------- 2 files changed, 61 insertions(+), 70 deletions(-) diff --git a/pkg/registry/core/pod/storage/eviction.go b/pkg/registry/core/pod/storage/eviction.go index 0b20125203b..d40c739fb7f 100644 --- a/pkg/registry/core/pod/storage/eviction.go +++ b/pkg/registry/core/pod/storage/eviction.go @@ -67,7 +67,7 @@ type EvictionREST struct { podDisruptionBudgetClient policyclient.PodDisruptionBudgetsGetter } -var _ = rest.Creater(&EvictionREST{}) +var _ = rest.NamedCreater(&EvictionREST{}) var _ = rest.GroupVersionKindProvider(&EvictionREST{}) // GroupVersionKind specifies a particular GroupVersionKind to discovery @@ -101,8 +101,15 @@ func propagateDryRun(eviction *policy.Eviction, options *metav1.CreateOptions) ( } // Create attempts to create a new eviction. That is, it tries to evict a pod. -func (r *EvictionREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { - eviction := obj.(*policy.Eviction) +func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + eviction, ok := obj.(*policy.Eviction) + if !ok { + return nil, errors.NewBadRequest(fmt.Sprintf("not a Eviction object: %T", obj)) + } + + if name != eviction.Name { + return nil, errors.NewBadRequest("name in URL does not match name in Eviction object") + } deletionOptions, err := propagateDryRun(eviction, options) if err != nil { diff --git a/pkg/registry/core/pod/storage/eviction_test.go b/pkg/registry/core/pod/storage/eviction_test.go index c6890291e45..8a6f24a3d08 100644 --- a/pkg/registry/core/pod/storage/eviction_test.go +++ b/pkg/registry/core/pod/storage/eviction_test.go @@ -42,9 +42,10 @@ func TestEviction(t *testing.T) { testcases := []struct { name string pdbs []runtime.Object - pod *api.Pod eviction *policy.Eviction + badNameInURL bool + expectError bool expectDeleted bool }{ @@ -55,12 +56,6 @@ func TestEviction(t *testing.T) { Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 0}, }}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, expectError: true, }, @@ -71,12 +66,6 @@ func TestEviction(t *testing.T) { Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 1}, }}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, expectDeleted: true, }, @@ -87,15 +76,20 @@ func TestEviction(t *testing.T) { Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"b": "true"}}}, Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 0}, }}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, expectDeleted: true, }, + { + name: "matching pdbs with disruptions allowed but bad name in Url", + pdbs: []runtime.Object{&policyv1beta1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, + Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, + Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 1}, + }}, + badNameInURL: true, + eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)}, + expectError: true, + }, } for _, tc := range testcases { @@ -104,43 +98,58 @@ func TestEviction(t *testing.T) { storage, _, _, server := newStorage(t) defer server.Terminate(t) defer storage.Store.DestroyFunc() - if tc.pod != nil { - if _, err := storage.Create(testContext, tc.pod, nil, &metav1.CreateOptions{}); err != nil { - t.Error(err) - } + + pod := validNewPod() + pod.Labels = map[string]string{"a": "true"} + pod.Spec.NodeName = "foo" + + if _, err := storage.Create(testContext, pod, nil, &metav1.CreateOptions{}); err != nil { + t.Error(err) } client := fake.NewSimpleClientset(tc.pdbs...) evictionRest := newEvictionStorage(storage.Store, client.PolicyV1beta1()) - _, err := evictionRest.Create(testContext, tc.eviction, nil, &metav1.CreateOptions{}) + + name := pod.Name + if tc.badNameInURL { + name += "bad-name" + } + _, err := evictionRest.Create(testContext, name, tc.eviction, nil, &metav1.CreateOptions{}) if (err != nil) != tc.expectError { t.Errorf("expected error=%v, got %v", tc.expectError, err) return } + if tc.badNameInURL { + if err == nil { + t.Error("expected error here, but got nil") + return + } + if err.Error() != "name in URL does not match name in Eviction object" { + t.Errorf("got unexpected error: %v", err) + } + } if tc.expectError { return } - if tc.pod != nil { - existingPod, err := storage.Get(testContext, tc.pod.Name, &metav1.GetOptions{}) - if tc.expectDeleted { - if !apierrors.IsNotFound(err) { - t.Errorf("expected to be deleted, lookup returned %#v", existingPod) - } - return - } else if apierrors.IsNotFound(err) { - t.Errorf("expected graceful deletion, got %v", err) - return + existingPod, err := storage.Get(testContext, pod.Name, &metav1.GetOptions{}) + if tc.expectDeleted { + if !apierrors.IsNotFound(err) { + t.Errorf("expected to be deleted, lookup returned %#v", existingPod) } + return + } else if apierrors.IsNotFound(err) { + t.Errorf("expected graceful deletion, got %v", err) + return + } - if err != nil { - t.Errorf("%#v", err) - return - } + if err != nil { + t.Errorf("%#v", err) + return + } - if existingPod.(*api.Pod).DeletionTimestamp == nil { - t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod) - } + if existingPod.(*api.Pod).DeletionTimestamp == nil { + t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod) } }) } @@ -186,52 +195,27 @@ func TestEvictionDryRun(t *testing.T) { name string evictionOptions *metav1.DeleteOptions requestOptions *metav1.CreateOptions - pod *api.Pod pdbs []runtime.Object }{ { name: "just request-options", requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}}, evictionOptions: &metav1.DeleteOptions{}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), }, { name: "just eviction-options", requestOptions: &metav1.CreateOptions{}, evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), }, { name: "both options", evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}}, requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), }, { name: "with pdbs", evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}}, requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}}, - pod: func() *api.Pod { - pod := validNewPod() - pod.Labels = map[string]string{"a": "true"} - pod.Spec.NodeName = "foo" - return pod - }(), pdbs: []runtime.Object{&policyv1beta1.PodDisruptionBudget{ ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}}, @@ -257,7 +241,7 @@ func TestEvictionDryRun(t *testing.T) { client := fake.NewSimpleClientset(tc.pdbs...) evictionRest := newEvictionStorage(storage.Store, client.PolicyV1beta1()) eviction := &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: tc.evictionOptions} - _, err := evictionRest.Create(testContext, eviction, nil, tc.requestOptions) + _, err := evictionRest.Create(testContext, pod.Name, eviction, nil, tc.requestOptions) if err != nil { t.Fatalf("Failed to run eviction: %v", err) }