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

Enable to fetch and upload images using HTTP protocol #23

Closed
wants to merge 3 commits into from

Conversation

ktock
Copy link
Contributor

@ktock ktock commented Oct 11, 2019

Fixes: #22

Stargzifying an image on the HTTP registry fails when the registry isn't
"localhost"(or 127.0.0.1).
Recently, go-containerregistry supported to read/write images using HTTP even
for fat images. (google/go-containerregistry#567)
So we can introduce "insecure" options to fetch and upload images using HTTP by
upgrading the module dependencies and using the functionality.

Stargzifying an image on a HTTP registry fails when the registry isn't
"localhost"(or 127.0.0.1).
Recently, go-containerregistry supported to read/write images using HTTP even
for fat images. (google/go-containerregistry#567)
So we can introduce "insecure" options to fetch and upload images using HTTP by
upgrading the module dependencies and using the functionality.

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@googlebot googlebot added the cla: yes Signed a CLA label Oct 11, 2019
upgrade = flag.Bool("upgrade", false, "upgrade the image in-place by overwriting the tag")
flatten = flag.Bool("flatten", false, "flatten the image's layers into a single layer")
insecAll = flag.Bool("insecure", false, "allow connections to all registries using HTTP")
insecSrc = flag.Bool("insecure-src", false, "allow connections to the source registry using HTTP")
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding --insecure is okay, but the other two are just too many flags.

Let's overload the "input" and "output" non-flag arguments to mean that if they start with "http://", then they're HTTP. (And require --insecure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your review. I fixed it to use the "input" and "output" arguments to specify HTTP registries.

Overload the "input" and "output" non-flag arguments to mean that if they start with "http://", then they're HTTP. (And require --insecure)

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock requested a review from bradfitz October 11, 2019 23:58
// fetch the image using HTTP protocol.
opts = append(opts, name.Insecure)
if *insecure {
opts = append(opts, name.Insecure)
Copy link
Contributor

Choose a reason for hiding this comment

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

} else { log.Fatal or otherwise blow up and say that's not allowed.

// "-insecure" option is specified,
// upload the image using HTTP protocol.
opts = append(opts, name.Insecure)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Pull all this out into a helper func, since it's shared between src and dst?

upgrade = flag.Bool("upgrade", false, "upgrade the image in-place by overwriting the tag")
flatten = flag.Bool("flatten", false, "flatten the image's layers into a single layer")
insecure = flag.Bool("insecure", false, "allow HTTP connections to the registry which has the prefix \"http://\"")
httpPrefix = "http://"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be a const, not a var, and it shouldn't be in this var ( ... ) block with the flags.

But really, no need for it to even be a const. Just write it in-line when it's needed once. (if you add a new helper func, it only shows up once.)

Signed-off-by: Kohei Tokunaga <ktokunaga.mail@gmail.com>
@ktock ktock requested a review from bradfitz October 12, 2019 00:51
@ktock
Copy link
Contributor Author

ktock commented Oct 12, 2019

Thank you for your review, I fixed them.

@bradfitz
Copy link
Contributor

I pushed this with some modifications: aea3638

@bradfitz bradfitz closed this Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed a CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stargzify: Stargzifying images using HTTP fails
3 participants