Fixed blob media_type during image push. #414
Conversation
|
Attached issue: https://pulp.plan.io/issues/9229 |
ipanova
force-pushed
the
i9229
branch
3 times, most recently
from
a41449b to
f2fc944
Compare
pulp_container/app/registry_api.py
Outdated
| ): | ||
| raise BlobInvalid(digest=config_blob.digest) | ||
| config_blob.media_type = config_media_type | ||
| config_blob.save() |
There was a problem hiding this comment.
I'd really prefer if we didn't update existing content units.
pulp_container/app/registry_api.py
Outdated
| thru.append(models.BlobManifest(manifest=manifest, manifest_blob=blob)) | ||
| models.BlobManifest.objects.bulk_create(objs=thru, ignore_conflicts=True, batch_size=1000) | ||
| models.Blob.objects.bulk_update(objs=blobs_qs, fields=["media_type"]) |
There was a problem hiding this comment.
The same here. Is there a way to have the Blob content type available when creating the blob?
There was a problem hiding this comment.
Tthe client does not send any information regarding that and since blobs come first and manifest as last i don't see any other way then updating the content post creation. I agree with you though, this is not my preferred way but at least we're not changing any foreign keys like we did with tags
There was a problem hiding this comment.
Are we sure that the same binary blob can only ever be used with the same content-type? (Can it be CONFIG_BLOG in one and CONFIG_BLOG_OCI in another repo?)
If not we can either create a new Blob with the proper content-type (adding the latter to the natural key), or we would need to be even more clever at delivering the content injecting the type there.
There was a problem hiding this comment.
When it comes to docker, I can almost never say things with 100% certainty.
While there is a full compatibility between blobs docker and oci media_types https://github.com/opencontainers/image-spec/blob/main/media-types.md#applicationvndociimagelayerv1targzip , having set regular blob type in the config blob is quite wrong.
For the config blobs, they are very similar https://github.com/opencontainers/image-spec/blob/main/media-types.md#applicationvndociimageconfigv1json I'd expect the digest to change since some of the fields are not used in the OCI format.
There was a problem hiding this comment.
I have an idea, I need to check it out 💡
There was a problem hiding this comment.
we can also use https://github.com/ahupp/python-magic to identify the file type but that's adding a dependency
I looked into the code and unfortunately they do not distinguish between gzipped blob and json blob which looks more like a bug to me https://github.com/containers/image/blob/main/docker/docker_image_dest.go#L171
ipanova
force-pushed
the
i9229
branch
3 times, most recently
from
0a92af6 to
c4b0902
Compare
ipanova
force-pushed
the
i9229
branch
2 times, most recently
from
6e60fce to
dc52145
Compare
CHANGES/9229.bugfix
Outdated
| @@ -0,0 +1 @@ | |||
| Fixed blob media_type during image push. | |||
There was a problem hiding this comment.
@mdellweg I am not sure what to do with it. I learned that the blob_media_type is not important to client at all, instead it cares about the content_type returned, which I've fixed.
I suggest to remove this changelog entry from the PR and deal with 9229 separately - not sure how, we can probably keep blob.media_type field on the model but treat it as informative only, or deprecate entirely.
There was a problem hiding this comment.
If all the bugs are fixed without that, we should remove it and get the rest shipped.
When we realize that we really ignore the media_type field, we can remove it eventually. But if we can keep that migration out of this PR, we get more flexibility for backports. Right?
closes #9571 closes #8303
closes #9229