From a74ee1463ef8d83e4cfd72a196e6ac60599e6fd1 Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Tue, 25 Jul 2023 12:06:37 -0400 Subject: [PATCH 1/9] Initial support for OCI labels on image/bundle push Signed-off-by: Joe Searcy --- pkg/imgpkg/bundle/contents.go | 4 ++-- pkg/imgpkg/bundle/contents_test.go | 4 ++-- pkg/imgpkg/cmd/push.go | 6 ++++-- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/imgpkg/bundle/contents.go b/pkg/imgpkg/bundle/contents.go index 42df385fc..690b8e0b4 100644 --- a/pkg/imgpkg/bundle/contents.go +++ b/pkg/imgpkg/bundle/contents.go @@ -43,13 +43,13 @@ func NewContents(paths []string, excludedPaths []string, preservePermissions boo } // Push the contents of the bundle to the registry as an OCI Image -func (b Contents) Push(uploadRef regname.Tag, registry ImagesMetadataWriter, logger Logger) (string, error) { +func (b Contents) Push(uploadRef regname.Tag, labels map[string]string, registry ImagesMetadataWriter, logger Logger) (string, error) { err := b.validate() if err != nil { return "", err } - labels := map[string]string{BundleConfigLabel: "true"} + labels[BundleConfigLabel] = "true" return plainimage.NewContents(b.paths, b.excludedPaths, b.preservePermissions).Push(uploadRef, labels, registry, logger) } diff --git a/pkg/imgpkg/bundle/contents_test.go b/pkg/imgpkg/bundle/contents_test.go index 3a5577c3a..b88e4216f 100644 --- a/pkg/imgpkg/bundle/contents_test.go +++ b/pkg/imgpkg/bundle/contents_test.go @@ -44,7 +44,7 @@ images: t.Fatalf("failed to read tag: %s", err) } - _, err = subject.Push(imgTag, fakeRegistry, util.NewNoopLevelLogger()) + _, err = subject.Push(imgTag, make(map[string]string), fakeRegistry, util.NewNoopLevelLogger()) if err != nil { t.Fatalf("not expecting push to fail: %s", err) } @@ -78,7 +78,7 @@ images: t.Fatalf("failed to read tag: %s", err) } - _, err = subject.Push(imgTag, fakeRegistry, util.NewNoopLevelLogger()) + _, err = subject.Push(imgTag, map[string]string{}, fakeRegistry, util.NewNoopLevelLogger()) if err != nil { t.Fatalf("not expecting push to fail: %s", err) } diff --git a/pkg/imgpkg/cmd/push.go b/pkg/imgpkg/cmd/push.go index 96b381025..12bb6538e 100644 --- a/pkg/imgpkg/cmd/push.go +++ b/pkg/imgpkg/cmd/push.go @@ -24,6 +24,7 @@ type PushOptions struct { LockOutputFlags LockOutputFlags FileFlags FileFlags RegistryFlags RegistryFlags + Labels map[string]string } func NewPushOptions(ui ui.UI) *PushOptions { @@ -47,6 +48,7 @@ func NewPushCmd(o *PushOptions) *cobra.Command { o.LockOutputFlags.SetOnPush(cmd) o.FileFlags.Set(cmd) o.RegistryFlags.Set(cmd) + cmd.Flags().StringToStringVarP(&o.Labels, "labels", "l", map[string]string{}, "Set labels on image") return cmd } @@ -96,7 +98,7 @@ func (po *PushOptions) pushBundle(registry registry.Registry) (string, error) { } logger := util.NewUILevelLogger(util.LogWarn, util.NewLogger(po.ui)) - imageURL, err := bundle.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, registry, logger) + imageURL, err := bundle.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.Labels, registry, logger) if err != nil { return "", err } @@ -141,5 +143,5 @@ func (po *PushOptions) pushImage(registry registry.Registry) (string, error) { } logger := util.NewUILevelLogger(util.LogWarn, util.NewLogger(po.ui)) - return plainimage.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, nil, registry, logger) + return plainimage.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.Labels, registry, logger) } From 63e8348e9cf371b92a84fef29583cfdd750ae58c Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Tue, 25 Jul 2023 16:52:20 -0400 Subject: [PATCH 2/9] Add validation for reserved label Signed-off-by: Joe Searcy --- pkg/imgpkg/cmd/push.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/pkg/imgpkg/cmd/push.go b/pkg/imgpkg/cmd/push.go index 12bb6538e..ac7798125 100644 --- a/pkg/imgpkg/cmd/push.go +++ b/pkg/imgpkg/cmd/push.go @@ -58,6 +58,11 @@ func (po *PushOptions) Run() error { return err } + err = po.validateFlags() + if err != nil { + return err + } + var imageURL string isBundle := po.BundleFlags.Bundle != "" @@ -145,3 +150,17 @@ func (po *PushOptions) pushImage(registry registry.Registry) (string, error) { logger := util.NewUILevelLogger(util.LogWarn, util.NewLogger(po.ui)) return plainimage.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.Labels, registry, logger) } + +// validateFlags checks if the provided flags are valid +func (po *PushOptions) validateFlags() error { + + // Verify the user did NOT specify a reserved OCI label + _, present := po.Labels[bundle.BundleConfigLabel] + + if present { + return fmt.Errorf("label '%s' is reserved and cannot be overriden. Please use a different key", bundle.BundleConfigLabel) + } + + return nil + +} From f924bcb86a1f49f12dbf08f56fd207128dd72f78 Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Sun, 30 Jul 2023 00:46:13 -0600 Subject: [PATCH 3/9] Fix potential nil assignment for default label Signed-off-by: Joe Searcy --- pkg/imgpkg/bundle/contents.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/imgpkg/bundle/contents.go b/pkg/imgpkg/bundle/contents.go index 690b8e0b4..839b2fce5 100644 --- a/pkg/imgpkg/bundle/contents.go +++ b/pkg/imgpkg/bundle/contents.go @@ -49,6 +49,10 @@ func (b Contents) Push(uploadRef regname.Tag, labels map[string]string, registry return "", err } + if labels == nil { + labels = map[string]string{} + } + labels[BundleConfigLabel] = "true" return plainimage.NewContents(b.paths, b.excludedPaths, b.preservePermissions).Push(uploadRef, labels, registry, logger) } From debaeb2fa6b104ecfef3433330f7779ddffdf7ff Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Tue, 1 Aug 2023 17:32:31 -0400 Subject: [PATCH 4/9] Model label flags after other push option flags Signed-off-by: Joe Searcy --- pkg/imgpkg/bundle/contents.go | 5 +---- pkg/imgpkg/cmd/label_flags.go | 16 ++++++++++++++++ pkg/imgpkg/cmd/push.go | 15 ++++++++++----- 3 files changed, 27 insertions(+), 9 deletions(-) create mode 100644 pkg/imgpkg/cmd/label_flags.go diff --git a/pkg/imgpkg/bundle/contents.go b/pkg/imgpkg/bundle/contents.go index 839b2fce5..d949548be 100644 --- a/pkg/imgpkg/bundle/contents.go +++ b/pkg/imgpkg/bundle/contents.go @@ -49,11 +49,8 @@ func (b Contents) Push(uploadRef regname.Tag, labels map[string]string, registry return "", err } - if labels == nil { - labels = map[string]string{} - } - labels[BundleConfigLabel] = "true" + return plainimage.NewContents(b.paths, b.excludedPaths, b.preservePermissions).Push(uploadRef, labels, registry, logger) } diff --git a/pkg/imgpkg/cmd/label_flags.go b/pkg/imgpkg/cmd/label_flags.go new file mode 100644 index 000000000..fe39dd441 --- /dev/null +++ b/pkg/imgpkg/cmd/label_flags.go @@ -0,0 +1,16 @@ +// Copyright 2020 VMware, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package cmd + +import ( + "github.com/spf13/cobra" +) + +type LabelFlags struct { + Labels map[string]string +} + +func (l *LabelFlags) Set(cmd *cobra.Command) { + cmd.Flags().StringToStringVarP(&l.Labels, "labels", "l", make(map[string]string), "Set labels on image") +} diff --git a/pkg/imgpkg/cmd/push.go b/pkg/imgpkg/cmd/push.go index ac7798125..e1cc4eed0 100644 --- a/pkg/imgpkg/cmd/push.go +++ b/pkg/imgpkg/cmd/push.go @@ -24,7 +24,7 @@ type PushOptions struct { LockOutputFlags LockOutputFlags FileFlags FileFlags RegistryFlags RegistryFlags - Labels map[string]string + LabelFlags LabelFlags } func NewPushOptions(ui ui.UI) *PushOptions { @@ -48,7 +48,8 @@ func NewPushCmd(o *PushOptions) *cobra.Command { o.LockOutputFlags.SetOnPush(cmd) o.FileFlags.Set(cmd) o.RegistryFlags.Set(cmd) - cmd.Flags().StringToStringVarP(&o.Labels, "labels", "l", map[string]string{}, "Set labels on image") + o.LabelFlags.Set(cmd) + return cmd } @@ -68,6 +69,10 @@ func (po *PushOptions) Run() error { isBundle := po.BundleFlags.Bundle != "" isImage := po.ImageFlags.Image != "" + if po.LabelFlags.Labels == nil { + po.LabelFlags.Labels = map[string]string{} + } + switch { case isBundle && isImage: return fmt.Errorf("Expected only one of image or bundle") @@ -103,7 +108,7 @@ func (po *PushOptions) pushBundle(registry registry.Registry) (string, error) { } logger := util.NewUILevelLogger(util.LogWarn, util.NewLogger(po.ui)) - imageURL, err := bundle.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.Labels, registry, logger) + imageURL, err := bundle.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.LabelFlags.Labels, registry, logger) if err != nil { return "", err } @@ -148,14 +153,14 @@ func (po *PushOptions) pushImage(registry registry.Registry) (string, error) { } logger := util.NewUILevelLogger(util.LogWarn, util.NewLogger(po.ui)) - return plainimage.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.Labels, registry, logger) + return plainimage.NewContents(po.FileFlags.Files, po.FileFlags.ExcludedFilePaths, po.FileFlags.PreservePermissions).Push(uploadRef, po.LabelFlags.Labels, registry, logger) } // validateFlags checks if the provided flags are valid func (po *PushOptions) validateFlags() error { // Verify the user did NOT specify a reserved OCI label - _, present := po.Labels[bundle.BundleConfigLabel] + _, present := po.LabelFlags.Labels[bundle.BundleConfigLabel] if present { return fmt.Errorf("label '%s' is reserved and cannot be overriden. Please use a different key", bundle.BundleConfigLabel) From a6db6ee2ab2f394a760c1ca5259b8a6500381255 Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Tue, 1 Aug 2023 17:33:53 -0400 Subject: [PATCH 5/9] Add testing for image/bundle labels Signed-off-by: Joe Searcy --- pkg/imgpkg/cmd/push_test.go | 84 +++++++++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/pkg/imgpkg/cmd/push_test.go b/pkg/imgpkg/cmd/push_test.go index bf0a571de..b8a265172 100644 --- a/pkg/imgpkg/cmd/push_test.go +++ b/pkg/imgpkg/cmd/push_test.go @@ -11,8 +11,12 @@ import ( "strings" "testing" + "github.com/google/go-containerregistry/pkg/authn" + "github.com/google/go-containerregistry/pkg/name" + "github.com/google/go-containerregistry/pkg/v1/remote" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/vmware-tanzu/carvel-imgpkg/test/helpers" ) const emptyImagesYaml = `apiVersion: imgpkg.carvel.dev/v1alpha1 @@ -235,6 +239,86 @@ func TestImageAndBundleLockError(t *testing.T) { } } +func TestLabels(t *testing.T) { + testCases := []struct { + name string + opType string + expectedError string + expectedLabels map[string]string + labelInput string + }{ + { + name: "bundle with multiple labels", + opType: "bundle", + expectedError: "", + labelInput: "foo=bar,bar=baz", + expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "foo": "bar", "bar": "baz"}, + }, + { + name: "image with multiple labels", + opType: "image", + expectedError: "", + labelInput: "foo=bar,bar=baz", + expectedLabels: map[string]string{"foo": "bar", "bar": "baz"}, + }, + { + name: "bundle with \".\" in label key", + opType: "bundle", + expectedError: "", + labelInput: "foo.bar=baz", + expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "foo.bar": "baz"}, + }, + { + name: "bundle with long label key (> 64 chars)", + opType: "bundle", + expectedError: "", + labelInput: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa=baz", + expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa": "baz"}, + }, + { + name: "bundle with long label value (> 256 chars)", + opType: "bundle", + expectedError: "", + labelInput: "foo=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "foo": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, + }, + } + + for _, tc := range testCases { + f := func(t *testing.T) { + env := helpers.BuildEnv(t) + imgpkg := helpers.Imgpkg{T: t, ImgpkgPath: env.ImgpkgPath} + defer env.Cleanup() + + opTypeFlag := "-b" + pushDir := env.BundleFactory.CreateBundleDir(helpers.BundleYAML, helpers.ImagesYAML) + + if tc.opType == "image" { + opTypeFlag = "-i" + pushDir = env.Assets.CreateAndCopySimpleApp("image-to-push") + } + + if tc.labelInput == "" { + imgpkg.Run([]string{"push", opTypeFlag, env.Image, "-f", pushDir}) + } else { + imgpkg.Run([]string{"push", opTypeFlag, env.Image, "-l", tc.labelInput, "-f", pushDir}) + } + + ref, _ := name.NewTag(env.Image, name.WeakValidation) + image, err := remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain)) + require.NoError(t, err) + + config, err := image.ConfigFile() + require.NoError(t, err) + + require.Equal(t, tc.expectedLabels, config.Config.Labels, "Expected labels provided via flags to match labels discovered on image") + + } + + t.Run(tc.name, f) + } +} + func Cleanup(dirs ...string) { for _, dir := range dirs { os.RemoveAll(dir) From 17a42cb350274554d97421773ae056e27cf3a01c Mon Sep 17 00:00:00 2001 From: Joe Searcy <40017920+phenixblue@users.noreply.github.com> Date: Wed, 2 Aug 2023 13:31:56 -0400 Subject: [PATCH 6/9] Update pkg/imgpkg/bundle/contents_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: João Pereira Signed-off-by: Joe Searcy --- pkg/imgpkg/bundle/contents_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/imgpkg/bundle/contents_test.go b/pkg/imgpkg/bundle/contents_test.go index b88e4216f..8e31d2ab0 100644 --- a/pkg/imgpkg/bundle/contents_test.go +++ b/pkg/imgpkg/bundle/contents_test.go @@ -44,7 +44,7 @@ images: t.Fatalf("failed to read tag: %s", err) } - _, err = subject.Push(imgTag, make(map[string]string), fakeRegistry, util.NewNoopLevelLogger()) + _, err = subject.Push(imgTag, map[string]string{}, fakeRegistry, util.NewNoopLevelLogger()) if err != nil { t.Fatalf("not expecting push to fail: %s", err) } From 5011eec9648210140d91eabf5ca7ebb61a977720 Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Wed, 2 Aug 2023 14:33:35 -0400 Subject: [PATCH 7/9] Add comments and address some PR feedback Signed-off-by: Joe Searcy --- pkg/imgpkg/cmd/label_flags.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/imgpkg/cmd/label_flags.go b/pkg/imgpkg/cmd/label_flags.go index fe39dd441..d85b08390 100644 --- a/pkg/imgpkg/cmd/label_flags.go +++ b/pkg/imgpkg/cmd/label_flags.go @@ -7,10 +7,12 @@ import ( "github.com/spf13/cobra" ) +// LabelFlags is a struct that holds the labels for an OCI artifact type LabelFlags struct { Labels map[string]string } +// Set sets the labels for an OCI artifact func (l *LabelFlags) Set(cmd *cobra.Command) { - cmd.Flags().StringToStringVarP(&l.Labels, "labels", "l", make(map[string]string), "Set labels on image") + cmd.Flags().StringToStringVarP(&l.Labels, "labels", "l", map[string]string{}, "Set labels on image") } From f5d15ff0d240ccb69e06b5a178fee875521ae46b Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Wed, 2 Aug 2023 14:34:19 -0400 Subject: [PATCH 8/9] Add test for label value with spaces Signed-off-by: Joe Searcy --- pkg/imgpkg/cmd/push_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/imgpkg/cmd/push_test.go b/pkg/imgpkg/cmd/push_test.go index b8a265172..df743b8c6 100644 --- a/pkg/imgpkg/cmd/push_test.go +++ b/pkg/imgpkg/cmd/push_test.go @@ -282,6 +282,13 @@ func TestLabels(t *testing.T) { labelInput: "foo=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "foo": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"}, }, + { + name: "bundle with spaces in label value", + opType: "bundle", + expectedError: "", + labelInput: "foo.bar=baz bar", + expectedLabels: map[string]string{"dev.carvel.imgpkg.bundle": "true", "foo.bar": "baz bar"}, + }, } for _, tc := range testCases { From 5c0c2b3ca6a0b94f9fbb55307289a5fedc3636ce Mon Sep 17 00:00:00 2001 From: Joe Searcy Date: Wed, 2 Aug 2023 15:11:49 -0400 Subject: [PATCH 9/9] Relocate nil map check as per PR feedback Signed-off-by: Joe Searcy --- pkg/imgpkg/bundle/contents.go | 3 +++ pkg/imgpkg/cmd/push.go | 4 ---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/imgpkg/bundle/contents.go b/pkg/imgpkg/bundle/contents.go index d949548be..90b9a3114 100644 --- a/pkg/imgpkg/bundle/contents.go +++ b/pkg/imgpkg/bundle/contents.go @@ -49,6 +49,9 @@ func (b Contents) Push(uploadRef regname.Tag, labels map[string]string, registry return "", err } + if labels == nil { + labels = map[string]string{} + } labels[BundleConfigLabel] = "true" return plainimage.NewContents(b.paths, b.excludedPaths, b.preservePermissions).Push(uploadRef, labels, registry, logger) diff --git a/pkg/imgpkg/cmd/push.go b/pkg/imgpkg/cmd/push.go index e1cc4eed0..7ad0c0d38 100644 --- a/pkg/imgpkg/cmd/push.go +++ b/pkg/imgpkg/cmd/push.go @@ -69,10 +69,6 @@ func (po *PushOptions) Run() error { isBundle := po.BundleFlags.Bundle != "" isImage := po.ImageFlags.Image != "" - if po.LabelFlags.Labels == nil { - po.LabelFlags.Labels = map[string]string{} - } - switch { case isBundle && isImage: return fmt.Errorf("Expected only one of image or bundle")