Merge pull request kubernetes#34375 from euank/rkt-image-with-port

Automatic merge from submit-queue rkt: Convert image name to be a valid acidentifier **Release note**:  ```release-note Fix a bug under the rkt runtime whereby image-registries with ports would not be fetched from ``` This fixes a bug whereby an image reference that included a port was not recognized after being downloaded, and so could not be run This is the quick-and-simple fix. In the longer term, we'll want to refactor image logic a bit more to handle the many special cases that the current code does not, mostly related to library images on dockerhub. /cc @yifan-gu @kubernetes/sig-rktnetes
markturansky · Oct 20, 2016 · c04aec3 · c04aec3
2 parents 35943d6 + aff6940
commit c04aec3
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 13 deletions.
diff --git a/pkg/kubelet/rkt/image.go b/pkg/kubelet/rkt/image.go
@@ -28,6 +28,7 @@ import (
 	"strings"
 
 	appcschema "github.com/appc/spec/schema"
+	appctypes "github.com/appc/spec/schema/types"
 	rktapi "github.com/coreos/rkt/api/v1alpha"
 	dockertypes "github.com/docker/engine-api/types"
 	"github.com/golang/glog"
@@ -124,7 +125,25 @@ func (r *Runtime) RemoveImage(image kubecontainer.ImageSpec) error {
 }
 
 // buildImageName constructs the image name for kubecontainer.Image.
+// If the annotations contain the docker2aci metadata for this image, those are
+// used instead as they may be more accurate in some cases, namely if a
+// non-appc valid character is present
 func buildImageName(img *rktapi.Image) string {
+	registry := ""
+	repository := ""
+	for _, anno := range img.Annotations {
+		if anno.Key == appcDockerRegistryURL {
+			registry = anno.Value
+		}
+		if anno.Key == appcDockerRepository {
+			repository = anno.Value
+		}
+	}
+	if registry != "" && repository != "" {
+		// TODO(euank): This could do the special casing for dockerhub and library images
+		return fmt.Sprintf("%s/%s:%s", registry, repository, img.Version)
+	}
+
 	return fmt.Sprintf("%s:%s", img.Name, img.Version)
 }
 
@@ -160,19 +179,34 @@ func (r *Runtime) listImages(image string, detail bool) ([]*rktapi.Image, error)
 		return nil, err
 	}
 
+	imageFilters := []*rktapi.ImageFilter{
+		{
+			// TODO(yifan): Add a field in the ImageFilter to match the whole name,
+			// not just keywords.
+			// https://github.com/coreos/rkt/issues/1872#issuecomment-166456938
+			Keywords: []string{repoToPull},
+			Labels:   []*rktapi.KeyValue{{Key: "version", Value: tag}},
+		},
+	}
+
+	// If the repo name is not a valid ACIdentifier (namely if it has a port),
+	// then it will have a different name in the store. Search for both the
+	// original name and this modified name in case we choose to also change the
+	// api-service to do this un-conversion on its end.
+	if appcRepoToPull, err := appctypes.SanitizeACIdentifier(repoToPull); err != nil {
+		glog.Warningf("could not convert %v to an aci identifier: %v", err)
+	} else {
+		imageFilters = append(imageFilters, &rktapi.ImageFilter{
+			Keywords: []string{appcRepoToPull},
+			Labels:   []*rktapi.KeyValue{{Key: "version", Value: tag}},
+		})
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), r.requestTimeout)
 	defer cancel()
 	listResp, err := r.apisvc.ListImages(ctx, &rktapi.ListImagesRequest{
-		Detail: detail,
-		Filters: []*rktapi.ImageFilter{
-			{
-				// TODO(yifan): Add a field in the ImageFilter to match the whole name,
-				// not just keywords.
-				// https://github.com/coreos/rkt/issues/1872#issuecomment-166456938
-				Keywords: []string{repoToPull},
-				Labels:   []*rktapi.KeyValue{{Key: "version", Value: tag}},
-			},
-		},
+		Detail:  detail,
+		Filters: imageFilters,
 	})
 	if err != nil {
 		return nil, fmt.Errorf("couldn't list images: %v", err)

diff --git a/pkg/kubelet/rkt/rkt.go b/pkg/kubelet/rkt/rkt.go
@@ -116,9 +116,11 @@ const (
 	defaultDNSOption = "ndots:5"
 
 	// Annotations for the ENTRYPOINT and CMD for an ACI that's converted from Docker image.
-	// TODO(yifan): Import them from docker2aci. See https://github.com/appc/docker2aci/issues/133.
-	appcDockerEntrypoint = "appc.io/docker/entrypoint"
-	appcDockerCmd        = "appc.io/docker/cmd"
+	// Taken from https://github.com/appc/docker2aci/blob/v0.12.3/lib/common/common.go#L33
+	appcDockerEntrypoint  = "appc.io/docker/entrypoint"
+	appcDockerCmd         = "appc.io/docker/cmd"
+	appcDockerRegistryURL = "appc.io/docker/registryurl"
+	appcDockerRepository  = "appc.io/docker/repository"
 
 	// TODO(yifan): Reuse this const with Docker runtime.
 	minimumGracePeriodInSeconds = 2

diff --git a/pkg/kubelet/rkt/rkt_test.go b/pkg/kubelet/rkt/rkt_test.go
@@ -314,6 +314,33 @@ func TestListImages(t *testing.T) {
 				},
 			},
 		},
+		{
+			[]*rktapi.Image{
+				{
+					Id:      "sha512-a2fb8f390702",
+					Name:    "quay.io_443/coreos/alpine-sh",
+					Version: "latest",
+					Annotations: []*rktapi.KeyValue{
+						{
+							Key:   appcDockerRegistryURL,
+							Value: "quay.io:443",
+						},
+						{
+							Key:   appcDockerRepository,
+							Value: "coreos/alpine-sh",
+						},
+					},
+					Size: 400,
+				},
+			},
+			[]kubecontainer.Image{
+				{
+					ID:       "sha512-a2fb8f390702",
+					RepoTags: []string{"quay.io:443/coreos/alpine-sh:latest"},
+					Size:     400,
+				},
+			},
+		},
 	}
 
 	for i, tt := range tests {