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

ko:// image references are broken #5492

Closed
PriyaModali opened this issue Mar 4, 2021 · 2 comments · Fixed by #5495
Closed

ko:// image references are broken #5492

PriyaModali opened this issue Mar 4, 2021 · 2 comments · Fixed by #5495
Assignees
@PriyaModali
Labels
area/build kind/bug Something isn't working priority/p2 May take a couple of releases

Comments

@PriyaModali
Copy link
Contributor

PriyaModali commented Mar 4, 2021 •
edited
Loading

Custom build example is currently failing. See the attached error .

image
@PriyaModali PriyaModali added kind/bug Something isn't working area/build priority/p2 May take a couple of releases labels Mar 4, 2021
@PriyaModali PriyaModali self-assigned this Mar 4, 2021
@PriyaModali PriyaModali changed the title Custom build example need to be updated Custom build example is currently broken Mar 5, 2021
This was referenced Mar 5, 2021
Merged
Closed
@MarlonGamez
Copy link
Contributor

As far as I'm aware, it looks like the custom builder example should work, however, it seems like this piece of code from #4952 isn't being applied?

skaffold/pkg/skaffold/docker/reference.go

Lines 83 to 97 in 2e55bec

// SanitizeImageName removes prefixes and lowercases the name portion of images.
// This is primarily used to handle `ko` import paths in image fields, e.g.
// `ko://github.com/GoogleContainerTools/skaffold`.
// Tags can contain uppercase characters, so this function takes care not to
// change tag cases.
func SanitizeImageName(image string) string {
for _, prefix := range allowedImagePrefixes {
image = strings.TrimPrefix(image, prefix)
}
matches := caseInsensitiveReferenceRegexp.FindStringSubmatch(image)
if len(matches) > 1 {
image = strings.Replace(image, matches[1], strings.ToLower(matches[1]), 1)
}
return image
}

@PriyaModali PriyaModali mentioned this issue Mar 6, 2021
Merged
@briandealwis briandealwis mentioned this issue Mar 8, 2021
Open
@briandealwis briandealwis reopened this Mar 8, 2021
@piotr-szybicki piotr-szybicki mentioned this issue Mar 16, 2021
Merged
@PriyaModali PriyaModali modified the milestone: v1.21.0 Mar 21, 2021
@briandealwis
Copy link
Member

Closing again: #5505 is to investigate why SanitizeImageName() isn't called.

@briandealwis briandealwis changed the title Custom build example is currently broken ko:// image references are broken Jun 10, 2021
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

@PriyaModali PriyaModali

Labels
area/build 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.

Fix custom build example PriyaModali/skaffold
3 participants
@briandealwis @PriyaModali @MarlonGamez