From ff0bffc756fd3f2cb105e44e8d476bddbc239641 Mon Sep 17 00:00:00 2001 From: Sachin Sampras M Date: Mon, 20 Jul 2026 21:38:04 +0100 Subject: [PATCH 1/2] fix: simplify download fallback and fix resource leaks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplify the openshift strategy to a two-tier fallback: 1. Current version from ConsoleCLIDownload CR (production) 2. Stable version (1.4.2) via CDN redirect resolution Remove all staging Developer Portal references and squid proxy configuration — preprod instances are not accessible outside the Red Hat VPN. Additional fixes from audit: - Fix goroutine/pipe reader leak on failed downloads (defer pr.Close) - Clean up temp directories on download failure (os.RemoveAll) - Add bare cgwName as FindBinary candidate for CLIs where cliName differs from cgwName (gitsign, rekor-cli) - Retry body-read errors during download instead of failing immediately Implements SECURESIGN-2158 Signed-off-by: Sachin Sampras M --- pkg/strategy/openshift/openshift.go | 24 ++++++++------------ pkg/support/cgw.go | 15 ++++++++++--- pkg/support/testSupport.go | 35 +++++++++-------------------- 3 files changed, 31 insertions(+), 43 deletions(-) diff --git a/pkg/strategy/openshift/openshift.go b/pkg/strategy/openshift/openshift.go index 56cdcda..d2757b1 100644 --- a/pkg/strategy/openshift/openshift.go +++ b/pkg/strategy/openshift/openshift.go @@ -26,8 +26,7 @@ func init() { const ( prodHost = "developers.redhat.com" - stagingHost = "developers.qa.redhat.com" - fallbackVersion = "1.4.1" + fallbackVersion = "1.4.2" ) var versionRegexp = regexp.MustCompile(`/RHTAS/[^/]+/`) @@ -42,20 +41,14 @@ func download(ctx context.Context, client controller.Reader, cliName string) (st if isTarGz(link) { path, err := downloadTarGz(ctx, cliName, link) if err != nil && strings.Contains(link, prodHost) { - stagingLink := strings.Replace(link, prodHost, stagingHost, 1) - logrus.Infof("Production download failed, falling back to staging: %s", stagingLink) - path, err = downloadTarGz(ctx, cliName, stagingLink) - if err != nil { - fallbackLink := versionRegexp.ReplaceAllString(link, "/RHTAS/"+fallbackVersion+"/") - logrus.Infof("Staging download failed, falling back to stable %s via CDN: %s", fallbackVersion, fallbackLink) - cdnLink, cdnErr := support.ResolveCDNLink(ctx, fallbackLink) - if cdnErr != nil { - return "", fmt.Errorf("all download attempts failed (prod, staging, CDN fallback): %w", cdnErr) - } - logrus.Infof("Resolved CDN link: %s", cdnLink) - return downloadTarGz(ctx, cliName, cdnLink) + fallbackLink := versionRegexp.ReplaceAllString(link, "/RHTAS/"+fallbackVersion+"/") + logrus.Infof("Download failed, falling back to stable %s via CDN: %s", fallbackVersion, fallbackLink) + cdnLink, cdnErr := support.ResolveCDNLink(ctx, fallbackLink) + if cdnErr != nil { + return "", fmt.Errorf("all download attempts failed (current version, CDN fallback %s): %w", fallbackVersion, cdnErr) } - return path, nil + logrus.Infof("Resolved CDN link: %s", cdnLink) + return downloadTarGz(ctx, cliName, cdnLink) } return path, err } @@ -79,6 +72,7 @@ func downloadTarGz(ctx context.Context, cliName string, link string) (string, er } if err = support.DownloadAndUntarArchive(ctx, link, tmp); err != nil { + os.RemoveAll(tmp) return "", err } diff --git a/pkg/support/cgw.go b/pkg/support/cgw.go index 5e13783..95d96a8 100644 --- a/pkg/support/cgw.go +++ b/pkg/support/cgw.go @@ -23,12 +23,15 @@ func ContentGatewayName(name string) string { // FindBinary searches for a CLI binary in the given directory using candidate name patterns. func FindBinary(dir, cliName, goos, goarch string) (string, error) { cgwName := ContentGatewayName(cliName) - candidates := []string{ - cliName, + candidates := []string{cliName} + if cgwName != cliName { + candidates = append(candidates, cgwName) + } + candidates = append(candidates, fmt.Sprintf("%s_%s_%s", cgwName, goos, goarch), fmt.Sprintf("%s_%s", cgwName, goos), fmt.Sprintf("%s-%s-%s", cliName, goos, goarch), - } + ) if goos == "windows" { for i, name := range candidates { candidates[i] = name + ".exe" @@ -38,6 +41,12 @@ func FindBinary(dir, cliName, goos, goarch string) (string, error) { for _, name := range candidates { path := filepath.Join(dir, name) if _, err := os.Stat(path); err == nil { + if name != cliName { + link := filepath.Join(dir, cliName) + if err := os.Symlink(path, link); err == nil { + return link, nil + } + } return path, nil } } diff --git a/pkg/support/testSupport.go b/pkg/support/testSupport.go index 0e6cfb3..e138eda 100644 --- a/pkg/support/testSupport.go +++ b/pkg/support/testSupport.go @@ -65,6 +65,7 @@ func GitCloneWithAuth(url string, auth transport.AuthMethod) (string, *git.Repos func DownloadAndUnzip(ctx context.Context, link string, writer io.Writer) error { pr, pw := io.Pipe() + defer pr.Close() go func() { _, err := Download(ctx, link, pw) @@ -75,6 +76,7 @@ func DownloadAndUnzip(ctx context.Context, link string, writer io.Writer) error func DownloadAndUntarArchive(ctx context.Context, link string, dst string) error { pr, pw := io.Pipe() + defer pr.Close() go func() { _, err := Download(ctx, link, pw) @@ -83,30 +85,8 @@ func DownloadAndUntarArchive(ctx context.Context, link string, dst string) error return UntarArchive(dst, pr) } -const squidProxy = "http://squid.corp.redhat.com:3128" - -var stagingProxySuffixes = []string{ - ".qa.redhat.com", - ".dev.redhat.com", - ".stage.redhat.com", - ".preprod.redhat.com", -} - -func proxyForStagingOnly(req *http.Request) (*url.URL, error) { - host := req.URL.Hostname() - for _, suffix := range stagingProxySuffixes { - if strings.HasSuffix(host, suffix) { - return url.Parse(squidProxy) - } - } - return nil, nil -} - func Download(ctx context.Context, link string, writer io.Writer) (int64, error) { - client := &http.Client{ - Timeout: 2 * time.Minute, //nolint:mnd - Transport: &http.Transport{Proxy: proxyForStagingOnly}, - } + client := &http.Client{Timeout: 2 * time.Minute} //nolint:mnd const maxRetries = 5 var lastErr error @@ -136,8 +116,13 @@ func Download(ctx context.Context, link string, writer io.Writer) (int64, error) lastErr = fmt.Errorf("bad status: %s", resp.Status) continue } - defer resp.Body.Close() - return io.Copy(writer, resp.Body) + n, copyErr := io.Copy(writer, resp.Body) + resp.Body.Close() + if copyErr != nil { + lastErr = copyErr + continue + } + return n, nil } return 0, fmt.Errorf("download failed after %d attempts: %w", maxRetries, lastErr) } From 31ca81855420b06b747bf19e5d65c3d041de23dd Mon Sep 17 00:00:00 2001 From: Sachin Sampras M Date: Mon, 20 Jul 2026 21:39:41 +0100 Subject: [PATCH 2/2] fix: suppress errcheck for os.RemoveAll on temp cleanup Implements SECURESIGN-2158 Signed-off-by: Sachin Sampras M --- pkg/strategy/openshift/openshift.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/strategy/openshift/openshift.go b/pkg/strategy/openshift/openshift.go index d2757b1..0216171 100644 --- a/pkg/strategy/openshift/openshift.go +++ b/pkg/strategy/openshift/openshift.go @@ -72,7 +72,7 @@ func downloadTarGz(ctx context.Context, cliName string, link string) (string, er } if err = support.DownloadAndUntarArchive(ctx, link, tmp); err != nil { - os.RemoveAll(tmp) + _ = os.RemoveAll(tmp) return "", err }