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

feat(referrers): delete manifest with subject #174

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,14 @@
using System.Collections.Generic;
using System.Net;
using System.Net.Http;
using System.Text;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;

namespace OrasProject.Oras.Registry.Remote;

public class ResponseException : HttpRequestException
{
{
public static readonly string ErrorCodeNameUnknown = "NAME_UNKNOWN";
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be grouped as an enum for ErrorCode?

public class Error
{
[JsonPropertyName("code")]
Expand All @@ -34,45 +33,45 @@ public class Error

[JsonPropertyName("detail")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]
public JsonElement? Detail { get; set; }
public JsonElement? Detail { get; set; }
}

public class ErrorResponse
{
public class ErrorResponse
{
[JsonPropertyName("errors")]
public required IList<Error> Errors { get; set; }
public required IList<Error> Errors { get; set; }
}

public HttpMethod? Method { get; }

public Uri? RequestUri { get; }

public IList<Error>? Errors { get; }
public ResponseException(HttpResponseMessage response, string? responseBody = null)
: this(response, responseBody, null)
{
}
public ResponseException(HttpResponseMessage response, string? responseBody, string? message)
: this(response, responseBody, response.StatusCode == HttpStatusCode.Unauthorized ? HttpRequestError.UserAuthenticationError : HttpRequestError.Unknown, message, null)
{
}
public ResponseException(HttpResponseMessage response, string? responseBody, HttpRequestError httpRequestError, string? message, Exception? inner)
: base(httpRequestError, message, inner, response.StatusCode)
{
var request = response.RequestMessage;
Method = request?.Method;
RequestUri = request?.RequestUri;
if (responseBody != null)
{
try
{
var errorResponse = JsonSerializer.Deserialize<ErrorResponse>(responseBody);
Errors = errorResponse?.Errors;
}
catch { }
public IList<Error>? Errors { get; }

public ResponseException(HttpResponseMessage response, string? responseBody = null)
: this(response, responseBody, null)
{
}

public ResponseException(HttpResponseMessage response, string? responseBody, string? message)
: this(response, responseBody, response.StatusCode == HttpStatusCode.Unauthorized ? HttpRequestError.UserAuthenticationError : HttpRequestError.Unknown, message, null)
{
}

public ResponseException(HttpResponseMessage response, string? responseBody, HttpRequestError httpRequestError, string? message, Exception? inner)
: base(httpRequestError, message, inner, response.StatusCode)
{
var request = response.RequestMessage;
Method = request?.Method;
RequestUri = request?.RequestUri;
if (responseBody != null)
{
try
{
var errorResponse = JsonSerializer.Deserialize<ErrorResponse>(responseBody);
Errors = errorResponse?.Errors;
}
catch { }
}
}
}
5 changes: 5 additions & 0 deletions src/OrasProject.Oras/Registry/Reference.cs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,11 @@ public static bool TryParse(string reference, [NotNullWhen(true)] out Reference?
}
}

public Reference Clone()
{
return new Reference(Registry, Repository, ContentReference);
}
Comment on lines +204 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

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


public Reference(string registry) => _registry = ValidateRegistry(registry);

public Reference(string registry, string? repository) : this(registry)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public static void VerifyContentDigest(this HttpResponseMessage response, string
}
if (contentDigest != expected)
{
throw new HttpIOException(HttpRequestError.InvalidResponse, $"{response.RequestMessage!.Method} {response.RequestMessage.RequestUri}: invalid response; digest mismatch in Docker-Content-Digest: received {contentDigest} when expecting {digestStr}");
throw new HttpIOException(HttpRequestError.InvalidResponse, $"{response.RequestMessage!.Method} {response.RequestMessage.RequestUri}: invalid response; digest mismatch in Docker-Content-Digest: received {contentDigest} when expecting {expected}");
}
}

Expand Down
73 changes: 72 additions & 1 deletion src/OrasProject.Oras/Registry/Remote/ManifestStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@

public class ManifestStore(Repository repository) : IManifestStore
{
public Repository Repository { get; init; } = repository;

Check warning on line 32 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View workflow job for this annotation

GitHub Actions / Analyze (8.0.x)

Parameter 'Repository repository' is captured into the state of the enclosing type and its value is also used to initialize a field, property, or event.

Check warning on line 32 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View workflow job for this annotation

GitHub Actions / Analyze (8.0.x)

Parameter 'Repository repository' is captured into the state of the enclosing type and its value is also used to initialize a field, property, or event.

Check warning on line 32 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View workflow job for this annotation

GitHub Actions / build (8.0.x)

Parameter 'Repository repository' is captured into the state of the enclosing type and its value is also used to initialize a field, property, or event.

/// <summary>
/// Fetches the content identified by the descriptor.
Expand Down Expand Up @@ -306,7 +306,7 @@
}

// 4. delete the dangling original referrers index, if applicable
await DeleteAsync(oldDesc, cancellationToken).ConfigureAwait(false);

Check warning on line 309 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View workflow job for this annotation

GitHub Actions / Analyze (8.0.x)

Possible null reference argument for parameter 'target' in 'Task ManifestStore.DeleteAsync(Descriptor target, CancellationToken cancellationToken = default(CancellationToken))'.

Check warning on line 309 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View workflow job for this annotation

GitHub Actions / build (8.0.x)

Possible null reference argument for parameter 'target' in 'Task ManifestStore.DeleteAsync(Descriptor target, CancellationToken cancellationToken = default(CancellationToken))'.
}

/// <summary>
Expand Down Expand Up @@ -397,5 +397,76 @@
/// <param name="cancellationToken"></param>
/// <returns></returns>
public async Task DeleteAsync(Descriptor target, CancellationToken cancellationToken = default)
=> await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
=> await DeleteWithIndexing(target, cancellationToken).ConfigureAwait(false);

/// <summary>
/// DeleteWithIndexing deletes the specified target (Descriptor) from the repository,
/// handling referrer indexing if necessary.
/// </summary>
/// <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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the method modifier be private instead of internal?

{
switch (target.MediaType)
{
case MediaType.ImageManifest:
case MediaType.ImageIndex:
if (Repository.ReferrersState == Referrers.ReferrersState.Supported)
{
// referrers API is available, no client-side indexing needed
await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
return;
}
var manifest = await FetchAsync(target, cancellationToken).ConfigureAwait(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

potential resource leak as the manifest is never disposed.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

await IndexReferrersForDelete(target, manifest, cancellationToken).ConfigureAwait(false);
break;
}
await Repository.DeleteAsync(target, true, cancellationToken).ConfigureAwait(false);
}

/// <summary>
/// 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
/// </summary>
/// <param name="target"></param>
/// <param name="manifestContent"></param>
/// <param name="cancellationToken"></param>
private async Task IndexReferrersForDelete(Descriptor target, Stream manifestContent, CancellationToken cancellationToken = default)
{
Descriptor subject;
switch (target.MediaType)
{
case MediaType.ImageManifest:
var imageManifest = JsonSerializer.Deserialize<Manifest>(manifestContent);
if (imageManifest?.Subject == null)
{
// no subject, no indexing needed
return;
}
subject = imageManifest.Subject;
break;
case MediaType.ImageIndex:
var imageIndex = JsonSerializer.Deserialize<Index>(manifestContent);
if (imageIndex?.Subject == null)
{
// no subject, no indexing needed
return;
}
subject = imageIndex.Subject;
break;
default:
return;

Check warning on line 460 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L460 was not covered by tests
}

var isReferrersSupported = Repository.PingReferrers(cancellationToken);
if (isReferrersSupported)
{

Check warning on line 465 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L465 was not covered by tests
// referrers API is available, no client-side indexing needed
return;

Check warning on line 467 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L467 was not covered by tests
}
await UpdateReferrersIndex(subject, new Referrers.ReferrerChange(target, Referrers.ReferrerOperation.Delete), cancellationToken)
.ConfigureAwait(false);
}
}
4 changes: 2 additions & 2 deletions src/OrasProject.Oras/Registry/Remote/Referrers.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@
// limitations under the License.

using System.Collections.Generic;
using System.Linq;
using OrasProject.Oras.Content;
using OrasProject.Oras.Exceptions;
using OrasProject.Oras.Oci;

namespace OrasProject.Oras.Registry.Remote;
Expand All @@ -29,6 +27,8 @@ internal enum ReferrersState
}

internal record ReferrerChange(Descriptor Referrer, ReferrerOperation ReferrerOperation);

internal const string ZeroDigest = "sha256:0000000000000000000000000000000000000000000000000000000000000000";

internal enum ReferrerOperation
{
Expand Down
65 changes: 65 additions & 0 deletions src/OrasProject.Oras/Registry/Remote/Repository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@

private RepositoryOptions _opts;

private static readonly Object _referrersPingLock = new();

/// <summary>
/// Creates a client to the remote repository identified by a reference
/// Example: localhost:5000/hello-world
Expand Down Expand Up @@ -369,6 +371,69 @@
public async Task MountAsync(Descriptor descriptor, string fromRepository, Func<CancellationToken, Task<Stream>>? getContent = null, CancellationToken cancellationToken = default)
=> await ((IMounter)Blobs).MountAsync(descriptor, fromRepository, getContent, cancellationToken).ConfigureAwait(false);

/// <summary>
/// PingReferrers returns true if the Referrers API is available for the repository,
/// otherwise returns false
/// </summary>
/// <param name="cancellationToken"></param>
/// <returns></returns>
/// <exception cref="ResponseException"></exception>
/// <exception cref="Exception"></exception>
internal bool PingReferrers(CancellationToken cancellationToken = default)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the modifier be private instead of internal?

{
switch (ReferrersState)
{
case Referrers.ReferrersState.Supported:
return true;

Check warning on line 387 in src/OrasProject.Oras/Registry/Remote/Repository.cs

View check run for this annotation

Codecov / codecov/patch

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

Added line #L387 was not covered by tests
case Referrers.ReferrersState.NotSupported:
return false;
}

lock (_referrersPingLock)
{
// referrers state is unknown
// lock to limit the rate of pinging referrers API
if (ReferrersState == Referrers.ReferrersState.Supported)
{
return true;

Check warning on line 398 in src/OrasProject.Oras/Registry/Remote/Repository.cs

View check run for this annotation

Codecov / codecov/patch

src/OrasProject.Oras/Registry/Remote/Repository.cs#L397-L398

Added lines #L397 - L398 were not covered by tests
}

if (ReferrersState == Referrers.ReferrersState.NotSupported)
{
return false;

Check warning on line 403 in src/OrasProject.Oras/Registry/Remote/Repository.cs

View check run for this annotation

Codecov / codecov/patch

src/OrasProject.Oras/Registry/Remote/Repository.cs#L402-L403

Added lines #L402 - L403 were not covered by tests
}

var reference = Options.Reference.Clone();
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()
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

.GetResult();

switch (response.StatusCode)
{
case HttpStatusCode.OK:
var supported = response.Content.Headers.ContentType?.MediaType == MediaType.ImageIndex;
SetReferrersState(supported);
return supported;
case HttpStatusCode.NotFound:
var err = (ResponseException)response.ParseErrorResponseAsync(cancellationToken)
.ConfigureAwait(true).GetAwaiter().GetResult();
if (err.Errors?.First().Code == ResponseException.ErrorCodeNameUnknown)
{
// referrer state is unknown because the repository is not found
throw err;
}

SetReferrersState(false);
return false;
default:
throw response.ParseErrorResponseAsync(cancellationToken).ConfigureAwait(true).GetAwaiter()
.GetResult();
}
}
}

/// <summary>
/// SetReferrersState indicates the Referrers API state of the remote repository. true: supported; false: not supported.
/// SetReferrersState is valid only when it is called for the first time.
Expand Down
21 changes: 21 additions & 0 deletions src/OrasProject.Oras/Registry/Remote/UriFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

using OrasProject.Oras.Exceptions;
using System;
using System.Web;

namespace OrasProject.Oras.Registry.Remote;

Expand Down Expand Up @@ -102,6 +103,26 @@ public Uri BuildRepositoryBlobUpload()
return builder.Uri;
}

/// <summary>
/// Builds the URL for accessing the Referrers API
/// Format: <scheme>://<registry>/v2/<repository>/referrers/<reference>?artifactType=<artifactType>
/// </summary>
/// <param name="artifactType"></param>
/// <returns></returns>
public Uri BuildReferrersUrl(string? artifactType = null)
{
var builder = NewRepositoryBaseBuilder();
builder.Path += $"/referrers/{_reference.ContentReference}";
if (!string.IsNullOrEmpty(artifactType))
{
var query = HttpUtility.ParseQueryString(builder.Query);
query.Add("artifactType", artifactType);
builder.Query = query.ToString();
}

return builder.Uri;
}

/// <summary>
/// Generates a UriBuilder with the base endpoint of the remote repository.
/// Format: <scheme>://<registry>/v2/<repository>
Expand Down
Loading
Loading