Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

populate object name for admission attributes when CREATE #53185

Merged
merged 5 commits into from
Aug 22, 2019
Merged

populate object name for admission attributes when CREATE #53185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34cab8f
populate object name for admission attributes when CREATE
dixudx Sep 28, 2017
5ed1b8f
check pod name with that in binding object
dixudx Jun 30, 2019
b28f62c
check pod name with that in pod eviction object
dixudx Jun 30, 2019
d4d696d
add legacyBinding for non-Named Binding Creater
dixudx Jul 1, 2019
2771503
drop unused check
dixudx Aug 9, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/registry/core/pod/storage/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ go_library(
"//pkg/registry/core/pod/rest:go_default_library",
"//staging/src/k8s.io/api/policy/v1beta1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
Expand Down
13 changes: 10 additions & 3 deletions pkg/registry/core/pod/storage/eviction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
liggitt marked this conversation as resolved.
Show resolved Hide resolved
return nil, errors.NewBadRequest("name in URL does not match name in Eviction object")
}

deletionOptions, err := propagateDryRun(eviction, options)
if err != nil {
Expand Down
118 changes: 51 additions & 67 deletions pkg/registry/core/pod/storage/eviction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand All @@ -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,
},
Expand All @@ -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,
},
Expand All @@ -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,
dixudx marked this conversation as resolved.
Show resolved Hide resolved
},
}

for _, tc := range testcases {
Expand All @@ -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)
}
})
}
Expand Down Expand Up @@ -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"}}},
Expand All @@ -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)
}
Expand Down
43 changes: 40 additions & 3 deletions pkg/registry/core/pod/storage/storage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"net/url"

"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/registry/generic"
Expand All @@ -49,6 +50,7 @@ import (
type PodStorage struct {
Pod *REST
Binding *BindingREST
LegacyBinding *LegacyBindingREST
Eviction *EvictionREST
Status *StatusREST
EphemeralContainers *EphemeralContainersREST
Expand Down Expand Up @@ -95,9 +97,11 @@ func NewStorage(optsGetter generic.RESTOptionsGetter, k client.ConnectionInfoGet
ephemeralContainersStore := *store
ephemeralContainersStore.UpdateStrategy = pod.EphemeralContainersStrategy

bindingREST := &BindingREST{store: store}
return PodStorage{
Pod: &REST{store, proxyTransport},
Binding: &BindingREST{store: store},
LegacyBinding: &LegacyBindingREST{bindingREST},
Eviction: newEvictionStorage(store, podDisruptionBudgetClient),
Status: &StatusREST{store: &statusStore},
EphemeralContainers: &EphemeralContainersREST{store: &ephemeralContainersStore},
Expand Down Expand Up @@ -148,11 +152,18 @@ func (r *BindingREST) New() runtime.Object {
return &api.Binding{}
}

var _ = rest.Creater(&BindingREST{})
var _ = rest.NamedCreater(&BindingREST{})

// Create ensures a pod is bound to a specific host.
func (r *BindingREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
binding := obj.(*api.Binding)
func (r *BindingREST) Create(ctx context.Context, name string, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
binding, ok := obj.(*api.Binding)
if !ok {
return nil, errors.NewBadRequest(fmt.Sprintf("not a Binding object: %#v", obj))
}

if name != binding.Name {
return nil, errors.NewBadRequest("name in URL does not match name in Binding object")
}

// TODO: move me to a binding strategy
if errs := validation.ValidatePodBinding(binding); len(errs) != 0 {
Expand Down Expand Up @@ -218,6 +229,32 @@ func (r *BindingREST) assignPod(ctx context.Context, podID string, machine strin
return
}

var _ = rest.Creater(&LegacyBindingREST{})

// LegacyBindingREST implements the REST endpoint for binding pods to nodes when etcd is in use.
type LegacyBindingREST struct {
liggitt marked this conversation as resolved.
Show resolved Hide resolved
bindingRest *BindingREST
}

// NamespaceScoped fulfill rest.Scoper
func (r *LegacyBindingREST) NamespaceScoped() bool {
return r.bindingRest.NamespaceScoped()
}

// New creates a new binding resource
func (r *LegacyBindingREST) New() runtime.Object {
return r.bindingRest.New()
}

// Create ensures a pod is bound to a specific host.
func (r *LegacyBindingREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
metadata, err := meta.Accessor(obj)
if err != nil {
return nil, errors.NewBadRequest(fmt.Sprintf("not a Binding object: %T", obj))
}
return r.bindingRest.Create(ctx, metadata.GetName(), obj, createValidation, options)
}

// StatusREST implements the REST endpoint for changing the status of a pod.
type StatusREST struct {
store *genericregistry.Store
Expand Down
Loading