Merge pull request #75 from grammarly/f-fix-s3-name-schema

Fix S3 naming schema

src/build/build.go

@@ -270,6 +270,9 @@ func (b *Build) getExportsContainer() (c *docker.Container, err error) {
 //
 // See also TestBuild_LookupImage_* test cases in build_test.go
 func (b *Build) lookupImage(name string) (img *docker.Image, err error) {
+	if isOld, warning := imagename.WarnIfOldS3ImageName(name); isOld {
+		log.Warn(warning)
+	}
 	var (
 		candidate, remoteCandidate *imagename.ImageName
 

src/build/client.go

@@ -120,6 +120,10 @@ func (c *DockerClient) PullImage(name string) error {
 
 	// e.g. s3:bucket-name/image-name
 	if image.Storage == imagename.StorageS3 {
+		if isOld, warning := imagename.WarnIfOldS3ImageName(name); isOld {
+			c.log.Warn(warning)
+		}
+
 		return c.s3storage.Pull(name)
 	}
 
@@ -183,6 +187,9 @@ func (c *DockerClient) ListImages() (images []*imagename.ImageName, err error) {
 func (c *DockerClient) ListImageTags(name string) (images []*imagename.ImageName, err error) {
 	img := imagename.NewFromString(name)
 	if img.Storage == imagename.StorageS3 {
+		if isOld, warning := imagename.WarnIfOldS3ImageName(name); isOld {
+			c.log.Warn(warning)
+		}
 		return c.s3storage.ListTags(name)
 	}
 	return dockerclient.RegistryListTags(imagename.NewFromString(name), c.auth)
@@ -432,6 +439,9 @@ func (c *DockerClient) UploadToContainer(containerID string, stream io.Reader, p
 // TagImage adds tag to the image
 func (c *DockerClient) TagImage(imageID, imageName string) error {
 	img := imagename.NewFromString(imageName)
+	if isOld, warning := imagename.WarnIfOldS3ImageName(imageName); isOld {
+		c.log.Warn(warning)
+	}
 
 	c.log.Infof("| Tag %.12s -> %s", imageID, img)
 
@@ -475,6 +485,9 @@ func (c *DockerClient) pushImageInner(imageName string) (digest string, err erro
 
 	// Use direct S3 image pusher instead
 	if img.Storage == imagename.StorageS3 {
+		if isOld, warning := imagename.WarnIfOldS3ImageName(imageName); isOld {
+			c.log.Warn(warning)
+		}
 		return c.s3storage.Push(imageName)
 	}
 

src/build/commands.go

@@ -997,7 +997,7 @@ func (c *CommandPush) Execute(b *Build) (State, error) {
 		filePath := filepath.Join(b.cfg.ArtifactsPath, artifact.GetFileName())
 
 		artifacts := imagename.Artifacts{
-			[]imagename.Artifact{artifact},
+			RockerArtifacts: []imagename.Artifact{artifact},
 		}
 		content, err := yaml.Marshal(artifacts)
 		if err != nil {

src/imagename/imagename.go

@@ -21,6 +21,7 @@ package imagename
 
 import (
 	"encoding/json"
+	"fmt"
 	"regexp"
 	"sort"
 	"strings"
@@ -44,6 +45,11 @@ const (
 	StorageS3 = "s3"
 )
 
+const (
+	s3Prefix    = "s3.amazonaws.com/"
+	s3OldPrefix = "s3:"
+)
+
 var (
 	ecrRe = regexp.MustCompile("^\\d+\\.dkr\\.ecr\\.[^\\.]+\\.amazonaws\\.com$")
 )
@@ -55,6 +61,8 @@ type ImageName struct {
 	Tag      string
 	Storage  string
 	Version  *semver.Range
+
+	isOld bool
 }
 
 // NewFromString parses a given string and returns ImageName
@@ -63,6 +71,33 @@ func NewFromString(image string) *ImageName {
 	return New(name, tag)
 }
 
+// isOldS3ImageName Check whether an s3 image is referenced by old schema
+func isOldS3ImageName(imageName string) bool {
+	return strings.HasPrefix(imageName, s3OldPrefix)
+}
+
+// WarnIfOldS3ImageName Check whether old image format is used. Also return warning message if yes
+func WarnIfOldS3ImageName(imageName string) (bool, string) {
+	if !isOldS3ImageName(imageName) {
+		return false, ""
+	}
+
+	warning := fmt.Sprintf("Your image '%s' is using old name style (s3:<repo>/<image>) for s3 images."+
+		" This style isn't supported by docker 1.10 and would be removed from rocker in the future as well."+
+		" Please consider changing to the new schema (s3.amazonaws.com/<repo>/<image>)."+
+		" For now, I'll do the conversion internally to not break your old Rockerfiles", imageName)
+
+	return true, warning
+}
+
+func (img *ImageName) makeOldS3Compatible(image string) string {
+	if isOldS3ImageName(image) {
+		img.isOld = true
+		return strings.Replace(image, s3OldPrefix, s3Prefix, 1)
+	}
+	return image
+}
+
 // New parses a given 'image' and 'tag' strings and returns ImageName
 func New(image string, tag string) *ImageName {
 	dockerImage := &ImageName{}
@@ -74,19 +109,17 @@ func New(image string, tag string) *ImageName {
 	// default storage driver
 	dockerImage.Storage = StorageRegistry
 
-	// In case storage is specified, e.g. s3://bucket-name/image-name
-	storages := []string{StorageRegistry, StorageS3}
-	firstIsHost := false
+	//Replace 's3:' to 's3.amazonaws.com/' if any.
+	//We are doing it for backward compatibility reasons
+	//In future this function should be removed
+	image = dockerImage.makeOldS3Compatible(image)
 
-	for _, storage := range storages {
-		prefix := storage + ":"
+	firstIsHost := false
 
-		if strings.HasPrefix(image, prefix) {
-			image = strings.TrimPrefix(image, prefix)
-			dockerImage.Storage = storage
-			firstIsHost = true
-			break
-		}
+	if strings.HasPrefix(image, s3Prefix) {
+		dockerImage.Storage = StorageS3
+		firstIsHost = true
+		image = strings.TrimPrefix(image, s3Prefix)
 	}
 
 	nameParts := strings.SplitN(image, "/", 2)
@@ -234,7 +267,7 @@ func (img ImageName) TagAsVersion() (ver *semver.Version) {
 
 // IsSameKind returns true if current image and the given one are same but may have different versions (tags)
 func (img ImageName) IsSameKind(b ImageName) bool {
-	return img.Registry == b.Registry && img.Name == b.Name
+	return img.Registry == b.Registry && img.Name == b.Name && img.isOld == b.isOld
 }
 
 // NameWithRegistry returns the [registry/]name of the current image name
@@ -243,8 +276,8 @@ func (img ImageName) NameWithRegistry() string {
 	if img.Registry != "" {
 		registryPrefix = img.Registry + "/"
 	}
-	if img.Storage != StorageRegistry {
-		registryPrefix = img.Storage + ":" + registryPrefix
+	if img.Storage == StorageS3 {
+		registryPrefix = s3Prefix + registryPrefix
 	}
 	return registryPrefix + img.Name
 }

src/imagename/imagename_test.go

@@ -493,34 +493,66 @@ func TestImagename_ToYaml(t *testing.T) {
 	assert.Equal(t, "name: hub/ns/name:1\n", string(data))
 }
 
-func TestImagename_S3_Basic(t *testing.T) {
+func TestImagename_S3_Basic_Old(t *testing.T) {
 	img := NewFromString("s3:bucket-name/image-name:1.2.3")
 	assert.Equal(t, "bucket-name", img.Registry)
 	assert.Equal(t, "image-name", img.Name)
 	assert.Equal(t, false, img.TagIsSha())
 	assert.Equal(t, "1.2.3", img.GetTag())
-	assert.Equal(t, "s3:bucket-name/image-name", img.NameWithRegistry())
-	assert.Equal(t, "s3:bucket-name/image-name:1.2.3", img.String())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name:1.2.3", img.String())
 }
 
-func TestImagename_S3_Digest(t *testing.T) {
+func TestImagename_S3_Digest_Old(t *testing.T) {
 	img := NewFromString("s3:bucket-name/image-name@sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11")
 	assert.Equal(t, "bucket-name", img.Registry)
 	assert.Equal(t, "image-name", img.Name)
 	assert.Equal(t, true, img.TagIsSha())
 	assert.Equal(t, true, img.TagIsDigest())
-	assert.Equal(t, "s3:bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
 	assert.Equal(t, "sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.GetTag())
-	assert.Equal(t, "s3:bucket-name/image-name@sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name@sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
 }
 
-func TestImagename_S3_Sha(t *testing.T) {
+func TestImagename_S3_Sha_Old(t *testing.T) {
 	img := NewFromString("s3:bucket-name/image-name:sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11")
 	assert.Equal(t, "bucket-name", img.Registry)
 	assert.Equal(t, "image-name", img.Name)
 	assert.Equal(t, true, img.TagIsSha())
 	assert.Equal(t, false, img.TagIsDigest())
-	assert.Equal(t, "s3:bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.GetTag())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name:sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
+}
+
+func TestImagename_S3_Basic(t *testing.T) {
+	img := NewFromString("s3.amazonaws.com/bucket-name/image-name:1.2.3")
+	assert.Equal(t, "bucket-name", img.Registry)
+	assert.Equal(t, "image-name", img.Name)
+	assert.Equal(t, false, img.TagIsSha())
+	assert.Equal(t, "1.2.3", img.GetTag())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name:1.2.3", img.String())
+}
+
+func TestImagename_S3_Digest(t *testing.T) {
+	img := NewFromString("s3.amazonaws.com/bucket-name/image-name@sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11")
+	assert.Equal(t, "bucket-name", img.Registry)
+	assert.Equal(t, "image-name", img.Name)
+	assert.Equal(t, true, img.TagIsSha())
+	assert.Equal(t, true, img.TagIsDigest())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
+	assert.Equal(t, "sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.GetTag())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name@sha256:ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
+}
+
+func TestImagename_S3_Sha(t *testing.T) {
+	img := NewFromString("s3.amazonaws.com/bucket-name/image-name:sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11")
+	assert.Equal(t, "bucket-name", img.Registry)
+	assert.Equal(t, "image-name", img.Name)
+	assert.Equal(t, true, img.TagIsSha())
+	assert.Equal(t, false, img.TagIsDigest())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name", img.NameWithRegistry())
 	assert.Equal(t, "sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.GetTag())
-	assert.Equal(t, "s3:bucket-name/image-name:sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
+	assert.Equal(t, "s3.amazonaws.com/bucket-name/image-name:sha256-ead434cd278824865d6e3b67e5d4579ded02eb2e8367fc165efa21138b225f11", img.String())
 }

src/storage/s3/s3.go

@@ -155,7 +155,7 @@ func (s *StorageS3) Push(imageName string) (digest string, err error) {
 		}
 		defer fd.Close()
 
-		log.Infof("| Uploading image to s3://%s/%s", img.Registry, imgPathDigest)
+		log.Infof("| Uploading image to s3.amazonaws.com/%s/%s", img.Registry, imgPathDigest)
 
 		uploadParams := &s3manager.UploadInput{
 			Bucket:      aws.String(img.Registry),
@@ -184,7 +184,7 @@ func (s *StorageS3) Push(imageName string) (digest string, err error) {
 		Key:        aws.String(imgPathTag),
 	}
 
-	log.Infof("| Make alias s3://%s/%s", img.Registry, imgPathTag)
+	log.Infof("| Make alias s3.amazonaws.com/%s/%s", img.Registry, imgPathTag)
 
 	if _, err = s.s3.CopyObject(copyParams); err != nil {
 		return "", fmt.Errorf("Failed to PUT object to S3, error: %s", err)
@@ -226,7 +226,7 @@ func (s *StorageS3) Pull(name string) error {
 		}
 	)
 
-	log.Infof("| Import s3://%s/%s to %s", img.Registry, imgPath, tmpf.Name())
+	log.Infof("| Import s3.amazonaws.com/%s/%s to %s", img.Registry, imgPath, tmpf.Name())
 
 	if err := s.retryer.Outer(func() error {
 		_, err := downloader.Download(tmpf, downloadParams)
@@ -483,7 +483,7 @@ func (s *StorageS3) ListTags(imageName string) (images []*imagename.ImageName, e
 		}
 
 		imgName := strings.Join(split[:len(split)-1], "/")
-		imgName = fmt.Sprintf("s3:%s/%s", image.Registry, imgName)
+		imgName = fmt.Sprintf("s3.amazonaws.com/%s/%s", image.Registry, imgName)
 
 		tag := strings.TrimSuffix(split[len(split)-1], ".tar")
 		candidate := imagename.New(imgName, tag)