Should we cherry pick for 1.13.1?

yup I think that's a good idea

ping @stevvooe

LGTM

Updated to add application/json. This was pointed out as another case in #30083.

Should we issue a warning to make people aware the content-type is invalid?

@thaJeztah the problem with a warning is that it is only actionable if the puller is also the owner. The "fix" is to re-push the image.

I'm not sure if this requires more effort, but it would be better if we could position these as aliases. Schema2 should really only have one mediatype, but these media types get treated as aliases to a default.

@stevvooe these are already acting like aliases to the default type, which is "application/vnd.docker.container.image.v1+json". Not sure what you mean by schema2 should only have 1 media type, there is only one schema2 media type we allow "application/vnd.docker.distribution.manifest.v2+json" (manifest list is not relevant here). For registries that do not return media type, we treat the responses as schema1 and this code would not be relevant.

@dmcgowan I mean, I would expect the package to declare a canonical mediatype. The aliasing seems like it belongs at the registration level.

@aaronlehmann @stevvooe @dmcgowan please can you agree on how to implement this today as we need to fix this for 1.13.0-rc1 as it is causing distress to our users of gcp registry...

This PR should be ready to go. If I understand @stevvooe's comments correctly, they are suggesting a refactoring which is independent from adding these media types to the list.

@dmcgowan

If the solution is for the image author to repush, does that mean it could be useful

I would assume the author will pull his image at some point. I would prefer adding a warning to invite him to repush the image.

Otherwise, this is good. Let's agree whether we want to add warnings or not, and i'll approve the PR.

I'd prefer not to add warnings. I think it will be confusing to end users who can't do anything about the situation. And why does the user need to know the mediatype is weird if we're handling it properly?

Ok, then LGTM

@aaronlehmann what about just logging from the daemon?

No objection to logging, but it means breaking up this list into "good" and "bad" media types. It does seem like some extra complexity for something that's not usually actionable. @dmcgowan WDYT?

Absolutely, this should be in the logs on pull and it was an oversight that there wasn't one there originally.

LGTM

@aaronlehmann I am not asking for refactoring. I am asking that this be structured appropriately to represent the problem without creating technical debt such that someone else going to have to clean it up later. The fact that this contains the empty string "" shows something is wrong here.

Mapping this to ImageTypes is an expedient way to solve this problem, but these are really only "image types" in certain contexts. I understand this is to interact with the repo-class system, but that seems like the wrong area to making that decision. It is very to call these "image types" when they are "manifest types scene in an image-class repository".

These should be aliases to schema2.ManifestMediaType, which should then be looked up for the repository class. Although, I am unsure why we are using the repository class for resolving mediatypes. The mediatype should only be checked against the class.

It occurred to me that @duglin opened a PR that improves the error messages to provide more visibility into unrecognized mediatypes: #30047

I suggest we merge this PR as-is, and move discussion around logging and error messages to that PR, to avoid making changes that conflict with it.