Skip to content

Commit

Permalink
Return user friendly error when package doesn't exist (#1322)
Browse files Browse the repository at this point in the history 
Signed-off-by: rcmadhankumar <[email protected]>
  • Loading branch information
@rcmadhankumar
rcmadhankumar authored Jan 18, 2024
1 parent 1ad322b commit 0972b2f
Show file tree
Hide file tree
Showing 5 changed files with 80 additions and 31 deletions.
2 changes: 1 addition & 1 deletion pkg/apiserver/apiserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func NewAPIServer(clientConfig *rest.Config, coreClient kubernetes.Interface, kc
}

packageMetadatasStorage := packagerest.NewPackageMetadataCRDREST(kcClient, coreClient, opts.GlobalNamespace)
packageStorage := packagerest.NewPackageCRDREST(kcClient, coreClient, opts.GlobalNamespace)
packageStorage := packagerest.NewPackageCRDREST(kcClient, coreClient, opts.GlobalNamespace, opts.Logger)

pkgGroup := genericapiserver.NewDefaultAPIGroupInfo(datapackaging.GroupName, Scheme, metav1.ParameterCodec, Codecs)
pkgv1alpha1Storage := map[string]apirest.Storage{}
Expand Down
21 changes: 12 additions & 9 deletions pkg/apiserver/registry/datapackaging/package_crd_rest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"time"

"github.com/go-logr/logr"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/validation"
installclient "github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset/versioned"
Expand All @@ -31,15 +32,17 @@ type PackageCRDREST struct {
crdClient installclient.Interface
nsClient kubernetes.Interface
globalNamespace string
logger logr.Logger
}

var (
_ rest.StandardStorage = &PackageCRDREST{}
_ rest.ShortNamesProvider = &PackageCRDREST{}
)

func NewPackageCRDREST(crdClient installclient.Interface, nsClient kubernetes.Interface, globalNS string) *PackageCRDREST {
return &PackageCRDREST{crdClient, nsClient, globalNS}
// NewPackageCRDREST creates a new instance of the PackageCRDREST type
func NewPackageCRDREST(crdClient installclient.Interface, nsClient kubernetes.Interface, globalNS string, logger logr.Logger) *PackageCRDREST {
return &PackageCRDREST{crdClient, nsClient, globalNS, logger}
}

func (r *PackageCRDREST) ShortNames() []string {
Expand Down Expand Up @@ -70,7 +73,7 @@ func (r *PackageCRDREST) NewList() runtime.Object {

func (r *PackageCRDREST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

if createValidation != nil {
if err := createValidation(ctx, obj); err != nil {
Expand Down Expand Up @@ -98,7 +101,7 @@ func (r *PackageCRDREST) shouldFetchGlobal(ctx context.Context, namespace string

func (r *PackageCRDREST) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

pkg, err := client.Get(ctx, namespace, name, *options)
if errors.IsNotFound(err) && r.shouldFetchGlobal(ctx, namespace) {
Expand All @@ -109,7 +112,7 @@ func (r *PackageCRDREST) Get(ctx context.Context, name string, options *metav1.G

func (r *PackageCRDREST) List(ctx context.Context, options *internalversion.ListOptions) (runtime.Object, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

// field selector isnt supported by CRD's so reset it, we will apply it later
fs := options.FieldSelector
Expand Down Expand Up @@ -157,7 +160,7 @@ func (r *PackageCRDREST) List(ctx context.Context, options *internalversion.List

func (r *PackageCRDREST) Update(ctx context.Context, name string, objInfo rest.UpdatedObjectInfo, createValidation rest.ValidateObjectFunc, updateValidation rest.ValidateObjectUpdateFunc, forceAllowCreate bool, options *metav1.UpdateOptions) (runtime.Object, bool, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

pkg, err := client.Get(ctx, namespace, name, metav1.GetOptions{})
if errors.IsNotFound(err) {
Expand Down Expand Up @@ -225,7 +228,7 @@ func (r *PackageCRDREST) Update(ctx context.Context, name string, objInfo rest.U

func (r *PackageCRDREST) Delete(ctx context.Context, name string, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions) (runtime.Object, bool, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

pkg, err := client.Get(ctx, namespace, name, metav1.GetOptions{})
if errors.IsNotFound(err) {
Expand All @@ -252,7 +255,7 @@ func (r *PackageCRDREST) Delete(ctx context.Context, name string, deleteValidati

func (r *PackageCRDREST) DeleteCollection(ctx context.Context, deleteValidation rest.ValidateObjectFunc, options *metav1.DeleteOptions, listOptions *metainternalversion.ListOptions) (runtime.Object, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

// clear unsupported field selectors
fs := listOptions.FieldSelector
Expand Down Expand Up @@ -300,7 +303,7 @@ func (r *PackageCRDREST) DeleteCollection(ctx context.Context, deleteValidation

func (r *PackageCRDREST) Watch(ctx context.Context, options *internalversion.ListOptions) (watch.Interface, error) {
namespace := request.NamespaceValue(ctx)
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace))
client := NewPackageStorageClient(r.crdClient, NewPackageTranslator(namespace, r.logger))

watcher, err := client.Watch(ctx, namespace, r.internalToMetaListOpts(*options))
if errors.IsNotFound(err) && namespace != r.globalNamespace {
Expand Down
29 changes: 15 additions & 14 deletions pkg/apiserver/registry/datapackaging/package_crd_rest_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/require"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging"
datapkgreg "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/registry/datapackaging"
Expand All @@ -20,6 +21,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
k8sfake "k8s.io/client-go/kubernetes/fake"
cgtesting "k8s.io/client-go/testing"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

const globalNamespace = "global.packaging.kapp-controller.carvel.dev"
Expand All @@ -31,7 +33,7 @@ func TestPackageVersionListIncludesGlobalAndNamespaced(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalIntPackageVersion(), namespacedIntPackageVersion(), excludedNonGlobalIntPackageVersion())
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

pkgvList, err := pkgvCRDREST.List(namespacedCtx(nonGlobalNamespace), &internalversion.ListOptions{})
if err != nil {
Expand Down Expand Up @@ -66,7 +68,7 @@ func TestPackageVersionListPrefersNamespacedOverGlobal(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalIntPackageVersion(), overrideIntPackageVersion())
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

// list package versions and verify all of them are there
pkgvList, err := pkgvCRDREST.List(namespacedCtx(nonGlobalNamespace), &internalversion.ListOptions{})
Expand Down Expand Up @@ -101,7 +103,7 @@ func TestPackageVersionGetNotPresentInNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

obj, err := pkgvCRDREST.Get(namespacedCtx(nonGlobalNamespace), name, &metav1.GetOptions{})
if err != nil {
Expand All @@ -126,7 +128,7 @@ func TestPackageVersionGetPresentInOnlyNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(namespacedPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

obj, err := pkgvCRDREST.Get(namespacedCtx(nonGlobalNamespace), name, &metav1.GetOptions{})
if err != nil {
Expand All @@ -146,21 +148,20 @@ func TestPackageVersionGetPresentInOnlyNS(t *testing.T) {
func TestPackageVersionGetNotFound(t *testing.T) {
namespacedPackageVersion := excludedNonGlobalIntPackageVersion()
name := namespacedPackageName
expectedError := "package.data.packaging.carvel.dev \"" + name + "\" not found"

internalClient := fake.NewSimpleClientset(namespacedPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

_, err := pkgvCRDREST.Get(namespacedCtx(nonGlobalNamespace), name, &metav1.GetOptions{})
if err == nil {
t.Fatalf("Expected get operation to fail, but it didn't")
}

if !errors.IsNotFound(err) {
t.Fatalf("Expected a not found error, got: %v", err)
}

require.True(t, errors.IsNotFound(err))
require.ErrorContains(t, err, expectedError)
}

func TestPackageVersionGetPreferNS(t *testing.T) {
Expand All @@ -171,7 +172,7 @@ func TestPackageVersionGetPreferNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(overridePackageVersion, globalIntPackageVersion())
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

obj, err := pkgvCRDREST.Get(namespacedCtx(nonGlobalNamespace), name, &metav1.GetOptions{})
if err != nil {
Expand Down Expand Up @@ -201,7 +202,7 @@ func TestPackageVersionUpdateDoesntUpdateGlobal(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalPackageVersion, namespacedPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

obj, created, err := pkgvCRDREST.Update(namespacedCtx(nonGlobalNamespace), name, UpdatePackageVersionTestImpl{updateReleaseNotesFn(newReleaseNotes, name, packageName, version)}, nil, nil, false, &metav1.UpdateOptions{})
if err != nil {
Expand Down Expand Up @@ -242,7 +243,7 @@ func TestPackageVersionUpdateCreatesInNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

obj, created, err := pkgvCRDREST.Update(namespacedCtx(nonGlobalNamespace), name, UpdatePackageVersionTestImpl{updateReleaseNotesFn(newReleaseNotes, name, packageName, version)}, nil, nil, false, &metav1.UpdateOptions{})
if err != nil {
Expand Down Expand Up @@ -276,7 +277,7 @@ func TestPackageVersionDeleteExistsInNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(namespacedPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

_, _, err := pkgvCRDREST.Delete(namespacedCtx(nonGlobalNamespace), name, nil, &metav1.DeleteOptions{})
if err != nil {
Expand Down Expand Up @@ -308,7 +309,7 @@ func TestPackageVersionDeleteExistsGlobalNotInNS(t *testing.T) {
internalClient := fake.NewSimpleClientset(globalPackageVersion)
fakeCoreClient := k8sfake.NewSimpleClientset(namespace())

pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace)
pkgvCRDREST := datapkgreg.NewPackageCRDREST(internalClient, fakeCoreClient, globalNamespace, logf.Log)

_, _, err := pkgvCRDREST.Delete(namespacedCtx(nonGlobalNamespace), name, nil, &metav1.DeleteOptions{})
if !errors.IsNotFound(err) {
Expand Down
40 changes: 33 additions & 7 deletions pkg/apiserver/registry/datapackaging/package_storage_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@ package datapackaging
import (
"context"
"encoding/base32"
"errors"
"fmt"
"strings"

"github.com/go-logr/logr"
internalpkgingv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apis/internalpackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging"
datapkgingv1alpha1 "github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/watchers"
internalclient "github.com/vmware-tanzu/carvel-kapp-controller/pkg/client/clientset/versioned"
"k8s.io/apimachinery/pkg/api/errors"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/fields"
"k8s.io/apimachinery/pkg/watch"
Expand All @@ -27,10 +29,12 @@ const (

type PackageTranslator struct {
namespace string
logger logr.Logger
}

func NewPackageTranslator(namespace string) PackageTranslator {
return PackageTranslator{namespace}
// NewPackageTranslator creates a new instance of the PackageTranslator type
func NewPackageTranslator(namespace string, logger logr.Logger) PackageTranslator {
return PackageTranslator{namespace, logger}
}

func (t PackageTranslator) ToInternalName(name string) string {
Expand Down Expand Up @@ -65,7 +69,7 @@ func (t PackageTranslator) ToExternalObj(intObj *internalpkgingv1alpha1.Internal
var err error
obj.Name, err = t.ToExternalName(intObj.Name)
if err != nil {
return nil, errors.NewInternalError(fmt.Errorf("decoding internal obj name '%s': %v", intObj.Name, err))
return nil, apierrors.NewInternalError(fmt.Errorf("decoding internal obj name '%s': %v", intObj.Name, err))
}

// Self link is deprecated and planned for removal, so we don't translate it
Expand Down Expand Up @@ -122,10 +126,10 @@ func (t PackageTranslator) ToExternalWatcher(intObjWatcher watch.Interface, fiel
evt.Object, err = t.ToExternalObj(intpkg)
if err != nil {
var status metav1.Status
if statusErr, ok := err.(*errors.StatusError); ok {
if statusErr, ok := err.(*apierrors.StatusError); ok {
status = statusErr.Status()
} else {
status = errors.NewInternalError(err).Status()
status = apierrors.NewInternalError(err).Status()
}
return watch.Event{Type: watch.Error, Object: &status}
}
Expand All @@ -152,7 +156,29 @@ func (t PackageTranslator) ToExternalWatcher(intObjWatcher watch.Interface, fiel
}

func (t PackageTranslator) ToExternalError(err error) error {
// TODO: implement
var status apierrors.APIStatus
if !(errors.As(err, &status)) {
return err
}

details := status.Status().Details
if details != nil && details.Kind == "internalpackages" && details.Group == internalpkgingv1alpha1.SchemeGroupVersion.Group {
packageName, translateErr := t.ToExternalName(details.Name)
if translateErr != nil {
t.logger.Error(translateErr, "Error translating to external name")
return err
}

switch status.Status().Reason {
case metav1.StatusReasonNotFound:
return apierrors.NewNotFound(datapkgingv1alpha1.Resource("package"), packageName)
case metav1.StatusReasonAlreadyExists:
return apierrors.NewAlreadyExists(datapkgingv1alpha1.Resource("package"), packageName)
default:
return err
}
}

return err
}

Expand Down
19 changes: 19 additions & 0 deletions test/e2e/kappcontroller/package_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"testing"
"time"

"github.com/stretchr/testify/require"
"github.com/vmware-tanzu/carvel-kapp-controller/pkg/apiserver/apis/datapackaging/v1alpha1"
"github.com/vmware-tanzu/carvel-kapp-controller/test/e2e"
"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -409,3 +410,21 @@ spec:
}
})
}

func TestPackageNotFound(t *testing.T) {
env := e2e.BuildEnv(t)
logger := e2e.Logger{}
k := e2e.Kubectl{t, env.Namespace, logger}
packageName := "foo.1.0.0"
expectedError := "stderr: 'Error from server (NotFound): package.data.packaging.carvel.dev \"foo.1.0.0\" not found"

logger.Section("Get Package", func() {
_, err := k.RunWithOpts([]string{"get", "package", packageName}, e2e.RunOpts{AllowError: true})
require.ErrorContains(t, err, expectedError)
})

logger.Section("delete Package", func() {
_, err := k.RunWithOpts([]string{"delete", "package", packageName}, e2e.RunOpts{AllowError: true})
require.ErrorContains(t, err, expectedError)
})
}

0 comments on commit 0972b2f

Please sign in to comment.