From 81dcd8c836b586ee20f7ba3b6815fc5344614b2b Mon Sep 17 00:00:00 2001 From: derekwaynecarr Date: Tue, 14 Apr 2015 18:03:26 -0400 Subject: [PATCH] Improve error message when name is omitted but generateName is available --- pkg/admission/attributes.go | 8 +++- pkg/admission/errors.go | 48 +++++++++++++++++++ pkg/admission/interfaces.go | 1 + pkg/api/meta/interfaces.go | 5 ++ pkg/api/meta/meta.go | 35 ++++++++++++++ pkg/api/meta/meta_test.go | 33 +++++++++++++ pkg/apiserver/resthandler.go | 8 ++-- plugin/pkg/admission/deny/admission.go | 3 +- plugin/pkg/admission/deny/admission_test.go | 2 +- plugin/pkg/admission/limitranger/admission.go | 12 +++-- .../namespace/autoprovision/admission.go | 8 ++-- .../namespace/autoprovision/admission_test.go | 8 ++-- .../admission/namespace/exists/admission.go | 14 ++---- .../namespace/lifecycle/admission.go | 14 ++---- .../namespace/lifecycle/admission_test.go | 8 ++-- .../pkg/admission/resourcequota/admission.go | 36 +++++--------- .../admission/resourcequota/admission_test.go | 22 ++++----- 17 files changed, 184 insertions(+), 81 deletions(-) create mode 100644 pkg/admission/errors.go diff --git a/pkg/admission/attributes.go b/pkg/admission/attributes.go index b4eac199e0d..12d70fd48ac 100644 --- a/pkg/admission/attributes.go +++ b/pkg/admission/attributes.go @@ -21,14 +21,16 @@ import ( ) type attributesRecord struct { + kind string namespace string resource string operation string object runtime.Object } -func NewAttributesRecord(object runtime.Object, namespace, resource, operation string) Attributes { +func NewAttributesRecord(object runtime.Object, kind, namespace, resource, operation string) Attributes { return &attributesRecord{ + kind: kind, namespace: namespace, resource: resource, operation: operation, @@ -36,6 +38,10 @@ func NewAttributesRecord(object runtime.Object, namespace, resource, operation s } } +func (record *attributesRecord) GetKind() string { + return record.kind +} + func (record *attributesRecord) GetNamespace() string { return record.namespace } diff --git a/pkg/admission/errors.go b/pkg/admission/errors.go new file mode 100644 index 00000000000..f9bb5210705 --- /dev/null +++ b/pkg/admission/errors.go @@ -0,0 +1,48 @@ +/* +Copyright 2015 Google Inc. All rights reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package admission + +import ( + "github.com/GoogleCloudPlatform/kubernetes/pkg/api" + apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" +) + +// NewForbidden is a utility function to return a well-formatted admission control error response +func NewForbidden(a Attributes, internalError error) error { + // do not double wrap an error of same type + if apierrors.IsForbidden(internalError) { + return internalError + } + + name := "Unknown" + kind := a.GetKind() + obj := a.GetObject() + if obj != nil { + objectMeta, err := api.ObjectMetaFor(obj) + if err != nil { + return apierrors.NewForbidden(kind, name, err) + } + + // this is necessary because name object name generation has not occurred yet + if len(objectMeta.Name) > 0 { + name = objectMeta.Name + } else if len(objectMeta.GenerateName) > 0 { + name = objectMeta.GenerateName + } + } + return apierrors.NewForbidden(kind, name, internalError) +} diff --git a/pkg/admission/interfaces.go b/pkg/admission/interfaces.go index af5d8b877a8..3850ea2d87e 100644 --- a/pkg/admission/interfaces.go +++ b/pkg/admission/interfaces.go @@ -27,6 +27,7 @@ type Attributes interface { GetResource() string GetOperation() string GetObject() runtime.Object + GetKind() string } // Interface is an abstract, pluggable interface for Admission Control decisions. diff --git a/pkg/api/meta/interfaces.go b/pkg/api/meta/interfaces.go index 64d17ffd7d7..8d471a9a69a 100644 --- a/pkg/api/meta/interfaces.go +++ b/pkg/api/meta/interfaces.go @@ -40,6 +40,8 @@ type Interface interface { SetNamespace(namespace string) Name() string SetName(name string) + GenerateName() string + SetGenerateName(name string) UID() types.UID SetUID(uid types.UID) ResourceVersion() string @@ -79,6 +81,9 @@ type MetadataAccessor interface { Name(obj runtime.Object) (string, error) SetName(obj runtime.Object, name string) error + GenerateName(obj runtime.Object) (string, error) + SetGenerateName(obj runtime.Object, name string) error + UID(obj runtime.Object) (types.UID, error) SetUID(obj runtime.Object, uid types.UID) error diff --git a/pkg/api/meta/meta.go b/pkg/api/meta/meta.go index 6886fa3e430..898dec1cae7 100644 --- a/pkg/api/meta/meta.go +++ b/pkg/api/meta/meta.go @@ -178,6 +178,23 @@ func (resourceAccessor) SetName(obj runtime.Object, name string) error { return nil } +func (resourceAccessor) GenerateName(obj runtime.Object) (string, error) { + accessor, err := Accessor(obj) + if err != nil { + return "", err + } + return accessor.GenerateName(), nil +} + +func (resourceAccessor) SetGenerateName(obj runtime.Object, name string) error { + accessor, err := Accessor(obj) + if err != nil { + return err + } + accessor.SetGenerateName(name) + return nil +} + func (resourceAccessor) UID(obj runtime.Object) (types.UID, error) { accessor, err := Accessor(obj) if err != nil { @@ -268,6 +285,7 @@ func (resourceAccessor) SetResourceVersion(obj runtime.Object, version string) e type genericAccessor struct { namespace *string name *string + generateName *string uid *types.UID apiVersion *string kind *string @@ -305,6 +323,20 @@ func (a genericAccessor) SetName(name string) { *a.name = name } +func (a genericAccessor) GenerateName() string { + if a.generateName == nil { + return "" + } + return *a.generateName +} + +func (a genericAccessor) SetGenerateName(generateName string) { + if a.generateName == nil { + return + } + *a.generateName = generateName +} + func (a genericAccessor) UID() types.UID { if a.uid == nil { return "" @@ -392,6 +424,9 @@ func extractFromObjectMeta(v reflect.Value, a *genericAccessor) error { if err := runtime.FieldPtr(v, "Name", &a.name); err != nil { return err } + if err := runtime.FieldPtr(v, "GenerateName", &a.generateName); err != nil { + return err + } if err := runtime.FieldPtr(v, "UID", &a.uid); err != nil { return err } diff --git a/pkg/api/meta/meta_test.go b/pkg/api/meta/meta_test.go index b357fca60d8..094e13bef2b 100644 --- a/pkg/api/meta/meta_test.go +++ b/pkg/api/meta/meta_test.go @@ -29,6 +29,7 @@ func TestGenericTypeMeta(t *testing.T) { Kind string `json:"kind,omitempty"` Namespace string `json:"namespace,omitempty"` Name string `json:"name,omitempty"` + GenerateName string `json:"generateName,omitempty"` UID string `json:"uid,omitempty"` CreationTimestamp util.Time `json:"creationTimestamp,omitempty"` SelfLink string `json:"selfLink,omitempty"` @@ -44,6 +45,7 @@ func TestGenericTypeMeta(t *testing.T) { TypeMeta{ Namespace: "bar", Name: "foo", + GenerateName: "prefix", UID: "uid", APIVersion: "a", Kind: "b", @@ -63,6 +65,9 @@ func TestGenericTypeMeta(t *testing.T) { if e, a := "foo", accessor.Name(); e != a { t.Errorf("expected %v, got %v", e, a) } + if e, a := "prefix", accessor.GenerateName(); e != a { + t.Errorf("expected %v, got %v", e, a) + } if e, a := "uid", string(accessor.UID()); e != a { t.Errorf("expected %v, got %v", e, a) } @@ -92,6 +97,7 @@ func TestGenericTypeMeta(t *testing.T) { accessor.SetNamespace("baz") accessor.SetName("bar") + accessor.SetGenerateName("generate") accessor.SetUID("other") accessor.SetAPIVersion("c") accessor.SetKind("d") @@ -105,6 +111,9 @@ func TestGenericTypeMeta(t *testing.T) { if e, a := "bar", j.Name; e != a { t.Errorf("expected %v, got %v", e, a) } + if e, a := "generate", j.GenerateName; e != a { + t.Errorf("expected %v, got %v", e, a) + } if e, a := "other", j.UID; e != a { t.Errorf("expected %v, got %v", e, a) } @@ -135,6 +144,7 @@ type InternalTypeMeta struct { Kind string `json:"kind,omitempty"` Namespace string `json:"namespace,omitempty"` Name string `json:"name,omitempty"` + GenerateName string `json:"generateName,omitempty"` UID string `json:"uid,omitempty"` CreationTimestamp util.Time `json:"creationTimestamp,omitempty"` SelfLink string `json:"selfLink,omitempty"` @@ -154,6 +164,7 @@ func TestGenericTypeMetaAccessor(t *testing.T) { InternalTypeMeta{ Namespace: "bar", Name: "foo", + GenerateName: "prefix", UID: "uid", APIVersion: "a", Kind: "b", @@ -178,6 +189,13 @@ func TestGenericTypeMetaAccessor(t *testing.T) { if e, a := "foo", name; e != a { t.Errorf("expected %v, got %v", e, a) } + generateName, err := accessor.GenerateName(j) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + if e, a := "prefix", generateName; e != a { + t.Errorf("expected %v, got %v", e, a) + } uid, err := accessor.UID(j) if err != nil { t.Errorf("unexpected error: %v", err) @@ -234,6 +252,9 @@ func TestGenericTypeMetaAccessor(t *testing.T) { if err := accessor.SetName(j, "bar"); err != nil { t.Errorf("unexpected error: %v", err) } + if err := accessor.SetGenerateName(j, "generate"); err != nil { + t.Errorf("unexpected error: %v", err) + } if err := accessor.SetUID(j, "other"); err != nil { t.Errorf("unexpected error: %v", err) } @@ -264,6 +285,9 @@ func TestGenericTypeMetaAccessor(t *testing.T) { if e, a := "bar", j.TypeMeta.Name; e != a { t.Errorf("expected %v, got %v", e, a) } + if e, a := "generate", j.TypeMeta.GenerateName; e != a { + t.Errorf("expected %v, got %v", e, a) + } if e, a := "other", j.TypeMeta.UID; e != a { t.Errorf("expected %v, got %v", e, a) } @@ -295,6 +319,7 @@ func TestGenericObjectMeta(t *testing.T) { type ObjectMeta struct { Namespace string `json:"namespace,omitempty"` Name string `json:"name,omitempty"` + GenerateName string `json:"generateName,omitempty"` UID string `json:"uid,omitempty"` CreationTimestamp util.Time `json:"creationTimestamp,omitempty"` SelfLink string `json:"selfLink,omitempty"` @@ -314,6 +339,7 @@ func TestGenericObjectMeta(t *testing.T) { ObjectMeta{ Namespace: "bar", Name: "foo", + GenerateName: "prefix", UID: "uid", ResourceVersion: "1", SelfLink: "some/place/only/we/know", @@ -331,6 +357,9 @@ func TestGenericObjectMeta(t *testing.T) { if e, a := "foo", accessor.Name(); e != a { t.Errorf("expected %v, got %v", e, a) } + if e, a := "prefix", accessor.GenerateName(); e != a { + t.Errorf("expected %v, got %v", e, a) + } if e, a := "uid", string(accessor.UID()); e != a { t.Errorf("expected %v, got %v", e, a) } @@ -355,6 +384,7 @@ func TestGenericObjectMeta(t *testing.T) { accessor.SetNamespace("baz") accessor.SetName("bar") + accessor.SetGenerateName("generate") accessor.SetUID("other") accessor.SetAPIVersion("c") accessor.SetKind("d") @@ -370,6 +400,9 @@ func TestGenericObjectMeta(t *testing.T) { if e, a := "bar", j.Name; e != a { t.Errorf("expected %v, got %v", e, a) } + if e, a := "generate", j.GenerateName; e != a { + t.Errorf("expected %v, got %v", e, a) + } if e, a := "other", j.UID; e != a { t.Errorf("expected %v, got %v", e, a) } diff --git a/pkg/apiserver/resthandler.go b/pkg/apiserver/resthandler.go index e92f7b30a83..26cd92384cc 100644 --- a/pkg/apiserver/resthandler.go +++ b/pkg/apiserver/resthandler.go @@ -216,7 +216,7 @@ func CreateResource(r rest.Creater, scope RequestScope, typer runtime.ObjectType return } - err = admit.Admit(admission.NewAttributesRecord(obj, namespace, scope.Resource, "CREATE")) + err = admit.Admit(admission.NewAttributesRecord(obj, scope.Kind, namespace, scope.Resource, "CREATE")) if err != nil { errorJSON(err, scope.Codec, w) return @@ -262,7 +262,7 @@ func PatchResource(r rest.Patcher, scope RequestScope, typer runtime.ObjectTyper obj := r.New() // PATCH requires same permission as UPDATE - err = admit.Admit(admission.NewAttributesRecord(obj, namespace, scope.Resource, "UPDATE")) + err = admit.Admit(admission.NewAttributesRecord(obj, scope.Kind, namespace, scope.Resource, "UPDATE")) if err != nil { errorJSON(err, scope.Codec, w) return @@ -362,7 +362,7 @@ func UpdateResource(r rest.Updater, scope RequestScope, typer runtime.ObjectType return } - err = admit.Admit(admission.NewAttributesRecord(obj, namespace, scope.Resource, "UPDATE")) + err = admit.Admit(admission.NewAttributesRecord(obj, scope.Kind, namespace, scope.Resource, "UPDATE")) if err != nil { errorJSON(err, scope.Codec, w) return @@ -423,7 +423,7 @@ func DeleteResource(r rest.GracefulDeleter, checkBody bool, scope RequestScope, } } - err = admit.Admit(admission.NewAttributesRecord(nil, namespace, scope.Resource, "DELETE")) + err = admit.Admit(admission.NewAttributesRecord(nil, scope.Kind, namespace, scope.Resource, "DELETE")) if err != nil { errorJSON(err, scope.Codec, w) return diff --git a/plugin/pkg/admission/deny/admission.go b/plugin/pkg/admission/deny/admission.go index 203cc4f12b7..dec629b7f19 100644 --- a/plugin/pkg/admission/deny/admission.go +++ b/plugin/pkg/admission/deny/admission.go @@ -21,7 +21,6 @@ import ( "io" "github.com/GoogleCloudPlatform/kubernetes/pkg/admission" - apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" ) @@ -36,7 +35,7 @@ func init() { type alwaysDeny struct{} func (alwaysDeny) Admit(a admission.Attributes) (err error) { - return apierrors.NewForbidden(a.GetResource(), "", errors.New("Admission control is denying all modifications")) + return admission.NewForbidden(a, errors.New("Admission control is denying all modifications")) } func NewAlwaysDeny() admission.Interface { diff --git a/plugin/pkg/admission/deny/admission_test.go b/plugin/pkg/admission/deny/admission_test.go index 50e25971319..56caf133369 100644 --- a/plugin/pkg/admission/deny/admission_test.go +++ b/plugin/pkg/admission/deny/admission_test.go @@ -24,7 +24,7 @@ import ( func TestAdmission(t *testing.T) { handler := NewAlwaysDeny() - err := handler.Admit(admission.NewAttributesRecord(nil, "foo", "Pod", "ignored")) + err := handler.Admit(admission.NewAttributesRecord(nil, "Pod", "foo", "Pod", "ignored")) if err == nil { t.Errorf("Expected error returned from admission handler") } diff --git a/plugin/pkg/admission/limitranger/admission.go b/plugin/pkg/admission/limitranger/admission.go index a9182988f82..584a7fe346e 100644 --- a/plugin/pkg/admission/limitranger/admission.go +++ b/plugin/pkg/admission/limitranger/admission.go @@ -22,7 +22,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/admission" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" @@ -58,6 +57,9 @@ func (l *limitRanger) Admit(a admission.Attributes) (err error) { name := "Unknown" if obj != nil { name, _ = meta.NewAccessor().Name(obj) + if len(name) == 0 { + name, _ = meta.NewAccessor().GenerateName(obj) + } } key := &api.LimitRange{ @@ -68,7 +70,7 @@ func (l *limitRanger) Admit(a admission.Attributes) (err error) { } items, err := l.indexer.Index("namespace", key) if err != nil { - return apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing limit ranges", a.GetOperation(), resource)) + return admission.NewForbidden(a, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing limit ranges", a.GetOperation(), resource)) } if len(items) == 0 { return nil @@ -79,7 +81,7 @@ func (l *limitRanger) Admit(a admission.Attributes) (err error) { limitRange := items[i].(*api.LimitRange) err = l.limitFunc(limitRange, a.GetResource(), a.GetObject()) if err != nil { - return err + return admission.NewForbidden(a, err) } } return nil @@ -250,11 +252,11 @@ func PodLimitFunc(limitRange *api.LimitRange, pod *api.Pod) error { switch minOrMax { case "Min": if observed < enforced { - return apierrors.NewForbidden("pods", pod.Name, err) + return err } case "Max": if observed > enforced { - return apierrors.NewForbidden("pods", pod.Name, err) + return err } } } diff --git a/plugin/pkg/admission/namespace/autoprovision/admission.go b/plugin/pkg/admission/namespace/autoprovision/admission.go index 86747ed513b..6395260bea5 100644 --- a/plugin/pkg/admission/namespace/autoprovision/admission.go +++ b/plugin/pkg/admission/namespace/autoprovision/admission.go @@ -53,11 +53,11 @@ func (p *provision) Admit(a admission.Attributes) (err error) { } defaultVersion, kind, err := latest.RESTMapper.VersionAndKindForResource(a.GetResource()) if err != nil { - return err + return admission.NewForbidden(a, err) } mapping, err := latest.RESTMapper.RESTMapping(kind, defaultVersion) if err != nil { - return err + return admission.NewForbidden(a, err) } if mapping.Scope.Name() != meta.RESTScopeNameNamespace { return nil @@ -71,14 +71,14 @@ func (p *provision) Admit(a admission.Attributes) (err error) { } _, exists, err := p.store.Get(namespace) if err != nil { - return err + return admission.NewForbidden(a, err) } if exists { return nil } _, err = p.client.Namespaces().Create(namespace) if err != nil && !errors.IsAlreadyExists(err) { - return err + return admission.NewForbidden(a, err) } return nil } diff --git a/plugin/pkg/admission/namespace/autoprovision/admission_test.go b/plugin/pkg/admission/namespace/autoprovision/admission_test.go index e4fda499146..78dd6d939b6 100644 --- a/plugin/pkg/admission/namespace/autoprovision/admission_test.go +++ b/plugin/pkg/admission/namespace/autoprovision/admission_test.go @@ -41,7 +41,7 @@ func TestAdmission(t *testing.T) { Containers: []api.Container{{Name: "ctr", Image: "image"}}, }, } - err := handler.Admit(admission.NewAttributesRecord(&pod, namespace, "pods", "CREATE")) + err := handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespace, "pods", "CREATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler") } @@ -72,7 +72,7 @@ func TestAdmissionNamespaceExists(t *testing.T) { Containers: []api.Container{{Name: "ctr", Image: "image"}}, }, } - err := handler.Admit(admission.NewAttributesRecord(&pod, namespace, "pods", "CREATE")) + err := handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespace, "pods", "CREATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler") } @@ -96,7 +96,7 @@ func TestIgnoreAdmission(t *testing.T) { Containers: []api.Container{{Name: "ctr", Image: "image"}}, }, } - err := handler.Admit(admission.NewAttributesRecord(&pod, namespace, "pods", "UPDATE")) + err := handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespace, "pods", "UPDATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler") } @@ -123,7 +123,7 @@ func TestAdmissionNamespaceExistsUnknownToHandler(t *testing.T) { Containers: []api.Container{{Name: "ctr", Image: "image"}}, }, } - err := handler.Admit(admission.NewAttributesRecord(&pod, namespace, "pods", "CREATE")) + err := handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespace, "pods", "CREATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler") } diff --git a/plugin/pkg/admission/namespace/exists/admission.go b/plugin/pkg/admission/namespace/exists/admission.go index 10f6a12d675..eb2d671408b 100644 --- a/plugin/pkg/admission/namespace/exists/admission.go +++ b/plugin/pkg/admission/namespace/exists/admission.go @@ -22,7 +22,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/admission" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" @@ -50,11 +49,11 @@ type exists struct { func (e *exists) Admit(a admission.Attributes) (err error) { defaultVersion, kind, err := latest.RESTMapper.VersionAndKindForResource(a.GetResource()) if err != nil { - return err + return admission.NewForbidden(a, err) } mapping, err := latest.RESTMapper.RESTMapping(kind, defaultVersion) if err != nil { - return err + return admission.NewForbidden(a, err) } if mapping.Scope.Name() != meta.RESTScopeNameNamespace { return nil @@ -68,17 +67,12 @@ func (e *exists) Admit(a admission.Attributes) (err error) { } _, exists, err := e.store.Get(namespace) if err != nil { - return err + return admission.NewForbidden(a, err) } if exists { return nil } - obj := a.GetObject() - name := "Unknown" - if obj != nil { - name, _ = meta.NewAccessor().Name(obj) - } - return apierrors.NewForbidden(kind, name, fmt.Errorf("Namespace %s does not exist", a.GetNamespace())) + return admission.NewForbidden(a, fmt.Errorf("Namespace %s does not exist", a.GetNamespace())) } func NewExists(c client.Interface) admission.Interface { diff --git a/plugin/pkg/admission/namespace/lifecycle/admission.go b/plugin/pkg/admission/namespace/lifecycle/admission.go index 23696201533..ed731e9c187 100644 --- a/plugin/pkg/admission/namespace/lifecycle/admission.go +++ b/plugin/pkg/admission/namespace/lifecycle/admission.go @@ -22,7 +22,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/admission" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" @@ -52,11 +51,11 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { } defaultVersion, kind, err := latest.RESTMapper.VersionAndKindForResource(a.GetResource()) if err != nil { - return err + return admission.NewForbidden(a, err) } mapping, err := latest.RESTMapper.RESTMapping(kind, defaultVersion) if err != nil { - return err + return admission.NewForbidden(a, err) } if mapping.Scope.Name() != meta.RESTScopeNameNamespace { return nil @@ -68,7 +67,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { }, }) if err != nil { - return err + return admission.NewForbidden(a, err) } if !exists { return nil @@ -78,12 +77,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { return nil } - name := "Unknown" - obj := a.GetObject() - if obj != nil { - name, _ = meta.NewAccessor().Name(obj) - } - return apierrors.NewForbidden(kind, name, fmt.Errorf("Namespace %s is terminating", a.GetNamespace())) + return admission.NewForbidden(a, fmt.Errorf("Namespace %s is terminating", a.GetNamespace())) } func NewLifecycle(c client.Interface) admission.Interface { diff --git a/plugin/pkg/admission/namespace/lifecycle/admission_test.go b/plugin/pkg/admission/namespace/lifecycle/admission_test.go index d8d9d8ef70d..6bc0bd84fa4 100644 --- a/plugin/pkg/admission/namespace/lifecycle/admission_test.go +++ b/plugin/pkg/admission/namespace/lifecycle/admission_test.go @@ -50,7 +50,7 @@ func TestAdmission(t *testing.T) { Containers: []api.Container{{Name: "ctr", Image: "image"}}, }, } - err := handler.Admit(admission.NewAttributesRecord(&pod, namespaceObj.Namespace, "pods", "CREATE")) + err := handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespaceObj.Namespace, "pods", "CREATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler: %v", err) } @@ -60,19 +60,19 @@ func TestAdmission(t *testing.T) { store.Add(namespaceObj) // verify create operations in the namespace cause an error - err = handler.Admit(admission.NewAttributesRecord(&pod, namespaceObj.Namespace, "pods", "CREATE")) + err = handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespaceObj.Namespace, "pods", "CREATE")) if err == nil { t.Errorf("Expected error rejecting creates in a namespace when it is terminating") } // verify update operations in the namespace can proceed - err = handler.Admit(admission.NewAttributesRecord(&pod, namespaceObj.Namespace, "pods", "UPDATE")) + err = handler.Admit(admission.NewAttributesRecord(&pod, "Pod", namespaceObj.Namespace, "pods", "UPDATE")) if err != nil { t.Errorf("Unexpected error returned from admission handler: %v", err) } // verify delete operations in the namespace can proceed - err = handler.Admit(admission.NewAttributesRecord(nil, namespaceObj.Namespace, "pods", "DELETE")) + err = handler.Admit(admission.NewAttributesRecord(nil, "Pod", namespaceObj.Namespace, "pods", "DELETE")) if err != nil { t.Errorf("Unexpected error returned from admission handler: %v", err) } diff --git a/plugin/pkg/admission/resourcequota/admission.go b/plugin/pkg/admission/resourcequota/admission.go index c4dacafe288..092832e15a7 100644 --- a/plugin/pkg/admission/resourcequota/admission.go +++ b/plugin/pkg/admission/resourcequota/admission.go @@ -22,8 +22,6 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/admission" "github.com/GoogleCloudPlatform/kubernetes/pkg/api" - apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" - "github.com/GoogleCloudPlatform/kubernetes/pkg/api/meta" "github.com/GoogleCloudPlatform/kubernetes/pkg/api/resource" "github.com/GoogleCloudPlatform/kubernetes/pkg/client" "github.com/GoogleCloudPlatform/kubernetes/pkg/client/cache" @@ -71,13 +69,6 @@ func (q *quota) Admit(a admission.Attributes) (err error) { return nil } - obj := a.GetObject() - resource := a.GetResource() - name := "Unknown" - if obj != nil { - name, _ = meta.NewAccessor().Name(obj) - } - key := &api.ResourceQuota{ ObjectMeta: api.ObjectMeta{ Namespace: a.GetNamespace(), @@ -86,7 +77,7 @@ func (q *quota) Admit(a admission.Attributes) (err error) { } items, err := q.indexer.Index("namespace", key) if err != nil { - return apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing quota", a.GetOperation(), resource)) + return admission.NewForbidden(a, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing quota", a.GetOperation(), a.GetResource())) } if len(items) == 0 { return nil @@ -109,7 +100,7 @@ func (q *quota) Admit(a admission.Attributes) (err error) { dirty, err := IncrementUsage(a, status, q.client) if err != nil { - return err + return admission.NewForbidden(a, err) } if dirty { @@ -125,7 +116,7 @@ func (q *quota) Admit(a admission.Attributes) (err error) { usage.Status = *status _, err = q.client.ResourceQuotas(usage.Namespace).UpdateStatus(&usage) if err != nil { - return apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing quota", a.GetOperation(), a.GetResource())) + return admission.NewForbidden(a, fmt.Errorf("Unable to %s %s at this time because there was an error enforcing quota", a.GetOperation(), a.GetResource())) } } } @@ -136,17 +127,12 @@ func (q *quota) Admit(a admission.Attributes) (err error) { // Return true if the usage must be recorded prior to admitting the new resource // Return an error if the operation should not pass admission control func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, client client.Interface) (bool, error) { - obj := a.GetObject() - resourceName := a.GetResource() - name := "Unknown" - if obj != nil { - name, _ = meta.NewAccessor().Name(obj) - } dirty := false set := map[api.ResourceName]bool{} for k := range status.Hard { set[k] = true } + obj := a.GetObject() // handle max counts for each kind of resource (pods, services, replicationControllers, etc.) if a.GetOperation() == "CREATE" { resourceName := resourceToResourceName[a.GetResource()] @@ -154,10 +140,10 @@ func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, cli if hardFound { used, usedFound := status.Used[resourceName] if !usedFound { - return false, apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.")) + return false, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.") } if used.Value() >= hard.Value() { - return false, apierrors.NewForbidden(a.GetResource(), name, fmt.Errorf("Limited to %s %s", hard.String(), a.GetResource())) + return false, fmt.Errorf("Limited to %s %s", hard.String(), resourceName) } else { status.Used[resourceName] = *resource.NewQuantity(used.Value()+int64(1), resource.DecimalSI) dirty = true @@ -173,7 +159,7 @@ func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, cli if a.GetOperation() == "UPDATE" { oldPod, err := client.Pods(a.GetNamespace()).Get(pod.Name) if err != nil { - return false, apierrors.NewForbidden(resourceName, name, err) + return false, err } oldCPU := resourcequota.PodCPU(oldPod) oldMemory := resourcequota.PodMemory(oldPod) @@ -185,10 +171,10 @@ func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, cli if hardMemFound { used, usedFound := status.Used[api.ResourceMemory] if !usedFound { - return false, apierrors.NewForbidden(resourceName, name, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.")) + return false, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.") } if used.Value()+deltaMemory.Value() > hardMem.Value() { - return false, apierrors.NewForbidden(resourceName, name, fmt.Errorf("Limited to %s memory", hardMem.String())) + return false, fmt.Errorf("Limited to %s memory", hardMem.String()) } else { status.Used[api.ResourceMemory] = *resource.NewQuantity(used.Value()+deltaMemory.Value(), resource.DecimalSI) dirty = true @@ -198,10 +184,10 @@ func IncrementUsage(a admission.Attributes, status *api.ResourceQuotaStatus, cli if hardCPUFound { used, usedFound := status.Used[api.ResourceCPU] if !usedFound { - return false, apierrors.NewForbidden(resourceName, name, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.")) + return false, fmt.Errorf("Quota usage stats are not yet known, unable to admit resource until an accurate count is completed.") } if used.MilliValue()+deltaCPU.MilliValue() > hardCPU.MilliValue() { - return false, apierrors.NewForbidden(resourceName, name, fmt.Errorf("Limited to %s CPU", hardCPU.String())) + return false, fmt.Errorf("Limited to %s CPU", hardCPU.String()) } else { status.Used[api.ResourceCPU] = *resource.NewMilliQuantity(used.MilliValue()+deltaCPU.MilliValue(), resource.DecimalSI) dirty = true diff --git a/plugin/pkg/admission/resourcequota/admission_test.go b/plugin/pkg/admission/resourcequota/admission_test.go index 373844577ee..2b8e9e881c0 100644 --- a/plugin/pkg/admission/resourcequota/admission_test.go +++ b/plugin/pkg/admission/resourcequota/admission_test.go @@ -41,7 +41,7 @@ func getResourceRequirements(cpu, memory string) api.ResourceRequirements { func TestAdmissionIgnoresDelete(t *testing.T) { namespace := "default" handler := NewResourceQuota(&testclient.Fake{}) - err := handler.Admit(admission.NewAttributesRecord(nil, namespace, "pods", "DELETE")) + err := handler.Admit(admission.NewAttributesRecord(nil, "Pod", namespace, "pods", "DELETE")) if err != nil { t.Errorf("ResourceQuota should admit all deletes", err) } @@ -67,7 +67,7 @@ func TestIncrementUsagePods(t *testing.T) { r := api.ResourcePods status.Hard[r] = resource.MustParse("2") status.Used[r] = resource.MustParse("1") - dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, namespace, "pods", "CREATE"), status, client) + dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", namespace, "pods", "CREATE"), status, client) if err != nil { t.Errorf("Unexpected error", err) } @@ -107,7 +107,7 @@ func TestIncrementUsageMemory(t *testing.T) { Volumes: []api.Volume{{Name: "vol"}}, Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}}, }} - dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, namespace, "pods", "CREATE"), status, client) + dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "pods", "CREATE"), status, client) if err != nil { t.Errorf("Unexpected error", err) } @@ -148,7 +148,7 @@ func TestExceedUsageMemory(t *testing.T) { Volumes: []api.Volume{{Name: "vol"}}, Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "3Gi")}}, }} - _, err := IncrementUsage(admission.NewAttributesRecord(newPod, namespace, "pods", "CREATE"), status, client) + _, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "pods", "CREATE"), status, client) if err == nil { t.Errorf("Expected memory usage exceeded error") } @@ -181,7 +181,7 @@ func TestIncrementUsageCPU(t *testing.T) { Volumes: []api.Volume{{Name: "vol"}}, Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("100m", "1Gi")}}, }} - dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, namespace, "pods", "CREATE"), status, client) + dirty, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "pods", "CREATE"), status, client) if err != nil { t.Errorf("Unexpected error", err) } @@ -222,7 +222,7 @@ func TestExceedUsageCPU(t *testing.T) { Volumes: []api.Volume{{Name: "vol"}}, Containers: []api.Container{{Name: "ctr", Image: "image", Resources: getResourceRequirements("500m", "1Gi")}}, }} - _, err := IncrementUsage(admission.NewAttributesRecord(newPod, namespace, "pods", "CREATE"), status, client) + _, err := IncrementUsage(admission.NewAttributesRecord(newPod, "Pod", namespace, "pods", "CREATE"), status, client) if err == nil { t.Errorf("Expected CPU usage exceeded error") } @@ -248,7 +248,7 @@ func TestExceedUsagePods(t *testing.T) { r := api.ResourcePods status.Hard[r] = resource.MustParse("1") status.Used[r] = resource.MustParse("1") - _, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, namespace, "pods", "CREATE"), status, client) + _, err := IncrementUsage(admission.NewAttributesRecord(&api.Pod{}, "Pod", namespace, "pods", "CREATE"), status, client) if err == nil { t.Errorf("Expected error because this would exceed your quota") } @@ -270,7 +270,7 @@ func TestIncrementUsageServices(t *testing.T) { r := api.ResourceServices status.Hard[r] = resource.MustParse("2") status.Used[r] = resource.MustParse("1") - dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Service{}, namespace, "services", "CREATE"), status, client) + dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.Service{}, "Service", namespace, "services", "CREATE"), status, client) if err != nil { t.Errorf("Unexpected error", err) } @@ -299,7 +299,7 @@ func TestExceedUsageServices(t *testing.T) { r := api.ResourceServices status.Hard[r] = resource.MustParse("1") status.Used[r] = resource.MustParse("1") - _, err := IncrementUsage(admission.NewAttributesRecord(&api.Service{}, namespace, "services", "CREATE"), status, client) + _, err := IncrementUsage(admission.NewAttributesRecord(&api.Service{}, "Service", namespace, "services", "CREATE"), status, client) if err == nil { t.Errorf("Expected error because this would exceed usage") } @@ -321,7 +321,7 @@ func TestIncrementUsageReplicationControllers(t *testing.T) { r := api.ResourceReplicationControllers status.Hard[r] = resource.MustParse("2") status.Used[r] = resource.MustParse("1") - dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.ReplicationController{}, namespace, "replicationControllers", "CREATE"), status, client) + dirty, err := IncrementUsage(admission.NewAttributesRecord(&api.ReplicationController{}, "ReplicationController", namespace, "replicationControllers", "CREATE"), status, client) if err != nil { t.Errorf("Unexpected error", err) } @@ -350,7 +350,7 @@ func TestExceedUsageReplicationControllers(t *testing.T) { r := api.ResourceReplicationControllers status.Hard[r] = resource.MustParse("1") status.Used[r] = resource.MustParse("1") - _, err := IncrementUsage(admission.NewAttributesRecord(&api.ReplicationController{}, namespace, "replicationControllers", "CREATE"), status, client) + _, err := IncrementUsage(admission.NewAttributesRecord(&api.ReplicationController{}, "ReplicationController", namespace, "replicationControllers", "CREATE"), status, client) if err == nil { t.Errorf("Expected error for exceeding hard limits") }