*: pluggable transports #216

runcom · 2017-01-18T14:13:51Z

Close #125

As discussed in #125, this patch creates a new subpkg to import transports independently. projectatomic/docker would greatly benefit from this as it needs just the docker transport (right now).

Those will be the stats in projectatomic/docker + this PR:

Showing  76 changed files  with 1,715 additions and 6,770 deletions.

@mtrmac PTAL (far from being ready, just a start...)

Signed-off-by: Antonio Murdaca runcom@redhat.com

mtrmac

Broadly, ACK.

It is annoying that registeredtransports.ImageName is so long and ImageName is really unrelated… but I don’t have a good idea how to solve that.

We could cheat and put ImageName into types/commonutils.go or something.

Or we could have containers/image/transports (contains ImageName), containers/image/transports/registry (contains KnownTransports) and containers/image/transports/alltransports (contains the full list of transports, and ParseImageName). But that feels like overengineering it.

mtrmac · 2017-01-19T17:23:36Z

copy/copy.go

 	"github.com/containers/image/signature"
-	"github.com/containers/image/transports"
+	_ "github.com/containers/image/transports"


Why this? This would force any user of copy.Image to import all transports.

right, my bad, skopeo should actually import transports to register them all

mtrmac · 2017-01-19T17:25:21Z

signature/policy_config.go

@@ -123,7 +123,7 @@ func (m *policyTransportsMap) UnmarshalJSON(data []byte) error {
 	// So, use a temporary map of pointers-to-slices and convert.
 	tmpMap := map[string]*PolicyTransportScopes{}
 	if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} {
-		transport, ok := transports.KnownTransports[key]
+		transport, ok := registeredtransports.KnownTransports[key]


This will need more work; otherwise an user who imports only a subset of transports would refuse to use a policy.json which defines policies for other transports.

I suppose unmarshaling and remarshaling a policy.json should always keep it unmodified, so we can’t just drop the sections for unknown transports; instead, policyTransportScopesWithTransport will need to be taught to deal with a nil transport, and silently accept any key. It seems that requirementsForImageRef does the right thing, or close enough to it, even in such a situation.

It bothers me a bit that we would accept any typo in transport names, though… perhaps we could add a registeredtransports.AllExistingTransportsRegistered bool = false, with the “all-transports” transports.init() seting it to true, and refusing unknown values if AllExistingTransportsRegistered? The downside is that that would make it impossible to add policies private transports not known to containers/image/transports for a single-application-use.

This will need more work; otherwise an user who imports only a subset of transports would refuse to use a policy.json which defines policies for other transports.
I suppose unmarshaling and remarshaling a policy.json should always keep it unmodified, so we can’t just drop the sections for unknown transports; instead, policyTransportScopesWithTransport will need to be taught to deal with a nil transport, and silently accept any key. It seems that requirementsForImageRef does the right thing, or close enough to it, even in such a situation.

Could you shed some light on how you want this to be done in the code?

It bothers me a bit that we would accept any typo in transport names, though… perhaps we could add a registeredtransports.AllExistingTransportsRegistered bool = false, with the “all-transports” transports.init() seting it to true, and refusing unknown values if AllExistingTransportsRegistered? The downside is that that would make it impossible to add policies private transports not known to containers/image/transports for a single-application-use.

Not sure about this, it bothers me that ppl will need to know about this and set/unset this. It could be fine though.

This will need more work; otherwise an user who imports only a subset of transports would refuse to use a policy.json which defines policies for other transports. …policyTransportScopesWithTransport will need to be taught to deal with a nil transport, and silently accept any key. It seems that requirementsForImageRef does the right thing, or close enough to it, even in such a situation.

Could you shed some light on how you want this to be done in the code?

In policyTransportsMap.UnmarshalJSON, if KnownTransports[key] does not exist, just use nil.

In policyTransportScopesWithTransport.UnmarshalJSON, if m.transport is nil (from the change above), skip the ValidatePolicyConfigurationScope call (accept any scope string)

For requirementsForImageRef, add a test that when a transport is not in KnownTransports when parsing the policy, but it is in KnownTransports at the time of requirementsForImageRef, the correct set of scopes is used (more or less re-run the existing test, except creating the policy in a different way).

Plus as close to 100% test coverage as possible.

I hope that makes some sense.

It bothers me a bit that we would accept any typo in transport names, though… perhaps we could add a registeredtransports.AllExistingTransportsRegistered bool = false, with the “all-transports” transports.init() seting it to true, and refusing unknown values if AllExistingTransportsRegistered? The downside is that that would make it impossible to add policies private transports not known to containers/image/transports for a single-application-use.

Not sure about this, it bothers me that ppl will need to know about this and set/unset this. It could be fine though.

The idea is that this would work automatically for users who use both all of the known AllTransports, and any subset of them. The other side of this is that there would be nothing to manage, no extra work but also no flexibility; if skopeo thought it knows about all possible transport names, then it still possible to create a local tool with mypinkponytransport, but the system-wide /etc/containers/policy.json must not contain a policy for mypinkponytransport because skopeo would reject that policy file.

Yeah, not sure about this either.

mtrmac · 2017-01-19T17:27:36Z

signature/signature_test.go

@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 	"testing"

+	_ "github.com/containers/image/openshift"


What is this needed for?

wrong, file this is needed in policy_config_test.go where we use the atomic transport in https://github.com/containers/image/blob/master/signature/policy_config_test.go#L242. docker and dir are already imported in that test file so we just need this or it fails with:

--- FAIL: TestPolicyUnmarshalJSON (0.00s) Error Trace: policy_config_test.go:255 Error: Received unexpected error "Unknown key \"atomic\""

Btw, fixed

Thanks. Add a comment perhaps?

mtrmac · 2017-01-19T17:28:44Z

transports/transports.go

 		}
-		KnownTransports[name] = t
+		registeredtransports.Register(t)


These transports should have been registered merely by the fact of being imported here, shouldn’t they? So this could just verify their presence and panic if they are not already in KnownTransports. Or is the import and initialization order uncertain?

no need to panic I guess, I dropped this altogether.

mtrmac · 2017-01-19T17:42:14Z

Or we could have containers/image/transports (contains ImageName), containers/image/transports/registry (contains KnownTransports) and containers/image/transports/alltransports (contains the full list of transports, and ParseImageName). But that feels like overengineering it.

Actually, we only need two packages. Perhaps (/transports, /registeredtransports)→(/alltransports, /transports), or (transports/alltransports, /transports)? We are breaking the API for one of the two functions either way, and transports.ImageName reads a bit better while alltransports.ParseImageName is still acceptable.

fix containers/image progress

runcom · 2017-02-16T11:15:53Z

Actually, we only need two packages. Perhaps (/transports, /registeredtransports)→(/alltransports, /transports), or (transports/alltransports, /transports)? We are breaking the API for one of the two functions either way, and transports.ImageName reads a bit better while alltransports.ParseImageName is still acceptable.

done this, just have a question around signatures.

runcom · 2017-02-16T13:37:39Z

@cyphar could you also take a look at this? We are mainly trying to decouple each transports wrt the signature pkg. Right now, if you use the signature package, you are required to pull every other transport c/image has (that means, stuff like the storage transport which pulls in many files). This change aims at making easy to use the signature package with just a subset of interested transports (i.e. just docker, or oci and docker ...). Skopeo will keep pulling in all transports via the new transports/alltransports package but users of copy&signature pkgs can now choose which transport to use.

cyphar · 2017-02-16T16:02:06Z

transports/alltransports/alltransports.go

+)
+
+// ParseImageName converts a URL-like image name to a types.ImageReference.
+func ParseImageName(imgName string) (types.ImageReference, error) {


IMO this should be done in transports. This package should just be doing dumb imports and not providing any other functionality -- though I understand why you'd want to do this (so you don't have an annoying _ import).

The underlying idea is that the strings are exposed to the users, so they should always mean the same thing; we should not have skopeo and whateverothertool use strings which look the same but actually each accepting a different subset of transports. So, if any user of the library uses the strings in the UI and wants to use ParseImageName, they automatically get all transports: they can’t forget the _ import, as you say, and they can’t even opt out — this automatically tries to do the right thing for users. I understand that this may feel annoyingly heavy-handed; is it actually problematic? I think that on balance this does the right thing for users but I can of course be mistaken.

(More specialized users of containers/image can use somepackage.Transport.ParseReference to get a part of the parsing functionality, or somepackage.NewReference if working with the native values directly.)

cyphar · 2017-02-16T16:02:58Z

transports/transports.go

 	"github.com/containers/image/types"
-	"github.com/pkg/errors"
 )

 // KnownTransports is a registry of known ImageTransport instances.
 var KnownTransports map[string]types.ImageTransport


IMO this should be hidden (with some sort of Get function) and have a lock around it internally. It isn't a good idea to expose a map globally.

(No strong opinion on this; conceptually this is a separate idea from this PR; why not I guess.)

yeah, I'll do that

cyphar · 2017-02-16T16:03:10Z

transports/transports.go

-	if !ok {
-		return nil, errors.Errorf(`Invalid image name "%s", unknown transport "%s"`, imgName, parts[0])
+// Register TODO(runcom)
+func Register(t types.ImageTransport) {


As above, add a lock to this.

cyphar · 2017-02-16T16:03:47Z

transports/transports.go

 	}
-	return transport.ParseReference(parts[1])
+	KnownTransports[name] = t
 }



Can we add a Deregister function? Also it would be nice to have a simple Get function (as the above comment noted), so that we can hide KnownTransports?

When would it be useful to call Deregister? Lock or not, it is global state, and there would be a risk of one package deregistering a transport which a completely unrelated package has also imported and expects to be present.

If someone wants to switch out transports -- or even for mocking? Maybe an atomic Replace or something would reduce the issue?

mtrmac · 2017-02-16T16:04:34Z

transports/transports.go

 )

 // KnownTransports is a registry of known ImageTransport instances.
 var KnownTransports map[string]types.ImageTransport

 func init() {
 	KnownTransports = make(map[string]types.ImageTransport)
-	// NOTE: Make sure docs/policy.json.md is updated when adding or updating
-	// a transport.


Please preserve this comment (in the import block of alltransports.go probably).

runcom · 2017-02-20T11:04:24Z

@mtrmac PTAL (test failure is #238)

mtrmac

Broadly ACK, the policy part still needs more tests

mtrmac · 2017-02-21T18:46:00Z

signature/policy_config_test.go

@@ -269,9 +270,6 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
 		func(v mSI) { v["transports"] = []string{} },
 		// "default" is an invalid PolicyRequirements
 		func(v mSI) { v["default"] = PolicyRequirements{} },
-		// A key in "transports" is an invalid transport name
-		func(v mSI) { x(v, "transports")["this is unknown"] = x(v, "transports")["docker"] },
-		func(v mSI) { x(v, "transports")[""] = x(v, "transports")["docker"] },


There should be a test for an unknown transport name (that it is not rejected).

mtrmac · 2017-02-21T18:46:15Z

signature/policy_config.go

@@ -174,7 +172,7 @@ func (m *policyTransportScopesWithTransport) UnmarshalJSON(data []byte) error {
 		if _, ok := tmpMap[key]; ok {
 			return nil
 		}
-		if key != "" {
+		if key != "" && m.transport != nil {


This needs a test.

And another test:

For requirementsForImageRef, add a test that when a transport is not in KnownTransports when parsing the policy, but it is in KnownTransports at the time of requirementsForImageRef, the correct set of scopes is used (more or less re-run the existing test, except creating the policy in a different way).

isn't this https://github.com/containers/image/pull/216/files#diff-38040ba06b5a45464a80ca2fb88302c2R249 testing this code path?

That tests parsing the policy into a data structure, not using the data structure to look up the relevant rules to evaluate for a particular image (requirementsForImageRef). Right now, by inspection, it seems clear enough that requirementsForImageRef can correctly handle a transport which exists but is unregistered, or a transport which was not registered when parsing the policy but is registered afterwards—but the transport registry is a very non-obvious hidden state in the parsing/evaluation interaction, so it would be nice to have a test which ensures that the code keeps working in these corner cases.

added a test, hopefully done correctly (pretty late here now)

mtrmac · 2017-02-21T18:47:29Z

signature/policy_config.go

-		if !ok {
-			return nil
-		}
+		// transport can be nil


Also please update the comment at policyTransportScopesWithTransport: while validating using a specific ImageTransport if not nil

mtrmac · 2017-02-21T18:48:41Z

signature/policy_config_test.go

@@ -9,6 +9,7 @@ import (

 	"github.com/containers/image/directory"
 	"github.com/containers/image/docker"
+	_ "github.com/containers/image/openshift"


Add a comment explaining the need for this please.

#216 (comment)

mtrmac · 2017-02-21T18:53:55Z

transports/transports.go

-	if !ok {
-		return nil, errors.Errorf(`Invalid image name "%s", unknown transport "%s"`, imgName, parts[0])
+	kt.transports[name] = t
+	kt.mu.Unlock()


Shouldn’t this use defer? This way, if we panic above, the lock is permanently locked. Or perhaps it would be better to explicitly unlock it before that panic, randomly unlocking in the middle of some other code path could be risky… not that there are that many code paths here :)

mtrmac · 2017-02-21T18:55:09Z

transports/transports.go

-// KnownTransports is a registry of known ImageTransport instances.
-var KnownTransports map[string]types.ImageTransport
+// knownTransports is a registry of known ImageTransport instances.
+type knownTransports struct {


With this being a private variable of a private type, it’s really not obvious to me what defining the type and methods adds right now.

I guess it allows writing tests? Except that those tests don’t exist.

(But this does work, feel free to leave it as it is.)

mtrmac

Thanks for putting up with me!

LGTM pending a successful test in updated skopeo.

mtrmac · 2017-02-28T15:38:52Z

signature/policy_eval_test.go

+
+	assert.Nil(t, transports.Get("docker"))
+	transports.Register(docker.Transport)
+	assert.NotNil(t, transports.Get("docker"))


Please move this cleanup into a defer block right after unregistering trhe transport; otherwise one of the requires aborting would leave transports in an unexpected state.

runcom · 2017-02-28T16:21:36Z

@mtrmac fixed but your LGTM didn't work

mtrmac · 2017-02-28T16:34:00Z

👍 pending a successful test in updated skopeo.

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom · 2017-03-02T14:20:34Z

@mtrmac containers/skopeo#288 it's green over at skopeo 👍 (https://travis-ci.org/projectatomic/skopeo/builds/207004846)

runcom · 2017-03-02T14:20:39Z

LGTM

runcom force-pushed the pluggable-transports branch from ed1136e to b33e9eb Compare January 18, 2017 14:23

runcom mentioned this pull request Jan 18, 2017

vendor c/image for pluggable transports containers/skopeo#288

Merged

runcom changed the title *: pluggable transports [WIP] *: pluggable transports Jan 19, 2017

runcom force-pushed the pluggable-transports branch from b33e9eb to f755409 Compare January 19, 2017 17:24

mtrmac reviewed Jan 19, 2017

View reviewed changes

runcom force-pushed the pluggable-transports branch from f755409 to 20454d9 Compare January 20, 2017 08:49

mtrmac mentioned this pull request Jan 23, 2017

Allow choosing between a gpgme and openpgp signature backend using a build tag #207

Merged

giuseppe pushed a commit to giuseppe/image that referenced this pull request Jan 24, 2017

Merge pull request containers#216 from runcom/progress-fix

c813de9

fix containers/image progress

runcom force-pushed the pluggable-transports branch 2 times, most recently from 9edee29 to 9f73252 Compare February 16, 2017 11:10

runcom force-pushed the pluggable-transports branch 5 times, most recently from 61dccbe to 80f83ad Compare February 16, 2017 11:43

runcom force-pushed the pluggable-transports branch from 80f83ad to a39304a Compare February 16, 2017 13:46

cyphar reviewed Feb 16, 2017

View reviewed changes

mtrmac reviewed Feb 16, 2017

View reviewed changes

wking mentioned this pull request Feb 17, 2017

CAS in the granularity of file opencontainers/image-spec#577

Closed

runcom force-pushed the pluggable-transports branch from a39304a to add4ea6 Compare February 20, 2017 11:04

mtrmac reviewed Feb 21, 2017

View reviewed changes

runcom force-pushed the pluggable-transports branch 2 times, most recently from dd69424 to 3631acd Compare February 27, 2017 21:48

mtrmac reviewed Feb 28, 2017

View reviewed changes

runcom force-pushed the pluggable-transports branch from 3631acd to 3c3d25c Compare February 28, 2017 16:21

runcom changed the title ~~[WIP] *: pluggable transports~~ *: pluggable transports Feb 28, 2017

*: pluggable transports

29da2bc

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the pluggable-transports branch from 3c3d25c to 29da2bc Compare March 2, 2017 11:25

manifest: remove not needed mediatype

0a5f50d

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom merged commit 21bb303 into containers:master Mar 2, 2017

runcom deleted the pluggable-transports branch March 2, 2017 14:21

*: pluggable transports #216

*: pluggable transports #216

Conversation

runcom commented Jan 18, 2017 • edited Loading

mtrmac 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Jan 19, 2017

runcom commented Feb 16, 2017

runcom commented Feb 16, 2017

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

cyphar Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Feb 20, 2017

mtrmac 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

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

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcom commented Feb 28, 2017

mtrmac commented Feb 28, 2017 • edited by runcom Loading

runcom commented Mar 2, 2017 • edited Loading

runcom commented Mar 2, 2017 • edited Loading

runcom commented Jan 18, 2017 •

edited

Loading

cyphar Feb 16, 2017 •

edited

Loading

mtrmac commented Feb 28, 2017 •

edited by runcom

Loading

runcom commented Mar 2, 2017 •

edited

Loading

runcom commented Mar 2, 2017 •

edited

Loading