feat(referrers): delete manifest with subject #174

pat-pan · 2025-01-06T22:56:25Z

What this PR does / why we need it

The PR is to implement the feature to delete manifest with subject as per the OCI distribution spec v1.1.0.

Which issue(s) this PR resolves / fixes

Resolves / Fixes #160

Please check the following list

Does the affected code have corresponding tests, e.g. unit test, E2E test?
Does this change require a documentation update?
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have an appropriate license header?

Signed-off-by: Patrick Pan <[email protected]>

codecov · 2025-01-06T22:57:20Z

Codecov Report

Attention: Patch coverage is 84.61538% with 16 lines in your changes missing coverage. Please review.

Project coverage is 83.37%. Comparing base (220d7dd) to head (b13de61).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/OrasProject.Oras/Registry/Remote/Repository.cs	75.75%	5 Missing and 3 partials ⚠️
.../OrasProject.Oras/Registry/Remote/ManifestStore.cs	79.41%	3 Missing and 4 partials ⚠️
...c/OrasProject.Oras/Exceptions/ResponseException.cs	95.45%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #174      +/-   ##
==========================================
+ Coverage   83.25%   83.37%   +0.11%     
==========================================
  Files          37       37              
  Lines        1266     1347      +81     
  Branches      149      162      +13     
==========================================
+ Hits         1054     1123      +69     
- Misses        149      156       +7     
- Partials       63       68       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Patrick Pan <[email protected]>

Wwwsylvia · 2025-01-13T10:57:42Z

src/OrasProject.Oras/Exceptions/ResponseException.cs

This can be kept in the original directory.

Wwwsylvia · 2025-01-14T09:57:54Z

src/OrasProject.Oras/Registry/Remote/Referrers.cs

@@ -29,6 +27,8 @@ internal enum ReferrersState
    }

    internal record ReferrerChange(Descriptor Referrer, ReferrerOperation ReferrerOperation);
+
+    internal static readonly string ZeroDigest = "sha256:0000000000000000000000000000000000000000000000000000000000000000";


This can be made const.

Wwwsylvia · 2025-01-14T10:04:02Z

src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

+    /// IndexReferrersForDelete indexes referrers for manifests with a subject field on manifest delete.
+    /// References:
+    ///   - Latest spec: https://github.com/opencontainers/distribution-spec/blob/v1.1.0/spec.md#deleting-manifests
+    ///   - Compatible spec: https://github.com/opencontainers/distribution-spec/blob/v1.1.0-rc1/spec.md#deleting-manifests


nit: We don't need to mention the old spec here.

Wwwsylvia · 2025-01-14T10:19:15Z

src/OrasProject.Oras/Registry/Remote/Repository.cs

+        if (ReferrersState == Referrers.ReferrersState.Supported)
+        {
+            return true;
+        }
+
+        if (ReferrersState == Referrers.ReferrersState.NotSupported)
+        {
+            return false;
+        }


nit: How about using a switch clause?

Suggested change

if (ReferrersState == Referrers.ReferrersState.Supported)

{

return true;

}

if (ReferrersState == Referrers.ReferrersState.NotSupported)

{

return false;

}

switch (ReferrersState)

{

case Referrers.ReferrersState.Supported:

return true;

case Referrers.ReferrersState.NotSupported:

return false;

}

Wwwsylvia · 2025-01-14T10:42:37Z

src/OrasProject.Oras/Registry/Remote/Repository.cs

+            reference.ContentReference = Referrers.ZeroDigest;
+            var url = new UriFactory(reference, Options.PlainHttp).BuildReferrersUrl();
+            var request = new HttpRequestMessage(HttpMethod.Get, url);
+            var response = Options.HttpClient.SendAsync(request, cancellationToken).ConfigureAwait(true).GetAwaiter()


Should we await this function and make PingReferrers async? But async calls are not allowed within the lock statement. We can try SemaphoreSlim.

The SemaphoreSlim does not seem right either. We need to consider a lock-free version.

Signed-off-by: Patrick Pan <[email protected]>

shizhMSFT · 2025-01-15T05:07:02Z

src/OrasProject.Oras/Exceptions/ResponseException.cs

I'm curious on the reason moving this class to Exceptions where that namespace should contain generic exception while the ResponseException is scoped to remote server only.

shizhMSFT · 2025-01-15T05:08:30Z

src/OrasProject.Oras/Exceptions/ResponseException.cs

 using System.Text.Json.Serialization;

 namespace OrasProject.Oras.Registry.Remote;

 public class ResponseException : HttpRequestException
-{
+{
+    public static readonly string ErrorCodeNameUnknown = "NAME_UNKNOWN";


Should it be grouped as an enum for ErrorCode?

shizhMSFT · 2025-01-15T05:15:45Z

src/OrasProject.Oras/Registry/Reference.cs

+    public Reference Clone()
+    {
+        return new Reference(Registry, Repository, ContentReference);
+    }


This seems not be a good pattern as it does not meet the IClonable. You may consider about the copy constructor.

shizhMSFT · 2025-01-15T05:22:44Z

src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

+    /// <param name="target">The target descriptor to delete.</param>
+    /// <param name="cancellationToken">A cancellation token to cancel the operation if needed. Defaults to default.</param>
+    /// <returns></returns>
+    internal async Task DeleteWithIndexing(Descriptor target, CancellationToken cancellationToken = default)


Can the method modifier be private instead of internal?

shizhMSFT · 2025-01-15T05:28:49Z

src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

+                    await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
+                    return;
+                }
+                var manifest = await FetchAsync(target, cancellationToken).ConfigureAwait(false);


potential resource leak as the manifest is never disposed.

shizhMSFT · 2025-01-15T05:29:44Z

src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

+                    await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
+                    return;
+                }
+                var manifest = await FetchAsync(target, cancellationToken).ConfigureAwait(false);


The manifest is not verified against the target descriptor. It is possible that a corrupted manifest is fetched.

shizhMSFT · 2025-01-15T05:33:56Z

src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

+                    await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
+                    return;
+                }
+                var manifest = await FetchAsync(target, cancellationToken).ConfigureAwait(false);


The download size of the manifest is not limited / well-guarded. It means it is vulnerable to excessive resource attack.

shizhMSFT · 2025-01-15T05:37:55Z

src/OrasProject.Oras/Registry/Remote/Repository.cs

+    /// <returns></returns>
+    /// <exception cref="ResponseException"></exception>
+    /// <exception cref="Exception"></exception>
+    internal bool PingReferrers(CancellationToken cancellationToken = default)


Can the modifier be private instead of internal?

Patrick Pan added 23 commits November 18, 2024 18:16

push manifest with subject

e3e1217

Signed-off-by: Patrick Pan <[email protected]>

add unit tests

846e173

Signed-off-by: Patrick Pan <[email protected]>

add unit tests

7fca0cb

Signed-off-by: Patrick Pan <[email protected]>

Merge branch 'main' into feature/pushManifestWithSubject

ec93662

resolve merge conflicts

7809549

Signed-off-by: Patrick Pan <[email protected]>

add tests

c7e4418

Signed-off-by: Patrick Pan <[email protected]>

add comments

a6d552f

Signed-off-by: Patrick Pan <[email protected]>

add unit test

685ab70

Signed-off-by: Patrick Pan <[email protected]>

add unit tests

1b9ade3

Signed-off-by: Patrick Pan <[email protected]>

resolve comments

5041592

Signed-off-by: Patrick Pan <[email protected]>

add SetReferrersSupportLevel func and unit tests

c26ccb6

Signed-off-by: Patrick Pan <[email protected]>

remove NoReferrerUpdateException and update tests accordingly

ddbf048

Signed-off-by: Patrick Pan <[email protected]>

add Index constructor

d6e8499

Signed-off-by: Patrick Pan <[email protected]>

add license header

cf431f0

Signed-off-by: Patrick Pan <[email protected]>

resolve merge conflicts

50e696e

Signed-off-by: Patrick Pan <[email protected]>

add lock on SetReferrerState

856c0ef

Signed-off-by: Patrick Pan <[email protected]>

simplify ApplyReferrerChanges

fcb121e

Signed-off-by: Patrick Pan <[email protected]>

resolve comments

2b24e07

Signed-off-by: Patrick Pan <[email protected]>

Merge branch 'main' into feature/pushManifestWithSubject

7df36ca

Signed-off-by: Patrick Pan <[email protected]>

delete manifest with subject

36a428f

Signed-off-by: Patrick Pan <[email protected]>

add reference clone

1813869

Signed-off-by: Patrick Pan <[email protected]>

change variable naming

878535c

Signed-off-by: Patrick Pan <[email protected]>

fix merge conflict

4bee2cc

Signed-off-by: Patrick Pan <[email protected]>

pat-pan requested review from sammychinedu2ky, shizhMSFT and Wwwsylvia as code owners January 6, 2025 22:56

Patrick Pan added 3 commits January 7, 2025 10:03

polish pr

c848ed2

Signed-off-by: Patrick Pan <[email protected]>

fix responseException style

a7460f3

Signed-off-by: Patrick Pan <[email protected]>

Merge branch 'main' into feature/deleteManifestWithSubject

20dcda9

Patrick Pan added 4 commits January 13, 2025 15:22

add unit tests

d7eb9b5

Signed-off-by: Patrick Pan <[email protected]>

remove unnecessary changes

dfc538d

Signed-off-by: Patrick Pan <[email protected]>

remove unnecessary changes

ff334ca

Signed-off-by: Patrick Pan <[email protected]>

move ResponseException to Exceptions folder

53f8e12

Signed-off-by: Patrick Pan <[email protected]>

Wwwsylvia reviewed Jan 14, 2025

View reviewed changes

resolve comments

b13de61

Signed-off-by: Patrick Pan <[email protected]>

shizhMSFT reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(referrers): delete manifest with subject #174

feat(referrers): delete manifest with subject #174

pat-pan commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

Wwwsylvia Jan 13, 2025 •

edited

Loading

Wwwsylvia Jan 14, 2025

Wwwsylvia Jan 14, 2025

Wwwsylvia Jan 14, 2025

Wwwsylvia Jan 14, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

shizhMSFT Jan 15, 2025

feat(referrers): delete manifest with subject #174

Are you sure you want to change the base?

feat(referrers): delete manifest with subject #174

Conversation

pat-pan commented Jan 6, 2025 • edited Loading

What this PR does / why we need it

Which issue(s) this PR resolves / fixes

Please check the following list

codecov bot commented Jan 6, 2025 • edited Loading

Codecov Report

Wwwsylvia Jan 13, 2025 • 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

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

pat-pan commented Jan 6, 2025 •

edited

Loading

codecov bot commented Jan 6, 2025 •

edited

Loading

Wwwsylvia Jan 13, 2025 •

edited

Loading