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.

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

(fix):Pass full URI path to bucket.GetNameAndFilepathFromURI #2221

Merged
merged 1 commit into from
Sep 8, 2022
Merged

Conversation

almg80
Copy link
Contributor

@almg80 almg80 commented Aug 24, 2022

Fixes #2200

Description

On version 1.9.0 context fetch from S3 is broken due to url.Parse requiring full path:

url, err := url.Parse(bucketURI)

Currently when we do --context=s3://my-bucket/my-path/context.tar.gz we would pass only my-bucket/my-path/context.tar.gz which breaks context fetch. Tested with S3 but #2200 refers gs:// so passing srcContext for both cases.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Examples of user facing changes:
- kaniko adds a new flag `--registry-repo` to override registry

On version 1.9.0 context fetch from S3 is broken since `url.Parse` requires full path: https://github.com/GoogleContainerTools/kaniko/blob/90e426ba3fde4b72efbcec5f10e4f73963313228/pkg/util/bucket/bucket_util.go#L77
Currently on a --context=s3://my-bucket/my-path/context.tar.gz we would pass only my-bucket/my-path/context.tar.gz which breaks context fetch

Closes #2200
Copy link
Collaborator

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I'm sure, this behavior seems to have been in place since it was added in 65d7b0a, 4 years ago.

Does this mean S3 build contexts have just never worked? (Not that surprising since AFAIK there's no test)

Or was there some other more recent change that affected the inputs to GetBuildContext or the behavior of the S3 build context that broke this?

@almg80
Copy link
Contributor Author

almg80 commented Aug 24, 2022

I believe #2110 changed the way the context is being handled. No longer using util.GetBucketAndItem and now using bucket.GetNameAndFilepathFromURI.
S3 build context is still working with 1.8.1

@imjasonh
Copy link
Collaborator

I believe #2110 changed the way the context is being handled. No longer using util.GetBucketAndItem and now using bucket.GetNameAndFilepathFromURI. S3 build context is still working with 1.8.1

Ah excellent, thanks for that. This definitely looks like a culprit.

@liam-verta
Copy link
Contributor

@imjasonh
Can we get this out as hotfix sooner rather than later? #2110 was supposed to be a refactor and instead it introduced this bug. I don't see any workaround. This feature is broken until this PR is merged (or #2110 is reverted) and released.

@imjasonh imjasonh merged commit f9aaa9f into GoogleContainerTools:main Sep 8, 2022
@imjasonh
Copy link
Collaborator

imjasonh commented Sep 8, 2022

I've merged the PR, and a new commit-tagged image should be built soon (:f9aaa9fca7bf4077778ed527c1a8a6e09e60c53c) that includes this change.

However, I'd rather not cut releases without someone at Google around to update image refs if something goes wrong. cc @chuangw6

@liam-verta
Copy link
Contributor

@imjasonh Thanks! Grabbed the commit-tagged image and verified we no longer get this error.

@liam-verta
Copy link
Contributor

@chuangw6 Can we get a new release cut?

@chuangw6 chuangw6 mentioned this pull request Sep 26, 2022
4 tasks
@chuangw6
Copy link
Contributor

@chuangw6 Can we get a new release cut?

Done! https://github.com/GoogleContainerTools/kaniko/releases/tag/v1.9.1. Please see the tags in that release. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kaniko executor container image v1.9.0: "Error: error resolving source context: storage: bucket name is empty"
4 participants