error getting secrets: <nil>

Assigning a typed nil to the error interface gives a non-nil value.

That breaks the common pattern `if nil != nil { ... }`.

To avoid this problem, NewError should return an interface.

pkg/dockerregistry/server/manifestservice.go

@@ -41,7 +41,7 @@ func (m *manifestService) Exists(ctx context.Context, dgst digest.Digest) (bool,
 
 	image, err := m.imageStream.GetImageOfImageStream(ctx, dgst)
 	if err != nil {
-		switch err.Code {
+		switch err.Code() {
 		case imagestream.ErrImageStreamImageNotFoundCode:
 			dcontext.GetLogger(ctx).Errorf("manifestService.Exists: image %s is not found in imagestream %s", dgst.String(), m.imageStream.Reference())
 			fallthrough
@@ -59,7 +59,7 @@ func (m *manifestService) Get(ctx context.Context, dgst digest.Digest, options .
 
 	image, rErr := m.imageStream.GetImageOfImageStream(ctx, dgst)
 	if rErr != nil {
-		switch rErr.Code {
+		switch rErr.Code() {
 		case imagestream.ErrImageStreamNotFoundCode, imagestream.ErrImageStreamImageNotFoundCode:
 			dcontext.GetLogger(ctx).Errorf("manifestService.Get: unable to get image %s in imagestream %s: %v", dgst.String(), m.imageStream.Reference(), rErr)
 			return nil, distribution.ErrManifestUnknownRevision{
@@ -170,7 +170,7 @@ func (m *manifestService) Put(ctx context.Context, manifest distribution.Manifes
 
 	rErr := m.imageStream.CreateImageStreamMapping(ctx, uclient, tag, image)
 	if rErr != nil {
-		switch rErr.Code {
+		switch rErr.Code() {
 		case imagestream.ErrImageStreamNotFoundCode:
 			dcontext.GetLogger(ctx).Errorf("manifestService.Put: imagestreammapping failed for image %s@%s: %v", m.imageStream.Reference(), image.Name, rErr)
 			return "", distribution.ErrManifestUnknownRevision{
@@ -202,7 +202,7 @@ func (m *manifestService) Delete(ctx context.Context, dgst digest.Digest) error
 		return distribution.ErrUnsupported
 	}
 
-	switch err.Code {
+	switch err.Code() {
 	case imagestream.ErrImageStreamNotFoundCode, imagestream.ErrImageStreamImageNotFoundCode:
 		// There is no image/imagestream. Let's just delete the link.
 	case imagestream.ErrImageStreamForbiddenCode:

pkg/dockerregistry/server/pullthroughmanifestservice.go

@@ -46,7 +46,7 @@ func (m *pullthroughManifestService) remoteGet(ctx context.Context, dgst digest.
 	dcontext.GetLogger(ctx).Debugf("(*pullthroughManifestService).remoteGet: starting with dgst=%s", dgst.String())
 	image, rErr := m.imageStream.GetImageOfImageStream(ctx, dgst)
 	if rErr != nil {
-		switch rErr.Code {
+		switch rErr.Code() {
 		case imagestream.ErrImageStreamNotFoundCode, imagestream.ErrImageStreamImageNotFoundCode:
 			dcontext.GetLogger(ctx).Errorf("remoteGet: unable to get image %s in imagestream %s: %v", dgst.String(), m.imageStream.Reference(), rErr)
 			return nil, distribution.ErrManifestUnknownRevision{

pkg/dockerregistry/server/remoteblobgetter.go

@@ -26,7 +26,7 @@ type BlobGetterService interface {
 	distribution.BlobServer
 }
 
-type secretsGetter func() ([]corev1.Secret, *rerrors.Error)
+type secretsGetter func() ([]corev1.Secret, rerrors.Error)
 
 // digestBlobStoreCache caches BlobStores by digests. It is safe to use it
 // concurrently from different goroutines (from an HTTP handler and background
@@ -94,7 +94,7 @@ func (rbgs *remoteBlobGetterService) findBlobStore(ctx context.Context, dgst dig
 	// we don't know which image in the image stream surfaced the content).
 	ok, err := rbgs.imageStream.Exists(ctx)
 	if err != nil {
-		switch err.Code {
+		switch err.Code() {
 		case imagestream.ErrImageStreamNotFoundCode:
 			dcontext.GetLogger(ctx).Errorf("findBlobStore: imagestream %s not found: %v", rbgs.imageStream.Reference(), err)
 			return distribution.Descriptor{}, nil, distribution.ErrBlobUnknown

pkg/errors/errors.go

@@ -20,22 +20,44 @@ var (
 )
 
 // Error provides a wrapper around error.
-type Error struct {
-	Code    string
-	Message string
-	Err     error
+type Error interface {
+	error
+	Code() string
+	Message() string
+	Unwrap() error
+
+	// internal is an unexported method to prevent external implementations.
+	internal()
 }
 
-var _ error = Error{}
-
-func (e Error) Error() string {
-	return fmt.Sprintf("%s: %s: %s", e.Code, e.Message, e.Err.Error())
+type registryError struct {
+	code    string
+	message string
+	err     error
 }
 
-func NewError(code, msg string, err error) *Error {
-	return &Error{
-		Code:    code,
-		Message: msg,
-		Err:     err,
+func NewError(code, msg string, err error) Error {
+	return &registryError{
+		code:    code,
+		message: msg,
+		err:     err,
 	}
 }
+
+func (e registryError) Error() string {
+	return fmt.Sprintf("%s: %s: %s", e.code, e.message, e.err.Error())
+}
+
+func (e registryError) Code() string {
+	return e.code
+}
+
+func (e registryError) Message() string {
+	return e.message
+}
+
+func (e registryError) Unwrap() error {
+	return e.err
+}
+
+func (registryError) internal() {}

pkg/errors/errors_test.go

@@ -0,0 +1,27 @@
+package errors
+
+import (
+	"fmt"
+	"testing"
+)
+
+func check(fail bool) Error {
+	if fail {
+		return NewError("Check", "fail is true", fmt.Errorf("underlying error"))
+	}
+	return nil
+}
+
+func TestNewError(t *testing.T) {
+	var err error
+
+	err = check(true)
+	if expected := "Check: fail is true: underlying error"; err == nil || err.Error() != expected {
+		t.Errorf("check(true): got %v, want %v", err, expected)
+	}
+
+	err = check(false)
+	if err != nil {
+		t.Errorf("check(false): got %v, want nil", err)
+	}
+}

pkg/imagestream/cachedimagestreamgetter.go

@@ -30,7 +30,7 @@ type cachedImageStreamGetter struct {
 	cachedImageStreamLayers *imageapiv1.ImageStreamLayers
 }
 
-func (g *cachedImageStreamGetter) get() (*imageapiv1.ImageStream, *rerrors.Error) {
+func (g *cachedImageStreamGetter) get() (*imageapiv1.ImageStream, rerrors.Error) {
 	if g.cachedImageStream != nil {
 		return g.cachedImageStream, nil
 	}
@@ -50,7 +50,7 @@ func (g *cachedImageStreamGetter) get() (*imageapiv1.ImageStream, *rerrors.Error
 	return is, nil
 }
 
-func (g *cachedImageStreamGetter) layers() (*imageapiv1.ImageStreamLayers, *rerrors.Error) {
+func (g *cachedImageStreamGetter) layers() (*imageapiv1.ImageStreamLayers, rerrors.Error) {
 	if g.cachedImageStreamLayers != nil {
 		return g.cachedImageStreamLayers, nil
 	}

pkg/imagestream/image.go

@@ -31,7 +31,7 @@ func IsImageManaged(image *imageapiv1.Image) bool {
 }
 
 type imageGetter interface {
-	Get(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *rerrors.Error)
+	Get(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, rerrors.Error)
 }
 
 type cachedImageGetter struct {
@@ -47,7 +47,7 @@ func newCachedImageGetter(client client.Interface) imageGetter {
 }
 
 // Get retrieves the Image resource with the digest dgst. No authorization check is made.
-func (ig *cachedImageGetter) Get(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *rerrors.Error) {
+func (ig *cachedImageGetter) Get(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, rerrors.Error) {
 	if image, ok := ig.cache[dgst]; ok {
 		dcontext.GetLogger(ctx).Debugf("(*cachedImageGetter).Get: found image %s in cache", image.Name)
 		return image, nil

pkg/imagestream/imagestream.go

@@ -47,19 +47,19 @@ type ImagePullthroughSpec struct {
 
 type ImageStream interface {
 	Reference() string
-	Exists(ctx context.Context) (bool, *rerrors.Error)
+	Exists(ctx context.Context) (bool, rerrors.Error)
 
-	GetImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *rerrors.Error)
-	CreateImageStreamMapping(ctx context.Context, userClient client.Interface, tag string, image *imageapiv1.Image) *rerrors.Error
-	ResolveImageID(ctx context.Context, dgst digest.Digest) (*imageapiv1.TagEvent, *rerrors.Error)
+	GetImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, rerrors.Error)
+	CreateImageStreamMapping(ctx context.Context, userClient client.Interface, tag string, image *imageapiv1.Image) rerrors.Error
+	ResolveImageID(ctx context.Context, dgst digest.Digest) (*imageapiv1.TagEvent, rerrors.Error)
 
 	HasBlob(ctx context.Context, dgst digest.Digest) (bool, *imageapiv1.ImageStreamLayers, *imageapiv1.Image)
-	IdentifyCandidateRepositories(ctx context.Context, primary bool) ([]string, map[string]ImagePullthroughSpec, *rerrors.Error)
-	GetLimitRangeList(ctx context.Context, cache ProjectObjectListStore) (*corev1.LimitRangeList, *rerrors.Error)
-	GetSecrets() ([]corev1.Secret, *rerrors.Error)
+	IdentifyCandidateRepositories(ctx context.Context, primary bool) ([]string, map[string]ImagePullthroughSpec, rerrors.Error)
+	GetLimitRangeList(ctx context.Context, cache ProjectObjectListStore) (*corev1.LimitRangeList, rerrors.Error)
+	GetSecrets() ([]corev1.Secret, rerrors.Error)
 
-	TagIsInsecure(ctx context.Context, tag string, dgst digest.Digest) (bool, *rerrors.Error)
-	Tags(ctx context.Context) (map[string]digest.Digest, *rerrors.Error)
+	TagIsInsecure(ctx context.Context, tag string, dgst digest.Digest) (bool, rerrors.Error)
+	Tags(ctx context.Context) (map[string]digest.Digest, rerrors.Error)
 }
 
 type imageStream struct {
@@ -95,7 +95,7 @@ func (is *imageStream) Reference() string {
 }
 
 // getImage retrieves the Image with digest `dgst`. No authorization check is done.
-func (is *imageStream) getImage(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *rerrors.Error) {
+func (is *imageStream) getImage(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, rerrors.Error) {
 	image, err := is.imageClient.Get(ctx, dgst)
 
 	switch {
@@ -118,7 +118,7 @@ func (is *imageStream) getImage(ctx context.Context, dgst digest.Digest) (*image
 
 // ResolveImageID returns latest TagEvent for specified imageID and an error if
 // there's more than one image matching the ID or when one does not exist.
-func (is *imageStream) ResolveImageID(ctx context.Context, dgst digest.Digest) (*imageapiv1.TagEvent, *rerrors.Error) {
+func (is *imageStream) ResolveImageID(ctx context.Context, dgst digest.Digest) (*imageapiv1.TagEvent, rerrors.Error) {
 	stream, rErr := is.imageStreamGetter.get()
 
 	if rErr != nil {
@@ -154,7 +154,7 @@ func (is *imageStream) ResolveImageID(ctx context.Context, dgst digest.Digest) (
 //
 // If you need the image object to be modified according to image stream tag,
 // please use GetImageOfImageStream.
-func (is *imageStream) getStoredImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *imageapiv1.TagEvent, *rerrors.Error) {
+func (is *imageStream) getStoredImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *imageapiv1.TagEvent, rerrors.Error) {
 	tagEvent, err := is.ResolveImageID(ctx, dgst)
 	if err != nil {
 		return nil, nil, err
@@ -176,7 +176,7 @@ func (is *imageStream) getStoredImageOfImageStream(ctx context.Context, dgst dig
 // NOTE: due to on the fly modification, the returned image object should
 // not be sent to the master API. If you need unmodified version of the
 // image object, please use getStoredImageOfImageStream.
-func (is *imageStream) GetImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, *rerrors.Error) {
+func (is *imageStream) GetImageOfImageStream(ctx context.Context, dgst digest.Digest) (*imageapiv1.Image, rerrors.Error) {
 	image, tagEvent, err := is.getStoredImageOfImageStream(ctx, dgst)
 	if err != nil {
 		return nil, err
@@ -189,7 +189,7 @@ func (is *imageStream) GetImageOfImageStream(ctx context.Context, dgst digest.Di
 	return &img, nil
 }
 
-func (is *imageStream) GetSecrets() ([]corev1.Secret, *rerrors.Error) {
+func (is *imageStream) GetSecrets() ([]corev1.Secret, rerrors.Error) {
 	secrets, err := is.registryOSClient.ImageStreamSecrets(is.namespace).Secrets(context.TODO(), is.name, metav1.GetOptions{})
 	if err != nil {
 		return nil, rerrors.NewError(
@@ -203,7 +203,7 @@ func (is *imageStream) GetSecrets() ([]corev1.Secret, *rerrors.Error) {
 
 // TagIsInsecure returns true if the given image stream or its tag allow for
 // insecure transport.
-func (is *imageStream) TagIsInsecure(ctx context.Context, tag string, dgst digest.Digest) (bool, *rerrors.Error) {
+func (is *imageStream) TagIsInsecure(ctx context.Context, tag string, dgst digest.Digest) (bool, rerrors.Error) {
 	stream, err := is.imageStreamGetter.get()
 
 	if err != nil {
@@ -230,10 +230,10 @@ func (is *imageStream) TagIsInsecure(ctx context.Context, tag string, dgst diges
 	return false, nil
 }
 
-func (is *imageStream) Exists(ctx context.Context) (bool, *rerrors.Error) {
+func (is *imageStream) Exists(ctx context.Context) (bool, rerrors.Error) {
 	_, rErr := is.imageStreamGetter.get()
 	if rErr != nil {
-		if rErr.Code == ErrImageStreamGetterNotFoundCode {
+		if rErr.Code() == ErrImageStreamGetterNotFoundCode {
 			return false, nil
 		}
 		return false, convertImageStreamGetterError(rErr, fmt.Sprintf("Exists: failed to get image stream %s", is.Reference()))
@@ -241,7 +241,7 @@ func (is *imageStream) Exists(ctx context.Context) (bool, *rerrors.Error) {
 	return true, nil
 }
 
-func (is *imageStream) localRegistry(ctx context.Context) ([]string, *rerrors.Error) {
+func (is *imageStream) localRegistry(ctx context.Context) ([]string, rerrors.Error) {
 	stream, rErr := is.imageStreamGetter.get()
 	if rErr != nil {
 		return nil, convertImageStreamGetterError(rErr, fmt.Sprintf("localRegistry: failed to get image stream %s", is.Reference()))
@@ -270,7 +270,7 @@ func (is *imageStream) localRegistry(ctx context.Context) ([]string, *rerrors.Er
 	return localNames, nil
 }
 
-func (is *imageStream) IdentifyCandidateRepositories(ctx context.Context, primary bool) ([]string, map[string]ImagePullthroughSpec, *rerrors.Error) {
+func (is *imageStream) IdentifyCandidateRepositories(ctx context.Context, primary bool) ([]string, map[string]ImagePullthroughSpec, rerrors.Error) {
 	stream, err := is.imageStreamGetter.get()
 	if err != nil {
 		return nil, nil, convertImageStreamGetterError(err, fmt.Sprintf("IdentifyCandidateRepositories: failed to get image stream %s", is.Reference()))
@@ -282,7 +282,7 @@ func (is *imageStream) IdentifyCandidateRepositories(ctx context.Context, primar
 	return repositoryCandidates, search, nil
 }
 
-func (is *imageStream) Tags(ctx context.Context) (map[string]digest.Digest, *rerrors.Error) {
+func (is *imageStream) Tags(ctx context.Context) (map[string]digest.Digest, rerrors.Error) {
 	stream, err := is.imageStreamGetter.get()
 	if err != nil {
 		return nil, convertImageStreamGetterError(err, fmt.Sprintf("Tags: failed to get image stream %s", is.Reference()))
@@ -309,7 +309,7 @@ func (is *imageStream) Tags(ctx context.Context) (map[string]digest.Digest, *rer
 	return m, nil
 }
 
-func (is *imageStream) CreateImageStreamMapping(ctx context.Context, userClient client.Interface, tag string, image *imageapiv1.Image) *rerrors.Error {
+func (is *imageStream) CreateImageStreamMapping(ctx context.Context, userClient client.Interface, tag string, image *imageapiv1.Image) rerrors.Error {
 	ism := imageapiv1.ImageStreamMapping{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: is.namespace,
@@ -413,7 +413,7 @@ func (is *imageStream) CreateImageStreamMapping(ctx context.Context, userClient
 }
 
 // GetLimitRangeList returns list of limit ranges for repo.
-func (is *imageStream) GetLimitRangeList(ctx context.Context, cache ProjectObjectListStore) (*corev1.LimitRangeList, *rerrors.Error) {
+func (is *imageStream) GetLimitRangeList(ctx context.Context, cache ProjectObjectListStore) (*corev1.LimitRangeList, rerrors.Error) {
 	if cache != nil {
 		obj, exists, _ := cache.Get(is.namespace)
 		if exists {
@@ -442,10 +442,10 @@ func (is *imageStream) GetLimitRangeList(ctx context.Context, cache ProjectObjec
 	return lrs, nil
 }
 
-func convertImageStreamGetterError(err *rerrors.Error, msg string) *rerrors.Error {
+func convertImageStreamGetterError(err rerrors.Error, msg string) rerrors.Error {
 	code := ErrImageStreamUnknownErrorCode
 
-	switch err.Code {
+	switch err.Code() {
 	case ErrImageStreamGetterNotFoundCode:
 		code = ErrImageStreamNotFoundCode
 	case ErrImageStreamGetterForbiddenCode: