Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Investigate why SanitizeImageName isn't being called #5505

Closed
PriyaModali opened this issue Mar 8, 2021 · 3 comments · Fixed by #6286
Closed

Investigate why SanitizeImageName isn't being called #5505

PriyaModali opened this issue Mar 8, 2021 · 3 comments · Fixed by #6286
Assignees
@halvards
Labels
fixit help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/bug Something isn't working priority/p2 May take a couple of releases

Comments

@PriyaModali
Copy link
Contributor

SanitizeImageName isn't being called which is causing the custom test to fail with ko: images.

We need to investigate why SanitizeImageName isn't being called.
@PriyaModali PriyaModali added kind/bug Something isn't working priority/p2 May take a couple of releases labels Mar 8, 2021
@PriyaModali PriyaModali self-assigned this Mar 8, 2021
@PriyaModali PriyaModali changed the title SanitizeImageName isn't being called Investigate why SanitizeImageName isn't being called Mar 8, 2021
@briandealwis briandealwis added help wanted We would love to have this done, but don't have the bandwidth, need help from contributors priority/p3 agreed that this would be good to have, but no one is available at the moment. fixit priority/p2 May take a couple of releases and removed priority/p2 May take a couple of releases priority/p3 agreed that this would be good to have, but no one is available at the moment. labels Apr 23, 2021
@MarlonGamez
Copy link
Contributor

I believe this issue may be resolved with @halvards's proposed implementation of ko as a builder in skaffold. I'm going to hold off on serious investigation into this for now

linking for context:
#5611

@briandealwis
Copy link
Member

Ah! @PriyaModali and I had noticed that the image wasn't being rebuilt (#5500) — that it was being cached. It looks like SanitizeImageName() is called from the imageReplacer:

skaffold/pkg/skaffold/kubernetes/manifest/images.go

Lines 80 to 85 in 7aaa1c7

func newImageReplacer(builds []graph.Artifact) *imageReplacer {
tagsByImageName := make(map[string]string)
for _, build := range builds {
imageName := docker.SanitizeImageName(build.ImageName)
tagsByImageName[imageName] = build.Tag
}

which is called to replace image references. I wonder if cached images are not included in the built images passed in here:

skaffold/pkg/skaffold/kubernetes/manifest/images.go

Lines 60 to 64 in 7aaa1c7

// ReplaceImages replaces image names in a list of manifests.
func (l *ManifestList) ReplaceImages(builds []graph.Artifact) (ManifestList, error) {
replacer := newImageReplacer(builds)
updated, err := l.Visit(replacer)

@halvards
Copy link
Collaborator

I can look into this as part of the work for #5611

@briandealwis briandealwis mentioned this issue May 29, 2021
Closed
@briandealwis briandealwis mentioned this issue Jun 29, 2021
Merged
halvards added a commit to halvards/skaffold that referenced this issue Jul 26, 2021
@halvards
Restore import path with uppercase characters
2b74636 
I wasn't able to reproduce the issue described in GoogleContainerTools#5492 and GoogleContainerTools#5505, so
this change restores the import path with the uppercase characters
and the `ko://` prefix in the custom build example.

I had to tweak some values to get the integration tests to run,
because I don't have access to the `k8s-skaffold` project. Let's see
if the build passes.

Additional minor changes:

- Bump the ko version in the custom build example.

- Add the full path to the ko binary in the custom build script, in case
  `$GOPATH/bin` is not on the `PATH`.

Once we move to Go 1.16 for our builds, we can use the `go install`
command to install ko in the custom build shell script.
@halvards halvards mentioned this issue Jul 26, 2021
Merged
tejal29 pushed a commit to halvards/skaffold that referenced this issue Aug 11, 2021
@halvards @tejal29
Restore import path with uppercase characters
dde76eb 
I wasn't able to reproduce the issue described in GoogleContainerTools#5492 and GoogleContainerTools#5505, so
this change restores the import path with the uppercase characters
and the `ko://` prefix in the custom build example.

I had to tweak some values to get the integration tests to run,
because I don't have access to the `k8s-skaffold` project. Let's see
if the build passes.

Additional minor changes:

- Bump the ko version in the custom build example.

- Add the full path to the ko binary in the custom build script, in case
  `$GOPATH/bin` is not on the `PATH`.

Once we move to Go 1.16 for our builds, we can use the `go install`
command to install ko in the custom build shell script.
tejal29 pushed a commit that referenced this issue Aug 11, 2021
@halvards
Restore import path with uppercase characters in custom build ko exam…
41375f3 
…ple (#6286)

* Restore import path with uppercase characters

I wasn't able to reproduce the issue described in #5492 and #5505, so
this change restores the import path with the uppercase characters
and the `ko://` prefix in the custom build example.

I had to tweak some values to get the integration tests to run,
because I don't have access to the `k8s-skaffold` project. Let's see
if the build passes.

Additional minor changes:

- Bump the ko version in the custom build example.

- Add the full path to the ko binary in the custom build script, in case
  `$GOPATH/bin` is not on the `PATH`.

Once we move to Go 1.16 for our builds, we can use the `go install`
command to install ko in the custom build shell script.

* Look for ko binary in GOPATH/bin

It's difficult to know what's on the `PATH` in different environments.
This change to the custom builder example looks for the ko binary in the
`GOPATH/bin` directory.

* Remove tagPolicy from custom builder example

Hypothesis: `tagPolicy: sha256` doesn't behave correctly with images
sideloaded into kind snf k3d.

Also fix conditional in custom build example shell script to prevent
recompiling ko each time.

* Sanitize image names before deciding what to load

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@halvards halvards

Labels
fixit help wanted We would love to have this done, but don't have the bandwidth, need help from contributors kind/bug Something isn't working priority/p2 May take a couple of releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Restore import path with uppercase characters in custom build ko example halvards/skaffold
4 participants
@halvards @briandealwis @PriyaModali @MarlonGamez