-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
Changes from all commits
e3e1217
846e173
7fca0cb
ec93662
7809549
c7e4418
a6d552f
685ab70
1b9ade3
5041592
c26ccb6
ddbf048
d6e8499
cf431f0
50e696e
856c0ef
fcb121e
2b24e07
7df36ca
36a428f
1813869
878535c
4bee2cc
c848ed2
a7460f3
20dcda9
d7eb9b5
dfc538d
ff334ca
53f8e12
b13de61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be grouped as an enum for |
||
public class Error | ||
{ | ||
[JsonPropertyName("code")] | ||
|
@@ -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 { } | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
public Reference(string registry) => _registry = ValidateRegistry(registry); | ||
|
||
public Reference(string registry, string? repository) : this(registry) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 GitHub Actions / Analyze (8.0.x)
Check warning on line 32 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs GitHub Actions / Analyze (8.0.x)
Check warning on line 32 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs GitHub Actions / build (8.0.x)
|
||
|
||
/// <summary> | ||
/// Fetches the content identified by the descriptor. | ||
|
@@ -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 GitHub Actions / Analyze (8.0.x)
Check warning on line 309 in src/OrasProject.Oras/Registry/Remote/ManifestStore.cs GitHub Actions / build (8.0.x)
|
||
} | ||
|
||
/// <summary> | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the method modifier be |
||
{ | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. potential resource leak as the manifest is never disposed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The manifest is not verified against the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
var isReferrersSupported = Repository.PingReferrers(cancellationToken); | ||
if (isReferrersSupported) | ||
{ | ||
// referrers API is available, no client-side indexing needed | ||
return; | ||
} | ||
await UpdateReferrersIndex(subject, new Referrers.ReferrerChange(target, Referrers.ReferrerOperation.Delete), cancellationToken) | ||
.ConfigureAwait(false); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the modifier be |
||
{ | ||
switch (ReferrersState) | ||
{ | ||
case Referrers.ReferrersState.Supported: | ||
return true; | ||
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; | ||
} | ||
|
||
if (ReferrersState == Referrers.ReferrersState.NotSupported) | ||
{ | ||
return false; | ||
} | ||
|
||
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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we await this function and make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
.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. | ||
|
There was a problem hiding this comment.
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 theResponseException
is scoped to remote server only.