Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID #39365

tiborvass · 2019-06-15T06:54:26Z

Partially reverts #37874

This partially reverts 98fc091.

Because registry v2 schema1 was not officially deprecated and
registries are still relying on it, this patch puts its logic back.

However, registry v1 relics are not added back since v1 logic has been
removed a while ago.

This also fixes an engine upgrade issue in a swarm cluster. It was relying
on the Engine ID to be the same upon upgrade, but the mentioned commit
modified the logic to use UUID and from a different file.

Since the libtrust key is always needed to support v2 schema1 pushes,
that the old engine ID is based on the libtrust key, and that the engine ID
needs to be conserved across upgrades, adding a UUID-based engine ID logic
seems to add more complexity than it solves the problems.

For that reason, this patch also reverts the engine ID changes.

This reverts commit 13b7d11.

Added a test to guard the daemon ID when upgrading.

This also adds a deprecation message to the user as well as in the logs.

tiborvass · 2019-06-15T07:08:04Z

distribution/push_v2.go

+		logrus.Warnf("failed to upload schema2 manifest: %v - falling back to schema1", err)
+
+		logrus.Warnf("[DEPRECATION NOTICE] schema1 support will be removed in an upcoming release. Please contact admins of the %s registry NOW to avoid future disruption. Waiting 13 annoying seconds...", reference.Domain(ref))
+		time.Sleep(13 * time.Second)


to be debated

We could randomize it?

If this needs more discussion, might be good to move it to a separate PR to unblock this one

thaJeztah · 2019-06-15T07:21:55Z

hack/validate/default

@@ -14,4 +14,4 @@ export SCRIPTDIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 . ${SCRIPTDIR}/toml
 . ${SCRIPTDIR}/changelog-well-formed
 . ${SCRIPTDIR}/changelog-date-descending
-. ${SCRIPTDIR}/deprecate-integration-cli
+#. ${SCRIPTDIR}/deprecate-integration-cli


Guess this was to make CI pass?

thaJeztah · 2019-06-15T07:24:29Z

daemon/daemon.go

+	trustKey, err := libtrust.LoadKeyFile(config.TrustKeyPath)
+	switch {
+	case err == nil:
+		d.ID = trustKey.PublicKey().KeyID()


Can we write the ID to the new file in this case? That way we're able to preserve the ID once we really remove this logic.

FWIW, the format of the ID is not formalized (the new code uses an UUID for convenience), so storing the old ID format in the new location should not be a problem

I'll update

@thaJeztah @justincormack the only problem I see here is that we would put a non-UUID into a file called engine_uuid. Should we rename the file to engine_id ?

Makes sense to me; it's used to store an ID (and we describe it in the API that the format is not specified)

thaJeztah · 2019-06-15T07:33:37Z

distribution/push_v2.go

+		logrus.Warnf("failed to upload schema2 manifest: %v - falling back to schema1", err)
+
+		logrus.Warnf("[DEPRECATION NOTICE] schema1 support will be removed in an upcoming release. Please contact admins of the %s registry NOW to avoid future disruption. Waiting 13 annoying seconds...", reference.Domain(ref))
+		time.Sleep(13 * time.Second)


We could randomize it?

If this needs more discussion, might be good to move it to a separate PR to unblock this one

tiborvass · 2019-06-17T21:07:39Z

@thaJeztah @justincormack since this PR reverts v2 schema1 removal, and that libtrust key is needed to push with v2 schema1, I'm afraid we need to revert the engine ID change altogether. Migrating from libtrust key to engine UUID would mean removing the libtrust key, and thus breaking users who rely on pushing v2 schema1. So we will only be able to remove the libtrust key when we decide to remove support for pushing v2 schema1. But now that libtrust key stays, it makes little sense to introduce another engine ID.

codecov · 2019-06-17T21:47:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@882e26a). Click here to learn what that means.
The diff coverage is 29.68%.

@@            Coverage Diff            @@
##             master   #39365   +/-   ##
=========================================
  Coverage          ?   37.35%           
=========================================
  Files             ?      610           
  Lines             ?    45235           
  Branches          ?        0           
=========================================
  Hits              ?    16897           
  Misses            ?    26046           
  Partials          ?     2292

tiborvass · 2019-06-17T21:57:11Z

@thaJeztah @tonistiigi @justincormack PTAL I updated the code, the title, and the description based on the discussions. Removed the annoying sleep, will PR it separately.

This reverts commit 13b7d11. Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tibor Vass <tibor@docker.com>

This reverts commit 98fc091 in order to keep registry v2 schema1 handling and libtrust-key-based engine ID. Because registry v2 schema1 was not officially deprecated and registries are still relying on it, this patch puts its logic back. However, registry v1 relics are not added back since v1 logic has been removed a while ago. This also fixes an engine upgrade issue in a swarm cluster. It was relying on the Engine ID to be the same upon upgrade, but the mentioned commit modified the logic to use UUID and from a different file. Since the libtrust key is always needed to support v2 schema1 pushes, that the old engine ID is based on the libtrust key, and that the engine ID needs to be conserved across upgrades, adding a UUID-based engine ID logic seems to add more complexity than it solves the problems. Hence reverting the engine ID changes as well. Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tibor Vass <tibor@docker.com>

This will add a warning log in the daemon, and will send the message to be displayed by the CLI. Signed-off-by: Tibor Vass <tibor@docker.com>

…revert Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass · 2019-06-18T03:08:17Z

I updated the code by actually reverting the original commit 98fc091 and added a commit that is the change that it should have been. This way it is easier to compare with 98fc091.

Also made a change to send the deprecation message to the client in addition to logging a warning to the daemon.

thaJeztah · 2019-06-18T10:23:05Z

daemon/images/service.go

@@ -14,6 +14,7 @@ import (
 	"github.com/docker/docker/layer"
 	dockerreference "github.com/docker/docker/reference"
 	"github.com/docker/docker/registry"
+	"github.com/docker/libtrust"


FWIW; had a chat with @dmcgowan, and there's a desire to get rid of this dependency; not sure if it's worth doing so as part of this PR (if we plan to remove it in the next release); his suggestion was to copy the parts we use into this repository (somewhere in pkg or internal perhaps?), so that we can remove the dependency

@thaJeztah Sure I can do that in a followup, don't want to add more changes to this PR which is already big.

cpuguy83

LGTM :(

andrewhsu · 2019-06-18T18:08:01Z

SGTM

As much as I would prefer quay.io to support v2 schema2 pushes, at this point it fails to handle it. I just tried and daemon says:

Jun 18 17:58:29 ip-172-31-27-101 dockerd[1129]: time="2019-06-18T17:58:29.365129917Z" level=warning msg="failed to upload schema2 manifest: manifest invalid: manifest invalid"

tiborvass · 2019-06-18T18:52:26Z

Thanks for the reviews.

dmcgowan · 2019-08-08T18:02:03Z

This message should not have been added on pull, that can be unnecessarily confusing for users as there is nothing that must be done now related to pull and just causes noise. If a user is pushing to a registry which still only supports v1, then they do need to contact their registry administrator to upgrade. distribution/distribution#2976

GordonTheTurtle added the status/0-triage label Jun 15, 2019

tiborvass requested review from thaJeztah and justincormack June 15, 2019 06:54

tiborvass force-pushed the deprecate-v2-schema1 branch from fd2e9b8 to 04593cc Compare June 15, 2019 07:05

tiborvass commented Jun 15, 2019

View reviewed changes

tiborvass requested a review from tianon as a code owner June 15, 2019 07:12

thaJeztah requested changes Jun 15, 2019

View reviewed changes

tiborvass force-pushed the deprecate-v2-schema1 branch from db063e0 to 0bd619b Compare June 17, 2019 21:47

tiborvass force-pushed the deprecate-v2-schema1 branch 2 times, most recently from 2afb14b to 3cb4c1f Compare June 17, 2019 21:54

tiborvass changed the title ~~Keep but deprecate registry v2 schema1 logic and read old engine ID from disk~~ Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID Jun 17, 2019

tiborvass force-pushed the deprecate-v2-schema1 branch 3 times, most recently from fa1920c to f18350d Compare June 17, 2019 23:21

Revert "Remove Schema1 integration test suite"

f23a51a

This reverts commit 13b7d11. Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the deprecate-v2-schema1 branch from f18350d to eb1e31c Compare June 18, 2019 00:31

Tibor Vass added 2 commits June 18, 2019 00:36

use gotest.tools assertions in docker_cli_push_test.go

0811297

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the deprecate-v2-schema1 branch 3 times, most recently from 65f8392 to ecc8c8a Compare June 18, 2019 01:38

Tibor Vass added 4 commits June 18, 2019 01:40

Remove v1 manifest code

53dad9f

Signed-off-by: Tibor Vass <tibor@docker.com>

Add test for keeping same daemon ID on upgrade

f923321

Signed-off-by: Tibor Vass <tibor@docker.com>

Add deprecation message for schema1

d35f8f4

This will add a warning log in the daemon, and will send the message to be displayed by the CLI. Signed-off-by: Tibor Vass <tibor@docker.com>

validate: temporarily disable deprecate-integration-cli as part of a …

3f1cdd5

…revert Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the deprecate-v2-schema1 branch from ecc8c8a to 3f1cdd5 Compare June 18, 2019 01:40

tiborvass mentioned this pull request Jun 18, 2019

Remove more registry v1 code #39371

Closed

thaJeztah reviewed Jun 18, 2019

View reviewed changes

thaJeztah added impact/changelog impact/deprecation status/2-code-review area/distribution and removed status/0-triage labels Jun 18, 2019

cpuguy83 approved these changes Jun 18, 2019

View reviewed changes

tonistiigi approved these changes Jun 18, 2019

View reviewed changes

tiborvass merged commit 4866f51 into moby:master Jun 18, 2019

andrewhsu mentioned this pull request Jun 18, 2019

deprecate registry v2 schema 1 docker/cli#1956

Merged

tiborvass mentioned this pull request Jun 18, 2019

[19.03] Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID docker/engine#284

Merged

andrewhsu mentioned this pull request Jun 19, 2019

update engine deprecation policy docker/docs#8960

Merged

thaJeztah mentioned this pull request Jun 20, 2019

[19.03 backport] deprecate registry v2 schema 1 docker/cli#1960

Merged

tiborvass mentioned this pull request Jun 20, 2019

Remove v2 schema1 push #39384

Closed

dmcgowan mentioned this pull request Jul 1, 2019

Proposal: containerd image integration #38043

Closed

6 tasks

tiborvass deleted the deprecate-v2-schema1 branch July 17, 2019 00:34

This was referenced Nov 13, 2019

Error if base image registry uses V2 Schema1 GoogleContainerTools/jib#2147

Closed

Check using Quay as the registry. GoogleContainerTools/jib#500

Closed

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

thaJeztah mentioned this pull request Nov 12, 2020

Add HEAD endpoints opencontainers/distribution-spec#207

Closed

thaJeztah mentioned this pull request Apr 16, 2021

Remove support for pulling v2 schema1 #42300

Draft

thaJeztah mentioned this pull request Oct 5, 2021

registry: remove unused registry.ErrAlreadyExists #42908

Merged

thaJeztah mentioned this pull request Nov 18, 2021

integration-cli: remove manifest v2 schema 1 push tests #43029

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID #39365

Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID #39365

tiborvass commented Jun 15, 2019 •

edited

tiborvass Jun 15, 2019

thaJeztah Jun 15, 2019

thaJeztah Jun 15, 2019

tiborvass Jun 15, 2019

thaJeztah Jun 15, 2019

tiborvass Jun 15, 2019

tiborvass Jun 17, 2019

thaJeztah Jun 17, 2019

thaJeztah Jun 15, 2019

tiborvass commented Jun 17, 2019

codecov bot commented Jun 17, 2019 •

edited

tiborvass commented Jun 17, 2019

tiborvass commented Jun 18, 2019

thaJeztah Jun 18, 2019

tiborvass Jun 18, 2019

cpuguy83 left a comment

andrewhsu commented Jun 18, 2019

tiborvass commented Jun 18, 2019

dmcgowan commented Aug 8, 2019

Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID #39365

Keep but deprecate registry v2 schema1 logic and revert to libtrust-key-based engine ID #39365

Conversation

tiborvass commented Jun 15, 2019 • edited

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

tiborvass commented Jun 17, 2019

codecov bot commented Jun 17, 2019 • edited

Codecov Report

tiborvass commented Jun 17, 2019

tiborvass commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpuguy83 left a comment

Choose a reason for hiding this comment

andrewhsu commented Jun 18, 2019

tiborvass commented Jun 18, 2019

dmcgowan commented Aug 8, 2019

tiborvass commented Jun 15, 2019 •

edited

codecov bot commented Jun 17, 2019 •

edited