no-jira: cluster-api: use zstd for compression#10450
no-jira: cluster-api: use zstd for compression#10450patrickdillon wants to merge 1 commit intoopenshift:mainfrom
Conversation
Prior to this commit, the installer built all capi providers and zipped them with no compression. This updates that approach to use zstd to compress (and decompress the providers). It decreases the installer binary size from 618M -> 533M (roughly 14%).
openshift-ci-robot
added
the
jira/valid-reference
|
@patrickdillon: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/cc @dlom |
WalkthroughThe pull request transitions the binary packaging and extraction strategy from ZIP archives to Zstandard (zstd) compression format. The build script now generates a shared compression dictionary and produces individual Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
/test e2e-gcp-ovn e2e-azure-ovn e2e-openstack-ovn e2e-nutanix-ovn e2e-vsphere-ovn |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/clusterapi/providers.go`:
- Around line 119-127: The decompressed file at destPath is left with
unpredictable mode bits and Close errors are ignored; after writing with
os.OpenFile (destPath, ...), ensure you explicitly set the final executable mode
(e.g., os.Chmod(destPath, 0o755) or a properly restrictive mode) once io.Copy to
dest completes, and surface any errors from dest.Close() and os.Chmod by
returning/wrapping them; reference destPath, dest (the *os.File returned by
os.OpenFile), decoder and the io.Copy/Close sequence when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56146467-c2ca-4ad0-89e6-9903a8f8775b
📒 Files selected for processing (3)
go.modhack/build-cluster-api.shpkg/clusterapi/providers.go
| dest, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o777) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer destFile.Close() | ||
| if _, err := io.CopyN(destFile, src, f.FileInfo().Size()); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } | ||
| defer dest.Close() | ||
|
|
||
| func sanitizeArchivePath(d, t string) (v string, err error) { | ||
| v = filepath.Join(d, t) | ||
| if strings.HasPrefix(v, filepath.Clean(d)) { | ||
| return v, nil | ||
| if _, err := io.Copy(dest, decoder); err != nil { | ||
| return fmt.Errorf("failed to decompress: %w", err) | ||
| } |
There was a problem hiding this comment.
Restore executable permissions explicitly after decompression.
Raw .zst payloads no longer carry the mode bits the zip path used to preserve. OpenFile(..., 0o777) only affects brand-new files and relies on umask, so reruns can leave a stale non-executable mode in place and first-time extracts can be overly permissive. Since these files are executed later, set the final mode explicitly and surface Close errors.
Possible hardening
- dest, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o777)
+ dest, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)
if err != nil {
return err
}
- defer dest.Close()
if _, err := io.Copy(dest, decoder); err != nil {
- return fmt.Errorf("failed to decompress: %w", err)
+ dest.Close()
+ return fmt.Errorf("failed to decompress %s: %w", destPath, err)
}
+ if err := dest.Close(); err != nil {
+ return fmt.Errorf("failed to close %s: %w", destPath, err)
+ }
+ if err := os.Chmod(destPath, 0o755); err != nil {
+ return fmt.Errorf("failed to chmod %s: %w", destPath, err)
+ }
return nil📝 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.
| dest, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o777) | |
| if err != nil { | |
| return err | |
| } | |
| defer destFile.Close() | |
| if _, err := io.CopyN(destFile, src, f.FileInfo().Size()); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| defer dest.Close() | |
| func sanitizeArchivePath(d, t string) (v string, err error) { | |
| v = filepath.Join(d, t) | |
| if strings.HasPrefix(v, filepath.Clean(d)) { | |
| return v, nil | |
| if _, err := io.Copy(dest, decoder); err != nil { | |
| return fmt.Errorf("failed to decompress: %w", err) | |
| } | |
| dest, err := os.OpenFile(destPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600) | |
| if err != nil { | |
| return err | |
| } | |
| if _, err := io.Copy(dest, decoder); err != nil { | |
| dest.Close() | |
| return fmt.Errorf("failed to decompress %s: %w", destPath, err) | |
| } | |
| if err := dest.Close(); err != nil { | |
| return fmt.Errorf("failed to close %s: %w", destPath, err) | |
| } | |
| if err := os.Chmod(destPath, 0o755); err != nil { | |
| return fmt.Errorf("failed to chmod %s: %w", destPath, err) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/clusterapi/providers.go` around lines 119 - 127, The decompressed file at
destPath is left with unpredictable mode bits and Close errors are ignored;
after writing with os.OpenFile (destPath, ...), ensure you explicitly set the
final executable mode (e.g., os.Chmod(destPath, 0o755) or a properly restrictive
mode) once io.Copy to dest completes, and surface any errors from dest.Close()
and os.Chmod by returning/wrapping them; reference destPath, dest (the *os.File
returned by os.OpenFile), decoder and the io.Copy/Close sequence when making the
change.
|
well, that would be a problem |
|
/close |
|
@patrickdillon: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
following up with art about getting |
|
/open |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
|
Depends on openshift-eng/ocp-build-data#9740 |
Correction: zstd is being added to the builder image, should be available by tomorrow |
|
/retest |
|
@patrickdillon: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Prior to this commit, the installer built all capi providers and zipped them with no compression. This updates that approach to use zstd to compress (and decompress the providers). It decreases the installer binary size from 618M -> 533M (roughly 14%).