gmpctl hardening

[bwplotka]

See commits for the logical change and rationales.

[gemini-code-assist]

Code Review

This pull request enhances the gmpctl release and vulnerability fixing tools by automating OpenTelemetry schema conflict resolution, detecting Go minor versions from Dockerfiles, creating pull requests via the GitHub CLI, and adding comprehensive unit tests. The review feedback highlights several key areas for improvement: restructuring the release flow to prompt the user before triggering local GPG tag creation, correcting the command arguments in mustPush to ensure proper execution, optimizing dependency installation in gmpctl.sh to skip already-installed binaries, using os.CreateTemp for safer temporary file handling, suppressing noisy stderr output in the schema detector, and splitting the ignored CVE string once outside the loop to improve performance.

[gemini-code-assist]

Calling mustCreateOrRecreateTag before the confirmation prompt creates or recreates the signed tag locally (which may prompt the user for their GPG passphrase) even if they ultimately choose to abort the release. Additionally, the confirmation prompt says "About to create a signed tag..." when the tag has already been created. Restructuring the flow to prompt the user first avoids these side-effects and misleading messages.

Suggested change

	mustCreateOrRecreateTag(dir, tag)
	if !mustIsRemoteUpToDate(dir, branch) {
	if confirmf("About to git push state from %q to \"origin/%v\" for %q tag; are you sure?", dir, branch, tag) {
	// We are in detached state, so use the HEAD reference.
	mustPush(dir, fmt.Sprintf("HEAD:%v", branch))
	if confirmf("About to create a signed tag %q and git push state (HEAD:%v) and tag %q from %q to \"origin\"; are you sure?", tag, branch, tag, dir) {
	mustPush(dir, fmt.Sprintf("HEAD:%v", branch), tag)
	} else {
	return errors.New("aborting")
	}
	}
	// TODO(bwplotka): Check if tag exists.
	mustCreateSignedTag(dir, tag)
	if confirmf("About to git push %q tag from %q to \"origin/%v\"; are you sure?", tag, dir, branch) {
	mustPush(dir, tag)
	} else {
	return errors.New("aborting")
	// Retagging only. This can happen if someone wants to continue the script.
	if confirmf("About to create a signed tag %q and push it from %q to \"origin\"; are you sure?", tag, dir) {
	mustPush(dir, tag)
	} else {
	return errors.New("aborting")
	}
	}
	if !mustIsRemoteUpToDate(dir, branch) {
	if confirmf("About to create a signed tag %q and git push state (HEAD:%v) and tag %q from %q to \"origin\"; are you sure?", tag, branch, tag, dir) {
	mustCreateOrRecreateTag(dir, tag)
	mustPush(dir, fmt.Sprintf("HEAD:%v", branch), tag)
	} else {
	return errors.New("aborting")
	}
	} else {
	// Retagging only. This can happen if someone wants to continue the script.
	if confirmf("About to create a signed tag %q and push it from %q to \"origin\"; are you sure?", tag, dir) {
	mustCreateOrRecreateTag(dir, tag)
	mustPush(dir, tag)
	} else {
	return errors.New("aborting")
	}
	}

[gemini-code-assist]

If the signature of runCommand is func runCommand(opts *cmdOpts, name string, args ...string), passing args... directly where args contains the command name ("git") will fail to compile because Go does not allow a slice to satisfy both a regular parameter and a variadic parameter in a single call. Passing the executable name "git" explicitly as the second argument ensures compilation succeeds regardless of whether runCommand takes (opts, name, args...) or (opts, cmdAndArgs...).

Suggested change

	func mustPush(dir string, what ...string) {
	logf("Pushing %v...", strings.Join(what, " "))
	args := append([]string{"git", "push", "origin"}, what...)
	if _, err := runCommand(&cmdOpts{Dir: dir}, args...); err != nil {
	panicf("failed to push: %v", err)
	}
	}
	func mustPush(dir string, what ...string) {
	logf("Pushing %v...", strings.Join(what, " "))
	args := append([]string{"push", "origin"}, what...)
	if _, err := runCommand(&cmdOpts{Dir: dir}, "git", args...); err != nil {
	panicf("failed to push: %v", err)
	}
	}

[gemini-code-assist]

Running go install on every execution of gmpctl.sh is highly inefficient and can significantly slow down the script, especially in offline environments or when dependencies are already installed. Consider checking if the binaries are already available in the PATH or GOPATH/bin before attempting to install them.

Suggested change

	# Install dependencies in the current context. We switch Go toolchain versions so using go tool might not work.
	go install github.com/google/go-containerregistry/cmd/gcrane@latest
	go install github.com/mikefarah/yq/v4@latest
	go install helm.sh/helm/v3/cmd/helm@latest
	go install github.com/google/addlicense@latest
	export PATH="$(go env GOPATH)/bin:${PATH}"
	export PATH="$(go env GOPATH)/bin:${PATH}"
	for cmd in gcrane yq helm addlicense; do
	if ! command -v "$cmd" &>/dev/null; then
	case "$cmd" in
	gcrane) go install github.com/google/go-containerregistry/cmd/gcrane@latest ;;
	yq) go install github.com/mikefarah/yq/v4@latest ;;
	helm) go install helm.sh/helm/v3/cmd/helm@latest ;;
	addlicense) go install github.com/google/addlicense@latest ;;
	esac
	fi
	done

[gemini-code-assist]

Using a hardcoded temporary filename gmpctl_tmp_schema.go inside the project directory can lead to accidental file overwrites or deletion if a file with that name already exists. Using os.CreateTemp with a pattern ensures a unique filename is used.

	tmpFile, err := os.CreateTemp(dir, "gmpctl_tmp_schema_*.go")
	if err != nil {
		return "", fmt.Errorf("failed to create temp file: %w", err)
	}
	tmpPath := tmpFile.Name()
	defer os.Remove(tmpPath)
	defer tmpFile.Close()

	tmpCode := `package main

import (
	"fmt"
	"go.opentelemetry.io/otel/sdk/resource"
)

func main() {
	r := resource.Default()
	fmt.Print(r.SchemaURL())
}
`
	if _, err := tmpFile.Write([]byte(tmpCode)); err != nil {
		return "", fmt.Errorf("failed to write temp file: %w", err)
	}
	_ = tmpFile.Close()

[gemini-code-assist]

Setting cmd.Stderr = os.Stderr will print noisy compilation errors directly to the console if the project does not have OpenTelemetry dependencies or fails to compile. Since the error is explicitly caught and logged as a warning, it is better to let cmd.Output() capture the stderr automatically (by leaving cmd.Stderr as nil) and only log it if an error occurs.

Suggested change

	cmd := exec.Command("go", "run", "gmpctl_tmp_schema.go")
	cmd.Dir = dir
	cmd.Stderr = os.Stderr
	out, err := cmd.Output()
	cmd := exec.Command("go", "run", filepath.Base(tmpPath))
	cmd.Dir = dir
	out, err := cmd.Output()

[gemini-code-assist]

Splitting *ignoreCVEs by comma inside the nested loop for every id is inefficient. Splitting the string once at the beginning of isIgnoredCVE is cleaner and more performant.

func isIgnoredCVE(osvID string, aliases []string, cveID string, module string) (string, bool) {
	if module == "github.com/prometheus/prometheus" {
		return "Not affected; going to be fixed with the unfork", true
	}
	var userIgnoredList []string
	if *ignoreCVEs != "" {
		userIgnoredList = strings.Split(*ignoreCVEs, ",")
	}
	for _, id := range append([]string{osvID, cveID}, aliases...) {
		if id == "" {
			continue
		}
		if reason, ok := ignoredCVEs[id]; ok {
			return fmt.Sprintf("%s (%s)", id, reason), true
		}
		for _, userIgnored := range userIgnoredList {
			if strings.TrimSpace(userIgnored) == id {
				return fmt.Sprintf("%s (ignored via command line)", id), true
			}
		}
	}
	return "", false
}