fix(executor)!: give input artifacts without mode a default of 0644. Fixes #14792 - #16620
fix(executor)!: give input artifacts without mode a default of 0644. Fixes #14792#16620anneheartrecord wants to merge 2 commits into
mode a default of 0644. Fixes #14792#16620Conversation
Artifact repositories generally cannot store file permissions, so an artifact only kept the permissions it was created with if it was archived: a tarball restores the modes recorded in its headers, while an artifact stored with `archive: none` arrives with whatever mode its driver created the files with. The same artifact therefore landed with different permissions depending on how the template that produced it chose to store it. Give an artifact that does not set `mode` a well known set of permissions instead: 0600 for its files and 0700 for its directories, which have to stay traversable. This covers the whole artifact, as an artifact stored as-is can be a directory too. Symlinks are skipped, since os.Chmod follows them and untar preserves the symlinks within an artifact, so following one could change the permissions of a file outside it. Signed-off-by: Charles Cheng <chengxisheng777@gmail.com>
…0700 An artifact-plugin sidecar reads a saved output through its own mount of the main container's filesystem, and is not guaranteed to run as the same user as the container that loaded the input artifact. A 0600 default made that read fail with permission denied, caught by TestArtifactsSuite/TestOutputOnInputPlugin. 0644/0755 stays readable across containers while still restricting writes to the owner. Also rewrites the mode-selection if/else-if/else as a switch to satisfy golangci-lint's gocritic ifElseChain check. Signed-off-by: Charles Cheng <chengxisheng777@gmail.com>
cb50bf3 to
f58ca22
Compare
|
The code and the description differ on what permissions are set on the artifacts. (0600 in the descripiton, but 0644 in the code). I prefer the more restrictive option, but understand there might be reasons why your AI didn't do that |
|
Good catch, thanks — the description was stale, not intentional. Original submission used |
📝 WalkthroughWalkthroughInput artifacts without explicit modes now use ChangesArtifact permission normalization
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
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 `@workflow/executor/executor_test.go`:
- Around line 494-501: Update the file fixture near chmodDefault and the
external-target fixture to be created with mode 0600 instead of their expected
final modes. Keep the standalone-file assertion verifying chmodDefault changes
it to 0644, and assert the external target remains 0600 after processing so
no-op and symlink-following implementations fail.
🪄 Autofix
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 Plus
Run ID: f6306741-8285-48e9-a29a-045005a14984
📒 Files selected for processing (4)
docs/upgrading.mddocs/walk-through/artifacts.mdworkflow/executor/executor.goworkflow/executor/executor_test.go
| artPath := filepath.Join(t.TempDir(), "myfile.txt") | ||
| require.NoError(t, os.WriteFile(artPath, []byte("test content\n"), 0o644)) | ||
|
|
||
| require.NoError(t, chmodDefault(artPath)) | ||
|
|
||
| info, err := os.Stat(artPath) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "-rw-r--r--", info.Mode().String()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use non-default modes in these test fixtures.
Line 495 starts the file at its expected final mode. A no-op implementation passes this test.
Line 528 starts the external target at its expected final mode. An implementation that follows the symlink and chmods the target also passes this test.
Set both fixtures to 0600. Assert that the standalone file changes to 0644 and that the external target remains 0600.
Proposed test adjustment
- require.NoError(t, os.WriteFile(artPath, []byte("test content\n"), 0o644))
+ require.NoError(t, os.WriteFile(artPath, []byte("test content\n"), 0o600))
...
- require.NoError(t, os.WriteFile(outside, []byte("not part of the artifact\n"), 0o644))
+ require.NoError(t, os.WriteFile(outside, []byte("not part of the artifact\n"), 0o600))
...
- assert.Equal(t, "-rw-r--r--", info.Mode().String())
+ assert.Equal(t, "-rw-------", info.Mode().String())Also applies to: 527-537
🤖 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 `@workflow/executor/executor_test.go` around lines 494 - 501, Update the file
fixture near chmodDefault and the external-target fixture to be created with
mode 0600 instead of their expected final modes. Keep the standalone-file
assertion verifying chmodDefault changes it to 0644, and assert the external
target remains 0600 after processing so no-op and symlink-following
implementations fail.
mode a default of 0600. Fixes #14792mode a default of 0644. Fixes #14792
|
@Joibel I fixed the title as well — it still read On the restrictive option: |
|
@Joibel any preference here? If the plugin-mount test constraint makes 0644 acceptable I'll leave it as is; if you'd rather keep 0600 for the ordinary load path with the plugin case scoped separately, I can rework it that way — small change either way. |
2 participants
Fixes #14792
Motivation
An artifact repository generally cannot store file permissions, so an input artifact only kept the permissions it was created with if it happened to be archived. A tarball restores the modes recorded in its headers, so a file saved as
0644came back as0644; the same file saved witharchive: none: {}came back as0600, because there is nowhere to record the mode. Some repositories (http, for example) have nowhere to record it at all.That made an artifact's permissions a property of the template that produced it rather than of the template consuming it, which is what the issue reports.
This takes @Joibel's suggestion from the issue: rather than restoring metadata from the archive, give an artifact that does not set
modea well known set of defaults.Modifications
chmodDefaultapplies0644to an artifact's files and0755to its directories, for artifacts that do not setmode.archive: nonecan be a directory too (loadS3Artifactfalls back toGetDirectory).recurseModeis deliberately not consulted — it selects how an explicitmodeis applied, and this is the default for its absence.0755rather than0644so that they can still be entered.os.Chmodfollows them anduntarpreserves the symlinks within an artifact, so chmoding one could change the permissions of a file outside the artifact. The existingchmodhelper does follow them, but it only runs whenmodeis set explicitly, and that seemed worth not inheriting into a default.modestill wins, and artifacts loaded by an artifact plugin keep their existing0666default. That branch already has a deliberate default, so there is no inconsistency there to fix — happy to fold it in if you would rather it were0644as well.The default started as the more restrictive
0600/0700(owner-only), butTestArtifactsSuite/TestOutputOnInputPlugincaught a real problem with that: an artifact-plugin sidecar reads a saved output through its own mount of the main container's filesystem, and isn't guaranteed to run as the same user as the container that loaded the input artifact. Owner-only permissions made that cross-container read fail with permission denied.0644/0755stays world-readable, which fixes that case, while still restricting writes to the owner.Verification
TestChmodDefaultcovers a single file (the reported case), a directory tree, a symlink pointing outside the artifact, and a dangling symlink as the artifact itself.TestArtifactsSuite/TestOutputOnInputPlugincovers the cross-container artifact-plugin read.go test ./workflow/executor/...,go vet ./workflow/executor/..., and the full E2E suite pass.Documentation
docs/upgrading.mddocuments this as a breaking change under v4.1, with the two cases most likely to need an explicitmode: a file that has to stay executable, and a container that needs to write to an artifact loaded by a different user. It also notes that when an artifact's path falls inside a volume you mounted yourself the artifact is loaded into that volume, so on apersistentVolumeClaimthe new permissions outlive the pod.docs/walk-through/artifacts.mdmentions the default in itsmodesection.AI
This PR was written with Claude — code, tests and docs — and reviewed with it before submitting. The details were checked against this repository rather than taken from the model: that
untarcreates real symlinks, thatarchive: nonecan produce a directory viaGetDirectory, the0755YAML octal convention used in the docs example, and (after the amend below) that an artifact-plugin sidecar can run as a different user than the container that loaded the artifact.Summary by CodeRabbit
Enhancements
0644and directories use0755.Documentation