Skip to content

NO-JIRA: Fix issue in the downloads server: archives are not compressed #16677

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nunnatsa wants to merge 1 commit into
base: main
Choose a base branch
from
+213 −102
Open

NO-JIRA: Fix issue in the downloads server: archives are not compressed #16677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions Dockerfile.downloads
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,13 @@ FROM registry.access.redhat.com/ubi9/ubi-minimal:latest
RUN mkdir -p /opt/downloads

COPY --from=gobuilder /go/src/github.com/openshift/console/bin/downloads /opt/downloads
COPY --from=gobuilder /go/src/github.com/openshift/console/cmd/downloads/config/defaultArtifactsConfig.yaml /opt/downloads
COPY --from=origincli /usr/share/openshift /usr/share/openshift

WORKDIR /

# doesn't require a root user.
USER 1001
CMD ["/opt/downloads/downloads", "--config-path=/opt/downloads/defaultArtifactsConfig.yaml"]
CMD ["/opt/downloads/downloads"]

LABEL \
io.k8s.display-name="CLI Artifacts Downloads Server" \
Expand Down
112 changes: 59 additions & 53 deletions cmd/downloads/config/downloads_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package config
import (
"archive/tar"
"archive/zip"
"compress/gzip"
"fmt"
"io"
"io/fs"
"net/http"
"os"
"path/filepath"
Expand All @@ -13,8 +15,7 @@ import (
"text/template"

"gopkg.in/yaml.v3"

klog "k8s.io/klog/v2"
"k8s.io/klog/v2"
)

// HtmlPageData holds data passed to the HTML template
Expand Down Expand Up @@ -54,7 +55,6 @@ type ArtifactSpec struct {

// ArtifactsConfig holds the configuration for the artifacts server
type DownloadsServerConfig struct {
Port string
Spec []ArtifactSpec
TempDir string
TemplateHTML *template.Template
Expand All @@ -80,7 +80,7 @@ const templateStringHTML = `<!DOCTYPE html>
<ul>
{{ range .Items }}
{{ if eq .Type "binary" }}
<li><a href="{{ .URL }}">oc ({{ .Name }})</a> (<a href="{{ .TarURL }}">tar</a> <a href="{{ .ZipURL }}">zip</a>)</li>
<li><a href="{{ .URL }}">oc ({{ .Name }})</a> (<a href="{{ .TarURL }}">tar.gz</a> <a href="{{ .ZipURL }}">zip</a>)</li>
{{ else if eq .Type "license" }}
<li><a href="{{ .URL }}">license</a></li>
{{ end }}
Expand All @@ -91,16 +91,11 @@ const templateStringHTML = `<!DOCTYPE html>
</html>`

// load artifacts to be served from given path
func loadArtifactsSpec(path string) ([]ArtifactSpec, error) {
func loadArtifactsSpec(dlConfigBytes []byte) ([]ArtifactSpec, error) {
specs := ArtifactsConfig{}
// open the JSON spec config file
fileBytes, err := os.ReadFile(path)
if err != nil {
return nil, err
}

// unmarshal the YAML file into the ArtifactsConfig object
err = yaml.Unmarshal(fileBytes, &specs)
err := yaml.Unmarshal(dlConfigBytes, &specs)
if err != nil {
return nil, err
}
Expand All @@ -114,7 +109,7 @@ func loadArtifactsSpec(path string) ([]ArtifactSpec, error) {
}

// NewDownloadsServerConfig creates a new ArtifactsConfig object
func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerConfig, error) {
func NewDownloadsServerConfig(dlConfigBytes []byte) (*DownloadsServerConfig, error) {
tempDir := defaultArtifactsDir
matches, _ := filepath.Glob("/tmp/artifacts*")
for _, match := range matches {
Expand All @@ -124,7 +119,7 @@ func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerC
return nil, fmt.Errorf("failed to create artifacts directory: %w", err)
}
klog.Info("Created artifacts directory (cleaned up any stale data from previous runs)")
specs, err := loadArtifactsSpec(specsFilePath)
specs, err := loadArtifactsSpec(dlConfigBytes)
if err != nil {
return nil, err
}
Expand All @@ -136,7 +131,6 @@ func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerC
}

artifactsConfig := DownloadsServerConfig{
Port: fmt.Sprintf("%d", port),
Spec: specs,
TempDir: tempDir,
TemplateHTML: templateHTML,
Expand All @@ -150,20 +144,7 @@ func NewDownloadsServerConfig(port int, specsFilePath string) (*DownloadsServerC
return &artifactsConfig, nil
}

func addFileToTar(tw *tar.Writer, filename string) error {
// Open the archive
file, err := os.Open(filename)
if err != nil {
return err
}
defer file.Close()

// Get file information
info, err := file.Stat()
if err != nil {
return err
}

func addFileToTar(tw *tar.Writer, file io.Reader, info fs.FileInfo) error {
// Create a tar Header from the FileInfo data
header, err := tar.FileInfoHeader(info, info.Name())
if err != nil {
Expand All @@ -185,26 +166,15 @@ func addFileToTar(tw *tar.Writer, filename string) error {
return nil
}

func addFileToZip(zw *zip.Writer, filename string) error {
// Open the archive
file, err := os.Open(filename)
if err != nil {
return err
}
defer file.Close()

// Get file information
info, err := file.Stat()
if err != nil {
return err
}

func addFileToZip(zw *zip.Writer, file io.Reader, fileInfo fs.FileInfo) error {
// Create a zip header based on the file's information
header, err := zip.FileInfoHeader(info)
header, err := zip.FileInfoHeader(fileInfo)
if err != nil {
return err
}

header.Method = zip.Deflate

// Create a new writer for the file in the zip archive
writer, err := zw.CreateHeader(header)
if err != nil {
Expand Down Expand Up @@ -242,6 +212,17 @@ func configureArchivePath(pathToTargetFile string) string {

// create archive for the target binary
func createArchive(pathToTargetFile string, format string) error {
targetFileInfo, err := os.Stat(pathToTargetFile)
if err != nil {
return err
}

targetFile, err := os.Open(pathToTargetFile)
if err != nil {
return err
}
defer targetFile.Close()

// create archive in the same directory as the target binary
pathToArchive := configureArchivePath(pathToTargetFile) + format

Expand All @@ -251,14 +232,24 @@ func createArchive(pathToTargetFile string, format string) error {
}

defer file.Close()
if format == ".tar" {
archiveWriter := tar.NewWriter(file)
addFileToTar(archiveWriter, pathToTargetFile)
if format == ".tar.gz" {
compressWriter := gzip.NewWriter(file)
defer compressWriter.Close()
archiveWriter := tar.NewWriter(compressWriter)
defer archiveWriter.Close()

err = addFileToTar(archiveWriter, targetFile, targetFileInfo)
if err != nil {
return fmt.Errorf("could not create archive for %s: %w", pathToTargetFile, err)
}
} else if format == ".zip" {
archiveWriter := zip.NewWriter(file)
addFileToZip(archiveWriter, pathToTargetFile)
defer archiveWriter.Close()
err = addFileToZip(archiveWriter, targetFile, targetFileInfo)

if err != nil {
return fmt.Errorf("could not create archive for %s: %w", pathToTargetFile, err)
}
} else {
return fmt.Errorf("unsupported archive type")
}
Expand Down Expand Up @@ -351,7 +342,7 @@ func (downloadsConfig *DownloadsServerConfig) generateDirFileContents() ([]ListI
Type: Binary,
URL: relPath,
Name: displayName(spec.Arch, spec.OperatingSystem, basename),
TarURL: fmt.Sprintf("%s.tar", relArchivePath),
TarURL: fmt.Sprintf("%s.tar.gz", relArchivePath),
ZipURL: fmt.Sprintf("%s.zip", relArchivePath),
})
}
Expand All @@ -373,7 +364,7 @@ func (c *DownloadsServerConfig) CreateArchivesInBackground() {
wg.Add(2)
go func(p string) {
defer wg.Done()
if err := createArchive(p, ".tar"); err != nil {
if err := createArchive(p, ".tar.gz"); err != nil {
klog.Errorf("Failed to create tar archive for %s: %v", p, err)
}
}(artifactPath)
Expand All @@ -397,9 +388,21 @@ func (c *DownloadsServerConfig) CreateArchivesInBackground() {
func (c *DownloadsServerConfig) Handler() http.Handler {
fs := http.FileServer(http.Dir(c.TempDir))
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ext := filepath.Ext(r.URL.Path)
if ext == ".tar" || ext == ".zip" {
<-c.archivesReady
// temporary workaround for the CLI download configuration done in the console operator.
// can be dropped when the CLI download configuration is fixed to use "tar.gz" rather than
// "tar"
if strings.HasSuffix(r.URL.Path, ".tar") {
r.URL.Path += ".gz"
}

if strings.HasSuffix(r.URL.Path, ".zip") || strings.HasSuffix(r.URL.Path, ".tar.gz") {
select {
case <-r.Context().Done():
w.Header().Set("Retry-After", "3")
http.Error(w, http.StatusText(http.StatusServiceUnavailable), http.StatusServiceUnavailable)
return
case <-c.archivesReady:
}
}
fs.ServeHTTP(w, r)
})
Expand All @@ -408,7 +411,10 @@ func (c *DownloadsServerConfig) Handler() http.Handler {
// setupArtifactsDirectory creates the root HTML file and directories, files and archives for artifacts
func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error {
// symlink file in the temporary directory that points to the openshift license
os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
if err != nil {
klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err)
}
Comment on lines 412 to +417

@coderabbitai coderabbitai Bot Jun 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Return the LICENSE symlink error instead of serving a broken license link.

generateDirFileContents always adds the license URL, so continuing after symlink failure leaves the generated index pointing at missing content.

Proposed fix 
 	err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
 	if err != nil {
-		klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err)
+		return fmt.Errorf("can't create symlink to the LICENSE file in %s: %w", artifactsConfig.TempDir, err)
 	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error {
// symlink file in the temporary directory that points to the openshift license
os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
if err != nil {
klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err)
}
func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error {
// symlink file in the temporary directory that points to the openshift license
err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
if err != nil {
return fmt.Errorf("can't create symlink to the LICENSE file in %s: %w", artifactsConfig.TempDir, err)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/downloads/config/downloads_config.go` around lines 406 - 411, The
setupArtifactsDirectory function currently logs symlink creation failures but
still continues, which can leave generateDirFileContents serving a broken
LICENSE link. Update setupArtifactsDirectory so that when os.Symlink fails for
ocLicenseFile it returns that error immediately instead of only calling
klog.Errorf, and make sure the caller path in DownloadsServerConfig respects the
returned error and aborts index generation when the symlink cannot be created.


// generates content of the root html file and creates directories, files and archives for artifacts
content, err := artifactsConfig.generateDirFileContents()
Expand Down
Loading