diff --git a/pkg/registry/core/pod/storage/BUILD b/pkg/registry/core/pod/storage/BUILD index a9501a009e5..419a6cd6040 100644 --- a/pkg/registry/core/pod/storage/BUILD +++ b/pkg/registry/core/pod/storage/BUILD @@ -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", 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) } diff --git a/pkg/registry/core/pod/storage/storage.go b/pkg/registry/core/pod/storage/storage.go index 4947cf3d10d..f005bef0f1e 100644 --- a/pkg/registry/core/pod/storage/storage.go +++ b/pkg/registry/core/pod/storage/storage.go @@ -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" @@ -49,6 +50,7 @@ import ( type PodStorage struct { Pod *REST Binding *BindingREST + LegacyBinding *LegacyBindingREST Eviction *EvictionREST Status *StatusREST EphemeralContainers *EphemeralContainersREST @@ -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}, @@ -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 { @@ -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 { + 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 diff --git a/pkg/registry/core/pod/storage/storage_test.go b/pkg/registry/core/pod/storage/storage_test.go index d8c8e324f7c..545e6362cb9 100644 --- a/pkg/registry/core/pod/storage/storage_test.go +++ b/pkg/registry/core/pod/storage/storage_test.go @@ -587,7 +587,7 @@ func TestEtcdCreate(t *testing.T) { } // Suddenly, a wild scheduler appears: - _, err = bindingStorage.Create(ctx, &api.Binding{ + _, err = bindingStorage.Create(ctx, "foo", &api.Binding{ ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "foo"}, Target: api.ObjectReference{Name: "machine"}, }, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) @@ -613,7 +613,7 @@ func TestEtcdCreateBindingNoPod(t *testing.T) { // - Create (apiserver) // - Schedule (scheduler) // - Delete (apiserver) - _, err := bindingStorage.Create(ctx, &api.Binding{ + _, err := bindingStorage.Create(ctx, "foo", &api.Binding{ ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "foo"}, Target: api.ObjectReference{Name: "machine"}, }, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) @@ -657,7 +657,7 @@ func TestEtcdCreateWithContainersNotFound(t *testing.T) { } // Suddenly, a wild scheduler appears: - _, err = bindingStorage.Create(ctx, &api.Binding{ + _, err = bindingStorage.Create(ctx, "foo", &api.Binding{ ObjectMeta: metav1.ObjectMeta{ Namespace: metav1.NamespaceDefault, Name: "foo", @@ -700,12 +700,12 @@ func TestEtcdCreateWithConflict(t *testing.T) { }, Target: api.ObjectReference{Name: "machine"}, } - _, err = bindingStorage.Create(ctx, &binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + _, err = bindingStorage.Create(ctx, binding.Name, &binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { t.Fatalf("unexpected error: %v", err) } - _, err = bindingStorage.Create(ctx, &binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) + _, err = bindingStorage.Create(ctx, binding.Name, &binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err == nil || !errors.IsConflict(err) { t.Fatalf("expected resource conflict error, not: %v", err) } @@ -722,7 +722,7 @@ func TestEtcdCreateWithExistingContainers(t *testing.T) { } // Suddenly, a wild scheduler appears: - _, err = bindingStorage.Create(ctx, &api.Binding{ + _, err = bindingStorage.Create(ctx, "foo", &api.Binding{ ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "foo"}, Target: api.ObjectReference{Name: "machine"}, }, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) @@ -740,8 +740,9 @@ func TestEtcdCreateBinding(t *testing.T) { ctx := genericapirequest.NewDefaultContext() testCases := map[string]struct { - binding api.Binding - errOK func(error) bool + binding api.Binding + badNameInURL bool + errOK func(error) bool }{ "noName": { binding: api.Binding{ @@ -750,6 +751,14 @@ func TestEtcdCreateBinding(t *testing.T) { }, errOK: func(err error) bool { return err != nil }, }, + "badNameInURL": { + binding: api.Binding{ + ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "foo"}, + Target: api.ObjectReference{}, + }, + badNameInURL: true, + errOK: func(err error) bool { return err != nil }, + }, "badKind": { binding: api.Binding{ ObjectMeta: metav1.ObjectMeta{Namespace: metav1.NamespaceDefault, Name: "foo"}, @@ -778,7 +787,11 @@ func TestEtcdCreateBinding(t *testing.T) { if _, err := storage.Create(ctx, validNewPod(), rest.ValidateAllObjectFunc, &metav1.CreateOptions{}); err != nil { t.Fatalf("%s: unexpected error: %v", k, err) } - if _, err := bindingStorage.Create(ctx, &test.binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}); !test.errOK(err) { + name := test.binding.Name + if test.badNameInURL { + name += "badNameInURL" + } + if _, err := bindingStorage.Create(ctx, name, &test.binding, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}); !test.errOK(err) { t.Errorf("%s: unexpected error: %v", k, err) } else if err == nil { // If bind succeeded, verify Host field in pod's Spec. diff --git a/pkg/registry/core/rest/storage_core.go b/pkg/registry/core/rest/storage_core.go index bd5a56f7a6b..6a00feae183 100644 --- a/pkg/registry/core/rest/storage_core.go +++ b/pkg/registry/core/rest/storage_core.go @@ -248,7 +248,7 @@ func (c LegacyRESTStorageProvider) NewLegacyRESTStorage(restOptionsGetter generi "pods/portforward": podStorage.PortForward, "pods/proxy": podStorage.Proxy, "pods/binding": podStorage.Binding, - "bindings": podStorage.Binding, + "bindings": podStorage.LegacyBinding, "podTemplates": podTemplateStorage, diff --git a/plugin/pkg/admission/noderestriction/admission.go b/plugin/pkg/admission/noderestriction/admission.go index 20596cdab4c..c4870d27a23 100644 --- a/plugin/pkg/admission/noderestriction/admission.go +++ b/plugin/pkg/admission/noderestriction/admission.go @@ -361,11 +361,6 @@ func (p *Plugin) admitNode(nodeName string, a admission.Attributes) error { if forbiddenUpdateLabels := p.getForbiddenUpdateLabels(modifiedLabels); len(forbiddenUpdateLabels) > 0 { klog.Warningf("node %q added disallowed labels on node creation: %s", nodeName, strings.Join(forbiddenUpdateLabels.List(), ", ")) } - - // On create, get name from new object if unset in admission - if len(requestedName) == 0 { - requestedName = node.Name - } } if requestedName != nodeName { return admission.NewForbidden(a, fmt.Errorf("node %q is not allowed to modify node %q", nodeName, requestedName)) diff --git a/plugin/pkg/admission/noderestriction/admission_test.go b/plugin/pkg/admission/noderestriction/admission_test.go index 656fe363db3..6e009aa4d82 100644 --- a/plugin/pkg/admission/noderestriction/admission_test.go +++ b/plugin/pkg/admission/noderestriction/admission_test.go @@ -826,19 +826,19 @@ func Test_nodePlugin_Admit(t *testing.T) { { name: "allow create of my node pulling name from object", podsGetter: noExistingPods, - attributes: admission.NewAttributesRecord(mynodeObj, nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), + attributes: admission.NewAttributesRecord(mynodeObj, nil, nodeKind, mynodeObj.Namespace, "mynode", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), err: "", }, { name: "allow create of my node with taints", podsGetter: noExistingPods, - attributes: admission.NewAttributesRecord(mynodeObjTaintA, nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), + attributes: admission.NewAttributesRecord(mynodeObjTaintA, nil, nodeKind, mynodeObj.Namespace, "mynode", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), err: "", }, { name: "allow create of my node with labels", podsGetter: noExistingPods, - attributes: admission.NewAttributesRecord(setAllowedCreateLabels(mynodeObj, ""), nil, nodeKind, mynodeObj.Namespace, "", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), + attributes: admission.NewAttributesRecord(setAllowedCreateLabels(mynodeObj, ""), nil, nodeKind, mynodeObj.Namespace, "mynode", nodeResource, "", admission.Create, &metav1.CreateOptions{}, false, mynode), err: "", }, { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go index dd6a8224e5e..cddd43344db 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/apiserver_test.go @@ -3665,7 +3665,7 @@ func TestParentResourceIsRequired(t *testing.T) { } } -func TestCreateWithName(t *testing.T) { +func TestNamedCreaterWithName(t *testing.T) { pathName := "helloworld" storage := &NamedCreaterRESTStorage{SimpleRESTStorage: &SimpleRESTStorage{}} handler := handle(map[string]rest.Storage{ @@ -3697,6 +3697,145 @@ func TestCreateWithName(t *testing.T) { } } +func TestNamedCreaterWithoutName(t *testing.T) { + storage := &NamedCreaterRESTStorage{ + SimpleRESTStorage: &SimpleRESTStorage{ + injectedFunction: func(obj runtime.Object) (runtime.Object, error) { + time.Sleep(5 * time.Millisecond) + return obj, nil + }, + }, + } + + selfLinker := &setTestSelfLinker{ + t: t, + name: "bar", + namespace: "default", + expectedSet: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/foo", + } + handler := handleLinker(map[string]rest.Storage{"foo": storage}, selfLinker) + server := httptest.NewServer(handler) + defer server.Close() + client := http.Client{} + + simple := &genericapitesting.Simple{ + Other: "bar", + } + data, err := runtime.Encode(testCodec, simple) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/foo", bytes.NewBuffer(data)) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + wg := sync.WaitGroup{} + wg.Add(1) + var response *http.Response + go func() { + response, err = client.Do(request) + wg.Done() + }() + wg.Wait() + if err != nil { + t.Errorf("unexpected error: %v", err) + } + // empty name is not allowed for NamedCreater + if response.StatusCode != http.StatusBadRequest { + t.Errorf("Unexpected response %#v", response) + } +} + +type namePopulatorAdmissionControl struct { + t *testing.T + populateName string +} + +func (npac *namePopulatorAdmissionControl) Validate(a admission.Attributes, o admission.ObjectInterfaces) (err error) { + if a.GetName() != npac.populateName { + npac.t.Errorf("Unexpected name: got %q, expected %q", a.GetName(), npac.populateName) + } + return nil +} + +func (npac *namePopulatorAdmissionControl) Handles(operation admission.Operation) bool { + return true +} + +func TestNamedCreaterWithGenerateName(t *testing.T) { + populateName := "bar" + storage := &SimpleRESTStorage{ + injectedFunction: func(obj runtime.Object) (runtime.Object, error) { + time.Sleep(5 * time.Millisecond) + if metadata, err := meta.Accessor(obj); err == nil { + if len(metadata.GetName()) != 0 { + t.Errorf("Unexpected name %q", metadata.GetName()) + } + metadata.SetName(populateName) + } else { + return nil, err + } + return obj, nil + }, + } + + selfLinker := &setTestSelfLinker{ + t: t, + namespace: "default", + expectedSet: "/" + prefix + "/" + testGroupVersion.Group + "/" + testGroupVersion.Version + "/namespaces/default/foo", + } + + ac := &namePopulatorAdmissionControl{ + t: t, + populateName: populateName, + } + + handler := handleInternal(map[string]rest.Storage{"foo": storage}, ac, selfLinker, nil) + server := httptest.NewServer(handler) + defer server.Close() + client := http.Client{} + + simple := &genericapitesting.Simple{ + Other: "bar", + } + data, err := runtime.Encode(testCodec, simple) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + request, err := http.NewRequest("POST", server.URL+"/"+prefix+"/"+testGroupVersion.Group+"/"+testGroupVersion.Version+"/namespaces/default/foo", bytes.NewBuffer(data)) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + wg := sync.WaitGroup{} + wg.Add(1) + var response *http.Response + go func() { + response, err = client.Do(request) + wg.Done() + }() + wg.Wait() + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if response.StatusCode != http.StatusCreated { + t.Errorf("Unexpected status: %d, Expected: %d, %#v", response.StatusCode, http.StatusOK, response) + } + + var itemOut genericapitesting.Simple + body, err := extractBody(response, &itemOut) + if err != nil { + t.Errorf("unexpected error: %v %#v", err, response) + } + + itemOut.GetObjectKind().SetGroupVersionKind(schema.GroupVersionKind{}) + simple.Name = populateName + if !reflect.DeepEqual(&itemOut, simple) { + t.Errorf("Unexpected data: %#v, expected %#v (%s)", itemOut, simple, string(body)) + } +} + func TestUpdateChecksDecode(t *testing.T) { handler := handle(map[string]rest.Storage{"simple": &SimpleRESTStorage{}}) server := httptest.NewServer(handler) @@ -3879,6 +4018,7 @@ func TestCreateYAML(t *testing.T) { t.Errorf("Never set self link") } } + func TestCreateInNamespace(t *testing.T) { storage := SimpleRESTStorage{ injectedFunction: func(obj runtime.Object) (runtime.Object, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go index 8f1201c28f8..6dc143a7b06 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go @@ -57,18 +57,20 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int // TODO: we either want to remove timeout or document it (if we document, move timeout out of this function and declare it in api_installer) timeout := parseTimeout(req.URL.Query().Get("timeout")) - var ( - namespace, name string - err error - ) - if includeName { - namespace, name, err = scope.Namer.Name(req) - } else { - namespace, err = scope.Namer.Namespace(req) - } + namespace, name, err := scope.Namer.Name(req) if err != nil { - scope.err(err, w, req) - return + if includeName { + // name was required, return + scope.err(err, w, req) + return + } + + // otherwise attempt to look up the namespace + namespace, err = scope.Namer.Namespace(req) + if err != nil { + scope.err(err, w, req) + return + } } ctx, cancel := context.WithTimeout(req.Context(), timeout) @@ -130,6 +132,11 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int audit.LogRequestObject(ae, obj, scope.Resource, scope.Subresource, scope.Serializer) userInfo, _ := request.UserFrom(ctx) + + // On create, get name from new object if unset + if len(name) == 0 { + _, name, _ = scope.Namer.ObjectName(obj) + } admissionAttributes := admission.NewAttributesRecord(obj, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, options, dryrun.IsDryRun(options.DryRun), userInfo) if mutatingAdmission, ok := admit.(admission.MutationInterface); ok && mutatingAdmission.Handles(admission.Create) { err = mutatingAdmission.Admit(ctx, admissionAttributes, scope) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go index e5ae7562daf..dd70a4eb780 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/create.go @@ -163,12 +163,20 @@ func AdmissionToValidateObjectFunc(admit admission.Interface, staticAttributes a return func(ctx context.Context, obj runtime.Object) error { return nil } } return func(ctx context.Context, obj runtime.Object) error { + name := staticAttributes.GetName() + // in case the generated name is populated + if len(name) == 0 { + if metadata, err := meta.Accessor(obj); err == nil { + name = metadata.GetName() + } + } + finalAttributes := admission.NewAttributesRecord( obj, staticAttributes.GetOldObject(), staticAttributes.GetKind(), staticAttributes.GetNamespace(), - staticAttributes.GetName(), + name, staticAttributes.GetResource(), staticAttributes.GetSubresource(), staticAttributes.GetOperation(), diff --git a/test/integration/apiserver/admissionwebhook/admission_test.go b/test/integration/apiserver/admissionwebhook/admission_test.go index 27a70e15318..688dd6e3fff 100644 --- a/test/integration/apiserver/admissionwebhook/admission_test.go +++ b/test/integration/apiserver/admissionwebhook/admission_test.go @@ -295,9 +295,6 @@ func (h *holder) record(version string, phase string, converted bool, request *a } name := request.Name - if name == "" && request.Object.Object != nil { - name = request.Object.Object.(*unstructured.Unstructured).GetName() - } if name != h.recordName { if debug { h.t.Log(name, "!=", h.recordName)