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 checkPushPermissions by --insecure #737

Closed
wants to merge 13 commits into from

Conversation

cn-sunletian
Copy link

@cn-sunletian cn-sunletian commented Aug 9, 2019

Our registry is harbor by http. When I push image, error occurred on function CheckPushPermissions.
Even add "--insecure" that doesn't work, but add "--insecure-pull" it can pull well.So i find "push.go" an error here. have look at #683

@cn-sunletian
Copy link
Author

@priyawadhwa
Copy link
Collaborator

Hey @cn-sunletian , looks like a linter error on travis:

pkg/executor/push.go:62:19: name.NewInsecureRegistry is deprecated: Use the Insecure Option with NewRegistry instead.  (megacheck)
			newReg, err := name.NewInsecureRegistry(destRef.RegistryStr(), name.WeakValidation)

@cn-sunletian
Copy link
Author

Hey @cn-sunletian , looks like a linter error on travis:

pkg/executor/push.go:62:19: name.NewInsecureRegistry is deprecated: Use the Insecure Option with NewRegistry instead.  (megacheck)
			newReg, err := name.NewInsecureRegistry(destRef.RegistryStr(), name.WeakValidation)

ok i fixed it

@cn-sunletian
Copy link
Author

@kokoro-team

@cn-sunletian
Copy link
Author

cn-sunletian commented Aug 10, 2019

@@ -58,6 +58,13 @@ func CheckPushPermissions(opts *config.KanikoOptions) error {
checked := map[string]bool{}
for _, destination := range opts.Destinations {
destRef, err := name.NewTag(destination, name.WeakValidation)
if opts.Insecure {
newReg, err := name.NewRegistry(destRef.RegistryStr(), name.WeakValidation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you want to pass in the insecure option to name.NewRegistry -- found here in the go-containerregistry code

@cn-sunletian cn-sunletian changed the title fix checkPushPermissions by --insecur fix checkPushPermissions by --insecure Aug 21, 2019
sparshev pushed a commit to sparshev/kaniko that referenced this pull request Aug 27, 2019
@tejal29
Copy link
Member

tejal29 commented Aug 29, 2019

Looks like the integration test is failing due to

Building images for Dockerfile Dockerfile_test_copy_bucket
Error building images: Failed to build image gcr.io/kaniko-test/docker-dockerfile_test_arg_multi with docker command "[docker build -t gcr.io/kaniko-test/docker-dockerfile_test_arg_multi -f dockerfiles/Dockerfile_test_arg_multi .]": exit status 1 Sending build context to Docker daemon  117.2kB

Copy link

@drewbyp drewbyp left a comment

Choose a reason for hiding this comment

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

New to this (sorry if I'm doing it wrong) but I was looking at this last night and found that NewTag takes options and creates the registry based on these. What's the benefit of overwriting the registry with a new insecure registry rather than passing name.Insecure to NewTag()?

Thanks

@jwidauer
Copy link

jwidauer commented Sep 10, 2019

As @Dina0312 mentioned: Wouldn't something like this do the same thing, be cleaner and less code?

if opts.Insecure {
	destRef, err = name.NewTag(destination, name.WeakValidation, name.Insecure)
}

@cn-sunletian
Copy link
Author

As @Dina0312 mentioned: Wouldn't something like this do the same thing, be cleaner and less code?

if opts.Insecure {
	destRef, err = name.NewTag(destination, name.WeakValidation, name.Insecure)
}

ok,thanks

@@ -69,6 +69,13 @@ func CheckPushPermissions(opts *config.KanikoOptions) error {
checked := map[string]bool{}
for _, destination := range opts.Destinations {
destRef, err := name.NewTag(destination, name.WeakValidation)
if opts.Insecure || opts.InsecureRegistries.Contains(destination) {
Copy link
Member

Choose a reason for hiding this comment

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

shd this be && instead of ||

@priyawadhwa
Copy link
Collaborator

priyawadhwa commented Oct 14, 2019

Hey @cn-sunletian, we merged #685 a few weeks ago which should fix this issue. Could you try with gcr.io/kaniko-project/executor:v0.13.0? My guess is that this should work.

@tejal29
Copy link
Member

tejal29 commented Oct 24, 2019

Closing this since #685 addresses this issue.

@tejal29 tejal29 closed this Oct 24, 2019
@pvgbabu
Copy link

pvgbabu commented Jul 16, 2020

@priyawadhwa , i have tried using gcr.io/kaniko-project/executor:v0.13.0 but no luck. is there any way
i can pass '--skip-tls-verify' parameter to '_generate_kaniko_spec' in https://kubeflow-pipelines.readthedocs.io/en/latest/_modules/kfp/containers/_container_builder.html
appreciate all your help.

@pvgbabu
Copy link

pvgbabu commented Jul 16, 2020

@priyawadhwa where does cluster builder get the docker repository url. I am getting http instead of https causing the error .

@priyawadhwa
Copy link
Collaborator

Hey @pvgbabu sorry to hear that. Could you open a new issue with the error you're getting?

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.

7 participants