Skip to content
This repository was archived by the owner on Jun 3, 2025. It is now read-only.

Don't write whiteout files to directories that were replaced with files or links#2590

Merged
aaron-prindle merged 1 commit into
GoogleContainerTools:mainfrom
andreasf:fix_2308_files_or_links
Jun 22, 2023
Merged

Don't write whiteout files to directories that were replaced with files or links#2590
aaron-prindle merged 1 commit into
GoogleContainerTools:mainfrom
andreasf:fix_2308_files_or_links

Conversation

@andreasf

Copy link
Copy Markdown
Contributor

Fixes #2308

Description

If a non-empty directory gets replaced with something other than a directory (e.g. file or symlink), the files in that directory also get deleted. However, there should not be any whiteout files for them in the layer. If there were, the resulting tar file would not be extractable.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

@gabyx gabyx 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.

Looks good to me.
Some questions, and small typos I guess.

Comment thread pkg/snapshot/snapshot.go Outdated
Comment thread pkg/snapshot/snapshot.go
if err != nil {
return err
}
if skipWhiteout {

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.

nice!


for _, file := range kanikoFiles {
if strings.HasPrefix(file, "a/.wh.") {
t.Errorf("Last layer should not add whiteout files to deleted directory but found %s", file)

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.

Nice

Comment thread pkg/snapshot/snapshot_test.go
@aaron-prindle aaron-prindle modified the milestone: v1.12.0 Jun 20, 2023
@aaron-prindle

aaron-prindle commented Jun 20, 2023

Copy link
Copy Markdown
Collaborator

Agree w/ @gabyx, LGTM just address the single nit - s/err/nil and we should be able to merge! 🎊

@andreasf andreasf force-pushed the fix_2308_files_or_links branch from 2e765ba to 94fe941 Compare June 21, 2023 08:45
Comment thread integration/integration_test.go Outdated
var disableGcsAuth bool
flag.StringVar(&c.gcsBucket, "bucket", "gs://kaniko-test-bucket", "The gcs bucket argument to uploaded the tar-ed contents of the `integration` dir to.")
flag.StringVar(&c.imageRepo, "repo", "gcr.io/kaniko-test", "The (docker) image repo to build and push images to during the test. `gcloud` must be authenticated with this repo or serviceAccount must be set.")
flag.StringVar(&c.imageRepo, "repo", "localhost:5000", "The (docker) image repo to build and push images to during the test. `gcloud` must be authenticated with this repo or serviceAccount must be set.")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I missed this initially but can you also change this back to its original value

Comment thread integration/integration_test.go Outdated
flag.StringVar(&c.serviceAccount, "serviceAccount", "", "The path to the service account push images to GCR and upload/download files to GCS.")
flag.StringVar(&gcsEndpoint, "gcs-endpoint", "", "Custom endpoint for GCS. Used for local integration tests")
flag.BoolVar(&disableGcsAuth, "disable-gcs-auth", false, "Disable GCS Authentication. Used for local integration tests")
flag.BoolVar(&disableGcsAuth, "disable-gcs-auth", true, "Disable GCS Authentication. Used for local integration tests")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I missed this initially but can you also change this back to its original value

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oops. Fixed

…es or links

If a non-empty directory gets replaced with something other than a
directory (e.g. file or symlink), the files in that directory also get
deleted. However, there should not be any whiteout files for them in the
layer. If there were, the resulting tar file would not be extractable.

Fixes GoogleContainerTools#2308

@aaron-prindle aaron-prindle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the PR here @andreasf! LGTM

@aaron-prindle aaron-prindle merged commit 0d925dd into GoogleContainerTools:main Jun 22, 2023
kylecarbs pushed a commit to coder/kaniko that referenced this pull request Jul 12, 2023
…es or links (GoogleContainerTools#2590)

If a non-empty directory gets replaced with something other than a
directory (e.g. file or symlink), the files in that directory also get
deleted. However, there should not be any whiteout files for them in the
layer. If there were, the resulting tar file would not be extractable.

Fixes GoogleContainerTools#2308
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

when replacing a folder by symlink : i get corrupt images when pulling : failed to mknod("somefile", S_IFCHR, 0): no such file or directory

3 participants