Enable to apply registry configuration to stargz snapshotter #1733

ktock · 2020-10-13T09:23:41Z

Following-up: #1760
Related: containerd/stargz-snapshotter#149

Currently, stargz snapshotter + OCI worker can't do ~/.docker/config.json-based authentication because the registry configuration resolver.DefaultPool.GetResolver().hostsFunc() (including session-based authorizer *resolver.dockerAuthorizer) isn't passed to that snapshotter. This patch addresses this issue.

A remote snapshot is possibly used by multiple builds. And that snapshot's labels (including image refs, session IDs for fetching creds, etc.) for fetching that content possibly differ on each of these builds. So this commit allows these builds to append snapshot labels to that remote snapshot, every time they use that snapshots.

In this patch, on each time a remote snapshot is used, corresponding labels (containing image refs, session IDs etc.) are appended to that snapshot temporarily (so they are removed after that use). Here, these labels' keys are suffixed with unique IDs for avoiding collision among simultaneous uses of that snapshot. When the connection of that remote snapshot needs to be resolved (or refreshed), all labels appended to that snapshot at that moment are parsed and this source-related information (including session-based authorizer) is passed to the snapshotter (this is done in (cmd/buildkitd).sourceWithSession()). Then the snapshotter resolves/refreshes that snapshot's connection using that information.

*immutableRef.withRemoteSnapshotLabels() is a function wrapper for appending remote labels to snapshots temporarily. This takes a func() and ensure that:

Before the call of func(), remote snapshot labels contained in *immutableRef.descHandlers are appended to all remote snapshots chained in that *immutableRef.
After the call of func(), these labels are removed.

Here, no label will be appended to non-remote snapshots.
In this patch, functions that invoke snapshotter's Prepare/View/Mounts API are wrapped with *immutableRef.withRemoteSnapshotLabels().

The above integration is only available for oci worker. When we use containerd worker + stargz snapshotter, the registry logic is still separated.

ktock · 2020-10-19T12:10:56Z

Ready for review.

Currently, lazypull-related integration tests depend on containerd worker.
We might need to expose content store & snapshotter API for testing this with oci worker config as well.

ktock · 2020-10-20T00:42:54Z

@tonistiigi @AkihiroSuda PTAL 😄

AkihiroSuda · 2020-10-26T07:25:07Z

needs rebase

ktock · 2020-10-26T12:44:09Z

rebased.

tonistiigi · 2020-10-28T06:00:39Z

The session handling in here does not look ok to me. We can't keep the session ID as ref properties. It needs to come from the caller invoking the call as sessions can drop and switch anytime. I've also started some refactoring around the lazy pull to fix some issues related to this but wip, https://github.com/tonistiigi/buildkit/pull/new/session-remote

ktock · 2020-10-28T11:41:31Z

@tonistiigi Thanks for the comment and the refactoring patch! Based on your lazypull & session refactoring patch, this PR can be modified not to contain session IDs as ref properties in SnapshotSource JSON blob.

Instead, session IDs passed through ImmutableRef.Extract()/ImmutableRef.Mount() can be appended as temporary labels to the corresponding SnapshotSource JSON blob. These labels will only be appended to that blob during Prepare/View/Mount API calls where the connection refresh occurs in the snapshotter. Then the snapshotter can (re-)create a session-based docker.RegistryHosts using these session IDs contained in these temporary labels at that time. After snapshotter API calls, these session ID labels will be removed immediately.

ktock · 2020-11-09T13:01:53Z

@tonistiigi @AkihiroSuda
Redesigned this patch based on #1761 and #1733 (comment). This patch now doesn't use the content store but uses temporary labels for passing remote snapshot's source info (including image refs, session IDs) to the snapshotter (please see this PR's description in the above).
WDYT?

tonistiigi · 2021-01-25T17:39:49Z

Sorry, this got stuck. @ktock needs rebase @AkihiroSuda for re-review

ktock · 2021-01-26T03:46:03Z

Thanks for resuming this. Rebased.

tonistiigi · 2021-01-27T01:47:07Z

cache/refs.go

 		}
 		dh := dhs[desc.Digest]
 		if dh == nil {
-			return false, nil
+			continue // no info psased; skip


nit passed

Fixed this.

tonistiigi · 2021-01-27T01:54:34Z

cache/refs.go

-		snapshotID := getSnapshotID(sr.md)
-		if _, err := sr.cm.Snapshotter.Stat(ctx, snapshotID); err == nil {
-			return true, nil
+func (sr *immutableRef) withRemoteSnapshotLabels(ctx context.Context, s session.Group, f func()) error {


If this function is only called for stargz then please add it to the name. Same for the next one. Otherwise, they look expensive to be called by other code paths. Maybe even move stargz specific functions to separate file (this can be done as follow up if there is more code than these functions.

Added the prefix StargzMode to each function name.

ktock · 2021-01-28T02:08:29Z

TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/rootfs_and_readwrite_scratch_mount fails but similar tests (e.g. TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/rootfs_and_readwrite_mount) pass. 🤔
Tests by other workers (e.g. oci-snapshotter-stargz) also pass so seems unrelated to this patch?

https://github.com/moby/buildkit/pull/1733/checks?check_run_id=1774129959

    --- FAIL: TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd (6.90s)
        --- PASS: TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/only_rootfs (0.71s)
        --- FAIL: TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/rootfs_and_readwrite_scratch_mount (0.74s)
            build_test.go:1216: 
                	Error Trace:	build_test.go:1216
                	Error:      	Not equal: 
                	            	expected: "akc7wimyyp1smszst4cwpkk3d"
                	            	actual  : ""
                	            	
                	            	Diff:
                	            	--- Expected
                	            	+++ Actual
                	            	@@ -1 +1 @@
                	            	-akc7wimyyp1smszst4cwpkk3d
                	            	+
                	Test:       	TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/rootfs_and_readwrite_scratch_mount
         --- PASS: TestClientGatewayIntegration/TestClientGatewayExecError/worker=containerd/rootfs_and_readwrite_mount (0.78s)

ktock · 2021-02-01T02:08:21Z

All green except codecov/patch failure.

tonistiigi · 2021-06-08T07:05:00Z

@ktock Not sure why this got stuck. Can you rebase just in case to let the test run in the new merge base.

ktock · 2021-06-08T13:55:40Z

Rebased and bumped stargz snapshotter to the latest (v0.6.4).

tonistiigi · 2021-06-09T04:58:20Z

@ktock Another rebase required 😅

Signed-off-by: ktock <ktokunaga.mail@gmail.com>

ktock force-pushed the reghost branch from a25c7ce to 3e612c6 Compare October 14, 2020 03:12

ktock mentioned this pull request Oct 17, 2020

Move label-related logic into a separated package containerd/stargz-snapshotter#163

Merged

ktock force-pushed the reghost branch from 3e612c6 to a693a12 Compare October 19, 2020 11:44

ktock marked this pull request as ready for review October 19, 2020 12:10

ktock force-pushed the reghost branch 2 times, most recently from 3e50166 to bfc1628 Compare October 21, 2020 12:29

ktock force-pushed the reghost branch from bfc1628 to 353f0e9 Compare October 26, 2020 08:02

AkihiroSuda approved these changes Oct 26, 2020

View reviewed changes

AkihiroSuda marked this pull request as draft October 28, 2020 06:07

ktock mentioned this pull request Oct 29, 2020

Bump stargz-snapshotter and partial registry logic integration #1760

Merged

ktock force-pushed the reghost branch 2 times, most recently from d54eb45 to fc91fd9 Compare November 9, 2020 11:41

ktock force-pushed the reghost branch 2 times, most recently from 202df21 to 39ff855 Compare November 17, 2020 01:28

ktock marked this pull request as ready for review November 17, 2020 05:28

ktock force-pushed the reghost branch from 39ff855 to d83a420 Compare November 23, 2020 08:30

moby deleted a comment from codecov-io Nov 25, 2020

ktock force-pushed the reghost branch from d83a420 to 253a8ef Compare January 26, 2021 03:16

tonistiigi reviewed Jan 27, 2021

View reviewed changes

ktock force-pushed the reghost branch from 253a8ef to 941483b Compare January 27, 2021 03:24

ktock force-pushed the reghost branch from 941483b to f8c0f5c Compare January 29, 2021 00:41

tonistiigi requested a review from sipsma June 8, 2021 07:04

ktock force-pushed the reghost branch from f8c0f5c to b5b9ef6 Compare June 8, 2021 13:02

Enable stargz snapshotter to use session for snapshot auth

1764b3b

Signed-off-by: ktock <ktokunaga.mail@gmail.com>

ktock force-pushed the reghost branch from b5b9ef6 to 1764b3b Compare June 9, 2021 08:15

tonistiigi approved these changes Jun 9, 2021

View reviewed changes

tonistiigi merged commit 0de03b4 into moby:master Jun 9, 2021

ktock mentioned this pull request Jun 10, 2021

Fix doc about Stargz Snapshotter and registry authentication #2165

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable to apply registry configuration to stargz snapshotter #1733

Enable to apply registry configuration to stargz snapshotter #1733

ktock commented Oct 13, 2020 •

edited

Loading

ktock commented Oct 19, 2020

ktock commented Oct 20, 2020

AkihiroSuda commented Oct 26, 2020

ktock commented Oct 26, 2020

tonistiigi commented Oct 28, 2020

ktock commented Oct 28, 2020

ktock commented Nov 9, 2020

tonistiigi commented Jan 25, 2021

ktock commented Jan 26, 2021

tonistiigi Jan 27, 2021

ktock Jan 27, 2021

tonistiigi Jan 27, 2021

ktock Jan 27, 2021

ktock commented Jan 28, 2021

ktock commented Feb 1, 2021

tonistiigi commented Jun 8, 2021

ktock commented Jun 8, 2021

tonistiigi commented Jun 9, 2021

Enable to apply registry configuration to stargz snapshotter #1733

Enable to apply registry configuration to stargz snapshotter #1733

Conversation

ktock commented Oct 13, 2020 • edited Loading

ktock commented Oct 19, 2020

ktock commented Oct 20, 2020

AkihiroSuda commented Oct 26, 2020

ktock commented Oct 26, 2020

tonistiigi commented Oct 28, 2020

ktock commented Oct 28, 2020

ktock commented Nov 9, 2020

tonistiigi commented Jan 25, 2021

ktock commented Jan 26, 2021

tonistiigi Jan 27, 2021

Choose a reason for hiding this comment

ktock Jan 27, 2021

Choose a reason for hiding this comment

tonistiigi Jan 27, 2021

Choose a reason for hiding this comment

ktock Jan 27, 2021

Choose a reason for hiding this comment

ktock commented Jan 28, 2021

ktock commented Feb 1, 2021

tonistiigi commented Jun 8, 2021

ktock commented Jun 8, 2021

tonistiigi commented Jun 9, 2021

ktock commented Oct 13, 2020 •

edited

Loading