Prevent Acorn from assuming Docker Hub for auto-upgrade apps with no specified registry (#1427) (#1823)

Signed-off-by: Grant Linville <[email protected]>

Author: g-linville

integration/client/images/images_test.go

@@ -325,6 +325,13 @@ func TestImageDetails(t *testing.T) {
 	}
 
 	assert.True(t, strings.Contains(details.AppImage.Acornfile, "nginx"))
+
+	// Test an auto-upgrade pattern that matches no local images, and make sure the proper error is returned
+	_, err = c.ImageDetails(ctx, "dne:v#.#.#", nil)
+	if err == nil {
+		t.Fatal("expected error for auto-upgrade pattern that matches no local images")
+	}
+	assert.ErrorContains(t, err, "unable to find an image for dne:v#.#.# matching pattern v#.#.# - if you are trying to use a remote image, specify the full registry")
 }
 
 func TestImageDeleteTwoTags(t *testing.T) {

integration/run/run_test.go

@@ -1662,3 +1662,39 @@ func TestEnforcedQuota(t *testing.T) {
 		return obj.Status.Condition(v1.AppInstanceConditionQuotaAllocated).Success
 	})
 }
+
+func TestAutoUpgradeImageValidation(t *testing.T) {
+	ctx := helper.GetCTX(t)
+
+	helper.StartController(t)
+	restConfig, err := restconfig.New(scheme.Scheme)
+	if err != nil {
+		t.Fatal("error while getting rest config:", err)
+	}
+	kclient := helper.MustReturn(kclient.Default)
+	project := helper.TempProject(t, kclient)
+
+	c, err := client.New(restConfig, project.Name, project.Name)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	app, err := c.AppRun(ctx, "ghcr.io/acorn-io/library/nginx:latest", &client.AppRunOptions{
+		Name:        "myapp",
+		AutoUpgrade: &[]bool{true}[0],
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Attempt to update the app to "myimage:latest".
+	// Since no image exists with this tag, it should fail.
+	// Auto-upgrade apps are not supposed to implicitly use Docker Hub when no registry is specified.
+	_, err = c.AppUpdate(ctx, app.Name, &client.AppUpdateOptions{
+		Image: "myimage:latest",
+	})
+	if err == nil {
+		t.Fatal("expected error when failing to find local image for auto-upgrade app, got no error")
+	}
+	assert.ErrorContains(t, err, "could not find local image for myimage:latest - if you are trying to use a remote image, specify the full registry")
+}

pkg/controller/appdefinition/pullappimage.go

@@ -13,6 +13,7 @@ import (
 	"github.com/acorn-io/runtime/pkg/event"
 	"github.com/acorn-io/runtime/pkg/images"
 	"github.com/acorn-io/runtime/pkg/tags"
+	imagename "github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -56,9 +57,10 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler
 
 		// Skip the attempt to locally resolve if we already know that the image will be remote
 		var (
-			resolved string
-			err      error
-			isLocal  bool
+			_, autoUpgradeOn = autoupgrade.Mode(appInstance.Spec)
+			resolved         string
+			err              error
+			isLocal          bool
 		)
 		if !appInstance.Status.AvailableAppImageRemote {
 			resolved, isLocal, err = client.resolve(req.Ctx, req.Client, appInstance.Namespace, target)
@@ -67,6 +69,17 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler
 				return nil
 			}
 			if !isLocal {
+				if autoUpgradeOn && !tags.IsLocalReference(target) {
+					ref, err := imagename.ParseReference(target, imagename.WithDefaultRegistry(images.NoDefaultRegistry))
+					if err != nil {
+						return err
+					}
+					if ref.Context().RegistryStr() == images.NoDefaultRegistry {
+						// Prevent this from being resolved remotely, as we should never assume Docker Hub for auto-upgrade apps
+						return fmt.Errorf("no local image found for %v - if you are trying to use a remote image, specify the full registry", target)
+					}
+				}
+
 				// Force pull from remote, since the only local image we found was marked remote, and there might be a newer version
 				resolved = target
 			}
@@ -75,9 +88,8 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler
 		}
 
 		var (
-			_, autoUpgradeOn = autoupgrade.Mode(appInstance.Spec)
-			previousImage    = appInstance.Status.AppImage
-			targetImage      *v1.AppImage
+			previousImage = appInstance.Status.AppImage
+			targetImage   *v1.AppImage
 		)
 		defer func() {
 			// Record the results as an event

pkg/controller/appdefinition/pullappimage_test.go

@@ -3,13 +3,16 @@ package appdefinition
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"testing"
 
+	"github.com/acorn-io/baaah/pkg/router"
 	"github.com/acorn-io/baaah/pkg/router/tester"
 	apiv1 "github.com/acorn-io/runtime/pkg/apis/api.acorn.io/v1"
 	v1 "github.com/acorn-io/runtime/pkg/apis/internal.acorn.io/v1"
 	"github.com/acorn-io/runtime/pkg/event"
 	"github.com/acorn-io/runtime/pkg/scheme"
+	"github.com/acorn-io/runtime/pkg/tags"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -270,3 +273,49 @@ func testRecordPullEvent(t *testing.T, testName string, appInstance *v1.AppInsta
 		assert.EqualValues(t, expect, recording[0])
 	})
 }
+
+func TestAutoUpgradeImageResolution(t *testing.T) {
+	// Auto-upgrade apps are not supposed to use Docker Hub implicitly.
+	// In this test, we create an auto-upgrade App with the image "myimage:latest".
+	// In the first test case, this image exists locally and should be resolved properly.
+	// In the second test case, no such image exists locally, and Acorn should not reach out to Docker Hub, and should instead return an error.
+
+	fakeRecorder := func(_ context.Context, _ *apiv1.Event) error {
+		return nil
+	}
+
+	// First, test to make sure that the local image is properly resolved
+	tester.DefaultTest(t, scheme.Scheme, "testdata/autoupgrade/with-local-image", testPullAppImage(mockRoundTripper{}, event.RecorderFunc(fakeRecorder)))
+
+	// Next, test to make sure that Docker Hub is not implicitly used when no local image is found
+	// There should be a helpful error message instead
+	harness, obj, err := tester.FromDir(scheme.Scheme, "testdata/autoupgrade/without-local-image")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = harness.InvokeFunc(t, obj, testPullAppImage(mockRoundTripper{}, event.RecorderFunc(fakeRecorder)))
+	if err == nil {
+		t.Fatalf("expected error when no local image was found for auto-upgrade app without a specified registry")
+	}
+	assert.ErrorContains(t, err, "no local image found for myimage:latest - if you are trying to use a remote image, specify the full registry")
+}
+
+func testPullAppImage(transport http.RoundTripper, recorder event.Recorder) router.HandlerFunc {
+	return pullAppImage(transport, pullClient{
+		recorder: recorder,
+		resolve:  tags.ResolveLocal,
+		pull: func(_ context.Context, _ kclient.Reader, _ string, _ string, _ string, _ ...remote.Option) (*v1.AppImage, error) {
+			return &v1.AppImage{
+				Name:   "myimage:latest",
+				Digest: "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
+			}, nil
+		},
+		now: metav1.NowMicro,
+	})
+}
+
+type mockRoundTripper struct{}
+
+func (m mockRoundTripper) RoundTrip(_ *http.Request) (*http.Response, error) {
+	return nil, nil
+}

pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/existing.yaml

@@ -0,0 +1,9 @@
+---
+apiVersion: api.acorn.io/v1
+kind: Image
+metadata:
+  name: myimage:latest
+  namespace: app-namespace
+digest: sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+tags:
+  - myimage:latest

pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/expected.golden

@@ -0,0 +1,27 @@
+`apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  creationTimestamp: null
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  autoUpgrade: true
+  image: myimage:latest
+status:
+  appImage:
+    digest: sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
+    imageData: {}
+    name: myimage:latest
+    vcs: {}
+  appSpec: {}
+  appStatus: {}
+  columns: {}
+  conditions:
+    reason: Success
+    status: "True"
+    success: true
+    type: image-pull
+  defaults: {}
+  namespace: app-created-namespace
+`

pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/input.yaml

@@ -0,0 +1,12 @@
+---
+apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  autoUpgrade: true
+  image: myimage:latest
+status:
+  namespace: app-created-namespace

pkg/controller/appdefinition/testdata/autoupgrade/without-local-image/input.yaml

@@ -0,0 +1,12 @@
+---
+apiVersion: internal.acorn.io/v1
+kind: AppInstance
+metadata:
+  name: app-name
+  namespace: app-namespace
+  uid: 1234567890abcdef
+spec:
+  autoUpgrade: true
+  image: myimage:latest
+status:
+  namespace: app-created-namespace

pkg/imagedetails/imagedetails.go

@@ -10,6 +10,7 @@ import (
 	"github.com/acorn-io/runtime/pkg/autoupgrade"
 	"github.com/acorn-io/runtime/pkg/images"
 	"github.com/acorn-io/runtime/pkg/tags"
+	imagename "github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/v1/remote"
 	apierror "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -24,6 +25,13 @@ func GetImageDetails(ctx context.Context, c kclient.Client, namespace, imageName
 		if latestImage, found, err := autoupgrade.FindLatestTagForImageWithPattern(ctx, c, "", namespace, imageName, tagPattern); err != nil {
 			return nil, err
 		} else if !found {
+			// Check and see if no registry was specified on the image.
+			// If this is the case, notify the user that they need to explicitly specify docker.io if that is what they are trying to use.
+			ref, err := imagename.ParseReference(strings.TrimSuffix(imageName, ":"+tagPattern), imagename.WithDefaultRegistry(images.NoDefaultRegistry))
+			if err == nil && ref.Context().Registry.Name() == images.NoDefaultRegistry {
+				return nil, fmt.Errorf("unable to find an image for %v matching pattern %v - if you are trying to use a remote image, specify the full registry", imageName, tagPattern)
+			}
+
 			return nil, fmt.Errorf("unable to find an image for %v matching pattern %v", imageName, tagPattern)
 		} else {
 			imageName = latestImage

pkg/images/operations.go

@@ -87,15 +87,15 @@ func PullAppImage(ctx context.Context, c client.Reader, namespace, image, nested
 	return appImage, nil
 }
 
-const DefaultRegistry = "NO_DEFAULT"
+const NoDefaultRegistry = "xxx-no-reg"
 
 func ParseReferenceNoDefault(name string) (imagename.Reference, error) {
-	ref, err := imagename.ParseReference(name, imagename.WithDefaultRegistry(DefaultRegistry))
+	ref, err := imagename.ParseReference(name, imagename.WithDefaultRegistry(NoDefaultRegistry))
 	if err != nil {
 		return nil, err
 	}
 
-	if ref.Context().RegistryStr() == DefaultRegistry {
+	if ref.Context().RegistryStr() == NoDefaultRegistry {
 		return nil, fmt.Errorf("missing registry host in the tag [%s] (ie ghcr.io or docker.io)", name)
 	}
 	return ref, nil