-
Notifications
You must be signed in to change notification settings - Fork 236
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
Logic to pull bundle from quay.io #3221
Conversation
One initial comment, this makes the binaries 10MB/20% bigger |
Yes, because of c/image dependency, it is making the binary bit more bigger. |
20% is not just a bit ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm find the log confusing
This is initial pr to switch bundle download from mirror to quay.io
End user shouldn't affect with this change except there is no download
resume functionality.
After reading this, I don't know if bundle download is fully moved to getting it from quay.io or not ("initial PR", "end user shouldn't affect with this change"). I would say explicitly that after this commit, bundles are downloaded from quay.io even if the old download code is still there.
Fwiw, I find the name pkg/crc/image
very (too) generic, is this about picture, an ISO image, a qcow2 image, ... ? Might be worth naming this container-image
, or maybe just container
for the package name since the type used is then OCIImage..
?
Makefile
Outdated
@@ -68,6 +68,8 @@ endif | |||
|
|||
# https://golang.org/cmd/link/ | |||
LDFLAGS := $(VERSION_VARIABLES) ${GO_EXTRA_LDFLAGS} | |||
# Same build flags are used in the podman remote to cross build it https://github.com/containers/podman/blob/main/Makefile | |||
BUILDTAGS := remote exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper containers_image_openpgp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks quite odd in our build logs :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing much I can do :)
pkg/crc/image/image.go
Outdated
// CopyImage pull the image from the registry and put it to local storage | ||
func (img OCIImageHandler) CopyImage(destPath string) ([]byte, error) { | ||
// Source Image from docker transport | ||
srcImg := fmt.Sprintf("docker://%s:%s", img.uri(), img.ImageTag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
img.uri()
is not a uri, it becomes (closer to) one after you prepend docker://
. Since this is the only caller of uri()
, I guess docker://
could be moved there?
Also feels odd that ImageTag
needs to be separately added, while this is also the only location making use of it.
pkg/crc/image/image.go
Outdated
v1 "github.com/opencontainers/image-spec/specs-go/v1" | ||
) | ||
|
||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be const
pkg/crc/image/image.go
Outdated
|
||
type OCIImageHandler struct { | ||
ImageName string | ||
ImageTag string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me why we hardcode quay.io
and crcont
but we let external code specify which image
/tag
should be downloaded, especially with GetImageName
implemented in this file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't get what you mean about external code as of now GetImageName
only provide the image name for preset and then we define this struct and use the method to copy it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
preflight_checks_common.go
has code to fill ImageName/ImageTag:
imgHandler := image.OCIImageHandler{
ImageName: image.GetImageName(preset),
ImageTag: version.GetCRCVersion(),
Progress: os.Stdout,
}
but it calls into image
to fill this. Then image.go
hardcodes quay.io/crcont
. Half of what is needed to fetch the image is set in preflight_checks_common.go
and the other half in image.go
. preflight_checks_common.go
does not need to set this imo, ImageName/ImageTag could both be set internally in image.go
pkg/crc/image/image.go
Outdated
} | ||
} | ||
|
||
// CopyImage pull the image from the registry and put it to local storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
local storage
means destPath
? It could be misread as podman local image s torage
, so I'd mention destpath in the doc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau you mean podman load <image.tar>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I will put destpath in doc.
pkg/crc/image/image.go
Outdated
}) | ||
} | ||
|
||
func GetBundleImageLayerFromManifest(manifestData []byte) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you return a v1.Manifest
from CopyImage
, then you don't need the very long name, just GetBundleImageLayer
works, or GetLayerPath
or so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it is easy to read or understand I don't mind to have a very long function. if we return v1.Manifest
from image we need to process the byte there instead here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as it is easy to read or understand I don't mind to have a very long function.
Something shorter will be easier to read and understand most of the time :) With a longer function name with more details in higher level code, you still need to read it, parse it, and understand what you need to ignore from the long name,...
pkg/crc/image/image.go
Outdated
if err := json.Unmarshal(manifestData, manifest); err != nil { | ||
return "", err | ||
} | ||
return removeShaFromDigestString(manifest.Layers[0].Digest.String()), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this deserves a separate helper since it's going to be a one-liner used once, and this method is also not very long.
I tried this code with multi-layer images, and it did not work at all (expected, and not saying it has to be fixed). I was wondering if there's a way of detecting this though, maybe disallow multi-layer images, and also check the MediaType
to make sure it's the expected one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this code with multi-layer images, and it did not work at all (expected, and not saying it has to be fixed). I was wondering if there's a way of detecting this though, maybe disallow multi-layer images, and also check the
MediaType
to make sure it's the expected one?
I am not sure why it shouldn't work with multi layer, the logic here is just return the first layer digest, in your test does the first layer didn't have any digest or having a digest without sha256?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will do "something" for multilayer images, but will it be doing the right thing, or something unexpected to the user? Imo it's not very well defined what will happen, so it can just error out so that we catch immediatly that something was off.
if err != nil { | ||
return err | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if this deserves to be a helper in the image
package as this is quite a few low-level steps that needs to be done, not sure preflight should have that much knowledge about how a container image is stored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, not a strong opinion, can be part of image
package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd try to move it to image, but not 100% sure yet it will be the right place /o\ I leave that up to you :)
pkg/crc/image/image.go
Outdated
orgName = "crcont" | ||
) | ||
|
||
type OCIImageHandler struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure OCI
is really useful here, we hardcode docker:
for fetching from the remote repository, and use dir:
for local storage. Maybe this is still stored in an OCI compliant way, but this really is not the most important part of the implementation, especially as we don't support many layer formats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is good to be explicit around what kind of image format we are storing to dir and right now quay.io
pulled images are in oci image format so better to have it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user of this API don't care about the format it's in.. If oci
appears internally in the image.go implementation, why not, but I don't think it's useful in the public API. Also not clear why we are using dir:
rather than oci:
for example? If oci
is not exposed in the podman/... API we are using, not sure we need to expose it ourselves.
8e27579
to
522280a
Compare
pkg/crc/image/image.go
Outdated
) | ||
|
||
const ( | ||
registryURI = "quay.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be defined in a constants.go
file instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gbraad yes we can move it to constants.go, As of now it is here just to have all image related stuff in same package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pkg/crc/image/constants.go
? I also prefer to keep this private to the image
package as long as other code does not need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for 2-3 constants which also used for same package I don't feel create a different constants file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still some comments/questions, but looking good overall
Makefile
Outdated
GOOS=darwin $(TOOLS_BINDIR)/golangci-lint run | ||
GOOS=linux $(TOOLS_BINDIR)/golangci-lint run | ||
GOOS=windows $(TOOLS_BINDIR)/golangci-lint run | ||
GOOS=darwin $(TOOLS_BINDIR)/golangci-lint run --build-tags "$(BUILDTAGS)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already build tags defined in .golangci.yml
, they should be all set in the same place, either here or in the .yml file.
pkg/crc/image/image.go
Outdated
) | ||
|
||
const ( | ||
registryURI = "quay.io" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pkg/crc/image/constants.go
? I also prefer to keep this private to the image
package as long as other code does not need this.
pkg/crc/image/image.go
Outdated
policy := &signature.Policy{Default: []signature.PolicyRequirement{signature.NewPRInsecureAcceptAnything()}} | ||
policyContext, err := signature.NewPolicyContext(policy) | ||
if err != nil { | ||
return nil, fmt.Errorf("error creating security context: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be "...: %w
, err)`
@@ -98,7 +99,7 @@ func fixBundleExtracted(bundlePath string, preset preset.Preset) func() error { | |||
} | |||
if bundlePath == constants.GetDefaultBundlePath(preset) { | |||
logging.Infof("Downloading %s", constants.GetDefaultBundle(preset)) | |||
if err := bundle.Download(preset); err != nil { | |||
if err := image.PullBundleImage(preset, os.Stdout); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This could be checking the bundle sha256sum for now.
Not sure it's really useful to pass os.Stdout
as an argument, this could be hardcoded in the image module, similarly to context.Background()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be checking the bundle sha256sum for now.
After this initial PR next would be verify using signed bits so it will be taken care of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we don't want to wait for this 'signature' PR before merging this one, do we? Given our fast release cycles, I'm a bit uncomfortable merging a PR which depends on a non-existent PR which has to be in before our next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau you want to check hardcoded bundle sha256sum for bundle which are part of the image right now?
452b081
to
50bf396
Compare
/retest |
pkg/crc/image/image.go
Outdated
} | ||
} | ||
|
||
// copyImage pull the image from the registry and put it to destination path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pulls ... puts
Something which is not clear to me is how/when we want to ship this? Do we switch all bundles to quay.io? Do we want to keep shipping OpenShift bundles on mirror.openshift.com, while moving the more community-oriented ones to quay.io? Or do we ship everything on quay? |
End of this sprint.
Yes, plan is to switch all bundles to quay.io
Ship everything on quay (also update the bundle on mirror for time being (1-2 sprint) in case outage on quay end) so user can download it manually and use |
can we allow to use mirror first for this? For this we have been signed off. other locations I am not sure how they will respond. I would almost suggest to have a config parameter to allow this to be toggled ? @cfergeau WDYT? |
Works for me, though UI will be a bit odd if at a later point we ship some bundles only through mirror.openshift.com or only through an image registry. |
Therefore more of a preference option and not conclusive. |
pkg/crc/image/image.go
Outdated
OSChoice: runtime.GOOS, | ||
ArchitectureChoice: runtime.GOARCH, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw, these are the default values
// If not "", overrides the use of platform.GOARCH when choosing an image or verifying architecture match.
ArchitectureChoice string
// If not "", overrides the use of platform.GOOS when choosing an image or verifying OS match.
OSChoice string
probably not worth the helper/..., we can just use an empty context.
pkg/crc/image/image.go
Outdated
logging.Debugf("Bundle and sign path %v", fileList) | ||
|
||
logging.Info("Verifying the bundle signature...") | ||
if err := gpg.Verify(fileList[0], fileList[1]); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not hardcode that first file in the archive will always be the bundle, and second file its signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau this is the way our script will create image, should we compare the size or with the extension?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is now compare with extension so should be good.
3eeb176
to
59b79c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments left, but overall looks good!
@@ -31,6 +32,7 @@ require ( | |||
github.com/mdlayher/vsock v1.1.1 | |||
github.com/onsi/ginkgo v1.16.5 | |||
github.com/onsi/gomega v1.19.0 | |||
github.com/opencontainers/image-spec v1.0.2-0.20190823105129-775207bd45b6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why such an old version? podman uses go.mod: github.com/opencontainers/image-spec v1.0.3-0.20220114050600-8b9d41f48198
, go get -u github.com/opencontainers/image-spec
switches to the v1.0.2
tag which was created in 2021.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau I am not sure https://github.com/opencontainers/image-spec here I don't even find v1.0.3 tag or branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opencontainers/image-spec#918 there is request for release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't even find v1.0.3 tag or branch
$ go get -u github.com/opencontainers/image-spec@main
go: downloading github.com/opencontainers/image-spec v1.0.3-0.20220512140940-7b36cea86235
go: upgraded github.com/opencontainers/image-spec v1.0.2 => v1.0.3-0.20220512140940-7b36cea86235
It's go which refers to a non existent 1.0.3 version when we ask it to use the main
branch. But as I said in my earlier comment, using the latest release (1.0.2
) is also fine with me.
Looking again at this, it seems the 20190823105129 is misleading, and that v1.0.2-0.20190823105129-775207bd45b6
might be newer than v1.0.2, but history is complicated with a lot of merges, so not sure :-/
EDIT: there's a v1.0
branch with not many commits, and a main
branch with a lot more commits. The v1.0.2
tag was cut from the v1.0
branch, and does not have the commit corresponding to v1.0.2-0.20190823105129-775207bd45b6
for example. I still think we should either use v1.0.2
or main
, but not some very arbitrary untagged commit.
pkg/crc/image/image.go
Outdated
return err | ||
} | ||
defer os.RemoveAll(destDir) | ||
imgDigest, err := imgHandler.copyImage(destDir, os.Stdout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imgManifest
pkg/crc/image/image.go
Outdated
if err != nil { | ||
return err | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be return err
@@ -41,6 +41,21 @@ const ( | |||
RegistryURI = "quay.io/crcont" | |||
ClusterDomain = ".crc.testing" | |||
AppsDomain = ".apps-crc.testing" | |||
|
|||
GPGPublicKey = `-----BEGIN PGP PUBLIC KEY BLOCK----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a comment explaining the identity this key corresponds to, link to a public keyserver, ...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cfergeau yes I will to public keyserver and add link here.
pkg/crc/gpg/gpg.go
Outdated
} | ||
defer func() { | ||
if err1 := signature.Close(); err1 != nil { | ||
err = err1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm if err
is already set, won't this hide the initial error? (same question for the other occurrence in this function)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes https://trstringer.com/golang-deferred-function-error-handling/ looks like we need multi error in this case to append the error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are open file in reading mode so in defer I think we are OK to just use f.Close()
without handling the error. if we open file in write mode then it is more appropriate to handle defer function error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are open file in reading mode so in defer I think we are OK to just use f.Close()
I think I had the same thoughts in an earlier version of this PR :)
And in write mode, I'd just ignore Close errors if err
is already set rather than usin multi error.
go.mod
Outdated
@@ -192,6 +193,7 @@ require ( | |||
|
|||
replace ( | |||
github.com/apcera/gssapi => github.com/openshift/gssapi v0.0.0-20161010215902-5fb4217df13b | |||
golang.org/x/crypto/openpgp => github.com/ProtonMail/go-crypto/openpgp v0.0.0-20220623141421-5afb4c282135 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR directly imports github.com/ProtonMail/go-crypto/openpgp
, is the replace required?
pkg/crc/image/image.go
Outdated
|
||
logging.Info("Verifying the bundle signature...") | ||
if len(fileList) != 2 { | ||
return fmt.Errorf("image layer contains more files than expected %v", fileList) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: : %v
rather than just %v
pkg/crc/gpg/gpg.go
Outdated
) | ||
|
||
func Verify(filePath, signatureFilePath string) (err error) { | ||
signed, err := os.Open(filePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be just me, but signed
feels a bit unspecific. signedData
, signedFile
, ... ? Or even omit the signed
part of it?
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cfergeau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
In this pr OKD and podman bundles are downloaded from quay.io The bundle images are pushed to quay.io/crcont with following name - okd-bundle - podman-bundle ``` $ crc setup [...] INFO Getting bundle for the CRC executable INFO Downloading crc_libvirt_4.10.14_amd64.crcbundle Getting image source signatures Copying blob 7e0dbb47dc47 done Copying config 4f32695e8f done Writing manifest to image destination Storing signatures INFO Extracting the image bundle layer... crc_libvirt_4.10.14_amd64.crcbundle: 3.15 GiB / 3.15 GiB [---------------------------------------------------------------------------------------------------------------------------] 100.00% INFO Uncompressing /home/prkumar/.crc/cache/crc_libvirt_4.10.14_amd64.crcbundle crc.qcow2: 12.61 GiB / 12.61 GiB [---------------------------------------------------------------------------------------------------------------------------------------------------] 100.00% oc: 117.14 MiB / 117.14 MiB [--------------------------------------------------------------------------------------------------------------------------------------------------------] 100.00% ```
New changes are detected. LGTM label has been removed. |
As part of container image we also have detached gpg bundle signature and this pr is verify that with bundle to make sure bundle is coming from us.
This is initial pr to switch bundle download from mirror to quay.io
End user shouldn't affect with this change except there is no download
resume functionality.
The bundle images are pushed to quay.io/crcont with following name
This PR doesn't remove existing codebase to download bundles from mirror
as of now but it should be done in a separate commit so we can easily
revert back.
Fixes: #3217