From 58e7cc4106ddd61613b8bc7efbe6a5e3c5d54e72 Mon Sep 17 00:00:00 2001 From: Brendan Burns Date: Tue, 15 Sep 2015 13:38:03 -0700 Subject: [PATCH] Add a not found error to admission control --- pkg/admission/errors.go | 37 ++++++++++++++----- .../admission/namespace/exists/admission.go | 13 ++++--- .../namespace/lifecycle/admission.go | 9 +++-- 3 files changed, 40 insertions(+), 19 deletions(-) diff --git a/pkg/admission/errors.go b/pkg/admission/errors.go index 227b2e5448c..51204c4c4b6 100644 --- a/pkg/admission/errors.go +++ b/pkg/admission/errors.go @@ -19,22 +19,17 @@ package admission import ( "k8s.io/kubernetes/pkg/api" apierrors "k8s.io/kubernetes/pkg/api/errors" + errs "k8s.io/kubernetes/pkg/util/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() +func extractKindName(a Attributes) (name, kind string, err error) { + name = "Unknown" + kind = a.GetKind() obj := a.GetObject() if obj != nil { objectMeta, err := api.ObjectMetaFor(obj) if err != nil { - return apierrors.NewForbidden(kind, name, internalError) + return "", "", err } // this is necessary because name object name generation has not occurred yet @@ -44,5 +39,27 @@ func NewForbidden(a Attributes, internalError error) error { name = objectMeta.GenerateName } } + return name, kind, nil +} + +// 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, kind, err := extractKindName(a) + if err != nil { + return apierrors.NewInternalError(errs.NewAggregate([]error{internalError, err})) + } return apierrors.NewForbidden(kind, name, internalError) } + +// NewNotFound is a utility function to return a well-formatted admission control error response +func NewNotFound(a Attributes) error { + name, kind, err := extractKindName(a) + if err != nil { + return apierrors.NewInternalError(err) + } + return apierrors.NewNotFound(kind, name) +} diff --git a/plugin/pkg/admission/namespace/exists/admission.go b/plugin/pkg/admission/namespace/exists/admission.go index a405d4daced..ed5d33ceaac 100644 --- a/plugin/pkg/admission/namespace/exists/admission.go +++ b/plugin/pkg/admission/namespace/exists/admission.go @@ -17,12 +17,12 @@ limitations under the License. package exists import ( - "fmt" "io" "time" "k8s.io/kubernetes/pkg/admission" "k8s.io/kubernetes/pkg/api" + "k8s.io/kubernetes/pkg/api/errors" "k8s.io/kubernetes/pkg/api/meta" "k8s.io/kubernetes/pkg/client/cache" client "k8s.io/kubernetes/pkg/client/unversioned" @@ -50,11 +50,11 @@ type exists struct { func (e *exists) Admit(a admission.Attributes) (err error) { defaultVersion, kind, err := api.RESTMapper.VersionAndKindForResource(a.GetResource()) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } mapping, err := api.RESTMapper.RESTMapping(kind, defaultVersion) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } if mapping.Scope.Name() != meta.RESTScopeNameNamespace { return nil @@ -68,7 +68,7 @@ func (e *exists) Admit(a admission.Attributes) (err error) { } _, exists, err := e.store.Get(namespace) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } if exists { return nil @@ -77,7 +77,10 @@ func (e *exists) Admit(a admission.Attributes) (err error) { // in case of latency in our caches, make a call direct to storage to verify that it truly exists or not _, err = e.client.Namespaces().Get(a.GetNamespace()) if err != nil { - return admission.NewForbidden(a, fmt.Errorf("Namespace %s does not exist", a.GetNamespace())) + if errors.IsNotFound(err) { + return err + } + return errors.NewInternalError(err) } return nil diff --git a/plugin/pkg/admission/namespace/lifecycle/admission.go b/plugin/pkg/admission/namespace/lifecycle/admission.go index 80210f0cbf6..794808028b5 100644 --- a/plugin/pkg/admission/namespace/lifecycle/admission.go +++ b/plugin/pkg/admission/namespace/lifecycle/admission.go @@ -58,11 +58,11 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { defaultVersion, kind, err := api.RESTMapper.VersionAndKindForResource(a.GetResource()) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } mapping, err := api.RESTMapper.RESTMapping(kind, defaultVersion) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } if mapping.Scope.Name() != meta.RESTScopeNameNamespace { return nil @@ -74,7 +74,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { }, }) if err != nil { - return admission.NewForbidden(a, err) + return errors.NewInternalError(err) } // refuse to operate on non-existent namespaces @@ -82,7 +82,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { // in case of latency in our caches, make a call direct to storage to verify that it truly exists or not namespaceObj, err = l.client.Namespaces().Get(a.GetNamespace()) if err != nil { - return admission.NewForbidden(a, fmt.Errorf("Namespace %s does not exist", a.GetNamespace())) + return admission.NewNotFound(a) } } @@ -93,6 +93,7 @@ func (l *lifecycle) Admit(a admission.Attributes) (err error) { return nil } + // TODO: This should probably not be a 403 return admission.NewForbidden(a, fmt.Errorf("Unable to create new content in namespace %s because it is being terminated.", a.GetNamespace())) }