1743: preserve hardlinks during COPY --from - part 2

[mzihlmann]

Fixes GoogleContainerTools/kaniko#1743

Description

Part 1 (#630) fixed 2594, hardlinks broken by COPY --from=<image>. This second part covers COPY --from=<stage>, where kaniko first persists the source stage's files into its internal dependency dir and later extracts them into the dependent stage.

That persist step went through github.com/otiai10/copy, which copies every file on its own and knows nothing about hardlinks. Files sharing an inode were already independent copies before the dependent stage saw them, so FF_KANIKO_PRESERVE_HARDLINKS had nothing left to preserve. It now walks the tree once and links subsequent occurrences of an inode instead of duplicating content.

Capabilities were a narrower problem than hardlinks. The save path called CopyCapabilities afterwards, but only for the path named in the COPY, so a file carrying capabilities directly was fine while one sitting inside a copied directory lost them. Dockerfile_test_issue_cg73 covered the first shape already and now covers the second.

The same substitution covers the two other places the library was used, the cross-device fallback when a bind mount target is moved aside and the RUN --mount=type=bind source copy. Neither of those reapplied capabilities at all.

Gated behind FF_KANIKO_NATIVE_COPY=false. Becomes default in v1.29.0. Stacked on #948, which the save path needs so that a socket or device in a saved directory is skipped rather than read.

Summary by CodeRabbit

• New Features

  • Added documented feature flags to control special-file COPY handling and enable native copy behavior.
  • When enabled, RUN --mount=type=bind bind sources and multistage saved artifacts use native copy logic.

• Bug Fixes

  • COPY now properly recreates FIFOs (including metadata preservation) and improves symlink/hardlink handling across stages.
  • When configured, non-regular special files are skipped with warnings instead of failing the build.

• Tests

  • Added BusyBox Dockerfile reproductions to prevent FIFO-related hangs and verify symlink/hardlink/FIFO preservation.

[mzihlmann]

whether to FF gate this fix is debatable. the image will be different yes, but "more correct".

Then on the other hand spending a FF is also not too complicated and makes the issue come up neatly in the 1.28.0 release.

[mzihlmann]

AI summary review

Overview

Fixes a real, well-documented regression: CopyDir was creating independent inodes for every file, silently breaking hardlinks from the source stage and inflating image sizes (one reporter: 83 MB → 720 MB). The fix tracks inodes during the directory walk and calls os.Link for subsequent occurrences. Gated behind FF_KANIKO_PRESERVE_HARDLINKS.

---

Code Quality

Good:

• checkCopyHardlink is cleanly separated, well-commented, and mirrors the pattern of tar_util.go's checkHardlink.
• isHardlink guard correctly skips the timestamp-copy for hardlinks (which share an inode anyway).
• The else if chain placement is correct: after symlink check, before the regular file copy.
• Integration test (Dockerfile_test_issue_2594) covers both base and reversed ordering — important since the walk order determines which file becomes the "original".

Issues:

1. checkCopyHardlink runs even when flag is off (fs_util.go:746)

} else if linkDst, ok := checkCopyHardlink(fi, destPath, hardlinksSeen); ok && preserveHardlinks {

In Go, the initializer in else if always executes. When preserveHardlinks=false, checkCopyHardlink still runs and populates hardlinksSeen on every file unnecessarily. The guard should come first:

} else if preserveHardlinks {
    if linkDst, ok := checkCopyHardlink(fi, destPath, hardlinksSeen); ok {
        // link
    } else {
        // copy
    }
} else {
    // copy
}

Minor perf issue only when flag is off, but semantically confusing.

2. No unit test for checkCopyHardlink or CopyDir hardlink path (fs_util_test.go)

There's a filesAreHardlinks checker already defined (line 543) but no test that calls CopyDir with hardlinked files and asserts the result. The Dockerfile_test_issue_2594 integration test is good but slow/heavy. A unit test with os.Link + CopyDir + t.Setenv("FF_KANIKO_PRESERVE_HARDLINKS", "true") would be cheap and fast.

3. No EXDEV handling for cross-device os.Link (fs_util.go:749)

If src and dest are on different devices (possible in some kaniko overlay configurations), os.Link returns syscall.EXDEV. The error is currently propagated as fatal. A fallback to CopyFile on EXDEV would make the feature more robust, similar to MoveDir's EXDEV handling at line 800.

4. checkCopyHardlink diverges slightly from checkHardlink (tar_util.go:202)

checkHardlink has the guard original != p to avoid self-linking. checkCopyHardlink omits this — correct here since dest is the destination path (not the source), so self-collision can't happen. But worth a comment noting the intentional omission so future readers don't "fix" it.

---

Minor / Nit

• config.EnvBool("FF_KANIKO_PRESERVE_HARDLINKS") is read once per CopyDir call (not per file), which is correct and efficient.
• The README entry is clear and follows the existing pattern for feature flags exactly.
• The log at line 748 says "Creating hardlink %s -> %s", destPath, linkDst which is backwards from ln's convention (existing → new). Consider logrus.Tracef("Creating hardlink %s -> %s", linkDst, destPath) for consistency.

---

Summary

The core logic is correct and the approach is sound.

Priority	Issue
Low	checkCopyHardlink called unconditionally even when flag is off — restructure the else if
Low	No unit test for the new code path
Low	No EXDEV fallback in os.Link
Nit	Log message argument order is inverted

[codecov]

Codecov Report

❌ Patch coverage is 40.65934% with 54 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/util/fs_util.go	45.00%	20 Missing and 13 partials ⚠️
pkg/commands/run.go	18.18%	8 Missing and 1 partial ⚠️
pkg/executor/build.go	22.22%	6 Missing and 1 partial ⚠️
pkg/commands/copy.go	44.44%	3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 39f8b487-5cf4-4bbe-95c7-82003edb9f0b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds feature flags for skipping special files and using native copy operations. Copy utilities now recreate FIFOs, preserve hardlinks and symlinks, and support bulk tree/path copying across Dockerfile, bind-mount, multistage, integration, and unit-test paths.

Changes

Copy behavior and feature flags

Layer / File(s)	Summary
Feature flag contracts and integration wiring pkg/config/featureflags.go, integration/images.go, README.md	Adds, initializes, propagates, and documents both feature flags.
Filesystem copy primitives pkg/util/fs_util.go	Adds shared traversal, FIFO recreation, native tree/path copying, configurable ignore handling, and hardlink-preserving behavior.
COPY and native-copy entry points pkg/commands/copy.go, pkg/commands/run.go, pkg/executor/build.go	Handles FIFOs and special files in COPY, and selects native copying for bind mounts and inter-stage dependency files.
Filesystem copy validation integration/dockerfiles/*, pkg/executor/copy_multistage_test.go, pkg/util/fs_util_test.go	Validates FIFO metadata, symlink targets, hardlink identity, capability preservation, and updated helper signatures.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Dockerfile
  participant CopyCommand
  participant CopyTree
  participant copyDirInner
  participant Destination
  Dockerfile->>CopyCommand: COPY source
  CopyCommand->>CopyTree: copy tree or paths
  CopyTree->>copyDirInner: process filesystem entries
  copyDirInner->>Destination: recreate FIFOs and preserve links
  Destination-->>CopyCommand: copied entries

Loading

Possibly related PRs

• osscontainertools/kaniko#808: Related to propagating KanikoEnv feature flags through integration executor invocations.
• osscontainertools/kaniko#854: Related to centralizing feature-flag resolution and registration.
• osscontainertools/kaniko#948: Directly overlaps FIFO and special-file handling in copy paths.

Suggested labels: bug, tests

Suggested reviewers: 0hlov3, babs, nejch

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The PR also adds FIFO/special-file handling, native-copy support in other paths, docs, and tests for issues #1599 and cg73 beyond #1743.	Split unrelated FIFO/native-copy/docs/test changes into separate PRs or link the corresponding issues if they are intended scope.
Docstring Coverage	⚠️ Warning	Docstring coverage is 52.94% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The hardlink-preservation fix, inode-reuse behavior, and multistage COPY test all match #1743's requirements.
Description check	✅ Passed	The description clearly explains the issue, implementation, feature flag, affected paths, tests, and planned default change, although checklist sections are omitted.
Title check	✅ Passed	The title clearly identifies the primary change: preserving hardlinks during stage-to-stage COPY operations.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 2595-hardlinks

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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/util/fs_util.go`:
- Line 1319: Update both native copy helper calls in pkg/util/fs_util.go at
lines 1319 and 917 to pass config.FF.CopySkipSpecialFiles instead of hard-coded
true for the skipSpecialFiles argument; preserve CreateFifo’s safe FIFO
handling.

In `@README.md`:
- Around line 1266-1270: Update the FF_KANIKO_PRESERVE_HARDLINKS documentation
to clarify that preserving hardlinks also requires FF_KANIKO_NATIVE_COPY=true
until NativeCopy becomes the default in v1.29.0. Keep the existing explanation
of the preserve-hardlinks default unchanged.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4f8b489f-00e0-4792-b6cf-2152d30b22ad

📥 Commits

Reviewing files that changed from the base of the PR and between 25146d0 and b791c66.

📒 Files selected for processing (11)

• README.md
• integration/dockerfiles/Dockerfile_test_issue_1599
• integration/dockerfiles/Dockerfile_test_issue_1743
• integration/images.go
• pkg/commands/copy.go
• pkg/commands/run.go
• pkg/config/featureflags.go
• pkg/executor/build.go
• pkg/executor/copy_multistage_test.go
• pkg/util/fs_util.go
• pkg/util/fs_util_test.go

[mzihlmann]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[mzihlmann]

Second one applied, the FF_KANIKO_PRESERVE_HARDLINKS section now says COPY --from=<stage> also needs FF_KANIKO_NATIVE_COPY=true until that becomes the default in v1.29.0, and that COPY --from=<image> needs only the one flag.

Declining the first one. The hard-coded true in CopyPaths and CopyTree is deliberate, and passing config.FF.CopySkipSpecialFiles there would introduce the regression it is meant to prevent.

Those two call sites replace github.com/otiai10/copy, and that library already skipped devices silently. switchboard (vendor/github.com/otiai10/copy/copy.go:45) returns early when info.Mode()&os.ModeDevice != 0 && !opt.Specials, and since the named return err is nil at that point and neither call site sets OnError, onError returns nil. Neither call site sets Specials either. So skipping a device on these paths is parity with the code being replaced, not new behavior, and it needs no flag.

If they passed the flag instead, then FF_KANIKO_NATIVE_COPY=1 with FF_KANIKO_COPY_SKIP_SPECIAL_FILES=0 would send a device to CopyFile, which opens it and reads it as a file. That is strictly worse than today.

The flag exists for regular COPY, where the current behavior is to read the device and there was no library skipping it, so that one genuinely changes what a successful build produces and is gated. CopyDir passes config.FF.CopySkipSpecialFiles for exactly that reason.

[mzihlmann]

Retracting my previous reply, the finding was right and it is now fixed.

My argument was that hard-coding true preserved otiai10's silent device skip on these paths. That is true as far as it goes, but it made FF_KANIKO_COPY_SKIP_SPECIAL_FILES mean different things depending on which copy path you were on, and the old behavior is already reachable a simpler way: turn off FF_KANIKO_NATIVE_COPY and you get the library back wholesale, device handling included. Two orthogonal flags beat one flag with a path-dependent meaning.

Since all three call sites now want the same value, the skipSpecialFiles parameter was redundant, so it is gone and copyDirInner reads config.FF.CopySkipSpecialFiles directly. That also shrinks this PR: it no longer touches the special-file branches at all, they stay exactly as #948 writes them.

The fifo case is unaffected either way. CreateFifo sits in its own branch above the skip and is unconditional, so FF_KANIKO_NATIVE_COPY=1 with FF_KANIKO_COPY_SKIP_SPECIAL_FILES=0 still recreates fifos rather than blocking on them.

Also reverted the FF_KANIKO_PRESERVE_HARDLINKS doc note from the other comment, that section stays as it was.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/util/fs_util.go (1)

836-841: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Replace an existing destination before creating the hardlink.

If destPath already exists, Line 839 fails with EEXIST. For example, copying hardlinked a/b onto a layer that already contains b copies a then fails on b; regular-file copying instead overwrites. Apply the same destination replacement semantics before os.Link.

🤖 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/util/fs_util.go` around lines 836 - 841, Update the hardlink branch in
checkCopyHardlink’s caller before os.Link(linkDst, destPath) to remove or
replace any existing destPath, matching regular-file copy overwrite semantics;
then create the hardlink and preserve the existing error propagation.

🤖 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 `@integration/dockerfiles/Dockerfile_test_issue_cg73`:
- Around line 24-27: Update the capability assertions after getcap in the
Dockerfile to match the complete expected value cap_net_raw=ep for both /blubb
and /dir/nested, rather than accepting any output containing cap_net_raw.
Preserve the existing checks that fail when capabilities are missing or have a
different state.

In `@pkg/commands/copy.go`:
- Around line 130-135: Update the FIFO branch in the copy flow to apply the same
c.fileContext.ExcludesFile(fullPath) and kConfig.KanikoDir destination guards
used by CopySymlink and CopyFile before calling util.CreateFifo. Preserve the
existing exclusion behavior and reject protected Kaniko paths without removing
or recreating them.

In `@pkg/util/fs_util.go`:
- Line 779: Update hardlink tracking around hardlinksSeen and checkCopyHardlink
to key entries by both device and inode rather than inode alone. Introduce a
comparable key containing the file’s device and inode values, and use it
consistently for lookup and insertion so identical inode numbers on different
filesystems remain distinct.

---

Outside diff comments:
In `@pkg/util/fs_util.go`:
- Around line 836-841: Update the hardlink branch in checkCopyHardlink’s caller
before os.Link(linkDst, destPath) to remove or replace any existing destPath,
matching regular-file copy overwrite semantics; then create the hardlink and
preserve the existing error propagation.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 5270ec76-78e5-4100-a732-3679f114a167

📥 Commits

Reviewing files that changed from the base of the PR and between 4b0a85e and e5342c5.

📒 Files selected for processing (12)

• README.md
• integration/dockerfiles/Dockerfile_test_issue_1599
• integration/dockerfiles/Dockerfile_test_issue_1743
• integration/dockerfiles/Dockerfile_test_issue_cg73
• integration/images.go
• pkg/commands/copy.go
• pkg/commands/run.go
• pkg/config/featureflags.go
• pkg/executor/build.go
• pkg/executor/copy_multistage_test.go
• pkg/util/fs_util.go
• pkg/util/fs_util_test.go

🚧 Files skipped from review as they are similar to previous changes (9)

• integration/dockerfiles/Dockerfile_test_issue_1599
• integration/dockerfiles/Dockerfile_test_issue_1743
• README.md
• pkg/executor/copy_multistage_test.go
• pkg/config/featureflags.go
• pkg/executor/build.go
• pkg/commands/run.go
• pkg/util/fs_util_test.go
• integration/images.go

[coderabbitai]

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 `@pkg/executor/build.go`:
- Around line 1354-1357: Update the CopyPaths/copyDirInner hardlink handling so
an os.Link failure caused by EXDEV falls back to copying the file contents
normally, while preserving hardlinks when linking succeeds and retaining
existing errors for other failures. Ensure the NativeCopy flow continues to
report only unrecoverable copy errors.

🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2298364a-9237-4f1f-9ee5-eb773db4226f

📥 Commits

Reviewing files that changed from the base of the PR and between e5342c5 and 994aade.

📒 Files selected for processing (11)

• README.md
• integration/dockerfiles/Dockerfile_test_issue_1743
• integration/dockerfiles/Dockerfile_test_issue_cg73
• integration/images.go
• pkg/commands/copy.go
• pkg/commands/run.go
• pkg/config/featureflags.go
• pkg/executor/build.go
• pkg/executor/copy_multistage_test.go
• pkg/util/fs_util.go
• pkg/util/fs_util_test.go

🚧 Files skipped from review as they are similar to previous changes (8)

• pkg/executor/copy_multistage_test.go
• integration/dockerfiles/Dockerfile_test_issue_1743
• pkg/config/featureflags.go
• pkg/util/fs_util_test.go
• README.md
• pkg/commands/run.go
• pkg/commands/copy.go
• pkg/util/fs_util.go

[mzihlmann]

need to verify whether there is a performance impact of switching away from otiai10, i would assume positive