Skip to content

Commit

Permalink
Sanitize image names before deciding what to load
Browse files Browse the repository at this point in the history
I was able to reproduce the issue locally and work out a fix. I'd be
happy for feedback on whether I've chosen the right place to fix it.

When determining which images to sideload (for kind and k3d), we compare
`localImages` and `deployerImages`. The former from `skaffold.yaml`, and
the latter from Kubernetes manifest files. The image names from
Kubernetes manifests are sanitized in
`pkg/skaffold/kubernetes/manifests/images.go#L51` (and `L113`) in the
call to docker.ParseReference.

The same doesn't happen to image names from `skaffold.yaml`. This change
sanitizes these image names just for determining whether to sideload the
images.

In other parts of the code we look up image pipelines from
`skaffold.yaml` using the image name, so I was hesitant to change how
`localImages` is used (with 'raw' image names).

The hypothesis from the previous commit is disproven, so I'm
adding back the `sha256` tag policy in the custom builder example. To
make the test case easier to identify from the build logs, I renamed the
pod in the custom builder example.

New hypothesis: Could this be related to the issues some users are
reporting with images not being sideloaded when using Helm? E.g., #5159
  • Loading branch information
halvards committed Jul 27, 2021
1 parent 155ff8c commit a2aa63d
Show file tree
Hide file tree
Showing 5 changed files with 16 additions and 6 deletions.
6 changes: 3 additions & 3 deletions integration/examples/custom/k8s/pod.yaml
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
apiVersion: v1
kind: Pod
metadata:
name: getting-started
name: getting-started-custom
spec:
containers:
- name: getting-started
image: github.com/googlecontainertools/skaffold/examples/custom
- name: getting-started-custom
image: ko://github.com/GoogleContainerTools/skaffold/examples/custom
2 changes: 2 additions & 0 deletions integration/examples/custom/skaffold.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ build:
paths:
- "go.mod"
- "**.go"
tagPolicy:
sha256: {}
2 changes: 1 addition & 1 deletion integration/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ var tests = []struct {
{
description: "custom builder",
dir: "examples/custom",
pods: []string{"getting-started"},
pods: []string{"getting-started-custom"},
},
{
description: "buildpacks Go",
Expand Down
5 changes: 3 additions & 2 deletions pkg/skaffold/kubernetes/loader/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"k8s.io/client-go/tools/clientcmd/api"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/graph"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubectl"
kubectx "github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes/context"
Expand Down Expand Up @@ -60,7 +61,7 @@ func NewImageLoader(kubeContext string, cli *kubectl.CLI) *ImageLoader {
func imagesToLoad(localImages, deployerImages, images []graph.Artifact) []graph.Artifact {
local := map[string]bool{}
for _, image := range localImages {
local[image.ImageName] = true
local[docker.SanitizeImageName(image.ImageName)] = true
}

tracked := map[string]bool{}
Expand All @@ -72,7 +73,7 @@ func imagesToLoad(localImages, deployerImages, images []graph.Artifact) []graph.

var res []graph.Artifact
for _, image := range images {
if tracked[image.ImageName] {
if tracked[docker.SanitizeImageName(image.ImageName)] {
res = append(res, image)
}
}
Expand Down
7 changes: 7 additions & 0 deletions pkg/skaffold/kubernetes/loader/load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,13 @@ func TestImagesToLoad(t *testing.T) {
builtImages: []graph.Artifact{{ImageName: "image1", Tag: "foo"}},
expectedImages: nil,
},
{
name: "single image marked as local, with ko prefix and Go import path with uppercase characters",
localImages: []graph.Artifact{{ImageName: "ko://example.com/Organization/Image1", Tag: "Foo"}},
deployerImages: []graph.Artifact{{ImageName: "example.com/organization/image1", Tag: "Foo"}},
builtImages: []graph.Artifact{{ImageName: "ko://example.com/Organization/Image1", Tag: "Foo"}},
expectedImages: []graph.Artifact{{ImageName: "ko://example.com/Organization/Image1", Tag: "Foo"}},
},
{
name: "two images marked as local",
localImages: []graph.Artifact{{ImageName: "image1", Tag: "foo"}, {ImageName: "image2", Tag: "bar"}},
Expand Down

0 comments on commit a2aa63d

Please sign in to comment.