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

*: pluggable transports #216

Merged
merged 2 commits into from
Mar 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions directory/directory_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,15 @@ import (
"github.com/containers/image/directory/explicitfilepath"
"github.com/containers/image/docker/reference"
"github.com/containers/image/image"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/opencontainers/go-digest"
)

func init() {
transports.Register(Transport)
}

// Transport is an ImageTransport for directory paths.
var Transport = dirTransport{}

Expand Down
5 changes: 5 additions & 0 deletions docker/daemon/daemon_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,15 @@ import (

"github.com/containers/image/docker/reference"
"github.com/containers/image/image"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/opencontainers/go-digest"
)

func init() {
transports.Register(Transport)
}

// Transport is an ImageTransport for images managed by a local Docker daemon.
var Transport = daemonTransport{}

Expand Down
5 changes: 5 additions & 0 deletions docker/docker_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,15 @@ import (

"github.com/containers/image/docker/policyconfiguration"
"github.com/containers/image/docker/reference"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/pkg/errors"
)

func init() {
transports.Register(Transport)
}

// Transport is an ImageTransport for Docker registry-hosted images.
var Transport = dockerTransport{}

Expand Down
2 changes: 1 addition & 1 deletion manifest/manifest.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func GuessMIMEType(manifest []byte) string {
}

switch meta.MediaType {
case DockerV2Schema2MediaType, DockerV2ListMediaType, imgspecv1.MediaTypeImageManifest, imgspecv1.MediaTypeImageManifestList: // A recognized type.
case DockerV2Schema2MediaType, DockerV2ListMediaType: // A recognized type.
return meta.MediaType
}
// this is the only way the function can return DockerV2Schema1MediaType, and recognizing that is essential for stripping the JWS signatures = computing the correct manifest digest.
Expand Down
5 changes: 5 additions & 0 deletions oci/layout/oci_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@ import (
"github.com/containers/image/directory/explicitfilepath"
"github.com/containers/image/docker/reference"
"github.com/containers/image/image"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/opencontainers/go-digest"
"github.com/pkg/errors"
)

func init() {
transports.Register(Transport)
}

// Transport is an ImageTransport for OCI directories.
var Transport = ociTransport{}

Expand Down
5 changes: 5 additions & 0 deletions openshift/openshift_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,15 @@ import (
"github.com/containers/image/docker/policyconfiguration"
"github.com/containers/image/docker/reference"
genericImage "github.com/containers/image/image"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/pkg/errors"
)

func init() {
transports.Register(Transport)
}

// Transport is an ImageTransport for OpenShift registry-hosted images.
var Transport = openshiftTransport{}

Expand Down
10 changes: 4 additions & 6 deletions signature/policy_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,8 @@ 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]
if !ok {
return nil
}
// transport can be nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done

transport := transports.Get(key)
// paranoidUnmarshalJSONObject detects key duplication for us, check just to be safe.
if _, ok := tmpMap[key]; ok {
return nil
Expand Down Expand Up @@ -155,7 +153,7 @@ func (m *PolicyTransportScopes) UnmarshalJSON(data []byte) error {
}

// policyTransportScopesWithTransport is a way to unmarshal a PolicyTransportScopes
// while validating using a specific ImageTransport.
// while validating using a specific ImageTransport if not nil.
type policyTransportScopesWithTransport struct {
transport types.ImageTransport
dest *PolicyTransportScopes
Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs a test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

if err := m.transport.ValidatePolicyConfigurationScope(key); err != nil {
return nil
}
Expand Down
10 changes: 7 additions & 3 deletions signature/policy_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (

"github.com/containers/image/directory"
"github.com/containers/image/docker"
// this import is needed where we use the "atomic" transport in TestPolicyUnmarshalJSON
_ "github.com/containers/image/openshift"
"github.com/containers/image/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -244,6 +246,11 @@ func TestPolicyUnmarshalJSON(t *testing.T) {
xNewPRSignedByKeyData(SBKeyTypeSignedByGPGKeys, []byte("RHatomic"), NewPRMMatchRepository()),
},
},
"unknown": {
"registry.access.redhat.com/rhel7": []PolicyRequirement{
xNewPRSignedByKeyData(SBKeyTypeSignedByGPGKeys, []byte("RHatomic"), NewPRMMatchRepository()),
},
},
},
}
validJSON, err := json.Marshal(validPolicy)
Expand All @@ -269,9 +276,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"] },
}
for _, fn := range breakFns {
err = tryUnmarshalModifiedPolicy(t, &p, validJSON, fn)
Expand Down
33 changes: 33 additions & 0 deletions signature/policy_eval_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@ import (
"os"
"testing"

"github.com/containers/image/docker"
"github.com/containers/image/docker/policyconfiguration"
"github.com/containers/image/docker/reference"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -103,6 +105,37 @@ func (ref pcImageReferenceMock) DeleteImage(ctx *types.SystemContext) error {
panic("unexpected call to a mock function")
}

func TestPolicyContextRequirementsForImageRefNotRegisteredTransport(t *testing.T) {
transports.Delete("docker")
assert.Nil(t, transports.Get("docker"))

defer func() {
assert.Nil(t, transports.Get("docker"))
transports.Register(docker.Transport)
assert.NotNil(t, transports.Get("docker"))
}()

pr := []PolicyRequirement{
xNewPRSignedByKeyData(SBKeyTypeSignedByGPGKeys, []byte("RH"), NewPRMMatchRepository()),
}
policy := &Policy{
Default: PolicyRequirements{NewPRReject()},
Transports: map[string]PolicyTransportScopes{
"docker": {
"registry.access.redhat.com": pr,
},
},
}
pc, err := NewPolicyContext(policy)
require.NoError(t, err)
ref, err := reference.ParseNormalizedNamed("registry.access.redhat.com/rhel7:latest")
require.NoError(t, err)
reqs := pc.requirementsForImageRef(pcImageReferenceMock{"docker", ref})
assert.True(t, &(reqs[0]) == &(pr[0]))
assert.True(t, len(reqs) == len(pr))

}

func TestPolicyContextRequirementsForImageRef(t *testing.T) {
ktGPG := SBKeyTypeGPGKeys
prm := NewPRMMatchRepoDigestOrExact()
Expand Down
5 changes: 5 additions & 0 deletions storage/storage_transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,17 @@ import (

"github.com/Sirupsen/logrus"
"github.com/containers/image/docker/reference"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/containers/storage/storage"
"github.com/opencontainers/go-digest"
ddigest "github.com/opencontainers/go-digest"
)

func init() {
transports.Register(Transport)
}

var (
// Transport is an ImageTransport that uses either a default
// storage.Store or one that's it's explicitly told to use.
Expand Down
31 changes: 31 additions & 0 deletions transports/alltransports/alltransports.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package alltransports

import (
"strings"

// register all known transports
// NOTE: Make sure docs/policy.json.md is updated when adding or updating
// a transport.
_ "github.com/containers/image/directory"
_ "github.com/containers/image/docker"
_ "github.com/containers/image/docker/daemon"
_ "github.com/containers/image/oci/layout"
_ "github.com/containers/image/openshift"
_ "github.com/containers/image/storage"
"github.com/containers/image/transports"
"github.com/containers/image/types"
"github.com/pkg/errors"
)

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

Choose a reason for hiding this comment

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

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).

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

parts := strings.SplitN(imgName, ":", 2)
if len(parts) != 2 {
return nil, errors.Errorf(`Invalid image name "%s", expected colon-separated transport:reference`, imgName)
}
transport := transports.Get(parts[0])
if transport == nil {
return nil, errors.Errorf(`Invalid image name "%s", unknown transport "%s"`, imgName, parts[0])
}
return transport.ParseReference(parts[1])
}
Original file line number Diff line number Diff line change
@@ -1,17 +1,13 @@
package transports
package alltransports

import (
"testing"

"github.com/containers/image/transports"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestKnownTransports(t *testing.T) {
assert.NotNil(t, KnownTransports) // Ensure that the initialization has actually been run
assert.True(t, len(KnownTransports) > 1)
}

func TestParseImageName(t *testing.T) {
// This primarily tests error handling, TestImageNameHandling is a table-driven
// test for the expected values.
Expand All @@ -36,11 +32,12 @@ func TestImageNameHandling(t *testing.T) {
{"docker-daemon", "busybox:latest", "busybox:latest"},
{"oci", "/etc:sometag", "/etc:sometag"},
// "atomic" not tested here because it depends on per-user configuration for the default cluster.
// "containers-storage" not tested here because it needs to initialize various directories on the fs.
} {
fullInput := c.transport + ":" + c.input
ref, err := ParseImageName(fullInput)
require.NoError(t, err, fullInput)
s := ImageName(ref)
s := transports.ImageName(ref)
assert.Equal(t, c.transport+":"+c.roundtrip, s, fullInput)
}
}
85 changes: 47 additions & 38 deletions transports/transports.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,52 +2,61 @@ package transports

import (
"fmt"
"strings"

"github.com/containers/image/directory"
"github.com/containers/image/docker"
"github.com/containers/image/docker/daemon"
ociLayout "github.com/containers/image/oci/layout"
"github.com/containers/image/openshift"
"github.com/containers/image/storage"
"sync"

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

// 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.)

transports map[string]types.ImageTransport
mu sync.Mutex
}

func init() {
KnownTransports = make(map[string]types.ImageTransport)
// NOTE: Make sure docs/policy.json.md is updated when adding or updating
// a transport.
for _, t := range []types.ImageTransport{
directory.Transport,
docker.Transport,
daemon.Transport,
ociLayout.Transport,
openshift.Transport,
storage.Transport,
} {
name := t.Name()
if _, ok := KnownTransports[name]; ok {
panic(fmt.Sprintf("Duplicate image transport name %s", name))
}
KnownTransports[name] = t
}
func (kt *knownTransports) Get(k string) types.ImageTransport {
kt.mu.Lock()
t := kt.transports[k]
kt.mu.Unlock()
return t
}

// ParseImageName converts a URL-like image name to a types.ImageReference.
func ParseImageName(imgName string) (types.ImageReference, error) {
parts := strings.SplitN(imgName, ":", 2)
if len(parts) != 2 {
return nil, errors.Errorf(`Invalid image name "%s", expected colon-separated transport:reference`, imgName)
func (kt *knownTransports) Remove(k string) {
kt.mu.Lock()
delete(kt.transports, k)
kt.mu.Unlock()
}

func (kt *knownTransports) Add(t types.ImageTransport) {
kt.mu.Lock()
defer kt.mu.Unlock()
name := t.Name()
if t := kt.transports[name]; t != nil {
panic(fmt.Sprintf("Duplicate image transport name %s", name))
}
transport, ok := KnownTransports[parts[0]]
if !ok {
return nil, errors.Errorf(`Invalid image name "%s", unknown transport "%s"`, imgName, parts[0])
kt.transports[name] = t
}

var kt *knownTransports

func init() {
kt = &knownTransports{
transports: make(map[string]types.ImageTransport),
}
return transport.ParseReference(parts[1])
}

// Get returns the transport specified by name or nil when unavailable.
func Get(name string) types.ImageTransport {
return kt.Get(name)
}

// Delete deletes a transport from the registered transports.
func Delete(name string) {
kt.Remove(name)
}

// Register registers a transport.
func Register(t types.ImageTransport) {
kt.Add(t)
}

// ImageName converts a types.ImageReference into an URL-like image name, which MUST be such that
Expand Down