diff --git a/pkg/apiserver/apiserver.go b/pkg/apiserver/apiserver.go index bc1850d46..352484b40 100644 --- a/pkg/apiserver/apiserver.go +++ b/pkg/apiserver/apiserver.go @@ -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{} diff --git a/pkg/apiserver/registry/datapackaging/package_crd_rest.go b/pkg/apiserver/registry/datapackaging/package_crd_rest.go index 37b93e689..7e541e86a 100644 --- a/pkg/apiserver/registry/datapackaging/package_crd_rest.go +++ b/pkg/apiserver/registry/datapackaging/package_crd_rest.go @@ -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" @@ -31,6 +32,7 @@ type PackageCRDREST struct { crdClient installclient.Interface nsClient kubernetes.Interface globalNamespace string + logger logr.Logger } var ( @@ -38,8 +40,9 @@ var ( _ 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 { @@ -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 { @@ -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) { @@ -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 @@ -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) { @@ -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) { @@ -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 @@ -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 { diff --git a/pkg/apiserver/registry/datapackaging/package_crd_rest_test.go b/pkg/apiserver/registry/datapackaging/package_crd_rest_test.go index a1bdb167b..86aae8875 100644 --- a/pkg/apiserver/registry/datapackaging/package_crd_rest_test.go +++ b/pkg/apiserver/registry/datapackaging/package_crd_rest_test.go @@ -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" @@ -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" @@ -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 { @@ -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{}) @@ -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 { @@ -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 { @@ -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) { @@ -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 { @@ -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 { @@ -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 { @@ -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 { @@ -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) { diff --git a/pkg/apiserver/registry/datapackaging/package_storage_client.go b/pkg/apiserver/registry/datapackaging/package_storage_client.go index f7c5ab34f..35224ecf3 100644 --- a/pkg/apiserver/registry/datapackaging/package_storage_client.go +++ b/pkg/apiserver/registry/datapackaging/package_storage_client.go @@ -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" @@ -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 { @@ -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 @@ -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} } @@ -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 } diff --git a/test/e2e/kappcontroller/package_test.go b/test/e2e/kappcontroller/package_test.go index 2210f9214..2577afbfb 100644 --- a/test/e2e/kappcontroller/package_test.go +++ b/test/e2e/kappcontroller/package_test.go @@ -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" @@ -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) + }) +}