moby / buildkit

concurrent, cache-efficient, and Dockerfile-agnostic builder toolkit
https://github.com/moby/moby/issues/34227
Apache License 2.0
7.83k stars 1.09k forks source link

fix: dot and empty paths normalized correctly for COPY to WORKDIR #5080

Closed profnandaa closed 2 days ago

profnandaa commented 1 week ago

In my previous fix on #4825, I had removed this line knowing that all that had been addressed in pahtRelativeToWorkingDir:

    if cfg.params.DestPath == "." // <-- this one
        || cfg.params.DestPath == ""
                || cfg.params.DestPath[len(cfg.params.DestPath)-1] == filepath.Separator {
        dest += string(filepath.Separator)
    }

However, I had overlooked the "." and "" scenarios. "". The "/"case is handled correctly in system.NormalizePath().

This change therefore undoes this, to make sure "." is transformed correctly to "./" during COPY operation, same to "" -> "./". This is important especially for WORKDIR that are not /, so that COPY --link operations are handled properly.

fixes #5070

profnandaa commented 1 week ago

While we're here, ~I was torn between adding this case to NormalizePath or reverting to how it was initially at func dispatchCopy~. Also wanted to understand the intention for this block: @gabriel-samfira

https://github.com/moby/buildkit/blob/997156aacc32eb2856256e5088e1bd927484c35d/util/system/path.go#L77-L82

profnandaa commented 5 days ago

We need regression integration test for this as well. Is this something that should be handled by pathRelativeToWorkingDir (that part could be follow up).

Sure, it's better at pathRelativeToWorkingDir, I've moved it and also fixed how "" dir's were being handled.

Added the integration tests. It was very interesting that the tests were passing even without the fix, if you have / as the WORKDIR; and was getting a little thrown off. However, fixed with: WORKDIR /var/www and could repro the regression correctly.

profnandaa commented 3 days ago

@tonistiigi - PTAL again.