feat: Prefer local image #381

pmengelbert · 2023-10-17T19:36:52Z

Use gateway client for config resolution

Implement buildkit.ExtractFileFromState
This code will still not compile, since the signature of buildkit.SolveToLocal is not satisfied by its calls.
Replace SolveToLocal in probeDPKGStatus method
In this case, we will write the status type to a file, which we thereafter read from the state as a single byte.
Fix error in awk script
Use ExtractFileFromState for dpkg installUpdates
Use ExtractFileFromState in unpackAndMergeUpdates
APK: Use ExtractFileFromState for upgradePackages
Do not use SolveToLocal.
Use ExtractFileFromState for probeRPMStatus
Replace buildkit.SolveToLocal in rpm.go
Update patchWithContext to pass the correct client
Fix errors and achieve parity with main branch
Update unit tests

Closes #177

sozercan · 2023-10-17T19:42:11Z

@pmengelbert can we add an integration test that validates this?

codecov · 2023-10-17T19:50:11Z

Codecov Report

Attention: 238 lines in your changes are missing coverage. Please review.

Comparison is base (2b9f177) 33.02% compared to head (7037c84) 32.59%.
Report is 1 commits behind head on main.

Files	Patch %	Lines
pkg/patch/patch.go	0.00%	93 Missing ⚠️
pkg/pkgmgr/rpm.go	35.16%	58 Missing and 1 partial ⚠️
pkg/pkgmgr/dpkg.go	14.75%	49 Missing and 3 partials ⚠️
pkg/buildkit/buildkit.go	18.75%	26 Missing ⚠️
pkg/pkgmgr/apk.go	46.66%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #381      +/-   ##
==========================================
- Coverage   33.02%   32.59%   -0.44%     
==========================================
  Files          17       17              
  Lines        1626     1617       -9     
==========================================
- Hits          537      527      -10     
+ Misses       1060     1058       -2     
- Partials       29       32       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpuguy83 · 2023-10-17T20:01:15Z

pkg/pkgmgr/dpkg.go

-	probed := mkFolders.Run(llb.Shlex(probeCmd)).Root()
+	probed := mkFolders.
+		Run(llb.Args([]string{
+			`/bin/busybox`, `env`,


You can get rid of env and use llb.AddEnv.

cpuguy83 · 2023-10-17T20:04:54Z

pkg/pkgmgr/dpkg.go

 	outState := llb.Diff(busyBoxApplied, probed)
-	if err := buildkit.SolveToLocal(ctx, dm.config.Client, &outState, dm.workingFolder); err != nil {
+	typeBytes, err := buildkit.ExtractFileFromState(ctx, dm.config.Client, &outState, filepath.Join(resultsPath, statusdOutputFilename))


This diff can be removed and instead do

Run( // run args ). AddMount(resultsPath, llb.Scratch())

The return value of AddMount is the content of the mounted directory after the command has run.

I am not sure why this is, but changing to using .AddMount results in the buildkit.WithArrayFile calls not working. Those files just aren't in the container when the command is run. Any ideas why that might be?

(this only applies to the corresponding spot in rpm.go)

Note that its the contents of the mount only.
So if some file foo that gets written to resultsPath/foo, it will be in /foo for the result of .AddMount.

The issue I was having was because the files I was using in llb.MkFile were in the resultsPath directory. The Mount was clobbering that directory. I'm not entirely clear on the order of operation here, but it seems like adding AddMount at the end.

I worked around the issue by instead using a separate inputPath for the call to llb.MkFile. However, I'm not clear on the order of operations here, or on the syntax. Is the syntax AddMount(dst, src), returning src post-modification? In this case, "mount the (empty) filesystem from a scratch container into the resultsPath directory, and return that scratch container after it has been modified by the run command"? If that's what's happening, then it's clear why the directory was being clobbered.

It is the result of changing what is at the mount point.
If files are being written to that location first then yes they are being bind-mounted over with llb.Scratch.
Instead you can write the files to a different state (resultsBase:= llb.Scratch().File(llb.Mkfile(...)), then use resultsBase in the AddMount instead of llb.Scratch).

Did this not get pushed?

Did this not get pushed?

Fixed

cpuguy83 · 2023-10-18T00:38:04Z

pkg/pkgmgr/rpm.go

+	probed = buildkit.WithArrayFile(&probed, dbListPath, rpmDBList)
+	probed = probed.Run(llb.Args([]string{
+		`/usr/sbin/busybox`, `env`,
+		buildkit.Env("TOOL_LIST_PATH", toolListPath),


Same comment about env

IIRC, the reason I did it this way was that llb.AddEnv will result in env vars that are persisted into the image & included in the container at runtime -- is that correct?

In any case, it doesn't matter because these containers are discarded, so I'll switch to llb.AddEnv here.

It all depends on what you are returning.
Each state modification leaves the last state alone.
You can also add it to just the run step.

You can also add it to just the run step.

Ah, I missed that that was an option. That's what I was looking for!

cpuguy83 · 2023-10-18T00:39:16Z

pkg/pkgmgr/rpm.go

+	probed := buildkit.WithArrayFile(&mkFolders, toolListPath, toolList)
+	probed = buildkit.WithArrayFile(&probed, dbListPath, rpmDBList)
+	probed = probed.Run(llb.Args([]string{
+		`/usr/sbin/busybox`, `env`,


Also not sure I understand what is going on with the busybox exec here.
Should just be able to execute sh.

One would hope, but in practice busybox env just refused to execute sh. But since we are removing the env above, this will not be a problem.

sozercan · 2023-10-18T23:19:48Z

pkg/pkgmgr/rpm.go

-	outState := llb.Diff(toolsApplied, probed)
-	if err := buildkit.SolveToLocal(ctx, rm.config.Client, &outState, rm.workingFolder); err != nil {
-		return err
+	// probed := mkFolders.Run(llb.Shlex(probeToolsCmd)).Run(llb.Shlex(probeDBCmd)).Root()


do we need to remove this line?

sozercan · 2023-10-18T23:22:16Z

pkg/buildkit/buildkit.go

+	})
+}
+
+func ArrayFile(input []string) []byte {


should be easy to add a unit test for this?

sozercan · 2023-10-18T23:47:27Z

how do we want to handle the Error: repository name must be canonical error for non-registry prefixed images (i.e., local/ubuntu:bionic)? is this coming from buildkit?

do we expect all images to have a prefix? or will this PR address non-prefixed images?

sozercan · 2023-10-18T23:58:53Z

this seems to skip showing the commands being run, is that intentional?

previously:

$ copa patch ...
[+] Building 6.7s (9/9) FINISHED                                                                                                                                     
 => docker-image://docker.io/library/ubuntu:focal-20210723                                                                                                      0.3s
 => => resolve docker.io/library/ubuntu:focal-20210723                                                                                                          0.3s
 => CACHED docker-image://docker.io/library/ubuntu:20.04                                                                                                        0.4s
 => => resolve docker.io/library/ubuntu:20.04                                                                                                                   0.4s
 => apt update                                                                                                                                                  3.8s
 => apt install busybox-static                                                                                                                                  2.1s 
 => CACHED copy /bin/busybox /bin/busybox                                                                                                                       0.0s 
 => CACHED mkdir /copa-out                                                                                                                                      0.0s 
 => /bin/busybox sh -c if [ -f /var/lib/dpkg/status ]; then cp /var/lib/dpkg/status /copa-out ; fi && if [ -d /var/lib/dpkg/status.d ]; then ls -1 /var/lib/dp  0.1s 
...
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

with this PR:

$ copa patch ...
<waits for a while>
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

pmengelbert · 2023-10-19T14:14:04Z

this seems to skip showing the commands being run, is that intentional?

previously:

$ copa patch ...
[+] Building 6.7s (9/9) FINISHED                                                                                                                                     
 => docker-image://docker.io/library/ubuntu:focal-20210723                                                                                                      0.3s
 => => resolve docker.io/library/ubuntu:focal-20210723                                                                                                          0.3s
 => CACHED docker-image://docker.io/library/ubuntu:20.04                                                                                                        0.4s
 => => resolve docker.io/library/ubuntu:20.04                                                                                                                   0.4s
 => apt update                                                                                                                                                  3.8s
 => apt install busybox-static                                                                                                                                  2.1s 
 => CACHED copy /bin/busybox /bin/busybox                                                                                                                       0.0s 
 => CACHED mkdir /copa-out                                                                                                                                      0.0s 
 => /bin/busybox sh -c if [ -f /var/lib/dpkg/status ]; then cp /var/lib/dpkg/status /copa-out ; fi && if [ -d /var/lib/dpkg/status.d ]; then ls -1 /var/lib/dp  0.1s 
...
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

with this PR:

$ copa patch ...
<waits for a while>
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

I'll look into this

pmengelbert · 2023-10-19T14:20:14Z

how do we want to handle the Error: repository name must be canonical error for non-registry prefixed images (i.e., local/ubuntu:bionic)? is this coming from buildkit?

do we expect all images to have a prefix? or will this PR address non-prefixed images?

This is coming from github.com/distribution/reference.ParseNamed(). We can parse the name using another library instead.

Fix local image resolution

pmengelbert · 2023-10-19T18:14:38Z

this seems to skip showing the commands being run, is that intentional?
previously:

$ copa patch ...
[+] Building 6.7s (9/9) FINISHED                                                                                                                                     
 => docker-image://docker.io/library/ubuntu:focal-20210723                                                                                                      0.3s
 => => resolve docker.io/library/ubuntu:focal-20210723                                                                                                          0.3s
 => CACHED docker-image://docker.io/library/ubuntu:20.04                                                                                                        0.4s
 => => resolve docker.io/library/ubuntu:20.04                                                                                                                   0.4s
 => apt update                                                                                                                                                  3.8s
 => apt install busybox-static                                                                                                                                  2.1s 
 => CACHED copy /bin/busybox /bin/busybox                                                                                                                       0.0s 
 => CACHED mkdir /copa-out                                                                                                                                      0.0s 
 => /bin/busybox sh -c if [ -f /var/lib/dpkg/status ]; then cp /var/lib/dpkg/status /copa-out ; fi && if [ -d /var/lib/dpkg/status.d ]; then ls -1 /var/lib/dp  0.1s 
...
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

with this PR:

$ copa patch ...
<waits for a while>
INFO[0034] Validated package libsmartcols1 version 2.34-0.1ubuntu9.4 meets requested version 2.34-0.1ubuntu9.3                                                       
...

I'll look into this

This is fixed now.

pmengelbert · 2023-10-23T16:54:00Z

how do we want to handle the Error: repository name must be canonical error for non-registry prefixed images (i.e., local/ubuntu:bionic)? is this coming from buildkit?

do we expect all images to have a prefix? or will this PR address non-prefixed images?

This is fixed by the (currently) most recent commit:

copacetic/pkg/patch/patch.go

Line 256 in dca0f9b

func normalizeRef(image string) (string, error) {

pmengelbert · 2023-10-24T15:11:28Z

@pmengelbert can we add an integration test that validates this?

Integration and unit tests are done

sozercan · 2023-10-24T22:27:26Z

@pmengelbert looks like e2e is failing with ERROR: pull access denied, repository does not exist or may require authorization: server message: insufficient_scope: authorization failed

The gateway client has options which allow for fetching the image config locally before attempting to find it remotely. However, this commit is broken, since the `buildkit.Config` object now has a reference to a gateway client; however, `buildkit.SolveToLocal` requires a normal buildkit client. The following commit will replace `buildkit.SolveToLocal` with `buildkit.ExtractFileFromState`. Implement `buildkit.ExtractFileFromState` This code will still not compile, since the signature of `buildkit.SolveToLocal` is not satisfied by its calls. Replace `SolveToLocal` in `probeDPKGStatus` method In this case, we will write the status type to a file, which we thereafter read from the state as a single byte. Fix error in awk script There was a confusion in the awk script between bash arrays and awk arrays. You cannot pass in an awk array via the command line (see https://stackoverflow.com/a/33106291). The strings will be split by space to produce the array, so the parentheses are unnecessary here. Use `ExtractFileFromState` for dpkg `installUpdates` Read the bytes and return them, instead of writing them to the local filesystem. Use `ExtractFileFromState` in `unpackAndMergeUpdates` In dpkg.go APK: Use `ExtractFileFromState` for `upgradePackages` Do not use `SolveToLocal`. Use `ExtractFileFromState` for `probeRPMStatus` Replace `buildkit.SolveToLocal` in rpm.go In all places Update `patchWithContext` to pass the correct client Fix errors and achieve parity with `main` branch This commit passes the integration tests as well as the `main` branch does. Update unit tests Signed-off-by: Peter Engelbert <[email protected]>

In the Test Patch `docker/custom-unix` test, Docker is run in a `DIND` container, in order to be absolutely sure that docker > 24.0.0 is used. This is because the containerd snapshotter is required for copa (which requires docker > 22), and the package repo available to the github runner does not have docker 24 available. When running tests relevant to this PR, we need to pull/retag an image into the docker instance we are testing. In this case, that is the docker instance in the `DIND` container. Accordingly, when performing the pull and tag operations, we need to propagate the location of the custom docker address and include that in the `docker` cli invocation. This should fix the `docker/custom-unix` test in the CI. Another change is required to fix the other errors. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

Only the buildkit instance running within the docker daemon can work with locally-built or locally-tagged images. As a result, skip tests for local-only images when the daemon in question is not docker itself. i.e., don't test local images in buildx or with stock buildkit. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

This is necessary, because `copa` invokes `docker` directly to export the patched image. Signed-off-by: Peter Engelbert <[email protected]>

sozercan · 2023-11-22T19:14:45Z

integration/patch_test.go

@@ -44,14 +48,33 @@ func TestPatch(t *testing.T) {

 	for _, img := range images {
 		img := img
+
+		// Only the buildkit instance running within the docker daemon can work


sounds like this only works with docker with containerd snapshotter, is that right? can we add this to docs please?

Add to docs that copa only works with standard docker if using the containerd snapshotter.

integration/patch_test.go

This is the proper way to do this. Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

integration/patch_test.go

website/docs/quick-start.md

sozercan · 2023-11-22T21:57:39Z

website/docs/quick-start.md

+    > **NOTE:** if you're scanning and patching an image that is local-only
+    > (i.e. built or tagged locally but not pushed to a registry), you are
+    > limited to using `docker`'s built-in buildkit service. See
+    > [Prerequisites][#prerequisites] for more information.


Suggested change

> **NOTE:** if you're scanning and patching an image that is local-only

> (i.e. built or tagged locally but not pushed to a registry), you are

> limited to using `docker`'s built-in buildkit service. See

> [Prerequisites][#prerequisites] for more information.

> [!NOTE]

> if you're scanning and patching an image that is local-only (i.e. built or tagged locally but not pushed to a registry), `copa` is limited to using `docker`'s built-in buildkit service, and must use [`containerd image store`](https://docs.docker.com/storage/containerd/) feature due to <reason>. See [Prerequisites][#prerequisites] for more information.

please add the <reason>

@pmengelbert sorry, i edited my comment while you were addressing this, let's make sure to add the reason

Done, thanks.

website/docs/quick-start.md

Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: Peter Engelbert <[email protected]>

Signed-off-by: Peter Engelbert <[email protected]>

See diff for more info. Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert requested a review from cpuguy83 October 17, 2023 19:36

pmengelbert requested review from jeremyrickard, salaxander and sozercan as code owners October 17, 2023 19:36

pmengelbert changed the title ~~Use gateway client for config resolution~~ feat: Prefer local image Oct 17, 2023

pmengelbert mentioned this pull request Oct 17, 2023

feat: Add ability to fetch image locally when present #332

Closed

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch 2 times, most recently from f8e79dc to 1e0e435 Compare October 17, 2023 20:05

cpuguy83 requested changes Oct 18, 2023

View reviewed changes

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch 2 times, most recently from f95b8c4 to f706029 Compare October 18, 2023 14:16

sozercan reviewed Oct 18, 2023

View reviewed changes

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch 4 times, most recently from 8290bfd to dca0f9b Compare October 23, 2023 16:52

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch from ffacf6d to c43fa52 Compare October 24, 2023 15:11

sozercan mentioned this pull request Oct 30, 2023

[BUG] ERROR exporting to docker image format #339

Closed

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch from acfe7eb to b3e92df Compare November 22, 2023 16:41

pmengelbert added 5 commits November 22, 2023 11:46

Strip copa prefix from docker dind address

871bf1f

Signed-off-by: Peter Engelbert <[email protected]>

Set docker host on scanner too

db35d99

Signed-off-by: Peter Engelbert <[email protected]>

Only set trivy scan address when image is local

fb9c85c

Signed-off-by: Peter Engelbert <[email protected]>

Fix typo in docker/custom-unix ci script

900fba9

Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch from 8cebf36 to 5c3c249 Compare November 22, 2023 18:34

Propagate DOCKER_HOST env to copa invocation

e7c9904

This is necessary, because `copa` invokes `docker` directly to export the patched image. Signed-off-by: Peter Engelbert <[email protected]>

pmengelbert force-pushed the pmengelbert/prefer_local_image/5 branch from 5c3c249 to e7c9904 Compare November 22, 2023 18:38

sozercan reviewed Nov 22, 2023

View reviewed changes

cpuguy83 requested changes Nov 22, 2023

View reviewed changes

integration/patch_test.go Outdated Show resolved Hide resolved

pmengelbert added 2 commits November 22, 2023 16:18

Replace continue with t.Skip()

3bbed88

This is the proper way to do this. Signed-off-by: Peter Engelbert <[email protected]>

Update docs to mention limitations of local image patching

e615af8

Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 reviewed Nov 22, 2023

View reviewed changes

integration/patch_test.go Outdated Show resolved Hide resolved

sozercan reviewed Nov 22, 2023

View reviewed changes

website/docs/quick-start.md Outdated Show resolved Hide resolved

sozercan reviewed Nov 22, 2023

View reviewed changes

website/docs/quick-start.md Outdated Show resolved Hide resolved

pmengelbert and others added 5 commits November 22, 2023 17:04

Update website/docs/quick-start.md

b425df5

Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: Peter Engelbert <[email protected]>

Update website/docs/quick-start.md

37fbb47

Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: Peter Engelbert <[email protected]>

Update website/docs/quick-start.md

21291f5

Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: Peter Engelbert <[email protected]>

Move t.Skip() to the right place.

0d89fb2

Signed-off-by: Peter Engelbert <[email protected]>

Update quick-start.md with note on feature

7037c84

See diff for more info. Signed-off-by: Peter Engelbert <[email protected]>

cpuguy83 approved these changes Nov 27, 2023

View reviewed changes

pmengelbert merged commit e5288e8 into project-copacetic:main Nov 27, 2023
17 of 18 checks passed

pmengelbert mentioned this pull request Nov 27, 2023

[REQ] Skip request to container registry when image already exists locally #177

Closed

sozercan mentioned this pull request Jan 2, 2024

[REQ] Enhance Image Resolution Methods #439

Closed

1 task

BrewTestBot mentioned this pull request Jan 4, 2024

copa 0.6.0 Homebrew/homebrew-core#158940

Merged

sozercan mentioned this pull request Feb 15, 2024

[BUG] copa CLI gets to a deadlock if package validation fails #503

Closed

1 task

ashnamehrotra mentioned this pull request Jul 9, 2024

[DOC] Update development tips SolveToDocker reference #671

Closed

1 task

feat: Prefer local image #381

feat: Prefer local image #381

Conversation

pmengelbert commented Oct 17, 2023

sozercan commented Oct 17, 2023

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmengelbert Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

sozercan commented Oct 18, 2023 • edited Loading

sozercan commented Oct 18, 2023 • edited Loading

pmengelbert commented Oct 19, 2023

pmengelbert commented Oct 19, 2023 • edited Loading

pmengelbert commented Oct 19, 2023

pmengelbert commented Oct 23, 2023

pmengelbert commented Oct 24, 2023

sozercan commented Oct 24, 2023

sozercan Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

pmengelbert Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

sozercan Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 17, 2023 •

edited

Loading

pmengelbert Oct 18, 2023 •

edited

Loading

cpuguy83 Oct 18, 2023 •

edited

Loading

pmengelbert Oct 19, 2023 •

edited

Loading

sozercan commented Oct 18, 2023 •

edited

Loading

sozercan commented Oct 18, 2023 •

edited

Loading

pmengelbert commented Oct 19, 2023 •

edited

Loading

sozercan Nov 22, 2023 •

edited

Loading

pmengelbert Nov 22, 2023 •

edited

Loading

sozercan Nov 22, 2023 •

edited

Loading

sozercan Nov 22, 2023 •

edited

Loading