Repository: project-copacetic/copacetic (go)

Issue:
[REQ] Add support for custom suffix tag
### What kind of request is this?

None

### What is your request or suggestion?

Instead of "-patched" as a default tag suffix, consider adding a custom suffix flag.

### Are you willing to submit PRs to contribute to this feature request?

- [ ] Yes, I am willing to implement it.

Pull request:
feat: add support for custom suffix tag
<!--
Please include the type of change in the PR title as "<type>: <summary>"
Valid <type> include: build, ci, docs, feat, fix, perf, refactor, test
See https://github.com/project-copacetic/copacetic/blob/master/CONTRIBUTING.md#pull-requests) for guidelines.
-->

Adds a tag suffix to the patch command
Adds testing for the tag suffix logic
Adds extra testing to the patch command

Closes #923


Patch:
diff --git a/pkg/patch/cmd.go b/pkg/patch/cmd.go
index f980f978..85621cc6 100644
--- a/pkg/patch/cmd.go
+++ b/pkg/patch/cmd.go
@@ -19,6 +19,7 @@ type patchArgs struct {
 	appImage      string
 	reportFile    string
 	patchedTag    string
+	suffix        string
 	workingFolder string
 	timeout       time.Duration
 	scanner       string
@@ -46,6 +47,7 @@ func NewPatchCmd() *cobra.Command {
 				ua.appImage,
 				ua.reportFile,
 				ua.patchedTag,
+				ua.suffix,
 				ua.workingFolder,
 				ua.scanner,
 				ua.format,
@@ -58,6 +60,7 @@ func NewPatchCmd() *cobra.Command {
 	flags.StringVarP(&ua.appImage, "image", "i", "", "Application image name and tag to patch")
 	flags.StringVarP(&ua.reportFile, "report", "r", "", "Vulnerability report file path")
 	flags.StringVarP(&ua.patchedTag, "tag", "t", "", "Tag for the patched image")
+	flags.StringVarP(&ua.suffix, "tag-suffix", "", "patched", "Suffix for the patched image (if no explicit --tag provided)")
 	flags.StringVarP(&ua.workingFolder, "working-folder", "w", "", "Working folder, defaults to system temp folder")
 	flags.StringVarP(&ua.bkOpts.Addr, "addr", "a", "", "Address of buildkitd service, defaults to local docker daemon with fallback to "+buildkit.DefaultAddr)
 	flags.StringVarP(&ua.bkOpts.CACertPath, "cacert", "", "", "Absolute path to buildkitd CA certificate")
diff --git a/pkg/patch/patch.go b/pkg/patch/patch.go
index d73b29a0..106eb910 100644
--- a/pkg/patch/patch.go
+++ b/pkg/patch/patch.go
@@ -36,20 +36,19 @@ import (
 )
 
 const (
-	defaultPatchedTagSuffix = "patched"
-	copaProduct             = "copa"
-	defaultRegistry         = "docker.io"
-	defaultTag              = "latest"
+	copaProduct     = "copa"
+	defaultRegistry = "docker.io"
+	defaultTag      = "latest"
 )
 
 // Patch command applies package updates to an OCI image given a vulnerability report.
-func Patch(ctx context.Context, timeout time.Duration, image, reportFile, patchedTag, workingFolder, scanner, format, output string, ignoreError bool, bkOpts buildkit.Opts) error {
+func Patch(ctx context.Context, timeout time.Duration, image, reportFile, patchedTag, suffix, workingFolder, scanner, format, output string, ignoreError bool, bkOpts buildkit.Opts) error {
 	timeoutCtx, cancel := context.WithTimeout(ctx, timeout)
 	defer cancel()
 
 	ch := make(chan error)
 	go func() {
-		ch <- patchWithContext(timeoutCtx, ch, image, reportFile, patchedTag, workingFolder, scanner, format, output, ignoreError, bkOpts)
+		ch <- patchWithContext(timeoutCtx, ch, image, reportFile, patchedTag, suffix, workingFolder, scanner, format, output, ignoreError, bkOpts)
 	}()
 
 	select {
@@ -74,37 +73,27 @@ func removeIfNotDebug(workingFolder string) {
 	}
 }
 
-func patchWithContext(ctx context.Context, ch chan error, image, reportFile, patchedTag, workingFolder, scanner, format, output string, ignoreError bool, bkOpts buildkit.Opts) error {
+func patchWithContext(ctx context.Context, ch chan error, image, reportFile, patchedTag, suffix, workingFolder, scanner, format, output string, ignoreError bool, bkOpts buildkit.Opts) error {
 	if reportFile == "" && output != "" {
 		log.Warn("No vulnerability report was provided, so no VEX output will be generated.")
 	}
 
+	// parse the image reference
 	imageName, err := reference.ParseNormalizedNamed(image)
 	if err != nil {
-		return err
-	}
-	if reference.IsNameOnly(imageName) {
-		log.Warnf("Image name has no tag or digest, using latest as tag")
-		imageName = reference.TagNameOnly(imageName)
+		return fmt.Errorf("failed to parse reference: %w", err)
 	}
-	var tag string
-	taggedName, ok := imageName.(reference.Tagged)
-	if ok {
-		tag = taggedName.Tag()
-	} else {
-		log.Warnf("Image name has no tag")
-	}
-	if patchedTag == "" {
-		if tag == "" {
-			log.Warnf("No output tag specified for digest-referenced image, defaulting to `%s`", defaultPatchedTagSuffix)
-			patchedTag = defaultPatchedTagSuffix
-		} else {
-			patchedTag = fmt.Sprintf("%s-%s", tag, defaultPatchedTagSuffix)
-		}
+
+	// resolve final patched tag
+	patchedTag, err = resolvePatchedTag(imageName, patchedTag, suffix)
+	if err != nil {
+		return err
 	}
+
+	// create the patched image name
 	_, err = reference.WithTag(imageName, patchedTag)
 	if err != nil {
-		return fmt.Errorf("%w with patched tag %s", err, patchedTag)
+		return fmt.Errorf("invalid patched tag: %w with patched tag %s", err, patchedTag)
 	}
 	patchedImageName := fmt.Sprintf("%s:%s", imageName.Name(), patchedTag)
 
@@ -338,6 +327,34 @@ func patchWithContext(ctx context.Context, ch chan error, image, reportFile, pat
 	return err
 }
 
+// resolvePatchedTag merges explicit tag & suffix rules, returning the final patched tag.
+func resolvePatchedTag(imageRef reference.Named, explicitTag, suffix string) (string, error) {
+	// if user explicitly sets a final tag, that wins outright
+	if explicitTag != "" {
+		return explicitTag, nil
+	}
+
+	// parse out any existing tag from the image ref
+	var baseTag string
+	if tagged, ok := imageRef.(reference.Tagged); ok {
+		baseTag = tagged.Tag()
+	}
+
+	// if suffix is empty, default to "patched"
+	if suffix == "" {
+		suffix = "patched"
+	}
+
+	// if we have no original baseTag (the user’s image had no tag),
+	// then we can’t append a suffix to it
+	if baseTag == "" {
+		return "", fmt.Errorf("no tag found in image reference %s", imageRef.String())
+	}
+
+	// otherwise, combine them
+	return fmt.Sprintf("%s-%s", baseTag, suffix), nil
+}
+
 func getOSType(ctx context.Context, osreleaseBytes []byte) (string, error) {
 	r := bytes.NewReader(osreleaseBytes)
 	osData, err := osrelease.Parse(ctx, r)


Test patch:
diff --git a/pkg/patch/patch_test.go b/pkg/patch/patch_test.go
index 42e6cdab..4242993b 100644
--- a/pkg/patch/patch_test.go
+++ b/pkg/patch/patch_test.go
@@ -6,6 +6,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/distribution/reference"
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 )
@@ -46,6 +47,44 @@ func TestRemoveIfNotDebug(t *testing.T) {
 		// Clean up the working folder manually
 		os.RemoveAll(workingFolder)
 	})
+
+	t.Run("RemoveWorkingFolderWhenLogLevelIsInfo", func(t *testing.T) {
+		log.SetLevel(log.InfoLevel)
+		workingFolder := t.TempDir()
+
+		removeIfNotDebug(workingFolder)
+
+		// folder should be removed
+		if _, err := os.Stat(workingFolder); err == nil {
+			t.Errorf("working folder should have been removed but still exists at: %s", workingFolder)
+		}
+	})
+
+	t.Run("KeepWorkingFolderWhenLogLevelIsDebug", func(t *testing.T) {
+		log.SetLevel(log.DebugLevel)
+		workingFolder := t.TempDir()
+
+		removeIfNotDebug(workingFolder)
+
+		// folder should remain
+		if _, err := os.Stat(workingFolder); err != nil {
+			t.Errorf("working folder should have been kept but was removed at: %s", workingFolder)
+		}
+	})
+
+	t.Run("NoopWhenFolderDoesNotExist", func(t *testing.T) {
+		log.SetLevel(log.InfoLevel)
+		// create then remove
+		workingFolder := t.TempDir()
+		os.RemoveAll(workingFolder)
+
+		removeIfNotDebug(workingFolder)
+
+		// still doesn't exist, and no panic
+		if _, err := os.Stat(workingFolder); err == nil {
+			t.Errorf("folder unexpectedly re-created: %s", workingFolder)
+		}
+	})
 }
 
 func TestGetOSType(t *testing.T) {
@@ -269,6 +308,33 @@ func TestGetOSType(t *testing.T) {
 			err:            nil,
 			expectedOSType: "alma",
 		},
+		{
+			osRelease: []byte(`PRETTY_NAME="Debian GNU/Linux 11 (bullseye)"
+			NAME="Debian GNU/Linux"
+			VERSION_ID="11"
+			`),
+			err:            nil,
+			expectedOSType: "debian",
+		},
+		{
+			osRelease: []byte(`NAME="Alpine Linux"
+			ID=alpine
+			VERSION_ID=3.7.3`),
+			err:            nil,
+			expectedOSType: "alpine",
+		},
+		{
+			osRelease: []byte(`NAME="SomeRandomOS"
+			ID=someos
+			VERSION_ID=1.0`),
+			err:            errors.ErrUnsupported,
+			expectedOSType: "",
+		},
+		{
+			osRelease:      nil,
+			err:            errors.ErrUnsupported,
+			expectedOSType: "",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -307,6 +373,18 @@ func TestGetOSVersion(t *testing.T) {
 			errMsg:            "unable to parse os-release data osrelease: malformed line \"Cannot Parse Version_ID\"",
 			expectedOSVersion: "",
 		},
+		{
+			osRelease: []byte(`PRETTY_NAME="Debian GNU/Linux 11 (bullseye)"
+			VERSION_ID="11"
+			ID=debian`),
+			errMsg:            "",
+			expectedOSVersion: "11",
+		},
+		{
+			osRelease:         []byte("Cannot Parse Version_ID"),
+			errMsg:            `unable to parse os-release data osrelease: malformed line "Cannot Parse Version_ID"`,
+			expectedOSVersion: "",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -332,4 +410,86 @@ func TestGetRepoNameWithDigest(t *testing.T) {
 	if result != "nginx@sha256:mocked-digest" {
 		t.Fatalf("unexpected result: %s", result)
 	}
+	t.Run("WithTagAndDigest", func(t *testing.T) {
+		result := getRepoNameWithDigest("docker.io/library/nginx:1.21.6-patched", "sha256:mocked-digest")
+		assert.Equal(t, "nginx@sha256:mocked-digest", result)
+	})
+
+	t.Run("NoTagUsesFullImageName", func(t *testing.T) {
+		result := getRepoNameWithDigest("docker.io/library/nginx", "sha256:abc123")
+		// there's no trailing :tag, so we strip library/ prefix -> "nginx@sha256:abc123"
+		assert.Equal(t, "nginx@sha256:abc123", result)
+	})
+
+	t.Run("RandomLocalImageName", func(t *testing.T) {
+		result := getRepoNameWithDigest("localhost:5000/repo/image:mytag", "sha256:abcdef1234")
+		// last portion is "image:mytag" => we only keep "image" for the name portion
+		assert.Equal(t, "image@sha256:abcdef1234", result)
+	})
+
+	t.Run("NoRegistryNoTag", func(t *testing.T) {
+		result := getRepoNameWithDigest("myimage", "sha256:short")
+		// no registry, no tag, just "myimage" => name is "myimage@sha256:short"
+		assert.Equal(t, "myimage@sha256:short", result)
+	})
+}
+
+func TestResolvePatchedTag(t *testing.T) {
+	tests := []struct {
+		name        string
+		image       string
+		explicitTag string
+		suffix      string
+		want        string
+		wantErr     bool
+	}{
+		{
+			name:  "no explicitTag, no suffix, existing base tag",
+			image: "docker.io/library/nginx:1.23",
+			want:  "1.23-patched",
+		},
+		{
+			name:    "no explicitTag, no suffix, no base tag",
+			image:   "docker.io/library/nginx",
+			wantErr: true,
+		},
+		{
+			name:   "explicitTag overrides suffix and base tag",
+			image:  "docker.io/library/nginx:1.23",
+			suffix: "xyz",
+			// user sets an explicit tag, so we don't append the suffix
+			explicitTag: "my-funky-tag",
+			want:        "my-funky-tag",
+		},
+		{
+			name:   "custom suffix with base tag",
+			image:  "docker.io/library/nginx:1.23",
+			suffix: "security",
+			want:   "1.23-security",
+		},
+		{
+			name:    "custom suffix with no base tag",
+			image:   "docker.io/library/nginx",
+			suffix:  "foo",
+			wantErr: true,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// tags should always resolve here
+			imageRef, err := reference.ParseNormalizedNamed(tc.image)
+			if err != nil {
+				t.Fatalf("failed to parse image reference: %v", err)
+			}
+
+			got, err := resolvePatchedTag(imageRef, tc.explicitTag, tc.suffix)
+			if tc.wantErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, tc.want, got)
+			}
+		})
+	}
 }