Skip to content

Commit 315dcca

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#53185 from dixudx/fix_admission_attributes_populate_name
populate object name for admission attributes when CREATE
2 parents d54c516 + 2771503 commit 315dcca

File tree

12 files changed

+296
-107
lines changed

12 files changed

+296
-107
lines changed

‎pkg/registry/core/pod/storage/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ go_library(
6464
"//pkg/registry/core/pod/rest:go_default_library",
6565
"//staging/src/k8s.io/api/policy/v1beta1:go_default_library",
6666
"//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library",
67+
"//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library",
6768
"//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
6869
"//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
6970
"//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",

‎pkg/registry/core/pod/storage/eviction.go‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ type EvictionREST struct {
6767
podDisruptionBudgetClient policyclient.PodDisruptionBudgetsGetter
6868
}
6969

70-
var _ = rest.Creater(&EvictionREST{})
70+
var _ = rest.NamedCreater(&EvictionREST{})
7171
var _ = rest.GroupVersionKindProvider(&EvictionREST{})
7272

7373
// GroupVersionKind specifies a particular GroupVersionKind to discovery
@@ -101,8 +101,15 @@ func propagateDryRun(eviction *policy.Eviction, options *metav1.CreateOptions) (
101101
}
102102

103103
// Create attempts to create a new eviction. That is, it tries to evict a pod.
104-
func (r *EvictionREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
105-
eviction := obj.(*policy.Eviction)
104+
func (r *EvictionREST) Create(ctx context.Context, name string, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
105+
eviction, ok := obj.(*policy.Eviction)
106+
if !ok {
107+
return nil, errors.NewBadRequest(fmt.Sprintf("not a Eviction object: %T", obj))
108+
}
109+
110+
if name != eviction.Name {
111+
return nil, errors.NewBadRequest("name in URL does not match name in Eviction object")
112+
}
106113

107114
deletionOptions, err := propagateDryRun(eviction, options)
108115
if err != nil {

‎pkg/registry/core/pod/storage/eviction_test.go‎

Lines changed: 51 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -42,9 +42,10 @@ func TestEviction(t *testing.T) {
4242
testcases := []struct {
4343
name string
4444
pdbs []runtime.Object
45-
pod *api.Pod
4645
eviction *policy.Eviction
4746

47+
badNameInURL bool
48+
4849
expectError bool
4950
expectDeleted bool
5051
}{
@@ -55,12 +56,6 @@ func TestEviction(t *testing.T) {
5556
Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}},
5657
Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 0},
5758
}},
58-
pod: func() *api.Pod {
59-
pod := validNewPod()
60-
pod.Labels = map[string]string{"a": "true"}
61-
pod.Spec.NodeName = "foo"
62-
return pod
63-
}(),
6459
eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)},
6560
expectError: true,
6661
},
@@ -71,12 +66,6 @@ func TestEviction(t *testing.T) {
7166
Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}},
7267
Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 1},
7368
}},
74-
pod: func() *api.Pod {
75-
pod := validNewPod()
76-
pod.Labels = map[string]string{"a": "true"}
77-
pod.Spec.NodeName = "foo"
78-
return pod
79-
}(),
8069
eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)},
8170
expectDeleted: true,
8271
},
@@ -87,15 +76,20 @@ func TestEviction(t *testing.T) {
8776
Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"b": "true"}}},
8877
Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 0},
8978
}},
90-
pod: func() *api.Pod {
91-
pod := validNewPod()
92-
pod.Labels = map[string]string{"a": "true"}
93-
pod.Spec.NodeName = "foo"
94-
return pod
95-
}(),
9679
eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)},
9780
expectDeleted: true,
9881
},
82+
{
83+
name: "matching pdbs with disruptions allowed but bad name in Url",
84+
pdbs: []runtime.Object{&policyv1beta1.PodDisruptionBudget{
85+
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
86+
Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}},
87+
Status: policyv1beta1.PodDisruptionBudgetStatus{PodDisruptionsAllowed: 1},
88+
}},
89+
badNameInURL: true,
90+
eviction: &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: metav1.NewDeleteOptions(0)},
91+
expectError: true,
92+
},
9993
}
10094

10195
for _, tc := range testcases {
@@ -104,43 +98,58 @@ func TestEviction(t *testing.T) {
10498
storage, _, _, server := newStorage(t)
10599
defer server.Terminate(t)
106100
defer storage.Store.DestroyFunc()
107-
if tc.pod != nil {
108-
if _, err := storage.Create(testContext, tc.pod, nil, &metav1.CreateOptions{}); err != nil {
109-
t.Error(err)
110-
}
101+
102+
pod := validNewPod()
103+
pod.Labels = map[string]string{"a": "true"}
104+
pod.Spec.NodeName = "foo"
105+
106+
if _, err := storage.Create(testContext, pod, nil, &metav1.CreateOptions{}); err != nil {
107+
t.Error(err)
111108
}
112109

113110
client := fake.NewSimpleClientset(tc.pdbs...)
114111
evictionRest := newEvictionStorage(storage.Store, client.PolicyV1beta1())
115-
_, err := evictionRest.Create(testContext, tc.eviction, nil, &metav1.CreateOptions{})
112+
113+
name := pod.Name
114+
if tc.badNameInURL {
115+
name += "bad-name"
116+
}
117+
_, err := evictionRest.Create(testContext, name, tc.eviction, nil, &metav1.CreateOptions{})
116118
if (err != nil) != tc.expectError {
117119
t.Errorf("expected error=%v, got %v", tc.expectError, err)
118120
return
119121
}
122+
if tc.badNameInURL {
123+
if err == nil {
124+
t.Error("expected error here, but got nil")
125+
return
126+
}
127+
if err.Error() != "name in URL does not match name in Eviction object" {
128+
t.Errorf("got unexpected error: %v", err)
129+
}
130+
}
120131
if tc.expectError {
121132
return
122133
}
123134

124-
if tc.pod != nil {
125-
existingPod, err := storage.Get(testContext, tc.pod.Name, &metav1.GetOptions{})
126-
if tc.expectDeleted {
127-
if !apierrors.IsNotFound(err) {
128-
t.Errorf("expected to be deleted, lookup returned %#v", existingPod)
129-
}
130-
return
131-
} else if apierrors.IsNotFound(err) {
132-
t.Errorf("expected graceful deletion, got %v", err)
133-
return
135+
existingPod, err := storage.Get(testContext, pod.Name, &metav1.GetOptions{})
136+
if tc.expectDeleted {
137+
if !apierrors.IsNotFound(err) {
138+
t.Errorf("expected to be deleted, lookup returned %#v", existingPod)
134139
}
140+
return
141+
} else if apierrors.IsNotFound(err) {
142+
t.Errorf("expected graceful deletion, got %v", err)
143+
return
144+
}
135145

136-
if err != nil {
137-
t.Errorf("%#v", err)
138-
return
139-
}
146+
if err != nil {
147+
t.Errorf("%#v", err)
148+
return
149+
}
140150

141-
if existingPod.(*api.Pod).DeletionTimestamp == nil {
142-
t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod)
143-
}
151+
if existingPod.(*api.Pod).DeletionTimestamp == nil {
152+
t.Errorf("expected gracefully deleted pod with deletionTimestamp set, got %#v", existingPod)
144153
}
145154
})
146155
}
@@ -186,52 +195,27 @@ func TestEvictionDryRun(t *testing.T) {
186195
name string
187196
evictionOptions *metav1.DeleteOptions
188197
requestOptions *metav1.CreateOptions
189-
pod *api.Pod
190198
pdbs []runtime.Object
191199
}{
192200
{
193201
name: "just request-options",
194202
requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}},
195203
evictionOptions: &metav1.DeleteOptions{},
196-
pod: func() *api.Pod {
197-
pod := validNewPod()
198-
pod.Labels = map[string]string{"a": "true"}
199-
pod.Spec.NodeName = "foo"
200-
return pod
201-
}(),
202204
},
203205
{
204206
name: "just eviction-options",
205207
requestOptions: &metav1.CreateOptions{},
206208
evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}},
207-
pod: func() *api.Pod {
208-
pod := validNewPod()
209-
pod.Labels = map[string]string{"a": "true"}
210-
pod.Spec.NodeName = "foo"
211-
return pod
212-
}(),
213209
},
214210
{
215211
name: "both options",
216212
evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}},
217213
requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}},
218-
pod: func() *api.Pod {
219-
pod := validNewPod()
220-
pod.Labels = map[string]string{"a": "true"}
221-
pod.Spec.NodeName = "foo"
222-
return pod
223-
}(),
224214
},
225215
{
226216
name: "with pdbs",
227217
evictionOptions: &metav1.DeleteOptions{DryRun: []string{"All"}},
228218
requestOptions: &metav1.CreateOptions{DryRun: []string{"All"}},
229-
pod: func() *api.Pod {
230-
pod := validNewPod()
231-
pod.Labels = map[string]string{"a": "true"}
232-
pod.Spec.NodeName = "foo"
233-
return pod
234-
}(),
235219
pdbs: []runtime.Object{&policyv1beta1.PodDisruptionBudget{
236220
ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"},
237221
Spec: policyv1beta1.PodDisruptionBudgetSpec{Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"a": "true"}}},
@@ -257,7 +241,7 @@ func TestEvictionDryRun(t *testing.T) {
257241
client := fake.NewSimpleClientset(tc.pdbs...)
258242
evictionRest := newEvictionStorage(storage.Store, client.PolicyV1beta1())
259243
eviction := &policy.Eviction{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "default"}, DeleteOptions: tc.evictionOptions}
260-
_, err := evictionRest.Create(testContext, eviction, nil, tc.requestOptions)
244+
_, err := evictionRest.Create(testContext, pod.Name, eviction, nil, tc.requestOptions)
261245
if err != nil {
262246
t.Fatalf("Failed to run eviction: %v", err)
263247
}

‎pkg/registry/core/pod/storage/storage.go‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"net/url"
2424

2525
"k8s.io/apimachinery/pkg/api/errors"
26+
"k8s.io/apimachinery/pkg/api/meta"
2627
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2728
"k8s.io/apimachinery/pkg/runtime"
2829
"k8s.io/apiserver/pkg/registry/generic"
@@ -49,6 +50,7 @@ import (
4950
type PodStorage struct {
5051
Pod *REST
5152
Binding *BindingREST
53+
LegacyBinding *LegacyBindingREST
5254
Eviction *EvictionREST
5355
Status *StatusREST
5456
EphemeralContainers *EphemeralContainersREST
@@ -95,9 +97,11 @@ func NewStorage(optsGetter generic.RESTOptionsGetter, k client.ConnectionInfoGet
9597
ephemeralContainersStore := *store
9698
ephemeralContainersStore.UpdateStrategy = pod.EphemeralContainersStrategy
9799

100+
bindingREST := &BindingREST{store: store}
98101
return PodStorage{
99102
Pod: &REST{store, proxyTransport},
100103
Binding: &BindingREST{store: store},
104+
LegacyBinding: &LegacyBindingREST{bindingREST},
101105
Eviction: newEvictionStorage(store, podDisruptionBudgetClient),
102106
Status: &StatusREST{store: &statusStore},
103107
EphemeralContainers: &EphemeralContainersREST{store: &ephemeralContainersStore},
@@ -148,11 +152,18 @@ func (r *BindingREST) New() runtime.Object {
148152
return &api.Binding{}
149153
}
150154

151-
var _ = rest.Creater(&BindingREST{})
155+
var _ = rest.NamedCreater(&BindingREST{})
152156

153157
// Create ensures a pod is bound to a specific host.
154-
func (r *BindingREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
155-
binding := obj.(*api.Binding)
158+
func (r *BindingREST) Create(ctx context.Context, name string, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
159+
binding, ok := obj.(*api.Binding)
160+
if !ok {
161+
return nil, errors.NewBadRequest(fmt.Sprintf("not a Binding object: %#v", obj))
162+
}
163+
164+
if name != binding.Name {
165+
return nil, errors.NewBadRequest("name in URL does not match name in Binding object")
166+
}
156167

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

232+
var _ = rest.Creater(&LegacyBindingREST{})
233+
234+
// LegacyBindingREST implements the REST endpoint for binding pods to nodes when etcd is in use.
235+
type LegacyBindingREST struct {
236+
bindingRest *BindingREST
237+
}
238+
239+
// NamespaceScoped fulfill rest.Scoper
240+
func (r *LegacyBindingREST) NamespaceScoped() bool {
241+
return r.bindingRest.NamespaceScoped()
242+
}
243+
244+
// New creates a new binding resource
245+
func (r *LegacyBindingREST) New() runtime.Object {
246+
return r.bindingRest.New()
247+
}
248+
249+
// Create ensures a pod is bound to a specific host.
250+
func (r *LegacyBindingREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (out runtime.Object, err error) {
251+
metadata, err := meta.Accessor(obj)
252+
if err != nil {
253+
return nil, errors.NewBadRequest(fmt.Sprintf("not a Binding object: %T", obj))
254+
}
255+
return r.bindingRest.Create(ctx, metadata.GetName(), obj, createValidation, options)
256+
}
257+
221258
// StatusREST implements the REST endpoint for changing the status of a pod.
222259
type StatusREST struct {
223260
store *genericregistry.Store

0 commit comments

Comments
 (0)