immutable gateway: skip ..-prefixed dirs in artifact walker#2640
Conversation
|
|
📝 WalkthroughWalkthrough
ChangesArtifact discovery refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Hi @tricktron, Could you please rebase your branch on the latest main, resolve any conflicts if needed, and update your PR? That should fix these workflow failures. Thanks! |
5df20c1 to
5216575
Compare
@O-sura Rebased and updated. |
There was a problem hiding this comment.
The fix correctly handles the flat configMap layout but it introduces a regression for artifacts stored in nested directories. With this new change it skips ..* directories to avoid duplicate processing and relies on filepath.WalkDir, which does not follow directory symlinks.
This works for flat ConfigMap mounts, but fails for nested paths (e.g. rest-apis/petstore.yaml) because the timestamped directory is skipped, and WalkDir does not traverse directory symlinks.
/artifacts/
├── ..2026_07_13_10_00_00.123/ ← real dir
│ └── rest-apis/petstore.yaml
├── ..data ──► ..2026_07_13_.../ ← symlink to current revision
└── rest-apis ──► ..data/rest-apis ← top-level entry is a symlink to a DIRECTORY(In Kubernetes' atomic writer layout, the only real copy of the file exists inside the timestamped ..2026_* directory, while the exposed directory (rest-apis) is a symlink)
This will return zero artifacts without any error even though the file is there. Hence we need to consider this scenario as well
|
@O-sura Good catch. I'll add a test to reproduce it and try to think about a solution. |
|
@O-sura, Ok I added the test and fixed it by replacing |
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)
gateway/gateway-controller/pkg/immutable/loader.go (1)
93-103: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftEnforce path containment check before reading files.
As per coding guidelines, any file path must be resolved to its final absolute path and verified to stay strictly within the intended root directory before calling
os.ReadFileor any other read operation.Since
collectArtifactsfollows symlinks, failing to enforce containment allows a crafted symlink to escape theArtifactsDir, resulting in arbitrary file reads during startup.To prevent partial prefix matches and resolve symlinked root directories (common in container mounts), evaluate symlinks on both the
ArtifactsDirand the filepathbefore asserting containment.🛡️ Proposed fix to add containment check
artifactPaths, err := collectArtifacts(g.cfg.ArtifactsDir) if err != nil { return err } + finalRoot, err := filepath.EvalSymlinks(g.cfg.ArtifactsDir) + if err != nil { + return fmt.Errorf("failed to eval symlinks for artifacts dir: %w", err) + } + absRoot, err := filepath.Abs(finalRoot) + if err != nil { + return fmt.Errorf("failed to get absolute root path: %w", err) + } + if !strings.HasSuffix(absRoot, string(filepath.Separator)) { + absRoot += string(filepath.Separator) + } + for _, path := range artifactPaths { + finalPath, err := filepath.EvalSymlinks(path) + if err != nil { + return fmt.Errorf("failed to eval symlinks for %s: %w", path, err) + } + absPath, err := filepath.Abs(finalPath) + if err != nil { + return fmt.Errorf("failed to get absolute path for %s: %w", finalPath, err) + } + if !strings.HasPrefix(absPath, absRoot) { + return fmt.Errorf("artifact path escapes root directory: %s", absPath) + } + ext := strings.ToLower(filepath.Ext(path)) - data, err := os.ReadFile(path) + data, err := os.ReadFile(absPath) if err != nil { - return fmt.Errorf("failed to read artifact %s: %w", path, err) + return fmt.Errorf("failed to read artifact %s: %w", absPath, 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 `@gateway/gateway-controller/pkg/immutable/loader.go` around lines 93 - 103, Before os.ReadFile in the artifact loop, resolve symlinks for both g.cfg.ArtifactsDir and each path, then convert them to absolute paths and verify the file path is strictly contained within the resolved artifacts root without relying on string-prefix matching. Reject and return an error for paths outside the root, and read only the validated resolved path while preserving the existing artifact processing in collectArtifacts.Source: Coding guidelines
🤖 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 `@gateway/gateway-controller/pkg/immutable/loader.go`:
- Line 228: Update collectArtifacts to append only files with YAML or JSON
extensions, excluding README.md and other non-artifact files before
LoadArtifacts processes them. Apply the extension check immediately before the
paths append operation and preserve recursive artifact discovery for supported
files.
---
Outside diff comments:
In `@gateway/gateway-controller/pkg/immutable/loader.go`:
- Around line 93-103: Before os.ReadFile in the artifact loop, resolve symlinks
for both g.cfg.ArtifactsDir and each path, then convert them to absolute paths
and verify the file path is strictly contained within the resolved artifacts
root without relying on string-prefix matching. Reject and return an error for
paths outside the root, and read only the validated resolved path while
preserving the existing artifact processing in collectArtifacts.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3eb51671-3cf7-4957-ab92-665ca3fda846
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/immutable/loader.gogateway/gateway-controller/pkg/immutable/loader_test.go
cd80ffb to
dab6e7a
Compare
Replace filepath.WalkDir with os.ReadDir + os.Stat to handle Kubernetes ConfigMap atomic-writer mounts. Dot-prefixed entries (..data, ..TIMESTAMP) are skipped; os.Stat follows symlinks so user-visible entries resolve transparently. This matches the pattern used by the kubelet itself (filepath.Glob with [^.]*).
dab6e7a to
35f8b86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@gateway/gateway-controller/pkg/immutable/loader.go`:
- Around line 100-116: Enforce symlink-aware root containment in LoadArtifacts
and collectArtifacts: resolve g.cfg.ArtifactsDir to an absolute evaluated path,
then resolve each candidate path before any os.Stat, directory recursion, or
os.ReadFile call and require it to remain strictly under the root using a
trailing-separator prefix check. Reuse the resolved root throughout traversal
and return an error or skip the path consistently when containment fails, while
preserving existing artifact filtering and read-error behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ef5bffc9-837e-471a-8d62-e2ca3f382064
📒 Files selected for processing (2)
gateway/gateway-controller/pkg/immutable/loader.gogateway/gateway-controller/pkg/immutable/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- gateway/gateway-controller/pkg/immutable/loader_test.go
|
This PR fixes #2719 |
4 participants
Purpose
Kubernetes ConfigMap volume mounts create
..dataand..TIMESTAMPsymlink directories inside the mount path. The immutable gateway artifact walker descended into these, processing each artifact twice and producing a conflict error that prevented the controller from starting.Goals
Ensure
LoadArtifactsdiscovers each artifact file exactly once when the artifacts directory is a Kubernetes ConfigMap mount.Approach
collectArtifacts(dir string) ([]string, error)fromLoadArtifacts. A pure function that walks a directory and returns YAML/JSON file paths, skipping directories whose name starts with..viafs.SkipDir.collectArtifactsintoLoadArtifacts, replacing the inlinefilepath.WalkDircallback.The core check:
User stories
As an operator mounting a ConfigMap volume at the artifacts directory, I want the immutable gateway loader to ignore Kubernetes' internal
..-prefixed directories so that each artifact is loaded exactly once and the controller starts successfully.Documentation
N/A. No user-facing configuration changes. The fix is transparent to operators.
Automation tests
TestCollectArtifacts_ConfigMapMountYieldsFileOnce: creates a real ConfigMap mount layout with symlinks, asserts the artifact is found exactly once via the top-level symlink.TestCollectArtifacts_DescendsIntoNonDotDotDirs: table-driven. Verifies nested subdirectories (rest-apis/) and single-dot directories (.hidden/) are not skipped.Security checks
Test environment