fix: fix bazel-out path for rules_docker-0.23.0

[codersasha]

Fixes: #7228
Related: bazelbuild/rules_docker#1963
Merge before/after: Dependent or prerequisite PRs

Description

With the release of https://github.com/bazelbuild/rules_docker/releases/tag/v0.23.0, the output directory for bazel targets is calculated differently and is no longer a static path.

For example, a container_image target such as:

//foo/bar/server:server_img.tar

Previously was stored at:

bazel-out/k8-fastbuild/bin/foo/bar/server/server_img.tar

Internally, skaffold calculated this path by running bazel info bazel-bin, which returns the start of the path (e.g. /home/...../execroot/__main__/bazel-out/k8-fastbuild/bin), then appended the name of the rule with : replaced with / (e.g. foo/bar/server/server_img.tar). This results in the correct path as above.

However, with rules_docker-0.23.0, the output is now stored at a path like:

bazel-out/k8-fastbuild-ST-4a519fd6d3e4/bin/foo/bar/server/server_img.tar

So bazel info no longer returns the correct path prefix.

Using bazel cquery with a similar method to that described in https://github.com/bazelbuild/rules_pkg/tree/main/examples/where_is_my_output, as recommended in bazelbuild/rules_docker#1963 (comment), we can query Bazel for the actual output path to the target. Docker .tar output files are available in cquery as targets with a .files attribute, which contains the list of output files (there's only one), and each file has a .path attribute that gives the path to the file, which we can use internally in Skaffold. This is also backwards compatible with older versions of https://github.com/bazelbuild/rules_docker.

For example, the command:

bazel cquery //foo/bar/server:server_img.tar --output starlark --starlark:expr="target.files.to_list()[0].path"

May return:

bazel-out/k8-fastbuild/bin/foo/bar/server/server_img.tar

And with rules_docker-0.23.0, this may return something like:

bazel-out/k8-fastbuild-ST-4a519fd6d3e4/bin/foo/bar/server/server_img.tar

Which is the correct path to the target.

[MarlonGamez]

hi @sashamor , Thanks for opening this change! It looks like you'll have to go in and update TestBuildBazel unit test as well, but other than that these changes look good :)

[codersasha]

Thanks, fixed :) Also changed the arguments to no longer use nested quotes.

Tested locally with https://github.com/bazelbuild/rules_docker v0.23.0 and v0.22.0 and can confirm they both find the output tar correctly as expected.

[Anthony-Bible]

I wanted to add another data point, I had a similar problem and it worked when running with your patch with multiple images.

[codecov]

Codecov Report

Merging #7251 (1bab727) into main (290280e) will decrease coverage by 1.98%.
The diff coverage is 56.96%.

@@            Coverage Diff             @@
##             main    #7251      +/-   ##
==========================================
- Coverage   70.48%   68.50%   -1.99%     
==========================================
  Files         515      560      +45     
  Lines       23150    26507    +3357     
==========================================
+ Hits        16317    18158    +1841     
- Misses       5776     7095    +1319     
- Partials     1057     1254     +197     

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/deploy.go	52.00% <ø> (-1.85%)	⬇️
cmd/skaffold/app/cmd/dev.go	84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go	36.66% <0.00%> (-4.72%)	⬇️
cmd/skaffold/skaffold.go	0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go	62.50% <14.28%> (-1.14%)	⬇️
cmd/skaffold/app/cmd/lsp.go	28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go	68.85% <40.00%> (-7.62%)	⬇️
cmd/skaffold/app/cmd/lint.go	42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go	48.88% <50.00%> (+0.24%)	⬆️
cmd/skaffold/app/skaffold.go	76.19% <70.00%> (-8.43%)	⬇️
... and 224 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

[MarlonGamez]

@sashamor , thanks for making the change! I have a fix open for the github action failure we're seeing

Once #7263 is merged, could you rebase this PR to retrigger the tests? I think it should be fixed then

[codersasha]

Thanks for rebasing and merging :) Woohoo

[morozov]

It looks like this breaks the scenarios where the artifact has a non-default context and the skaffold command is executed from within a path different than the repository root.

The bazel cquery runs in the directory corresponding to the context and returns the tar path relative to that context but docker.Push() still runs in the current working directory and is now supplied with a path relative to the context, not relative to the current working directory.

Take the following example. There is a skaffold file in projects/acme/some-projects:

apiVersion: skaffold/v2beta26
kind: Config
build:
  artifacts:
  - image: quay.io/acme/some-project
    context: ../../..

If one does:

cd projects/acme/some-projects && skaffold build

It will no longer work. It will fail with an error like:

build [quay.io/acme/some-project] failed: reading image "bazel-out/darwin-fastbuild-ST-4a519fd6d3e4/bin/projects/some-project/docker.tar": open bazel-out/darwin-fastbuild-ST-4a519fd6d3e4/bin/projects/some-project//docker.tar: no such file or directory

I guess this is the same issue as reported in #7281.