cache: error on duplicate sets of cache options

Previously, we attempted to gracefully resolve duplicate cache export
options, however, we should explicitly error - a client has made an
error if the same options appear twice. If we really want
de-duplication, then clients such as buildx should implement it there.

Signed-off-by: Justin Chadwell <me@jedevc.com>

control/control.go

@@ -2,11 +2,13 @@ package control
 
 import (
 	"context"
+	"fmt"
 	"sync"
 	"sync/atomic"
 	"time"
 
 	"github.com/docker/distribution/reference"
+	"github.com/mitchellh/hashstructure/v2"
 	controlapi "github.com/moby/buildkit/api/services/control"
 	apitypes "github.com/moby/buildkit/api/types"
 	"github.com/moby/buildkit/cache/remotecache"
@@ -21,7 +23,6 @@ import (
 	"github.com/moby/buildkit/solver"
 	"github.com/moby/buildkit/solver/llbsolver"
 	"github.com/moby/buildkit/solver/llbsolver/proc"
-	solverutil "github.com/moby/buildkit/solver/llbsolver/util"
 	"github.com/moby/buildkit/solver/pb"
 	"github.com/moby/buildkit/util/bklog"
 	"github.com/moby/buildkit/util/imageutil"
@@ -293,14 +294,17 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (*
 		}
 	}
 
-	var cacheImports []frontend.CacheOptionsEntry
-
-	var cacheExporters []llbsolver.RemoteCacheExporter
-	exportCacheOptEntries, err := solverutil.DedupCacheOptions(req.Cache.Exports)
-	if err != nil {
+	if c, err := findDuplicateCacheOptions(req.Cache.Exports); err != nil {
 		return nil, err
+	} else if c != nil {
+		types := []string{}
+		for _, c := range c {
+			types = append(types, c.Type)
+		}
+		return nil, errors.Errorf("duplicate cache exports %s", types)
 	}
-	for _, e := range exportCacheOptEntries {
+	var cacheExporters []llbsolver.RemoteCacheExporter
+	for _, e := range req.Cache.Exports {
 		cacheExporterFunc, ok := c.opt.ResolveCacheExporterFuncs[e.Type]
 		if !ok {
 			return nil, errors.Errorf("unknown cache exporter: %q", e.Type)
@@ -317,6 +321,8 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (*
 		}
 		cacheExporters = append(cacheExporters, exp)
 	}
+
+	var cacheImports []frontend.CacheOptionsEntry
 	for _, im := range req.Cache.Imports {
 		cacheImports = append(cacheImports, frontend.CacheOptionsEntry{
 			Type:  im.Type,
@@ -562,3 +568,42 @@ func toPBBuildkitVersion(in client.BuildkitVersion) *apitypes.BuildkitVersion {
 		Revision: in.Revision,
 	}
 }
+
+func findDuplicateCacheOptions(cacheOpts []*controlapi.CacheOptionsEntry) ([]*controlapi.CacheOptionsEntry, error) {
+	seen := map[string]*controlapi.CacheOptionsEntry{}
+	duplicate := map[string]struct{}{}
+	for _, opt := range cacheOpts {
+		k, err := cacheOptKey(*opt)
+		if err != nil {
+			return nil, err
+		}
+		if _, ok := seen[k]; ok {
+			duplicate[k] = struct{}{}
+		}
+		seen[k] = opt
+	}
+
+	var duplicates []*controlapi.CacheOptionsEntry
+	for k := range duplicate {
+		duplicates = append(duplicates, seen[k])
+	}
+	return duplicates, nil
+}
+
+func cacheOptKey(opt controlapi.CacheOptionsEntry) (string, error) {
+	if opt.Type == "registry" && opt.Attrs["ref"] != "" {
+		return opt.Attrs["ref"], nil
+	}
+	var rawOpt = struct {
+		Type  string
+		Attrs map[string]string
+	}{
+		Type:  opt.Type,
+		Attrs: opt.Attrs,
+	}
+	hash, err := hashstructure.Hash(rawOpt, hashstructure.FormatV2, nil)
+	if err != nil {
+		return "", err
+	}
+	return fmt.Sprint(opt.Type, ":", hash), nil
+}

solver/llbsolver/util/util_test.go → control/control_test.go

@@ -1,4 +1,4 @@
-package util
+package control
 
 import (
 	"testing"
@@ -7,21 +7,15 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestDedupCacheOptions(t *testing.T) {
+func TestDuplicateCacheOptions(t *testing.T) {
 	var testCases = []struct {
 		name     string
 		opts     []*controlapi.CacheOptionsEntry
-		expected []*controlapi.CacheOptionsEntry
+		expected []uint
 	}{
 		{
-			name: "deduplicates opts",
+			name: "avoids unique opts",
 			opts: []*controlapi.CacheOptionsEntry{
-				{
-					Type: "registry",
-					Attrs: map[string]string{
-						"ref": "example.com/ref:v1.0.0",
-					},
-				},
 				{
 					Type: "registry",
 					Attrs: map[string]string{
@@ -34,30 +28,18 @@ func TestDedupCacheOptions(t *testing.T) {
 						"dest": "/path/for/export",
 					},
 				},
+			},
+			expected: nil,
+		},
+		{
+			name: "finds duplicate opts",
+			opts: []*controlapi.CacheOptionsEntry{
 				{
-					Type: "local",
-					Attrs: map[string]string{
-						"dest": "/path/for/export",
-					},
-				},
-				{
-					Type: "azblob",
-					Attrs: map[string]string{
-						"account-url":  "url",
-						"blobs_prefix": "prefix",
-						"name":         "name",
-					},
-				},
-				{
-					Type: "azblob",
+					Type: "registry",
 					Attrs: map[string]string{
-						"account-url":  "url",
-						"blobs_prefix": "prefix",
-						"name":         "name",
+						"ref": "example.com/ref:v1.0.0",
 					},
 				},
-			},
-			expected: []*controlapi.CacheOptionsEntry{
 				{
 					Type: "registry",
 					Attrs: map[string]string{
@@ -71,22 +53,23 @@ func TestDedupCacheOptions(t *testing.T) {
 					},
 				},
 				{
-					Type: "azblob",
+					Type: "local",
 					Attrs: map[string]string{
-						"account-url":  "url",
-						"blobs_prefix": "prefix",
-						"name":         "name",
+						"dest": "/path/for/export",
 					},
 				},
 			},
+			expected: []uint{0, 2},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			opts, err := DedupCacheOptions(tc.opts)
+			p, err := findDuplicateCacheOptions(tc.opts)
 			require.NoError(t, err)
-			require.ElementsMatch(t, opts, tc.expected)
+			for i, j := range tc.expected {
+				require.Equal(t, p[i], tc.opts[j])
+			}
 		})
 	}
 }