Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Create intermediate directories in COPY with correct uid and gid #2795

Merged
merged 4 commits into from
Nov 28, 2023
Merged

Create intermediate directories in COPY with correct uid and gid #2795

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e3f479
Create directories with the right UID/GID
rhaps0dy Oct 13, 2023
5c268fa
Forgot to create the actual directory
rhaps0dy Oct 13, 2023
6bc96ae
Integration test creation of intermediate files with correct ownership
rhaps0dy Nov 12, 2023
38747c1
ADD version of the test
rhaps0dy Nov 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions integration/dockerfiles/Dockerfile_test_add_chown_intermediate_dirs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
FROM alpine@sha256:5ce5f501c457015c4b91f91a15ac69157d9b06f1a75cf9107bf2b62e0843983a
# Create dev user and group, with id 1001
RUN yes | adduser -u 1001 dev

ADD --chown=dev:dev context/foo /path/to/foo
ADD --chown=dev:dev context/qux /path/to/qux
ADD --chown=1001:1001 context/foo /path2/to/foo
ADD --chown=1001:1001 context/qux /path2/to/qux

USER dev

# `mkdir` fails when `dev` does not own all of `/path{,2}/to{,/qux}`
RUN mkdir /path/to/new_dir
RUN mkdir /path/to/qux/new_dir
RUN mkdir /path2/to/new_dir
RUN mkdir /path2/to/qux/new_dir
16 changes: 16 additions & 0 deletions integration/dockerfiles/Dockerfile_test_copy_chown_intermediate_dirs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
FROM alpine@sha256:5ce5f501c457015c4b91f91a15ac69157d9b06f1a75cf9107bf2b62e0843983a
# Create dev user and group, with id 1001
RUN yes | adduser -u 1001 dev

COPY --chown=dev:dev context/foo /path/to/foo
COPY --chown=dev:dev context/qux /path/to/qux
COPY --chown=1001:1001 context/foo /path2/to/foo
COPY --chown=1001:1001 context/qux /path2/to/qux

USER dev

# `mkdir` fails when `dev` does not own all of `/path{,2}/to{,/qux}`
RUN mkdir /path/to/new_dir
RUN mkdir /path/to/qux/new_dir
RUN mkdir /path2/to/new_dir
RUN mkdir /path2/to/qux/new_dir
9 changes: 9 additions & 0 deletions pkg/commands/copy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ var copyTests = []struct {
sourcesAndDest: []string{"f[o][osp]", "tempCopyExecuteTest"},
expectedDest: []string{"tempCopyExecuteTest"},
},
{
name: "Copy into several to-be-created directories",
sourcesAndDest: []string{"f[o][osp]", "tempCopyExecuteTest/foo/bar"},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand that you included in the PR message about adding the test but it looks like this is not adding coverage for the CreateFile case with UID/GID :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

What might be valuable here is to add an integration test at https://github.com/GoogleContainerTools/kaniko/tree/main/integration/dockerfiles, with COPY --chown= ... and it would be very helpful so that we could verify the fix when comparing in the future release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right, it doesn't set UID/GID. I didn't know how to add that test :( I think I added this one to quickly be able to figure out how to create intermediate directories in the first place.

I'll add an integration test.

expectedDest: []string{"bar"},
},
}

func setupTestTemp(t *testing.T) string {
Expand Down Expand Up @@ -309,6 +314,10 @@ func TestCopyExecuteCmd(t *testing.T) {
if err != nil {
t.Error()
}
if fstat == nil {
t.Error()
return // Unrecoverable, will segfault in the next line
}
if fstat.IsDir() {
files, err := ioutil.ReadDir(dest)
if err != nil {
Expand Down
40 changes: 34 additions & 6 deletions pkg/util/fs_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,7 +578,7 @@ func resetFileOwnershipIfNotMatching(path string, newUID, newGID uint32) error {
// CreateFile creates a file at path and copies over contents from the reader
func CreateFile(path string, reader io.Reader, perm os.FileMode, uid uint32, gid uint32) error {
// Create directory path if it doesn't exist
if err := createParentDirectory(path); err != nil {
if err := createParentDirectory(path, int(uid), int(gid)); err != nil {
return errors.Wrap(err, "creating parent dir")
}

Expand Down Expand Up @@ -707,7 +707,7 @@ func CopySymlink(src, dest string, context FileContext) (bool, error) {
return false, err
}
}
if err := createParentDirectory(dest); err != nil {
if err := createParentDirectory(dest, DoNotChangeUID, DoNotChangeGID); err != nil {
return false, err
}
link, err := os.Readlink(src)
Expand Down Expand Up @@ -951,7 +951,7 @@ func CopyFileOrSymlink(src string, destDir string, root string) error {
if err != nil {
return errors.Wrap(err, "copying file or symlink")
}
if err := createParentDirectory(destFile); err != nil {
if err := createParentDirectory(destFile, DoNotChangeUID, DoNotChangeGID); err != nil {
return err
}
return os.Symlink(link, destFile)
Expand Down Expand Up @@ -1015,12 +1015,40 @@ func CopyOwnership(src string, destDir string, root string) error {
})
}

func createParentDirectory(path string) error {
func createParentDirectory(path string, uid int, gid int) error {
baseDir := filepath.Dir(path)
if info, err := os.Lstat(baseDir); os.IsNotExist(err) {
logrus.Tracef("BaseDir %s for file %s does not exist. Creating.", baseDir, path)
if err := os.MkdirAll(baseDir, 0755); err != nil {
return err

dir := baseDir
dirs := []string{baseDir}
for {
if dir == "/" || dir == "." || dir == "" {
break
}
dir = filepath.Dir(dir)
dirs = append(dirs, dir)
}

for i := len(dirs) - 1; i >= 0; i-- {
dir := dirs[i]

if _, err := os.Lstat(dir); os.IsNotExist(err) {
os.Mkdir(dir, 0755)
if uid != DoNotChangeUID {
if gid != DoNotChangeGID {
os.Chown(dir, uid, gid)
} else {
return errors.New(fmt.Sprintf("UID=%d but GID=-1, i.e. it is not set for %s", uid, dir))
}
} else {
if gid != DoNotChangeGID {
return errors.New(fmt.Sprintf("GID=%d but UID=-1, i.e. it is not set for %s", gid, dir))
}
}
} else if err != nil {
return err
}
}
} else if IsSymlink(info) {
logrus.Infof("Destination cannot be a symlink %v", baseDir)
Expand Down
Loading