ci-operator: support mounting dependency images as volumes#5286
Draft
petr-muller wants to merge 1 commit into
Draft
ci-operator: support mounting dependency images as volumes#5286petr-muller wants to merge 1 commit into
petr-muller wants to merge 1 commit into
Conversation
Steps can now optionally set mount_path on a dependency to have the dependency image mounted as a read-only Kubernetes image volume at the specified path. This uses the ImageVolume feature (GA in OCP 4.20+/k8s 1.33+) to make dependency image contents directly accessible to the step without requiring the step to pull and extract the image itself. The env var injection continues to work regardless — mount_path is purely additive. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petr-muller 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 |
📝 WalkthroughWalkthroughAdds a ChangesDependency Image Volume Mount Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/steps/multi_stage/gen.go (1)
425-445: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake dependency volume names collision-proof.
dependencyVolumeName()is not injective: duplicate dependency names,a-bvsa:bvsa.b, and long names with the same first 63 characters all collapse to the same volume name.validateDependencies()does not reject that, so a valid dependency list can generate duplicatepod.Spec.Volumes[*].Namevalues and fail pod admission.Proposed fix
- for _, dependency := range step.Dependencies { + for i, dependency := range step.Dependencies { var ref string @@ if dependency.MountPath != "" { - volName := dependencyVolumeName(dependency.Name) + volName := dependencyVolumeName(dependency.Name, i) volumes = append(volumes, coreapi.Volume{ Name: volName, @@ -func dependencyVolumeName(name string) string { +func dependencyVolumeName(name string, index int) string { sanitized := strings.NewReplacer(":", "-", ".", "-").Replace(name) - volName := fmt.Sprintf("dep-%s", sanitized) - if len(volName) > 63 { - volName = volName[:63] - } - return volName + suffix := fmt.Sprintf("-%d", index) + maxBaseLen := 63 - len("dep-") - len(suffix) + if len(sanitized) > maxBaseLen { + sanitized = sanitized[:maxBaseLen] + } + return fmt.Sprintf("dep-%s%s", sanitized, suffix) }Also applies to: 464-470
🤖 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 `@pkg/steps/multi_stage/gen.go` around lines 425 - 445, The volume-name generation in dependency handling is collision-prone because dependencyVolumeName() can map different dependency names to the same pod volume name, and validateDependencies() does not currently catch this. Update the logic around validateDependencies() and the volume creation path in gen.go to detect duplicate/aliasing names up front and/or make dependencyVolumeName() produce unique, collision-proof names for every dependency in a step, including cases with separators and long shared prefixes.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pkg/validation/test.go`:
- Around line 967-980: The dependency mount validation in the relative-path
checks is using a deny-list style string contains test, which can miss nested
paths whose first segment starts with “..”. Update the logic around the
filepath.Rel checks in the dependency validation block to use an anchored
parent-path check based on path segments or a normalized absolute-prefix
comparison instead of strings.Contains(relPath, ".."). Keep the existing error
handling in the same validation flow and apply the fix symmetrically for both
directions of the dependency pair.
---
Outside diff comments:
In `@pkg/steps/multi_stage/gen.go`:
- Around line 425-445: The volume-name generation in dependency handling is
collision-prone because dependencyVolumeName() can map different dependency
names to the same pod volume name, and validateDependencies() does not currently
catch this. Update the logic around validateDependencies() and the volume
creation path in gen.go to detect duplicate/aliasing names up front and/or make
dependencyVolumeName() produce unique, collision-proof names for every
dependency in a step, including cases with separators and long shared prefixes.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6f730622-1f82-486e-a574-7ee07e762727
⛔ Files ignored due to path filters (1)
pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (5)
pkg/api/types.gopkg/steps/multi_stage/gen.gopkg/steps/multi_stage/gen_test.gopkg/validation/test.gopkg/validation/test_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
Comment on lines
+967
to
+980
| relPath, err := filepath.Rel(other.MountPath, dependency.MountPath) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, i, index, err)) | ||
| continue | ||
| } | ||
| if !strings.Contains(relPath, "..") { | ||
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, i, dependency.MountPath, index, other.MountPath)) | ||
| } | ||
| relPath, err = filepath.Rel(dependency.MountPath, other.MountPath) | ||
| if err != nil { | ||
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, index, i, err)) | ||
| continue | ||
| } | ||
| if !strings.Contains(relPath, "..") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use an anchored parent-path check here.
strings.Contains(relPath, "..") misses child paths whose first segment merely starts with .. (for example /var/run/images/..foo). filepath.Rel("/var/run/images", "/var/run/images/..foo") returns ..foo, so this invalid nested mount currently passes validation.
Proposed fix
+ dependencyMountPath := filepath.Clean(dependency.MountPath)
+ otherMountPath := filepath.Clean(other.MountPath)
- relPath, err := filepath.Rel(other.MountPath, dependency.MountPath)
+ relPath, err := filepath.Rel(otherMountPath, dependencyMountPath)
if err != nil {
errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, i, index, err))
continue
}
- if !strings.Contains(relPath, "..") {
+ if relPath == "." || (relPath != ".." && !strings.HasPrefix(relPath, ".."+string(filepath.Separator))) {
errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, i, dependency.MountPath, index, other.MountPath))
}
- relPath, err = filepath.Rel(dependency.MountPath, other.MountPath)
+ relPath, err = filepath.Rel(dependencyMountPath, otherMountPath)
if err != nil {
errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, index, i, err))
continue
}
- if !strings.Contains(relPath, "..") {
+ if relPath == "." || (relPath != ".." && !strings.HasPrefix(relPath, ".."+string(filepath.Separator))) {
errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, index, other.MountPath, i, dependency.MountPath))
}As per path instructions, "Validate at trust boundaries with allow-lists, not deny-lists".
📝 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
| relPath, err := filepath.Rel(other.MountPath, dependency.MountPath) | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, i, index, err)) | |
| continue | |
| } | |
| if !strings.Contains(relPath, "..") { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, i, dependency.MountPath, index, other.MountPath)) | |
| } | |
| relPath, err = filepath.Rel(dependency.MountPath, other.MountPath) | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, index, i, err)) | |
| continue | |
| } | |
| if !strings.Contains(relPath, "..") { | |
| dependencyMountPath := filepath.Clean(dependency.MountPath) | |
| otherMountPath := filepath.Clean(other.MountPath) | |
| relPath, err := filepath.Rel(otherMountPath, dependencyMountPath) | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, i, index, err)) | |
| continue | |
| } | |
| if relPath == "." || (relPath != ".." && !strings.HasPrefix(relPath, ".."+string(filepath.Separator))) { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, i, dependency.MountPath, index, other.MountPath)) | |
| } | |
| relPath, err = filepath.Rel(dependencyMountPath, otherMountPath) | |
| if err != nil { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] could not check relative path to dependencies[%d] (%w)", fieldRoot, index, i, err)) | |
| continue | |
| } | |
| if relPath == "." || (relPath != ".." && !strings.HasPrefix(relPath, ".."+string(filepath.Separator))) { | |
| errs = append(errs, fmt.Errorf("%s.dependencies[%d] mounts at %s, which is under dependencies[%d] (%s)", fieldRoot, index, other.MountPath, i, dependency.MountPath)) | |
| } |
🤖 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 `@pkg/validation/test.go` around lines 967 - 980, The dependency mount
validation in the relative-path checks is using a deny-list style string
contains test, which can miss nested paths whose first segment starts with “..”.
Update the logic around the filepath.Rel checks in the dependency validation
block to use an anchored parent-path check based on path segments or a
normalized absolute-prefix comparison instead of strings.Contains(relPath,
".."). Keep the existing error handling in the same validation flow and apply
the fix symmetrically for both directions of the dependency pair.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Steps can now optionally set mount_path on a dependency to have the dependency image mounted as a read-only Kubernetes image volume at the specified path. This uses the ImageVolume feature (GA in OCP 4.20+/k8s 1.33+) to make dependency image contents directly accessible to the step without requiring the step to pull and extract the image itself.
ci-operator now lets CI configs attach a dependency image directly into a step container as a read-only volume by setting
mount_pathonstep.dependencies. In practice, this means a step can consume files from a dependency image without having to pull/extract them itself, while the existing dependency environment variables still work as before.The implementation updates multi-stage pod generation to create Kubernetes Image volumes and mounts when
mount_pathis provided, adds validation to ensure mount paths are absolute and don’t conflict or nest under each other, and adds tests for the new volume naming, pod wiring, and validation rules.