feat(opm): add unprivileged registry add #213

njhale · 2020-03-12T06:09:11Z

Add the ability to pull and unpack bundle images without using an external tool or daemon. Make this the only supported mode by removing the --container-tool option from the registry add subcommand.

Ex.

$ opm registry add --bundle-images "quay.io/olmtest/kiali:1.2.4"
INFO[0000] adding to the registry                        bundles="[quay.io/olmtest/kiali:1.2.4]"
INFO[0000] resolved name: quay.io/olmtest/kiali:1.2.4
INFO[0000] fetched                                       digest="sha256:a1bec450c104ceddbb25b252275eb59f1f1e6ca68e0ced76462042f72f7057d8"
INFO[0000] fetched                                       digest="sha256:8b19d5ea2c56e663fb886d9ca273dd1f8e20b107c9b6817dedd35aac90e16494"
INFO[0000] fetched                                       digest="sha256:1c019331644e2e826b6b8f11328b31a1c2f4aa2ae6ee225f0ab19a14af086d2b"
INFO[0000] fetched                                       digest="sha256:d9c98a446dd77b5e7cb1394748c2279d83668a519b2badda1259cd64cb3beb3f"
INFO[0000] unpacking layer: {application/vnd.docker.image.rootfs.diff.tar.gzip sha256:1c019331644e2e826b6b8f11328b31a1c2f4aa2ae6ee225f0ab19a14af086d2b 5916 [] map[] <nil>}
INFO[0000] unpacking layer: {application/vnd.docker.image.rootfs.diff.tar.gzip sha256:8b19d5ea2c56e663fb886d9ca273dd1f8e20b107c9b6817dedd35aac90e16494 296 [] map[] <nil>}
INFO[0000] found annotations file searching for csv      dir=bundle_tmp146087939 file=bundle_tmp146087939/metadata load=annotations
INFO[0000] found csv, loading bundle                     dir=bundle_tmp146087939 file=bundle_tmp146087939/manifests load=bundle
INFO[0000] loading bundle file                           dir=bundle_tmp146087939/manifests file=kiali.crd.yaml load=bundle
INFO[0000] loading bundle file                           dir=bundle_tmp146087939/manifests file=kiali.monitoringdashboards.crd.yaml load=bundle
INFO[0000] loading bundle file                           dir=bundle_tmp146087939/manifests file=kiali.package.yaml load=bundle
INFO[0000] loading bundle file                           dir=bundle_tmp146087939/manifests file=kiali.v1.4.2.clusterserviceversion.yaml load=bundle

Also, introduce a new bundle oriented image interface to decouple actions (pull, push, pack, unpack) from their underlying implementation.

njhale · 2020-03-12T15:39:06Z

/retest

exdx · 2020-03-12T17:15:40Z

pkg/image/registry.go

+
+type Registry interface {
+	// Pull fetches and stores an image by reference.
+	Pull(ctx context.Context, ref string) error


One thing I like is using custom types for specificity such a type ImageReference string and then in the interface it would take in a ref ImageReference type as the arg. I'm not sure how appropriate it is here but something worth noting as it makes the code more readable IMO.

I wouldn't insist on it, but I also appreciate the extra compile-time safety of derived types for this kind of thing.

Okay -- I'll change this

anik120 · 2020-03-12T19:41:05Z

pkg/image/shellout/registry.go

@@ -0,0 +1,38 @@
+package shellout


@njhale while reading through the code I realized that I understand what unprivileged means in the context on this PR due to the commit message, but I'm not sure what shellout is for. What are your thoughts on adding comments that summaries the intent of a package/it's name?
Even with unprivileged I guess I'd have to go back and open up the PR to understand the motive behind it.

@anik120 I totally understand -- I actually spent a long time thinking about the naming here without coming up with anything better.

FYI: "shellout" is for shelling out to an external tool.

ecordell

looking great - left comments inline.

ecordell · 2020-03-12T15:42:59Z

go.mod

 	gopkg.in/yaml.v2 v2.2.8
 	k8s.io/api v0.17.3
 	k8s.io/apiextensions-apiserver v0.17.3
 	k8s.io/apimachinery v0.17.3
 	k8s.io/client-go v0.17.3
 	k8s.io/klog v1.0.0
 	k8s.io/kubectl v0.17.3
+	rsc.io/letsencrypt v0.0.3 // indirect


this is unexpected

this should be gone now

ecordell · 2020-03-12T16:28:01Z

pkg/image/registry.go

+	Pack(ctx context.Context, ref string, from io.Reader) (next string, err error)
+}
+
+// We also need to generate Dockerfiles


I know this isn't final yet, but there are a few left over comments like this

It should be cleaned up now.

ecordell · 2020-03-12T16:29:09Z

pkg/image/unprivileged/options.go

+		return nil, err
+	}
+
+	bdb, err := bolt.Open(config.DBPath, 0644, nil)


At least we got bolt in here somehow!

We don't expect to have endianness concerns with this because this is a generated always at runtime and never persisted, correct?

Yup, it's always generated at runtime unless you really want to use an existing file.

ecordell · 2020-03-17T21:33:09Z

cmd/opm/registry/add.go

@@ -27,7 +27,6 @@ func newRegistryAddCmd() *cobra.Command {
 	rootCmd.Flags().StringP("database", "d", "bundles.db", "relative path to database file")
 	rootCmd.Flags().StringSliceP("bundle-images", "b", []string{}, "comma separated list of links to bundle image")
 	rootCmd.Flags().Bool("permissive", false, "allow registry load errors")
-	rootCmd.Flags().StringP("container-tool", "c", "podman", "tool to interact with container images (save, build, etc.). One of: [docker, podman]")


we should consider making this a hidden flag in case anyone was relying on it existing (instead of throwing an error about not knowing the flag).

If we're confident no one is relying on it yet, we can remove.

@benluddy and I were discussing this in an earlier review. Within Red Hat, I know there's some unreleased automation making use of the default value (not specifying --container-tool). However, I do think it's safer to use a hidden flag. Can we meet in the middle with a hidden flag that logs a warning when specified and continues with the daemonless implementation?

I made it a deprecated flag, which is both hidden and gives a warning when used.

ecordell · 2020-03-17T21:33:51Z

go.mod

@@ -31,3 +39,5 @@ require (
 	k8s.io/klog v1.0.0
 	k8s.io/kubectl v0.17.3
 )
+
+replace github.com/docker/distribution => github.com/docker/distribution v0.0.0-20191216044856-a8371794149d


is there any way to resolve this? we just got these all cleaned up...

We could just not run an in-process docker registry, which I expect would make the tests more complex.

This is also a test dependency, which I think means it's not required by dependents to use the operator-registry module.

ecordell · 2020-03-17T21:49:12Z

pkg/image/unprivileged/resolver.go

+
+func NewResolver(username, password string, configs ...string) (remotes.Resolver, error) {
+	// TODO(njhale): make plainHTTP and insecure optional
+	plainHTTP := true


We should figure out these options before we merge - not supporting tls verification will be an issue.

pkg/image/unprivileged/resolver.go

cmd/opm/registry/add.go

njhale · 2020-03-19T18:41:31Z

I received some internal feedback to implement this with opencontainers instead of containerd if possible.

/hold

- Add the ability to pull and unpack bundle images without using to an external tool or daemon - Introduce a new set of bundle image oriented interfaces to decouple image interaction from shelling out to external tools - Add testdata dir and dot-import ginkgo - Add pull and unpack unit tests - Deprecate registry add container-tool opt - Add skip-tls option to registry add

this still requires some refactoring and does not implement Unpack

- Add image.Reference interface and SimpleReference implementation as parameters of image.Registry methods (in place of strings) - Refactor image.Registry tests into suite - Add registry implementation notes - Remove buildah from builds (Note: should be re-added once it's complete)

Update vendor directory to include dependencies needed for the containerd image.Registry implementation.

ecordell · 2020-04-02T03:33:06Z

/lgtm

njhale · 2020-04-02T03:33:59Z

/hold

njhale · 2020-04-02T03:37:13Z

/hold cancel

Now I'm ready :)

njhale · 2020-04-02T13:12:08Z

/retest

ecordell · 2020-04-02T14:23:08Z

/lgtm

openshift-ci-robot · 2020-04-02T14:23:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ecordell, njhale

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:

~~OWNERS~~ [ecordell,njhale]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the do-not-merge/work-in-progress label Mar 12, 2020

openshift-ci-robot requested review from awgreene and dinhxuanvu March 12, 2020 06:09

openshift-ci-robot added needs-rebase approved size/XXL labels Mar 12, 2020

njhale force-pushed the daemonless branch from 029970c to 9189ad4 Compare March 12, 2020 06:19

openshift-ci-robot removed the needs-rebase label Mar 12, 2020

njhale force-pushed the daemonless branch 4 times, most recently from 456c309 to 0db37f3 Compare March 12, 2020 15:36

njhale marked this pull request as ready for review March 12, 2020 15:38

njhale changed the title ~~feat(opm): add unprivileged registry add~~ WIP: feat(opm): add unprivileged registry add Mar 12, 2020

openshift-ci-robot added do-not-merge/work-in-progress and removed do-not-merge/work-in-progress labels Mar 12, 2020

exdx reviewed Mar 12, 2020

View reviewed changes

anik120 reviewed Mar 12, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase label Mar 15, 2020

njhale force-pushed the daemonless branch from 27a5fd7 to 9e15293 Compare March 16, 2020 14:34

openshift-ci-robot removed the needs-rebase label Mar 16, 2020

njhale force-pushed the daemonless branch from 9e15293 to 111fcf6 Compare March 17, 2020 20:48

njhale changed the title ~~WIP: feat(opm): add unprivileged registry add~~ feat(opm): add unprivileged registry add Mar 17, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress label Mar 17, 2020

ecordell reviewed Mar 17, 2020

View reviewed changes

njhale force-pushed the daemonless branch 4 times, most recently from b709f6b to aa29131 Compare March 19, 2020 13:46

openshift-ci-robot added the do-not-merge/hold label Mar 19, 2020

njhale force-pushed the daemonless branch from 5efd370 to 95291b7 Compare March 25, 2020 18:40

openshift-ci-robot added needs-rebase and removed needs-rebase labels Mar 27, 2020

njhale force-pushed the daemonless branch from 6e13744 to a021cb5 Compare April 2, 2020 01:15

njhale and others added 5 commits April 1, 2020 22:25

docs(opm): add self-contained container-tools section

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

d05586a

feat(deamonless): add buildah-based deamonless image puller

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

f666f9d

this still requires some refactoring and does not implement Unpack

njhale force-pushed the daemonless branch from a021cb5 to 207a140 Compare April 2, 2020 02:57

openshift-ci-robot removed the needs-rebase label Apr 2, 2020

openshift-ci-robot assigned ecordell Apr 2, 2020

openshift-ci-robot added the lgtm label Apr 2, 2020

refactor(populator): ensure file descriptors are closed

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.

Learn about vigilant mode

3067694

openshift-ci-robot removed the lgtm label Apr 2, 2020

openshift-ci-robot removed the do-not-merge/hold label Apr 2, 2020

openshift-ci-robot added the lgtm label Apr 2, 2020

openshift-merge-robot merged commit aa1652d into operator-framework:master Apr 2, 2020

ecordell mentioned this pull request Apr 7, 2020

fix(imageloader): image loader should consider existing channels during add #260

Merged

5 tasks

This was referenced Apr 13, 2020

Unable to pull form insecure registry #268

Closed

opm should work with /etc/containers/registries.conf #264

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(opm): add unprivileged registry add #213

feat(opm): add unprivileged registry add #213

njhale commented Mar 12, 2020 •

edited

Loading

njhale commented Mar 12, 2020

exdx Mar 12, 2020

benluddy Mar 19, 2020

njhale Mar 19, 2020

anik120 Mar 12, 2020

njhale Mar 16, 2020

ecordell left a comment

ecordell Mar 12, 2020

njhale Mar 17, 2020

ecordell Mar 12, 2020

njhale Mar 17, 2020

ecordell Mar 12, 2020

njhale Mar 17, 2020

ecordell Mar 17, 2020

njhale Mar 17, 2020 •

edited

Loading

njhale Mar 19, 2020

ecordell Mar 17, 2020

njhale Mar 17, 2020

njhale Mar 19, 2020

ecordell Mar 17, 2020

njhale commented Mar 19, 2020

ecordell commented Apr 2, 2020

njhale commented Apr 2, 2020

njhale commented Apr 2, 2020

njhale commented Apr 2, 2020

ecordell commented Apr 2, 2020

openshift-ci-robot commented Apr 2, 2020

feat(opm): add unprivileged registry add #213

feat(opm): add unprivileged registry add #213

Conversation

njhale commented Mar 12, 2020 • edited Loading

njhale commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhale commented Mar 19, 2020

ecordell commented Apr 2, 2020

njhale commented Apr 2, 2020

njhale commented Apr 2, 2020

njhale commented Apr 2, 2020

ecordell commented Apr 2, 2020

openshift-ci-robot commented Apr 2, 2020

njhale commented Mar 12, 2020 •

edited

Loading

njhale Mar 17, 2020 •

edited

Loading