Skip to content

Commit

Permalink
Validate canonical image names (#866)
Browse files Browse the repository at this point in the history 
This validates that `imageName` and `cacheFrom.images` are canonical
during `Check`.

Previously, the user would only find out if these were invalid when
attempting to run an update.

This also makes the error message slightly more helpful by including the
problematic image name.
  • Loading branch information
@blampe
blampe committed Dec 1, 2023
1 parent c3702a6 commit e12a3ea
Show file tree
Hide file tree
Showing 5 changed files with 176 additions and 14 deletions.
4 changes: 2 additions & 2 deletions provider/image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -795,9 +795,9 @@ func getRegistryAddrForAuth(serverName, imgName string) (string, error) {
func getRegistryAddrFromImage(imgName string) (string, error) {
named, err := reference.ParseNamed(imgName)
if err != nil {
msg := fmt.Errorf("error: %s. This provider requires all image names to be fully qualified.\n"+
msg := fmt.Errorf("%q: %w.\nThis resource requires all image names to be fully qualified.\n"+
"For example, if you are attempting to push to Dockerhub, prefix your image name with `docker.io`:\n\n"+
"`docker.io/repository/image:tag`", err)
"`docker.io/repository/image:tag`", imgName, err)
return "", msg
}
addr := reference.Domain(named)
Expand Down
13 changes: 6 additions & 7 deletions provider/image_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,16 +404,15 @@ func TestGetRegistryAddrFromImage(t *testing.T) {
expected := ""
input := "pulumi-test-registry/unicorns/swiftwind:latest"

expectedError := fmt.Errorf(
"error: repository name must be canonical. This provider requires " +
"all image names to be fully qualified.\nFor example, if you are " +
"attempting to push to Dockerhub, prefix your image name with " +
"`docker.io`:\n\n`docker.io/repository/image:tag`",
)
expectedError := "\"pulumi-test-registry/unicorns/swiftwind:latest\": repository name must be canonical.\n" +
"This resource requires all image names to be fully qualified.\n" +
"For example, if you are attempting to push to Dockerhub, prefix your image name with `docker.io`:\n\n" +
"`docker.io/repository/image:tag`"

actual, err := getRegistryAddrFromImage(input)
assert.Equal(t, expected, actual)
assert.Error(t, err)
assert.Equal(t, expectedError, err)
assert.ErrorContains(t, err, expectedError)
})
}

Expand Down
35 changes: 31 additions & 4 deletions provider/provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/moby/buildkit/frontend/dockerfile/dockerignore"
"github.com/pkg/errors"
"github.com/pulumi/pulumi/pkg/v3/resource/provider"
"github.com/pulumi/pulumi/sdk/v3/go/common/diag"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin"
"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
Expand All @@ -38,7 +39,6 @@ type dockerNativeProvider struct {
version string
schemaBytes []byte
config map[string]string
//loginLock sync.Mutex
}

// docker native methods
Expand Down Expand Up @@ -116,6 +116,16 @@ func (p *dockerNativeProvider) StreamInvoke(
return fmt.Errorf("unknown StreamInvoke token '%s'", tok)
}

// log emits a log to our host, or no-ops if a host has not been configured (as
// in testing).
func (p *dockerNativeProvider) log(ctx context.Context, sev diag.Severity, urn resource.URN, msg string) error {
// no-op if we're in a test.
if p.host == nil {
return nil
}
return p.host.Log(ctx, sev, urn, msg)
}

// Check validates that the given property bag is valid for a resource of the given type and returns
// the inputs that should be passed to successive calls to Diff, Create, or Update for this
// resource. As a rule, the provider inputs returned by a call to Check should preserve the original
Expand Down Expand Up @@ -192,24 +202,41 @@ func (p *dockerNativeProvider) Check(ctx context.Context, req *rpc.CheckRequest)
"contextDigest": resource.NewStringProperty(contextDigest),
"platform": resource.NewStringProperty(hostPlatform),
})
err = p.host.Log(ctx, "info", urn, msg)
err = p.log(ctx, "info", urn, msg)
if err != nil {
return nil, err
}
} else {
inputs["build"].ObjectValue()["contextDigest"] = resource.NewStringProperty(contextDigest)
if inputs["build"].ObjectValue()["platform"].IsNull() {
inputs["build"].ObjectValue()["platform"] = resource.NewStringProperty(hostPlatform)
err = p.host.Log(ctx, "info", urn, msg)
err = p.log(ctx, "info", urn, msg)
if err != nil {
return nil, err
}
}

}
if _, err = marshalCachedImages(inputs["build"]); err != nil {

// Make sure image names are fully qualified.
cache, err := marshalCachedImages(inputs["build"])
if err != nil {
return nil, err
}
// imageName only needs to be canonical if we're pushing or using cacheFrom.
needCanonicalImage := len(cache) > 0 || !marshalSkipPush(inputs["skipPush"])
if needCanonicalImage && !inputs["imageName"].IsNull() {
registry := marshalRegistry(inputs["registry"])
host, err := getRegistryAddrForAuth(registry.Server, inputs["imageName"].StringValue())
if err != nil {
return nil, err
}
for _, i := range cache {
if _, err := getRegistryAddrForAuth(host, i); err != nil {
return nil, err
}
}
}

inputStruct, err := plugin.MarshalProperties(inputs, plugin.MarshalOptions{
KeepUnknowns: true,
Expand Down
138 changes: 137 additions & 1 deletion provider/provider_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package provider

import (
"context"
"os"
"path/filepath"
"testing"

"github.com/docker/distribution/reference"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource"
"github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin"
rpc "github.com/pulumi/pulumi/sdk/v3/proto/go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -232,7 +235,6 @@ func TestHashDeepSymlinks(t *testing.T) {
dir := "./testdata/symlinks"
_, err := hashContext(dir, filepath.Join(dir, "Dockerfile"))
assert.NoError(t, err)

}

func TestHashUnignoredDirs(t *testing.T) {
Expand Down Expand Up @@ -303,3 +305,137 @@ func TestSetConfiguration(t *testing.T) {
assert.Equal(t, expected, actual)
})
}

func TestCheck(t *testing.T) {
tests := []struct {
name string
news resource.PropertyMap

wantErr error
}{
{
name: "can't push a non-canonical image name",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("not-fully-qualified-image-name:latest"),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
},
),
},
wantErr: reference.ErrNameNotCanonical,
},
{
name: "image name can be non-canonical if not pushing",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("not-pushing:latest"),
"skipPush": resource.NewBoolProperty(true),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
},
),
},
wantErr: nil,
},
{
name: "image name can be non-canonical if registry server is provided",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("foo/bar:latest"),
"skipPush": resource.NewBoolProperty(true),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
},
),
"registry": resource.NewObjectProperty(
resource.PropertyMap{
"server": resource.NewStringProperty("docker.io"),
},
),
},
wantErr: nil,
},
{
name: "image name must be canonical if using caching, even when not pushing",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("not-pushing:latest"),
"skipPush": resource.NewBoolProperty(true),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
"cacheFrom": resource.NewObjectProperty(
resource.PropertyMap{
"images": resource.NewArrayProperty(
[]resource.PropertyValue{resource.NewStringProperty("docker.io/pulumi/pulumi:latest")},
),
},
),
},
),
},
wantErr: reference.ErrNameNotCanonical,
},
{
name: "cacheFrom can infer host from imageName",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("docker.io/foo/bar:latest"),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
"cacheFrom": resource.NewObjectProperty(
resource.PropertyMap{
"images": resource.NewArrayProperty(
[]resource.PropertyValue{resource.NewStringProperty("foo/bar:latest")},
),
},
),
},
),
},
wantErr: nil,
},
{
name: "can use non-canonical cacheFrom with a registry server",
news: resource.PropertyMap{
"imageName": resource.NewStringProperty("foo/bar:latest"),
"build": resource.NewObjectProperty(
resource.PropertyMap{
"dockerfile": resource.NewStringProperty("testdata/Dockerfile"),
"cacheFrom": resource.NewObjectProperty(
resource.PropertyMap{
"images": resource.NewArrayProperty(
[]resource.PropertyValue{resource.NewStringProperty("not-fully-qualified-cache:latest")},
),
},
),
},
),
"registry": resource.NewObjectProperty(
resource.PropertyMap{
"server": resource.NewStringProperty("docker.io"),
},
),
},
wantErr: nil,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
p := dockerNativeProvider{}

news, err := plugin.MarshalProperties(tt.news, plugin.MarshalOptions{})
require.NoError(t, err)

req := &rpc.CheckRequest{
Urn: string("urn:pulumi:test::docker-provider::docker:index/image:Image::foo"),
News: news,
}

_, err = p.Check(context.Background(), req)

assert.ErrorIs(t, err, tt.wantErr)
})
}
}
Empty file added provider/testdata/Dockerfile
View file
Empty file.

0 comments on commit e12a3ea

Please sign in to comment.