Skip to content

1599: recreate fifos and skip other special files during COPY - #948

Open
mzihlmann wants to merge 5 commits into
mainfrom
mz2594-copy-special-files
Open

1599: recreate fifos and skip other special files during COPY#948
mzihlmann wants to merge 5 commits into
mainfrom
mz2594-copy-special-files

Conversation

@mzihlmann

@mzihlmann mzihlmann commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Fixes GoogleContainerTools/kaniko#1599

Description

COPY copies a file by opening it and reading it, which is wrong for anything that is not a regular file.

Opening a fifo blocks until something else opens the other end, so a Dockerfile that copies a directory containing one hangs with no error, no timeout and no indication of which file is responsible. That is fixed unflagged: a build that never terminates is not behavior anyone can have depended on, which is the exception in docs/releases.md. Fifos are now recreated with mkfifo, the same thing docker does and the same thing the otiai10/copy library already did for the paths that use it.

Sockets and devices are gated behind FF_KANIKO_COPY_SKIP_SPECIAL_FILES=false instead. A socket currently fails the build with ENXIO, but a device opens fine and gets read as if it were a file, so its contents end up in the image in place of the device node. That build succeeds today and produces an image, so it gets a flag. Becomes default in v1.29.0.

Summary by CodeRabbit

  • New Features

    • Improved COPY handling for named pipes (FIFOs), including permissions and ownership.
    • Added FF_KANIKO_COPY_SKIP_SPECIAL_FILES to warn and skip sockets and device files.
  • Bug Fixes

    • Prevented FIFO-related COPY operations from hanging.
    • Preserved safety for protected destination paths.
  • Documentation

    • Documented the feature flag, default behavior, and planned default change.
  • Tests

    • Added FIFO-focused Dockerfile coverage and updated related validation.

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d2f05528-0efd-481d-abd2-46da94f21204

📥 Commits

Reviewing files that changed from the base of the PR and between 5708427 and 2041b1c.

📒 Files selected for processing (7)
  • README.md
  • integration/dockerfiles/Dockerfile_test_issue_1599
  • integration/dockerfiles/Dockerfile_test_issue_1599_2
  • integration/images.go
  • pkg/commands/copy.go
  • pkg/config/featureflags.go
  • pkg/util/fs_util.go
🚧 Files skipped from review as they are similar to previous changes (6)
  • pkg/config/featureflags.go
  • pkg/commands/copy.go
  • README.md
  • integration/dockerfiles/Dockerfile_test_issue_1599
  • integration/images.go
  • integration/dockerfiles/Dockerfile_test_issue_1599_2

📝 Walkthrough

Walkthrough

Adds the FF_KANIKO_COPY_SKIP_SPECIAL_FILES flag, recreates FIFOs during COPY, skips other special files when enabled, and adds integration scenarios for FIFO copying and /kaniko path protection.

Changes

Special-file copying

Layer / File(s) Summary
Feature flag contract and propagation
pkg/config/featureflags.go, README.md, integration/images.go
Registers and documents the disabled-by-default flag, then enables it in integration executor environments.
FIFO recreation and COPY handling
pkg/commands/copy.go, pkg/util/fs_util.go
Adds FIFO detection and recreation with exclusion, ownership, and permission handling. Other special files can be skipped with a warning. Filesystem wrappers expose metadata and link operations.
Integration regression coverage
integration/dockerfiles/Dockerfile_test_issue_1599*, integration/images.go
Adds FIFO build scenarios and validates FIFO attributes, regular-file copying, layer comparisons, and expected warnings.

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

Possibly related PRs

Suggested labels: tests

Suggested reviewers: babs, bobdu, nejch

Sequence Diagram(s)

sequenceDiagram
  participant IntegrationDockerfile
  participant KanikoExecutor
  participant CopyCommand
  participant CreateFifo
  participant Filesystem
  IntegrationDockerfile->>KanikoExecutor: Build stage containing FIFO
  KanikoExecutor->>CopyCommand: Execute COPY
  CopyCommand->>CreateFifo: Recreate FIFO destination
  CreateFifo->>Filesystem: Create FIFO and apply mode/ownership
  KanikoExecutor->>IntegrationDockerfile: Complete build and run assertions
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: FIFO recreation and skipping other special files during COPY.
Description check ✅ Passed The description links issue #1599 and explains the FIFO fix, special-file flag behavior, and release default.
Linked Issues check ✅ Passed The changes address issue #1599 by preventing FIFO hangs and handling special files during multi-stage COPY operations.
Out of Scope Changes check ✅ Passed The code, tests, integration fixtures, feature flag, and documentation changes support the linked issue and stated objectives.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch mz2594-copy-special-files

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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/util/fs_util.go`:
- Around line 836-839: Update the FIFO handling in CopyDir and the createFifo
function to accept and propagate the resolved uid, gid, chmod, and
useDefaultChmod values. Apply the same target ownership and effective mode logic
used for regular files and directories, so COPY --chown and explicit COPY
--chmod override source metadata for FIFOs.
🪄 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: 8615f711-686a-45ae-8c45-1107f1205035

📥 Commits

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

📒 Files selected for processing (5)
  • README.md
  • integration/dockerfiles/Dockerfile_test_copy_special_files
  • integration/images.go
  • pkg/config/featureflags.go
  • pkg/util/fs_util.go

Comment thread pkg/util/fs_util.go Outdated
@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from 2adeda7 to 06fe152 Compare July 25, 2026 21:08
@mzihlmann mzihlmann changed the title handle fifos and other special files during COPY recreate fifos and skip other special files during COPY Jul 25, 2026
@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from 06fe152 to 99fb216 Compare July 25, 2026 21:17
@mzihlmann mzihlmann changed the title recreate fifos and skip other special files during COPY 1599: recreate fifos and skip other special files during COPY Jul 25, 2026
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 42.85714% with 36 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pkg/util/fs_util.go 40.38% 21 Missing and 10 partials ⚠️
pkg/commands/copy.go 50.00% 3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from 99fb216 to d623def Compare July 25, 2026 21:54
@mzihlmann

Copy link
Copy Markdown
Collaborator Author

Good catch, fixed. createFifo is now CreateFifo(dest, fi, uid, gid, chmod, useDefaultChmod) and applies the same ownership and mode logic as regular files, so COPY --chown and COPY --chmod reach fifos.

Two things fell out of chasing it:

mkfifo(2) applies the umask, so the requested mode was being silently masked even before --chmod entered the picture. A fifo copied with mode 640 under the default 022 umask landed as 620. There is now an explicit os.Chmod after unix.Mkfifo, verified against a 022 umask.

The single-file COPY path in pkg/commands/copy.go had the same hang and was not covered by the original fix. COPY --from=builder /work/pipe /dest dispatches there rather than through CopyDir, so it still went to CopyFile and blocked on open(2). Same two branches added there.

Dockerfile_test_issue_1599 now covers all three: fifo inside a copied directory, fifo named directly as the source, and --chown/--chmod applied to a fifo.

@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch 3 times, most recently from 3c154b6 to 444c3e6 Compare July 25, 2026 22:18
@mzihlmann
mzihlmann requested review from 0hlov3, BobDu, babs and nejch July 25, 2026 22:50
@mzihlmann mzihlmann added the bug Something isn't working label Jul 26, 2026
Comment thread pkg/commands/copy.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/config/featureflags.go`:
- Line 103: Change CopySkipSpecialFiles in pkg/config/featureflags.go to default
to true, and remove the forced =1 override in integration/images.go so
integration runs validate the production default; retain explicit =false only
for compatibility coverage.

In `@pkg/util/fs_util.go`:
- Around line 836-840: Update the FIFO branch in the surrounding copy logic to
capture CreateFifo’s exclusion result and immediately continue when it indicates
the destination was excluded. Only queue timestamp handling and record the path
as copied when CreateFifo reports it was processed, matching the direct-file
COPY path 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c2b6ef95-b280-4000-9f00-fd9e494e31da

📥 Commits

Reviewing files that changed from the base of the PR and between 2adeda7 and be99c91.

📒 Files selected for processing (7)
  • README.md
  • integration/dockerfiles/Dockerfile_test_issue_1599
  • integration/dockerfiles/Dockerfile_test_issue_1599_2
  • integration/images.go
  • pkg/commands/copy.go
  • pkg/config/featureflags.go
  • pkg/util/fs_util.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • README.md

Comment thread pkg/config/featureflags.go
Comment thread pkg/util/fs_util.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/util/fs_util.go`:
- Line 40: In the filesystem copy loop, handle an exclude result from
CopySymlink, CreateFifo, or CopyFile by continuing before timestamp and
copied-file bookkeeping; then remove the assert.Assert call and the pkg/assert
import. Ensure protected /kaniko destinations are skipped without panicking.
🪄 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: ceed9566-1118-45e8-9aba-ba3abd047b16

📥 Commits

Reviewing files that changed from the base of the PR and between f04937a and b2a40dd.

📒 Files selected for processing (1)
  • pkg/util/fs_util.go

Comment thread pkg/util/fs_util.go
@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from b2a40dd to a48fecf Compare August 2, 2026 09:26
@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from a48fecf to 2041b1c Compare August 12, 2026 07:07
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@mzihlmann
mzihlmann force-pushed the mz2594-copy-special-files branch from 2041b1c to f126cb8 Compare August 12, 2026 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multi-stage build hangs when copying special file (mknod)

2 participants