Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

distribution: modify warning logic when pulling v2 schema1 manifests #39736

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

tiborvass
Copy link
Contributor

@tiborvass tiborvass commented Aug 13, 2019

The warning message should have been for push only, since there are images pushed with v2 schema1 years ago that we still want to be able to pull.

The warning on pull was incorrectly asking to contact registry admins.
It is kept on push however.

Pulling manifest lists with v2 schema1 manifests will not be supported thus
there is a warning for those, but wording changed to suggest repository author
to upgrade.

Finally, a milder warning on regular pull is kept ONLY for DockerHub users
in order to incite moving away from schema1.

Signed-off-by: Tibor Vass [email protected]

Closes #39701

Refs:

cc @dmcgowan @tonistiigi @josephschorr @thaJeztah

Needs to be backported to Docker 19.03

@dmcgowan
Copy link
Member

LGTM

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiborvass tiborvass force-pushed the remove-warning-on-v2schema1-pull branch from cb15706 to 61eadc3 Compare August 13, 2019 19:15
@tiborvass tiborvass changed the title distribution: remove v2 schema1 warning message on pull distribution: modify warning logic when pulling v2 schema1 manifests Aug 13, 2019
@tiborvass
Copy link
Contributor Author

After talking with @dmcgowan and @tonistiigi I've upgraded the logic, so this needs reviews again.
(cc @cpuguy83 @josephschorr)

progress.Message(p.config.ProgressOutput, "", msg)

// only warn on Docker Hub, to incite upgrading
if reference.Domain(ref) == "docker.io" {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could/should make this more extensible... like have DockerHub (and those registries that want to do so) return a header saying "Please upgrade!". For our existing customers, we wouldn't return the header, but for new installations, we might do so (or if a customer explicitly decides to toggle it on)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of being able to have this sort of extra metadata come down from the registry. Deciding on such a protocol is a longer discussion though. Maybe propose this in another issue or even on the distribution spec.

progress.Message(p.config.ProgressOutput, "", msg)

// only warn on Docker Hub, to incite upgrading
if reference.Domain(ref) == "docker.io" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand this.
Why would someone update an old image and why should someone pester other people to do so?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And even more so, why would it be specific to hub?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is specific to the hub because we know the timeframe of which the hub supported schema2. These represent old images which have not received any updates since that point. After push is disabled for all registries, then we can reconsider the deprecation notice for all schema 1 manifests. Even if they are still supported, schema1 manifests will be much slower in the future and suggesting to update the image is a valid suggestion. While the original message was incorrect, it did show that there are hub users who may be unknowingly using very outdated images. It may be worthwhile in the future to display such a message on any outdated images, for example older official images, if it is possible to make such a determination.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I think it would be worth putting behind a header; other registries (such as Quay) might want to add this header at some point if we deem it worth dropping support for Schema 2 V1. I suspect that support drop will be FAR into the future, but it would be nice to have the option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid the header would need updates to distribution too. A comment was added to explain why Docker Hub is hard-coded. Also reworded the message for clarity.

@tiborvass tiborvass force-pushed the remove-warning-on-v2schema1-pull branch 2 times, most recently from f7fdd5e to 56964ba Compare August 13, 2019 21:55
@tiborvass
Copy link
Contributor Author

Ping @dmcgowan @cpuguy83 I know you had approved it but it was the previous version, so I'm looking for a final approval.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message will not be understood by pretty much anyone unless they implement a registry or work on container distribution.
If we are going to issue warnings it should be something the user understands, because even the image author will not usually know what this means.

@tiborvass
Copy link
Contributor Author

@cpuguy83 @tonistiigi

How about?

-Image %s uses outdated schema1 manifest format. Please upgrade to a schema2 image for better future compatibility
+Image %s uses an outdated schema1 manifest format. To ensure future compatibility, the author should upgrade now to schema2 by pushing again with an up-to-date version of Docker`

and

-[DEPRECATION NOTICE] v2 schema1 manifests in manifest lists are not supported and will break in a future release. Suggest author of %s to upgrade to v2 schema2
+[DEPRECATION NOTICE] schema1 manifests in manifest lists are not supported and will break in a future release. Suggest author of %s to upgrade to schema2 by pushing again with an up-to-date version of Docker

@andrewhsu
Copy link
Member

What about having a link to a URL that has instructions on how to handle the situation? It can then go into depth about how to update the image manifest format to schema2 or to update the application FROM in the Dockerfile if the user should do so since it is a 3.5-yr-old image.

@cpuguy83
Copy link
Member

cpuguy83 commented Sep 4, 2019

Curious, is simply re-pushing enough if the image content already exists?
Otherwise messaging looks much better. ❤️

@adrian-plata
Copy link

Here's the page for updating deprecated v1 images. https://docs.docker.com/registry/spec/deprecated-schema-v1/

@tiborvass
Copy link
Contributor Author

In Maintainers meeting we agreed that we're adding the https://docs.docker.com/registry/spec/deprecated-schema-v1/ link to all 3 messages, but the content on the link will need more guidance from the various errors.

The warning on pull was incorrectly asking to contact registry admins.
It is kept on push however.

Pulling manifest lists with v2 schema1 manifests will not be supported thus
there is a warning for those, but wording changed to suggest repository author
to upgrade.

Finally, a milder warning on regular pull is kept ONLY for DockerHub users
in order to incite moving away from schema1.

Signed-off-by: Tibor Vass <[email protected]>
@tiborvass tiborvass force-pushed the remove-warning-on-v2schema1-pull branch from 56964ba to 647dfe9 Compare September 12, 2019 18:53
Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tiborvass tiborvass merged commit 700722f into moby:master Sep 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schema 1 images are NOT deprecated for pulling
10 participants