diff --git a/integration/client/images/images_test.go b/integration/client/images/images_test.go index a6f8bef14..a73bef732 100644 --- a/integration/client/images/images_test.go +++ b/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) { diff --git a/integration/run/run_test.go b/integration/run/run_test.go index 1091435b2..bbf4d2562 100644 --- a/integration/run/run_test.go +++ b/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") +} diff --git a/pkg/controller/appdefinition/pullappimage.go b/pkg/controller/appdefinition/pullappimage.go index 1e7fff12e..ca643fc01 100644 --- a/pkg/controller/appdefinition/pullappimage.go +++ b/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 diff --git a/pkg/controller/appdefinition/pullappimage_test.go b/pkg/controller/appdefinition/pullappimage_test.go index 5e95f257a..8833993ca 100644 --- a/pkg/controller/appdefinition/pullappimage_test.go +++ b/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 +} diff --git a/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/existing.yaml b/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/existing.yaml new file mode 100644 index 000000000..2af8e56aa --- /dev/null +++ b/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 diff --git a/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/expected.golden b/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/expected.golden new file mode 100644 index 000000000..a8ac60a6f --- /dev/null +++ b/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 +` diff --git a/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/input.yaml b/pkg/controller/appdefinition/testdata/autoupgrade/with-local-image/input.yaml new file mode 100644 index 000000000..09605a938 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/controller/appdefinition/testdata/autoupgrade/without-local-image/input.yaml b/pkg/controller/appdefinition/testdata/autoupgrade/without-local-image/input.yaml new file mode 100644 index 000000000..09605a938 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/imagedetails/imagedetails.go b/pkg/imagedetails/imagedetails.go index 067c8da70..3a15b94b0 100644 --- a/pkg/imagedetails/imagedetails.go +++ b/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 diff --git a/pkg/images/operations.go b/pkg/images/operations.go index 561178271..be96fabe4 100644 --- a/pkg/images/operations.go +++ b/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 diff --git a/pkg/server/registry/apigroups/acorn/apps/validator.go b/pkg/server/registry/apigroups/acorn/apps/validator.go index b362f181d..14ec6d696 100644 --- a/pkg/server/registry/apigroups/acorn/apps/validator.go +++ b/pkg/server/registry/apigroups/acorn/apps/validator.go @@ -18,6 +18,7 @@ import ( "github.com/acorn-io/runtime/pkg/computeclasses" apiv1config "github.com/acorn-io/runtime/pkg/config" "github.com/acorn-io/runtime/pkg/imageallowrules" + "github.com/acorn-io/runtime/pkg/images" "github.com/acorn-io/runtime/pkg/imagesystem" "github.com/acorn-io/runtime/pkg/labels" "github.com/acorn-io/runtime/pkg/pullsecret" @@ -86,6 +87,21 @@ func (s *Validator) Validate(ctx context.Context, obj runtime.Object) (result fi } if !local { + if _, autoUpgradeOn := autoupgrade.Mode(params.Spec); autoUpgradeOn { + // Make sure there is a registry specified here + // If there isn't one, return an error in order to avoid checking Docker Hub implicitly + ref, err := name.ParseReference(params.Spec.Image, name.WithDefaultRegistry(images.NoDefaultRegistry)) + if err != nil { + result = append(result, field.InternalError(field.NewPath("spec", "image"), err)) + return + } + + if ref.Context().RegistryStr() == images.NoDefaultRegistry { + result = append(result, field.Invalid(field.NewPath("spec", "image"), params.Spec.Image, + fmt.Sprintf("could not find local image for %v - if you are trying to use a remote image, specify the full registry", params.Spec.Image))) + } + } + if err := s.checkRemoteAccess(ctx, params.Namespace, image); err != nil { result = append(result, field.Invalid(field.NewPath("spec", "image"), params.Spec.Image, err.Error())) return