Allow selective disabling of registries and media types #239

cgonyeo · 2017-01-24T22:53:05Z

This commit tweaks the library's API to allow consumers to selectively
disable registry support (v1 or v2) and what mediatypes docker2aci
claims to be able to accept during an image pull (v2.1, v2.2, and oci).
By default all options are enabled, and this is still the case when
using docker2aci via the command line.

Fixes #216.

cgonyeo · 2017-01-24T22:53:56Z

And for the sake of cross linking, this PR will allow rkt/rkt#3134 to be implemented.

cgonyeo · 2017-01-24T22:56:13Z

Oh and for more cross linking, it's a continuation of #236, in which there's more explanation on the rationale of these changes.

lucab

I think most of this is already ok, but there are some leftovers.

lucab · 2017-01-25T13:58:02Z

lib/common/common.go

+// fetchimg images. As an example if a MediaTypeSet is equal to
+// {MediaTypeOptionDockerV22, MediaTypeOptionOCIV1Pre}, then when an image pull
+// is made V2.1 images will not be fetched. This doesn't apply to V1 pulls. As
+// an edge cast if a MedaTypeSet contains nothing, that means that _every_ type


"edge case"

@s-urbaniak this is our usual discussion regarding nullable field. Is it ok/idiomatic to use the nil value as the default/auto selector, or should we have an explicit entry for that?

@lucab yes I think it's fine to have the zero value (nil) or empty list as a valid value. I don't think we need to have an explicit entry.

lucab · 2017-01-25T14:00:59Z

main.go

@@ -37,6 +37,7 @@ var (
 	flagInsecureAllowHTTP  bool
 	flagCompression        string
 	flagVersion            bool
+	flagOCISupport         bool


I think this is not consumed anymore in the PR. Either you forgot to scratch this out, or there are some changes missing from this PR.

Whoops, forgot to remove this. This is leftover from when the PR was to add a flag to disable OCI support. Removing now.

lucab · 2017-01-25T14:08:10Z

main.go

@@ -47,6 +48,7 @@ func init() {
 	flag.BoolVar(&flagInsecureAllowHTTP, "insecure-allow-http", false, "Uses unencrypted connections when fetching images")
 	flag.StringVar(&flagCompression, "compression", "gzip", "Type of compression to use; allowed values: gzip, none")
 	flag.BoolVar(&flagVersion, "version", false, "Print version")
+	flag.BoolVar(&flagOCISupport, "experimental-oci", false, "Enable experimental OCI support")


I'm perfectly fine about not surfacing the internal complexity of the library regarding mediatype and registry, and just having a single switch for OCI if you prefer.

As I think we are already doing this silently now and it's going to be the default mode at some point, perhaps it should instead be a switch which limits to dockerv1/v2 only?

Same as above, I didn't intend to include this.

just having a single switch for OCI if you prefer

Wasn't that what this PR was: #236 ?

I intended to not change the cli at all, and library users can easily ignore the added complexity if they want to. The zero value of the fields added to the struct used by library consumers result in the same behavior as before (as in, they don't set the fields and the same thing happens as before this change). If consumers wish to disable support for one of the registry HTTP APIs or specific media types in the latest API version, then they simply provide a list of what they do want docker2aci to support.

lucab · 2017-01-25T14:13:03Z

Thinking aloud: I think the library consumer may be interested in knowing how an image has been pulled (or it may not? @s-urbaniak @euank).

If that is a useful information, we can either return the type back to the caller (but then it needs to be persisted somehow), or directly encode it as an image annotation.

cgonyeo · 2017-01-25T18:37:32Z

If we're going to inform docker2aci consumers how an image was pulled I'd prefer to encode it as an image annotation. We already encode a lot of information like this, and it means you can go back and re-inspect the value at any point.

Still, I'd prefer to get some confirmation this is useful before I do the work :)

s-urbaniak

just a few nits, hence LGTM'ing, thanks! 👍

s-urbaniak · 2017-01-26T08:29:25Z

lib/common/common.go

+// of media type is enabled.
+type MediaTypeSet []MediaTypeOption
+
+func (m MediaTypeSet) GetManifestMediaTypes() []string {


Given this is in a public facing lib package, let's make this and all the other getters idiomatic, i.e. no getter, see https://golang.org/doc/effective_go.html#Getters.

s-urbaniak · 2017-01-26T08:35:00Z

lib/common/common.go

+	MediaTypeOptionOCIV1Pre
+)
+
+// MediaTypeSet represents a set of media types which docker2aci is to use when


Given this aliases to a slice type, we should clarify that it is the user's responsibility to maintain the set invariant.

If it is violated, from what I see, nothing will panic, besides setting redundant Accept http headers. The same goes for the RegistryOptionSet.

You're right in that nothing particularly bad will happen if the set invariant is violated, but it would probably be best for me to mention it regardless.

s-urbaniak · 2017-01-26T08:37:16Z

lib/common/common.go

+// RegistryOptionSet represents a set of registry types which docker2aci is to
+// use when fetching images. As an example if a RegistryOptionSet is equal to
+// {RegistryOptionV2}, then v1 pulls are disabled. As an edge case if a
+// RegistryOptionSet contains nothing, that means that _every_ type of registry


s/"contains nothing"/"is nil or empty list"

@lucab yes we should make it clear that this works with the zero value of the underlying slice type, see my suggestion above.

s-urbaniak · 2017-01-26T08:43:58Z

lib/internal/backend/repository/repository.go

@@ -67,11 +67,13 @@ type RepositoryBackend struct {
 	imageV2Manifests  map[common.ParsedDockerURL]*typesV2.ImageManifest
 	imageConfigs      map[common.ParsedDockerURL]*typesV2.ImageConfig
 	layersIndex       map[string]int
+	ms                common.MediaTypeSet
+	rs                common.RegistryOptionSet


given the other local vars have spelled names, let's spell these out too, their usage is not local, so I think that's ok.

i.e.:
s/ms/mediaTypes
s/rs/registryOptions

s-urbaniak · 2017-01-26T09:12:12Z

The information how an image has been pulled can be interesting as an intermediate step in the transition to native image support in rkt, but currently I am not aware of its usefulness. Since it's doable easily I suggest we introduce such a feature when explicitely needed.

lucab · 2017-01-26T09:32:23Z

I agree that is both easy to add later and not required now, so let's just skip for the moment.

lucab

One minor name changing suggestion. Otherwise, except for Sergiusz's comments, looks fine.

lucab · 2017-01-26T10:00:05Z

lib/common/common.go

+	MediaTypeOCIManifest     = spec.MediaTypeImageManifest
+	MediaTypeOCIManifestList = spec.MediaTypeImageManifestList
+	MediaTypeOCIConfig       = spec.MediaTypeImageConfig
+	MediaTypeOCILayer        = spec.MediaTypeImageLayer


To match the name of the imported package, I think thouse should have a "V1" somewhere in them (eg. MediaTypeOCIV1Layer)

cgonyeo · 2017-01-26T18:03:23Z

All nits addressed

cgonyeo · 2017-01-30T20:51:35Z

@lucab bump on this

lucab · 2017-01-30T21:14:48Z

I think that except for a rebase and some tests adjustments, this is ready to go.

This commit tweaks the library's API to allow consumers to selectively disable registry support (v1 or v2) and what mediatypes docker2aci claims to be able to accept during an image pull (v2.1, v2.2, and oci). By default all options are enabled, and this is still the case when using docker2aci via the command line.

cgonyeo · 2017-02-03T18:46:11Z

Rebased and fixed tests

lucab

LGTM

lucab suggested changes Jan 25, 2017

View reviewed changes

cgonyeo force-pushed the master branch from 2e568f1 to 700c122 Compare January 25, 2017 18:46

s-urbaniak approved these changes Jan 26, 2017

View reviewed changes

lucab suggested changes Jan 26, 2017

View reviewed changes

cgonyeo force-pushed the master branch from 700c122 to 5bb7ef5 Compare January 26, 2017 18:02

cgonyeo force-pushed the master branch from 5bb7ef5 to a4e343c Compare February 3, 2017 18:45

lucab approved these changes Feb 7, 2017

View reviewed changes

lucab merged commit d6e05f6 into appc:master Feb 7, 2017

This was referenced Feb 10, 2017

docker2aci v0.16.0 #243

Merged

vendor: bump docker2aci to 0.16.0 rkt/rkt#3591

Merged

Allow selective disabling of registries and media types #239

Allow selective disabling of registries and media types #239

Uh oh!

Conversation

cgonyeo commented Jan 24, 2017

Uh oh!

cgonyeo commented Jan 24, 2017

Uh oh!

cgonyeo commented Jan 24, 2017

Uh oh!

lucab left a comment

Choose a reason for hiding this comment

Uh oh!

lucab Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucab commented Jan 25, 2017

Uh oh!

cgonyeo commented Jan 25, 2017

Uh oh!

s-urbaniak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Jan 26, 2017

Uh oh!

lucab commented Jan 26, 2017

Uh oh!

lucab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgonyeo commented Jan 26, 2017

Uh oh!

cgonyeo commented Jan 30, 2017

Uh oh!

lucab commented Jan 30, 2017

Uh oh!

cgonyeo commented Feb 3, 2017

Uh oh!

lucab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lucab Jan 25, 2017 •

edited

Loading