From b00001d8ac6bccd75c05e6a6a2041deb8f93bad1 Mon Sep 17 00:00:00 2001 From: Eli Treuherz Date: Mon, 10 Jun 2024 19:56:40 +0100 Subject: [PATCH 1/3] Make multi-type annotation settings match docs The Docker docs in multiple places describe passing an annotation at the command line like "index,manifest:com.example.name=my-cool-image", and say that this will result in the annotation being applied to both the index and the manifest. It doesn't seem like this was actually implemented, and instead it just results in an annotation key with "index,manifest:" at the beginning being applied to the manifest. This change splits the part of the key before the colon by comma, and creates an annotation for each type/platform given, so the implementation should now match the docs. Signed-off-by: Eli Treuherz --- go.mod | 2 +- util/buildflags/export.go | 53 +++++++++------ util/buildflags/export_test.go | 118 +++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+), 20 deletions(-) create mode 100644 util/buildflags/export_test.go diff --git a/go.mod b/go.mod index 7244a24e..14442e0b 100644 --- a/go.mod +++ b/go.mod @@ -21,6 +21,7 @@ require ( github.com/gofrs/flock v0.8.1 github.com/gogo/protobuf v1.3.2 github.com/golang/protobuf v1.5.4 + github.com/google/go-cmp v0.6.0 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/google/uuid v1.6.0 github.com/hashicorp/go-cty-funcs v0.0.0-20230405223818-a090f58aa992 @@ -98,7 +99,6 @@ require ( github.com/go-viper/mapstructure/v2 v2.0.0 // indirect github.com/gogo/googleapis v1.4.1 // indirect github.com/google/gnostic-models v0.6.8 // indirect - github.com/google/go-cmp v0.6.0 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/gorilla/mux v1.8.0 // indirect github.com/gorilla/websocket v1.5.0 // indirect diff --git a/util/buildflags/export.go b/util/buildflags/export.go index fb66d2a6..a9a2ffc6 100644 --- a/util/buildflags/export.go +++ b/util/buildflags/export.go @@ -81,7 +81,8 @@ func ParseExports(inp []string) ([]*controllerapi.ExportEntry, error) { func ParseAnnotations(inp []string) (map[exptypes.AnnotationKey]string, error) { // TODO: use buildkit's annotation parser once it supports setting custom prefix and ":" separator - annotationRegexp := regexp.MustCompile(`^(?:([a-z-]+)(?:\[([A-Za-z0-9_/-]+)\])?:)?(\S+)$`) + annotationRegexp := regexp.MustCompile(`^((?:[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)(?:,[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)*:)?(\S+)$`) + annotationTypeRegexp := regexp.MustCompile(`^([a-z-]+)(?:\[([A-Za-z0-9_/-]+)\])?$`) annotations := make(map[exptypes.AnnotationKey]string) for _, inp := range inp { k, v, ok := strings.Cut(inp, "=") @@ -94,29 +95,43 @@ func ParseAnnotations(inp []string) (map[exptypes.AnnotationKey]string, error) { return nil, errors.Errorf("invalid annotation format, expected :=, got %q", inp) } - typ, platform, key := groups[1], groups[2], groups[3] - switch typ { - case "": - case exptypes.AnnotationIndex, exptypes.AnnotationIndexDescriptor, exptypes.AnnotationManifest, exptypes.AnnotationManifestDescriptor: - default: - return nil, errors.Errorf("unknown annotation type %q", typ) + types, key := groups[1], groups[2] + + if types == "" { + ak := exptypes.AnnotationKey{Key: key} + annotations[ak] = v + continue } - var ociPlatform *ocispecs.Platform - if platform != "" { - p, err := platforms.Parse(platform) - if err != nil { - return nil, errors.Wrapf(err, "invalid platform %q", platform) + typesSplit := strings.Split(strings.TrimSuffix(types, ":"), ",") + for _, typeAndPlatform := range typesSplit { + groups := annotationTypeRegexp.FindStringSubmatch(typeAndPlatform) + typ, platform := groups[1], groups[2] + + switch typ { + case "": + case exptypes.AnnotationIndex, exptypes.AnnotationIndexDescriptor, exptypes.AnnotationManifest, exptypes.AnnotationManifestDescriptor: + default: + return nil, errors.Errorf("unknown annotation type %q", typ) } - ociPlatform = &p + + var ociPlatform *ocispecs.Platform + if platform != "" { + p, err := platforms.Parse(platform) + if err != nil { + return nil, errors.Wrapf(err, "invalid platform %q", platform) + } + ociPlatform = &p + } + + ak := exptypes.AnnotationKey{ + Type: typ, + Platform: ociPlatform, + Key: key, + } + annotations[ak] = v } - ak := exptypes.AnnotationKey{ - Type: typ, - Platform: ociPlatform, - Key: key, - } - annotations[ak] = v } return annotations, nil } diff --git a/util/buildflags/export_test.go b/util/buildflags/export_test.go new file mode 100644 index 00000000..b39ab130 --- /dev/null +++ b/util/buildflags/export_test.go @@ -0,0 +1,118 @@ +package buildflags + +import ( + "cmp" + "slices" + "testing" + + gocmp "github.com/google/go-cmp/cmp" + "github.com/moby/buildkit/exporter/containerimage/exptypes" + ocispecs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseAnnotations(t *testing.T) { + tests := []struct { + name string + in []string + want map[exptypes.AnnotationKey]string + wantErr string + }{ + { + name: "basic", + in: []string{"a=b"}, + want: map[exptypes.AnnotationKey]string{ + {Key: "a"}: "b", + }, + }, + { + name: "reverse-DNS key", + in: []string{"com.example=a"}, + want: map[exptypes.AnnotationKey]string{ + {Key: "com.example"}: "a", + }, + }, + { + name: "specify type", + in: []string{"manifest:com.example=a"}, + want: map[exptypes.AnnotationKey]string{ + {Type: "manifest", Key: "com.example"}: "a", + }, + }, + { + name: "specify bad type", + in: []string{"bad:com.example=a"}, + wantErr: "unknown annotation type", + }, + { + name: "specify type and platform", + in: []string{"manifest[plat/form]:com.example=a"}, + want: map[exptypes.AnnotationKey]string{ + { + Type: "manifest", + Platform: &ocispecs.Platform{ + OS: "plat", + Architecture: "form", + }, + Key: "com.example", + }: "a", + }, + }, + { + name: "specify multiple types", + in: []string{"index,manifest:com.example=a"}, + want: map[exptypes.AnnotationKey]string{ + {Type: "index", Key: "com.example"}: "a", + {Type: "manifest", Key: "com.example"}: "a", + }, + }, + { + name: "specify multiple types and platform", + in: []string{"index,manifest[plat/form]:com.example=a"}, + want: map[exptypes.AnnotationKey]string{ + {Type: "index", Key: "com.example"}: "a", + { + Type: "manifest", + Platform: &ocispecs.Platform{ + OS: "plat", + Architecture: "form", + }, + Key: "com.example", + }: "a", + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + got, gotErr := ParseAnnotations(test.in) + if test.wantErr != "" { + require.ErrorContains(t, gotErr, test.wantErr) + } else { + assert.NoError(t, gotErr) + } + + // Can't compare maps with pointer in their keys, need to extract and sort the map entries + type kv struct { + Key exptypes.AnnotationKey + Val string + } + var wantKVs, gotKVs []kv + for k, v := range test.want { + wantKVs = append(wantKVs, kv{k, v}) + } + for k, v := range got { + gotKVs = append(gotKVs, kv{k, v}) + } + + sortFunc := func(a, b kv) int { return cmp.Compare(a.Key.String(), b.Key.String()) } + slices.SortFunc(wantKVs, sortFunc) + slices.SortFunc(gotKVs, sortFunc) + + if diff := gocmp.Diff(wantKVs, gotKVs); diff != "" { + t.Error(diff) + } + }) + } +} From bcd04d5a6417d5cf13e03c93f59d577c92f81793 Mon Sep 17 00:00:00 2001 From: Eli Treuherz Date: Tue, 18 Jun 2024 12:05:49 +0100 Subject: [PATCH 2/3] Style fixes to test Signed-off-by: Eli Treuherz --- go.mod | 2 +- util/buildflags/export_test.go | 45 +++++++++++++++++----------------- 2 files changed, 24 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index 14442e0b..7244a24e 100644 --- a/go.mod +++ b/go.mod @@ -21,7 +21,6 @@ require ( github.com/gofrs/flock v0.8.1 github.com/gogo/protobuf v1.3.2 github.com/golang/protobuf v1.5.4 - github.com/google/go-cmp v0.6.0 github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/google/uuid v1.6.0 github.com/hashicorp/go-cty-funcs v0.0.0-20230405223818-a090f58aa992 @@ -99,6 +98,7 @@ require ( github.com/go-viper/mapstructure/v2 v2.0.0 // indirect github.com/gogo/googleapis v1.4.1 // indirect github.com/google/gnostic-models v0.6.8 // indirect + github.com/google/go-cmp v0.6.0 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/gorilla/mux v1.8.0 // indirect github.com/gorilla/websocket v1.5.0 // indirect diff --git a/util/buildflags/export_test.go b/util/buildflags/export_test.go index b39ab130..15aa7d25 100644 --- a/util/buildflags/export_test.go +++ b/util/buildflags/export_test.go @@ -5,7 +5,6 @@ import ( "slices" "testing" - gocmp "github.com/google/go-cmp/cmp" "github.com/moby/buildkit/exporter/containerimage/exptypes" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" @@ -86,33 +85,35 @@ func TestParseAnnotations(t *testing.T) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - got, gotErr := ParseAnnotations(test.in) + got, err := ParseAnnotations(test.in) if test.wantErr != "" { - require.ErrorContains(t, gotErr, test.wantErr) + require.ErrorContains(t, err, test.wantErr) } else { - assert.NoError(t, gotErr) + require.NoError(t, err) } // Can't compare maps with pointer in their keys, need to extract and sort the map entries - type kv struct { - Key exptypes.AnnotationKey - Val string - } - var wantKVs, gotKVs []kv - for k, v := range test.want { - wantKVs = append(wantKVs, kv{k, v}) - } - for k, v := range got { - gotKVs = append(gotKVs, kv{k, v}) - } + wantKVs := entries(test.want) + gotKVs := entries(got) - sortFunc := func(a, b kv) int { return cmp.Compare(a.Key.String(), b.Key.String()) } - slices.SortFunc(wantKVs, sortFunc) - slices.SortFunc(gotKVs, sortFunc) - - if diff := gocmp.Diff(wantKVs, gotKVs); diff != "" { - t.Error(diff) - } + assert.Equal(t, wantKVs, gotKVs) }) } } + +type kv struct { + Key exptypes.AnnotationKey + Val string +} + +func entries(in map[exptypes.AnnotationKey]string) []kv { + var out []kv + for k, v := range in { + out = append(out, kv{k, v}) + } + + sortFunc := func(a, b kv) int { return cmp.Compare(a.Key.String(), b.Key.String()) } + slices.SortFunc(out, sortFunc) + + return out +} From 3d0951b80069b76b8dc0871f220888b51d7650a4 Mon Sep 17 00:00:00 2001 From: Eli Treuherz Date: Tue, 18 Jun 2024 12:29:14 +0100 Subject: [PATCH 3/3] Reduce regex usage in annotation parser Signed-off-by: Eli Treuherz --- util/buildflags/export.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/util/buildflags/export.go b/util/buildflags/export.go index a9a2ffc6..8f484c01 100644 --- a/util/buildflags/export.go +++ b/util/buildflags/export.go @@ -81,8 +81,10 @@ func ParseExports(inp []string) ([]*controllerapi.ExportEntry, error) { func ParseAnnotations(inp []string) (map[exptypes.AnnotationKey]string, error) { // TODO: use buildkit's annotation parser once it supports setting custom prefix and ":" separator - annotationRegexp := regexp.MustCompile(`^((?:[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)(?:,[a-z-]+(?:\[[A-Za-z0-9_/-]+\])?)*:)?(\S+)$`) + + // type followed by optional platform specifier in square brackets annotationTypeRegexp := regexp.MustCompile(`^([a-z-]+)(?:\[([A-Za-z0-9_/-]+)\])?$`) + annotations := make(map[exptypes.AnnotationKey]string) for _, inp := range inp { k, v, ok := strings.Cut(inp, "=") @@ -90,27 +92,33 @@ func ParseAnnotations(inp []string) (map[exptypes.AnnotationKey]string, error) { return nil, errors.Errorf("invalid annotation %q, expected key=value", inp) } - groups := annotationRegexp.FindStringSubmatch(k) - if groups == nil { - return nil, errors.Errorf("invalid annotation format, expected :=, got %q", inp) - } + types, key, ok := strings.Cut(k, ":") + if !ok { + // no types specified, swap Cut outputs + key = types - types, key := groups[1], groups[2] - - if types == "" { ak := exptypes.AnnotationKey{Key: key} annotations[ak] = v continue } - typesSplit := strings.Split(strings.TrimSuffix(types, ":"), ",") + typesSplit := strings.Split(types, ",") for _, typeAndPlatform := range typesSplit { groups := annotationTypeRegexp.FindStringSubmatch(typeAndPlatform) + if groups == nil { + return nil, errors.Errorf( + "invalid annotation type %q, expected type and optional platform in square brackets", + typeAndPlatform) + } + typ, platform := groups[1], groups[2] switch typ { case "": - case exptypes.AnnotationIndex, exptypes.AnnotationIndexDescriptor, exptypes.AnnotationManifest, exptypes.AnnotationManifestDescriptor: + case exptypes.AnnotationIndex, + exptypes.AnnotationIndexDescriptor, + exptypes.AnnotationManifest, + exptypes.AnnotationManifestDescriptor: default: return nil, errors.Errorf("unknown annotation type %q", typ) }