Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return user friendly error when package doesn't exist #1322

Merged
merged 1 commit into from
Jan 18, 2024
Merged

Return user friendly error when package doesn't exist #1322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/apiserver/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,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
Open in desktop
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 All @@ -65,7 +68,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 @@ -93,7 +96,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 @@ -104,7 +107,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 @@ -152,7 +155,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 @@ -220,7 +223,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 @@ -247,7 +250,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 @@ -295,7 +298,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
Open in desktop
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
Open in desktop
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
rcmadhankumar marked this conversation as resolved.
Show resolved Hide resolved
rcmadhankumar marked this conversation as resolved.
Show resolved Hide resolved
}

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
Open in desktop
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)
})
}
Loading