Remove the manifest schema conversion machinery #1529

git-hyagi · 2024-02-26T19:24:00Z

closes: #1509

lubosmj · 2024-02-26T20:35:35Z

unittest_requirements.txt

@@ -2,3 +2,4 @@ mock
 pytest-django
 pulp-smash @ git+https://github.com/pulp/pulp-smash.git
 pytest<8
+pyjwkest>=1.4,<=1.4.2


Do we still need this dependency?

Hum... I probably had issues before removing the test_convert.py file:
https://github.com/pulp/pulp_container/pull/1529/files#diff-2f5092e427a2a7997e2ce75c8c252cbd9d3d30b33f2ab32dbe1b241194de98bdL6
I just tried again without it and had no dependency errors. Removing it.

lubosmj · 2024-02-26T20:37:07Z

pulp_container/tests/functional/api/test_content_cache.py

@@ -186,6 +192,11 @@ def fetch_response_metadata(self, response):
        if DEFAULT_FILE_STORAGE in STANDARD_FILE_STORAGE_FRAMEWORKS:
            return response.status_code, response.headers.get("X-PULP-CACHE")
        else:
+            """Corner case for deprecated v2 schema1"""


Which corner cases are we catching here? I thought that we were dealing only with manifest v2 schema 2 content inside this test case.

I don't remember now, but it was probably an error in test_05_different_headers.
I'm rerunning the tests without this block to confirm that the "corner case" was actually because of wrong headers in my tests.

lubosmj · 2024-02-26T20:42:55Z

pulp_container/app/registry.py

@@ -231,6 +229,11 @@ async def dispatch_tag(self, request, tag, response_headers):
                streamed back to the client.

        """
+        # return 404 in case the client is requesting docker manifest v2 schema 1


I think this part is not required inside dispatch_tag. How about we remove the code and add the highlighted comment just above the raise PathNotResolved call placed at the end of the get_tag method? So, we will not forget about the rationale behind raising the exception.

I think this part is not required inside dispatch_tag.

Agreed. I added this part when I was having issues with the test_05_different_headers and setting "wrong headers". I'm rerunning the tests without this modification in dispatch_tag and will check if the tests will pass.

How about we remove the code and add the highlighted comment just above the raise PathNotResolved call placed at the end of the get_tag method? So, we will not forget about the rationale behind raising the exception.

Good idea!

ipanova · 2024-02-27T12:25:52Z

you forgot about redirects.py file, we should not 302-->404 but raise 404 right away

ipanova · 2024-02-27T12:16:54Z

CHANGES/1509.removal

@@ -0,0 +1,2 @@
+Removed the manifest schema conversion machinery. This change also removes the ``pyjwkest``


Can we expand more on what it means?
e.g. Removed the manifest schema conversion machinery. If the manifest is stored locally in the newer format and old clients request v2 schema1 manifest they will receive 404.
v2 schema1 manifest is still going to be mirrored from remote source during sync if available and passed to the old clients on the request.

lubosmj · 2024-02-28T11:39:14Z

pulp_container/app/registry_api.py

+        if media_type == MEDIA_TYPE.MANIFEST_V1:
+            raise ManifestNotFound(reference=pk)
+


If a manifest's media type is MEDIA_TYPE.MANIFEST_V1, we should not raise ManifestNotFound. Instead, let's return the manifest because we have got the same media type as requested by a user. Also, the media_type variable can contain comma separated values.

lubosmj · 2024-02-28T11:43:28Z

pulp_container/app/registry_api.py

@@ -962,6 +970,9 @@ def get(self, request, path, pk):
    @RegistryApiCache(base_key=lambda req, cac: find_base_path_cached(req, cac))
    def handle_safe_method(self, request, path, pk):
        """Handles safe requests for Blobs."""
+        # Fail early in case of unsupported manifest schemas
+        self.handle_old_manifest_version(request.accepted_media_type, pk)


We do not care about media types when it comes to blobs. You can safely remove this check here.

lubosmj · 2024-02-28T19:34:18Z

pulp_container/app/redirects.py

+            manifest_media_type not in get_accepted_media_types(self.request.headers)
+            and manifest_media_type != MEDIA_TYPE.MANIFEST_V1
+        ):
+            raise Http404("Unsupported manifest version: %s" % manifest_media_type)


Have you tried raising ManifestNotFound instead?

hum....not yet after this change.
I'll try it!

lubosmj · 2024-02-28T19:35:50Z

CHANGES/1509.removal

+format and old clients request v2 schema1 manifest they will receive 404. v2 schema1 manifest is
+still going to be mirrored from remote source during sync if available and passed to the old clients
+on the request.
+This change also removed the ``pyjwkest`` dependency, since it is not used anymore.


I am not sure if this dependency removal is necessary to announce. @ipanova, what do you think?

lubosmj · 2024-02-29T20:26:53Z

pulp_container/app/redirects.py

+    def issue_tag_redirect(self, tag, pk):
        """
        Issue a redirect for the passed tag.
        """
+        manifest_media_type = tag.tagged_manifest.media_type
+        if (
+            manifest_media_type not in get_accepted_media_types(self.request.headers)
+            and manifest_media_type != MEDIA_TYPE.MANIFEST_V1
+        ):
+            raise ManifestNotFound(reference=pk)


Is it possible to use tag.name instead? If so, there is no need to have a an additional argument.

lubosmj

Would you update the staging variant of the documentation, please?

staging_docs/user/tutorials/01-sync-and-host.md

ipanova · 2024-03-01T11:57:29Z

pulp_container/app/redirects.py

@@ -60,7 +68,8 @@ class S3StorageRedirects(CommonRedirects):

    def issue_tag_redirect(self, tag):
        """
-        Issue a redirect or perform a schema conversion if an accepted media type requires it.
+        Issue a redirect if an accepted media type requires it or return not found if manifest
+        version is not supported.


your changes are not really related but i noticed and think that we should swap the if/elif branches because if we got schema1 we will not return signed schema as we should

closes: pulp#1509

ipanova

looks good,thanks!

ipanova · 2024-03-01T14:52:14Z

@lubosmj seems like pull-through tests is failing on azure, a fluke? https://github.com/pulp/pulp_container/actions/runs/8111223816/job/22170105281?pr=1529#step:14:2184

lubosmj · 2024-03-01T15:05:45Z

@lubosmj seems like pull-through tests is failing on azure, a fluke? https://github.com/pulp/pulp_container/actions/runs/8111223816/job/22170105281?pr=1529#step:14:2184

I have noticed this. It started failing a couple weeks ago. It might be related to #1493 or artifacts/content being already present in the storage and there is a race condition. I will open an issue for that next week if it occurs again.

ipanova · 2024-03-01T15:16:15Z

or artifacts/content being already present in the storage and there is a race condition.

This is test of pull operation and not push, so no.

lubosmj · 2024-03-01T15:17:46Z

Yes, the error is the same as noted in the aforementioned issue:

    raise NotImplementedError("Redirecting to this storage is not implemented.")

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch 2 times, most recently from aa81c74 to 9582ef9 Compare February 26, 2024 19:32

lubosmj reviewed Feb 26, 2024

View reviewed changes

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch from 9582ef9 to 585fea6 Compare February 27, 2024 11:53

ipanova reviewed Feb 27, 2024

View reviewed changes

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch from 585fea6 to eba4735 Compare February 27, 2024 18:38

lubosmj reviewed Feb 28, 2024

View reviewed changes

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch from eba4735 to a30f1d9 Compare February 28, 2024 16:57

lubosmj reviewed Feb 28, 2024

View reviewed changes

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch from a30f1d9 to 68c7b91 Compare February 29, 2024 11:16

lubosmj reviewed Feb 29, 2024

View reviewed changes

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch 2 times, most recently from cb6dcc4 to 8958ff7 Compare February 29, 2024 20:35

ipanova reviewed Mar 1, 2024

View reviewed changes

Remove the manifest schema conversion machinery

8b18ecd

closes: pulp#1509

git-hyagi force-pushed the remove-manifest-schema-conversion-machinery branch from 8958ff7 to 8b18ecd Compare March 1, 2024 12:18

ipanova approved these changes Mar 1, 2024

View reviewed changes

lubosmj merged commit 9c0919f into pulp:main Mar 1, 2024
15 of 16 checks passed

git-hyagi deleted the remove-manifest-schema-conversion-machinery branch March 1, 2024 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the manifest schema conversion machinery #1529

Remove the manifest schema conversion machinery #1529

git-hyagi commented Feb 26, 2024

lubosmj Feb 26, 2024

git-hyagi Feb 27, 2024

lubosmj Feb 26, 2024 •

edited

Loading

git-hyagi Feb 27, 2024

lubosmj Feb 26, 2024

git-hyagi Feb 27, 2024

ipanova commented Feb 27, 2024

ipanova Feb 27, 2024

lubosmj Feb 28, 2024 •

edited

Loading

lubosmj Feb 28, 2024

lubosmj Feb 28, 2024

git-hyagi Feb 28, 2024

lubosmj Feb 28, 2024

ipanova Feb 29, 2024

lubosmj Feb 29, 2024

lubosmj left a comment

ipanova Mar 1, 2024 •

edited

Loading

ipanova left a comment

ipanova commented Mar 1, 2024 •

edited

Loading

lubosmj commented Mar 1, 2024

ipanova commented Mar 1, 2024

lubosmj commented Mar 1, 2024

		@@ -0,0 +1,2 @@
		Removed the manifest schema conversion machinery. This change also removes the ``pyjwkest``

		if media_type == MEDIA_TYPE.MANIFEST_V1:
		raise ManifestNotFound(reference=pk)

Remove the manifest schema conversion machinery #1529

Remove the manifest schema conversion machinery #1529

Conversation

git-hyagi commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lubosmj Feb 26, 2024 • edited Loading

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

ipanova commented Feb 27, 2024

Choose a reason for hiding this comment

lubosmj Feb 28, 2024 • edited Loading

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

lubosmj left a comment

Choose a reason for hiding this comment

ipanova Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

ipanova left a comment

Choose a reason for hiding this comment

ipanova commented Mar 1, 2024 • edited Loading

lubosmj commented Mar 1, 2024

ipanova commented Mar 1, 2024

lubosmj commented Mar 1, 2024

lubosmj Feb 26, 2024 •

edited

Loading

lubosmj Feb 28, 2024 •

edited

Loading

ipanova Mar 1, 2024 •

edited

Loading

ipanova commented Mar 1, 2024 •

edited

Loading