Prevent implicit Docker Hub resolution when auto-upgrade is turned on (#1845)

g-linville · web-flow · commit bc2a23daf708 · 2023-07-06T16:54:41.000-04:00
Signed-off-by: Grant Linville &lt;grant@acorn.io&gt;
diff --git a/integration/build/build_test.go b/integration/build/build_test.go
@@ -241,7 +241,7 @@ func TestBuildNestedAcornWithLocalImage(t *testing.T) {
 	}
 
 	// build the Nginx image
-	source := imagesource.NewImageSource("./testdata/nested/nginx.Acornfile", []string{}, []string{}, []string{})
+	source := imagesource.NewImageSource("./testdata/nested/nginx.Acornfile", []string{}, []string{}, []string{}, false)
 	image, _, err := source.GetImageAndDeployArgs(helper.GetCTX(t), c)
 	if err != nil {
 		t.Fatal(err)
diff --git a/integration/dev/dev_test.go b/integration/dev/dev_test.go
@@ -60,7 +60,7 @@ func TestDev(t *testing.T) {
 	eg := errgroup.Group{}
 	eg.Go(func() error {
 		return dev.Dev(subCtx, helper.BuilderClient(t, ns.Name), &dev.Options{
-			ImageSource: imagesource.NewImageSource(acornCueFile, []string{tmp}, nil, nil),
+			ImageSource: imagesource.NewImageSource(acornCueFile, []string{tmp}, nil, nil, false),
 			Run: client.AppRunOptions{
 				Name: "test-app",
 			},
diff --git a/integration/run/run_test.go b/integration/run/run_test.go
@@ -19,6 +19,7 @@ import (
 	"github.com/acorn-io/runtime/pkg/appdefinition"
 	"github.com/acorn-io/runtime/pkg/client"
 	"github.com/acorn-io/runtime/pkg/config"
+	"github.com/acorn-io/runtime/pkg/imagesource"
 	kclient "github.com/acorn-io/runtime/pkg/k8sclient"
 	"github.com/acorn-io/runtime/pkg/labels"
 	"github.com/acorn-io/runtime/pkg/run"
@@ -1698,3 +1699,55 @@ func TestAutoUpgradeImageValidation(t *testing.T) {
 	}
 	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")
 }
+
+func TestAutoUpgradeLocalImage(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)
+	}
+
+	// Attempt to run an auto-upgrade app with a non-existent local image. Should get an error.
+	_, err = c.AppRun(ctx, "mylocalimage", &client.AppRunOptions{
+		AutoUpgrade: &[]bool{true}[0],
+	})
+	if err == nil {
+		t.Fatalf("expected to get a not found error, instead got %v", err)
+	}
+	assert.ErrorContains(t, err, "could not find local image for mylocalimage - if you are trying to use a remote image, specify the full registry")
+
+	// Next, build the local image
+	image, err := c.AcornImageBuild(ctx, "./testdata/named/Acornfile", &client.AcornImageBuildOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Tag the image
+	err = c.ImageTag(ctx, image.ID, "mylocalimage")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Deploy the app
+	imageSource := imagesource.NewImageSource("", []string{"mylocalimage"}, []string{}, nil, true)
+	appImage, _, err := imageSource.GetImageAndDeployArgs(ctx, c)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = c.AppRun(ctx, appImage, &client.AppRunOptions{
+		AutoUpgrade: &[]bool{true}[0],
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+}
diff --git a/pkg/apis/api.acorn.io/v1/types.go b/pkg/apis/api.acorn.io/v1/types.go
@@ -208,6 +208,8 @@ type ImageDetails struct {
 	DeployArgs   v1.GenericMap `json:"deployArgs,omitempty"`
 	Profiles     []string      `json:"profiles,omitempty"`
 	Auth         *RegistryAuth `json:"auth,omitempty"`
+	// NoDefaultRegistry - if true, do not assume a default registry on the image if none is specified
+	NoDefaultRegistry bool `json:"noDefaultRegistry,omitempty"`
 
 	// Output Params
 	AppImage   v1.AppImage   `json:"appImage,omitempty"`
diff --git a/pkg/apis/internal.acorn.io/v1/appinstance.go b/pkg/apis/internal.acorn.io/v1/appinstance.go
@@ -172,22 +172,20 @@ func (a AppInstanceStatus) GetDevMode() bool {
 }
 
 type AppInstanceStatus struct {
-	DevSession                   *DevSessionInstanceSpec `json:"devSession,omitempty"`
-	ObservedGeneration           int64                   `json:"observedGeneration,omitempty"`
-	ObservedImageDigest          string                  `json:"observedImageDigest,omitempty"`
-	Columns                      AppColumns              `json:"columns,omitempty"`
-	Ready                        bool                    `json:"ready,omitempty"`
-	Namespace                    string                  `json:"namespace,omitempty"`
-	AppImage                     AppImage                `json:"appImage,omitempty"`
-	AvailableAppImage            string                  `json:"availableAppImage,omitempty"`
-	AvailableAppImageRemote      bool                    `json:"availableAppImageRemote,omitempty"`
-	ConfirmUpgradeAppImage       string                  `json:"confirmUpgradeAppImage,omitempty"`
-	ConfirmUpgradeAppImageRemote bool                    `json:"confirmUpgradeAppImageRemote,omitempty"`
-	AppSpec                      AppSpec                 `json:"appSpec,omitempty"`
-	AppStatus                    AppStatus               `json:"appStatus,omitempty"`
-	Scheduling                   map[string]Scheduling   `json:"scheduling,omitempty"`
-	Conditions                   []Condition             `json:"conditions,omitempty"`
-	Defaults                     Defaults                `json:"defaults,omitempty"`
+	DevSession             *DevSessionInstanceSpec `json:"devSession,omitempty"`
+	ObservedGeneration     int64                   `json:"observedGeneration,omitempty"`
+	ObservedImageDigest    string                  `json:"observedImageDigest,omitempty"`
+	Columns                AppColumns              `json:"columns,omitempty"`
+	Ready                  bool                    `json:"ready,omitempty"`
+	Namespace              string                  `json:"namespace,omitempty"`
+	AppImage               AppImage                `json:"appImage,omitempty"`
+	AvailableAppImage      string                  `json:"availableAppImage,omitempty"`
+	ConfirmUpgradeAppImage string                  `json:"confirmUpgradeAppImage,omitempty"`
+	AppSpec                AppSpec                 `json:"appSpec,omitempty"`
+	AppStatus              AppStatus               `json:"appStatus,omitempty"`
+	Scheduling             map[string]Scheduling   `json:"scheduling,omitempty"`
+	Conditions             []Condition             `json:"conditions,omitempty"`
+	Defaults               Defaults                `json:"defaults,omitempty"`
 }
 
 type Defaults struct {
diff --git a/pkg/autoupgrade/daemon.go b/pkg/autoupgrade/daemon.go
@@ -209,7 +209,6 @@ func (d *daemon) refreshImages(ctx context.Context, apps map[kclient.ObjectKey]v
 			// This satisfies the usecase of autoUpgrade with an app's tag is something static, like "latest"
 			// However, if the tag is a pattern and the current image has no tag, we don't want to check for a digest because this would
 			// result in a digest upgrade even though no tag matched.
-			var remote bool
 			if !updated && (!isPattern || current.Identifier() != "") {
 				nextAppImage = imageKey.image
 				var pullErr error
@@ -222,8 +221,6 @@ func (d *daemon) refreshImages(ctx context.Context, apps map[kclient.ObjectKey]v
 					if localDigest, ok, _ := d.client.resolveLocalTag(ctx, app.Namespace, imageKey.image); ok && localDigest != "" {
 						digest = localDigest
 					}
-				} else {
-					remote = true
 				}
 
 				if digest == "" && pullErr != nil {
@@ -252,18 +249,14 @@ func (d *daemon) refreshImages(ctx context.Context, apps map[kclient.ObjectKey]v
 						continue
 					}
 					app.Status.AvailableAppImage = nextAppImage
-					app.Status.AvailableAppImageRemote = remote
 					app.Status.ConfirmUpgradeAppImage = ""
-					app.Status.ConfirmUpgradeAppImageRemote = false
 				case "notify":
 					if app.Status.ConfirmUpgradeAppImage == nextAppImage {
 						d.appKeysPrevCheck[appKey] = updateTime
 						continue
 					}
 					app.Status.ConfirmUpgradeAppImage = nextAppImage
-					app.Status.ConfirmUpgradeAppImageRemote = remote
 					app.Status.AvailableAppImage = ""
-					app.Status.AvailableAppImageRemote = false
 				default:
 					logrus.Warnf("Unrecognized auto-upgrade mode %v for %v", mode, app.Name)
 					continue
diff --git a/pkg/cli/build.go b/pkg/cli/build.go
@@ -44,7 +44,7 @@ func (s *Build) Run(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	helper := imagesource.NewImageSource(s.File, args, s.Profile, s.Platform)
+	helper := imagesource.NewImageSource(s.File, args, s.Profile, s.Platform, false)
 	image, _, err := helper.GetImageAndDeployArgs(cmd.Context(), c)
 	if err != nil {
 		return err
diff --git a/pkg/cli/dev.go b/pkg/cli/dev.go
@@ -57,7 +57,7 @@ func (s *Dev) Run(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	imageSource := imagesource.NewImageSource(s.File, args, s.Profile, nil)
+	imageSource := imagesource.NewImageSource(s.File, args, s.Profile, nil, s.AutoUpgrade != nil && *s.AutoUpgrade)
 
 	opts, err := s.ToOpts()
 	if err != nil {
diff --git a/pkg/cli/render.go b/pkg/cli/render.go
@@ -32,7 +32,7 @@ func (s *Render) Run(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	imageAndArgs := imagesource.NewImageSource(s.File, args, s.Profile, nil)
+	imageAndArgs := imagesource.NewImageSource(s.File, args, s.Profile, nil, false)
 
 	appDef, _, err := imageAndArgs.GetAppDefinition(cmd.Context(), c)
 	if err != nil {
diff --git a/pkg/cli/run.go b/pkg/cli/run.go
@@ -229,7 +229,7 @@ func (s *Run) Run(cmd *cobra.Command, args []string) (err error) {
 	}
 
 	var (
-		imageSource = imagesource.NewImageSource(s.File, args, s.Profile, nil)
+		imageSource = imagesource.NewImageSource(s.File, args, s.Profile, nil, s.AutoUpgrade != nil && *s.AutoUpgrade)
 		app         *apiv1.App
 		updated     bool
 	)
diff --git a/pkg/client/client.go b/pkg/client/client.go
@@ -309,6 +309,8 @@ type ImageDetailsOptions struct {
 	Profiles     []string
 	DeployArgs   map[string]any
 	Auth         *apiv1.RegistryAuth
+	// NoDefaultRegistry - if true, indicates that no default container registry should be assumed when getting image details
+	NoDefaultRegistry bool
 }
 
 type ImageDeleteOptions struct {
diff --git a/pkg/client/image.go b/pkg/client/image.go
@@ -42,6 +42,7 @@ func (c *DefaultClient) ImageDetails(ctx context.Context, imageName string, opts
 		detailsResult.Profiles = opts.Profiles
 		detailsResult.NestedDigest = opts.NestedDigest
 		detailsResult.Auth = opts.Auth
+		detailsResult.NoDefaultRegistry = opts.NoDefaultRegistry
 	}
 
 	err := c.RESTClient.Post().
diff --git a/pkg/controller/appdefinition/pullappimage.go b/pkg/controller/appdefinition/pullappimage.go
@@ -55,14 +55,14 @@ func pullAppImage(transport http.RoundTripper, client pullClient) router.Handler
 			return nil
 		}
 
-		// Skip the attempt to locally resolve if we already know that the image will be remote
 		var (
 			_, autoUpgradeOn = autoupgrade.Mode(appInstance.Spec)
 			resolved         string
 			err              error
 			isLocal          bool
 		)
-		if !appInstance.Status.AvailableAppImageRemote {
+		// Only attempt to resolve locally if auto-upgrade is not on, or if auto-upgrade is on but we know the image is not remote.
+		if !autoUpgradeOn || !images.IsImageRemote(target, true, remote.WithTransport(transport)) {
 			resolved, isLocal, err = client.resolve(req.Ctx, req.Client, appInstance.Namespace, target)
 			if err != nil {
 				cond.Error(err)
diff --git a/pkg/controller/appdefinition/pullappimage_test.go b/pkg/controller/appdefinition/pullappimage_test.go
@@ -247,7 +247,7 @@ func testRecordPullEvent(t *testing.T, testName string, appInstance *v1.AppInsta
 		return nil
 	}
 
-	handler := pullAppImage(nil, pullClient{
+	handler := pullAppImage(mockRoundTripper{}, pullClient{
 		recorder: event.RecorderFunc(fakeRecorder),
 		resolve:  resolve,
 		pull:     pull,
diff --git a/pkg/imagedetails/imagedetails.go b/pkg/imagedetails/imagedetails.go
@@ -17,7 +17,7 @@ import (
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
-func GetImageDetails(ctx context.Context, c kclient.Client, namespace, imageName string, profiles []string, deployArgs map[string]any, nested string, opts ...remote.Option) (*apiv1.ImageDetails, error) {
+func GetImageDetails(ctx context.Context, c kclient.Client, namespace, imageName string, profiles []string, deployArgs map[string]any, nested string, noDefaultReg bool, opts ...remote.Option) (*apiv1.ImageDetails, error) {
 	imageName = strings.ReplaceAll(imageName, "+", "/")
 	name := strings.ReplaceAll(imageName, "/", "+")
 
@@ -43,7 +43,7 @@ func GetImageDetails(ctx context.Context, c kclient.Client, namespace, imageName
 	err := c.Get(ctx, router.Key(namespace, name), image)
 	if err != nil && !apierror.IsNotFound(err) {
 		return nil, err
-	} else if err != nil && apierror.IsNotFound(err) && tags.IsLocalReference(name) {
+	} else if err != nil && apierror.IsNotFound(err) && (tags.IsLocalReference(name) || (noDefaultReg && tags.HasNoSpecifiedRegistry(imageName))) {
 		return nil, err
 	} else if err == nil {
 		namespace = image.Namespace
diff --git a/pkg/images/operations.go b/pkg/images/operations.go
@@ -116,6 +116,29 @@ func ResolveTag(tag imagename.Reference, image string) string {
 	return image
 }
 
+// IsImageRemote checks the remote registry to see if the given image name exists.
+// If noDefaultRegistry is true, and the image does not have a specified registry, this function will return false
+// without attempting to check any remote registries.
+func IsImageRemote(image string, noDefaultRegistry bool, opts ...remote.Option) bool {
+	var (
+		ref imagename.Reference
+		err error
+	)
+	if noDefaultRegistry {
+		ref, err = imagename.ParseReference(image, imagename.WithDefaultRegistry(NoDefaultRegistry))
+	} else {
+		ref, err = imagename.ParseReference(image)
+	}
+
+	if err != nil || ref.Context().RegistryStr() == NoDefaultRegistry {
+		return false
+	}
+
+	_, err = remote.Index(ref, opts...)
+
+	return err == nil
+}
+
 func pullIndex(tag imagename.Reference, opts []remote.Option) (*v1.AppImage, error) {
 	img, err := remote.Index(tag, opts...)
 	if err != nil {
diff --git a/pkg/imagesource/helper.go b/pkg/imagesource/helper.go
@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/acorn-io/runtime/pkg/appdefinition"
+	"github.com/acorn-io/runtime/pkg/autoupgrade"
 	"github.com/acorn-io/runtime/pkg/build"
 	"github.com/acorn-io/runtime/pkg/client"
 	"github.com/acorn-io/runtime/pkg/config"
@@ -21,13 +22,20 @@ type ImageSource struct {
 	Args      []string
 	Profiles  []string
 	Platforms []string
+	// NoDefaultRegistry - if true, indicates that no container registry should be assumed for the Image.
+	// This is used if the ImageSource is for an app with auto-upgrade enabled.
+	NoDefaultRegistry bool
 }
 
-func NewImageSource(file string, args, profiles, platforms []string) (result ImageSource) {
+func NewImageSource(file string, args, profiles, platforms []string, noDefaultReg bool) (result ImageSource) {
 	result.File = file
 	result.Image, result.Args = splitImageAndArgs(args)
 	result.Profiles = profiles
 	result.Platforms = platforms
+
+	// If the image is a pattern, auto-upgrade is on, so assume no default registry
+	_, isPattern := autoupgrade.AutoUpgradePattern(result.Image)
+	result.NoDefaultRegistry = noDefaultReg || isPattern
 	return
 }
 
@@ -64,7 +72,7 @@ func (i ImageSource) GetAppDefinition(ctx context.Context, c client.Client) (*ap
 	)
 	if file == "" {
 		sourceName = image
-		imageDetails, err := c.ImageDetails(ctx, image, nil)
+		imageDetails, err := c.ImageDetails(ctx, image, &client.ImageDetailsOptions{NoDefaultRegistry: i.NoDefaultRegistry})
 		if err != nil {
 			return nil, nil, err
 		}
diff --git a/pkg/imagesource/platforms_test.go b/pkg/imagesource/platforms_test.go
@@ -24,7 +24,7 @@ func TestParamsHelp(t *testing.T) {
 		"--i-default=3",
 		"--complex",
 		"@testdata/params/test.cue",
-	}, nil, nil).GetAppDefinition(context.Background(), nil)
+	}, nil, nil, false).GetAppDefinition(context.Background(), nil)
 	assert.Equal(t, pflag.ErrHelp, err)
 }
 
@@ -42,7 +42,7 @@ func TestParams(t *testing.T) {
 		"--i-default=3",
 		"--complex",
 		"@testdata/params/test.cue",
-	}, nil, nil).GetAppDefinition(context.Background(), nil)
+	}, nil, nil, false).GetAppDefinition(context.Background(), nil)
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/pkg/openapi/generated/openapi_generated.go b/pkg/openapi/generated/openapi_generated.go
diff --git a/pkg/server/registry/apigroups/acorn/apps/confirmupgrade.go b/pkg/server/registry/apigroups/acorn/apps/confirmupgrade.go
@@ -39,7 +39,6 @@ func (s *ConfirmUpgradeStrategy) Create(ctx context.Context, obj types.Object) (
 		return nil, err
 	}
 	app.Status.AvailableAppImage = app.Status.ConfirmUpgradeAppImage
-	app.Status.AvailableAppImageRemote = app.Status.ConfirmUpgradeAppImageRemote
 
 	err = s.client.Status().Update(ctx, app)
 	if err != nil {
diff --git a/pkg/server/registry/apigroups/acorn/apps/validator.go b/pkg/server/registry/apigroups/acorn/apps/validator.go
diff --git a/pkg/server/registry/apigroups/acorn/images/detail.go b/pkg/server/registry/apigroups/acorn/images/detail.go
diff --git a/pkg/tags/tags.go b/pkg/tags/tags.go

Original file line number	Diff line number	Diff line change
`@@ -241,7 +241,7 @@ func TestBuildNestedAcornWithLocalImage(t *testing.T) {`
`241`	`241`	`}`
`242`	`242`
`243`	`243`	`// build the Nginx image`
`244`		`- source := imagesource.NewImageSource("./testdata/nested/nginx.Acornfile", []string{}, []string{}, []string{})`
	`244`	`+ source := imagesource.NewImageSource("./testdata/nested/nginx.Acornfile", []string{}, []string{}, []string{}, false)`
`245`	`245`	`image, _, err := source.GetImageAndDeployArgs(helper.GetCTX(t), c)`
`246`	`246`	`if err != nil {`
`247`	`247`	`t.Fatal(err)`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ func (s Build) Run(cmd cobra.Command, args []string) error {`
`44`	`44`	`return err`
`45`	`45`	`}`
`46`	`46`
`47`		`- helper := imagesource.NewImageSource(s.File, args, s.Profile, s.Platform)`
	`47`	`+ helper := imagesource.NewImageSource(s.File, args, s.Profile, s.Platform, false)`
`48`	`48`	`image, _, err := helper.GetImageAndDeployArgs(cmd.Context(), c)`
`49`	`49`	`if err != nil {`
`50`	`50`	`return err`
Original file line number	Diff line number	Diff line change
`@@ -57,7 +57,7 @@ func (s Dev) Run(cmd cobra.Command, args []string) error {`
`57`	`57`	`return err`
`58`	`58`	`}`
`59`	`59`
`60`		`- imageSource := imagesource.NewImageSource(s.File, args, s.Profile, nil)`
	`60`	`+ imageSource := imagesource.NewImageSource(s.File, args, s.Profile, nil, s.AutoUpgrade != nil && *s.AutoUpgrade)`
`61`	`61`
`62`	`62`	`opts, err := s.ToOpts()`
`63`	`63`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -32,7 +32,7 @@ func (s Render) Run(cmd cobra.Command, args []string) error {`
`32`	`32`	`return err`
`33`	`33`	`}`
`34`	`34`
`35`		`- imageAndArgs := imagesource.NewImageSource(s.File, args, s.Profile, nil)`
	`35`	`+ imageAndArgs := imagesource.NewImageSource(s.File, args, s.Profile, nil, false)`
`36`	`36`
`37`	`37`	`appDef, _, err := imageAndArgs.GetAppDefinition(cmd.Context(), c)`
`38`	`38`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -229,7 +229,7 @@ func (s Run) Run(cmd cobra.Command, args []string) (err error) {`
`229`	`229`	`}`
`230`	`230`
`231`	`231`	`var (`
`232`		`- imageSource = imagesource.NewImageSource(s.File, args, s.Profile, nil)`
	`232`	`+ imageSource = imagesource.NewImageSource(s.File, args, s.Profile, nil, s.AutoUpgrade != nil && *s.AutoUpgrade)`
`233`	`233`	`app *apiv1.App`
`234`	`234`	`updated bool`
`235`	`235`	`)`