-
Notifications
You must be signed in to change notification settings - Fork 5
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!: add referrersHandler for artifact manifest #34
feat!: add referrersHandler for artifact manifest #34
Conversation
I have run a basic test.
Test result:
Prettified returned result:
|
Codecov Report
@@ Coverage Diff @@
## main #34 +/- ##
==========================================
- Coverage 57.16% 56.48% -0.69%
==========================================
Files 107 108 +1
Lines 11041 11176 +135
==========================================
+ Hits 6312 6313 +1
- Misses 4019 4153 +134
Partials 710 710
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
registry/handlers/referrers.go
Outdated
v1 "github.com/opencontainers/image-spec/specs-go/v1" | ||
) | ||
|
||
const createAnnotationName = "io.cncf.oras.artifact.created" |
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.
Should this be OCI version?
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.
Changed to "org.opencontainers.artifact.created"
.
return err | ||
} | ||
|
||
artifactManifest, ok := man.(*ociartifact.DeserializedManifest) |
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.
Do we want to cover OCI image manifest referrers in this PR?
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.
We would do that in a separate pr. For this pr we only covers OCI artifact manifest. Thanks for pointing this out.
registry/handlers/referrers.go
Outdated
} | ||
|
||
w.Header().Set("Content-Type", v1.MediaTypeImageIndex) | ||
w.Header().Set("ORAS-Api-Version", "oras/1.0") |
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.
Is this still relevant?
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.
removed it.
registry/handlers/referrers.go
Outdated
} | ||
|
||
func generateLinkHeader(repoName, subjectDigest, artifactType string, lastDigests []string, nPage int) string { | ||
url := fmt.Sprintf("/v2/%s/_oras/artifacts/referrers?digest=%s&nextToken=%s", |
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.
The url needs to be updated
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.
updated this url.
@wangxiaoxuan273 Could you split this PR into two PRs? One for indexing the newly pushed OCI Artifact manifest. One for query the referrers API? |
registry/handlers/referrers.go
Outdated
v1 "github.com/opencontainers/image-spec/specs-go/v1" | ||
) | ||
|
||
const createAnnotationName = "org.opencontainers.artifact.created" |
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.
@shizhMSFT Is this variable okay? I didn't find any related info about this in the spec.
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.
Try searching org.opencontainers.artifact.created
in https://github.com/opencontainers/distribution-spec/blob/main/spec.md
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.
Note: createAnnotationName
for Artifact manifest and image manifest are different.
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.
Found relevant information in this page. Added doc comments in the code.
registry/handlers/referrers.go
Outdated
} | ||
|
||
func generateLinkHeader(repoName, subjectDigest, artifactType string, lastDigests []string, nPage int) string { | ||
url := fmt.Sprintf("/v2/%s/_referrers/manifests/referrers?digest=%s&nextToken=%s", |
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.
@shizhMSFT Is this url okay?
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.
The URL is defined in https://github.com/opencontainers/distribution-spec/blob/main/spec.md#listing-referrers
There is no such a field called nextToken
.
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.
Therefore, nextToken
is an implementation specific field. You might want to document this somewhere.
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 think we may not need the nextToken
field. I've created an issue to keep track of the paging feature of referrers, and I'll look deeper into it when fixing that issue.
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.
Updated the url according to the distribution spec.
registry/storage/paths.go
Outdated
var ( | ||
rootPrefix = []string{storagePathRoot, storagePathVersion} | ||
repoPrefix = append(rootPrefix, "repositories") | ||
) |
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.
You may want to put it back as local variables.
Think about the following code.
storagePathVersion := "v2"
storagePathRoot := "/docker/registry/"
rootPrefix := []string{storagePathRoot, storagePathVersion}
repoPrefix := append(rootPrefix, "repositories")
pathA := append(repoPrefix, "foo")
pathB := append(repoPrefix, "bar")
fmt.Println(pathA)
fmt.Println(pathB)
The output is
[/docker/registry/ v2 repositories bar]
[/docker/registry/ v2 repositories bar]
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.
Put it back.
registry/handlers/referrers.go
Outdated
// Reference for pre-defined annotation keys: | ||
// https://github.com/opencontainers/image-spec/blob/main/annotations.md#pre-defined-annotation-keys | ||
const createAnnotationNameArtifactManifest = "org.opencontainers.artifact.created" | ||
const createAnnotationTimestampFormat = time.RFC3339 | ||
const maxPageSize = 100 | ||
|
||
// minimum page size used for # of digests to put in nextToken | ||
const minPageSize = 3 |
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.
You may want to make them const blocks.
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.
changed.
registry/handlers/referrers.go
Outdated
artifactType := r.FormValue("artifactType") | ||
nPage, nParseError := strconv.Atoi(r.FormValue("n")) |
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.
Do FormValue
apply to GET requests?
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.
replaced FormValue
with r.URL.Query()
registry/handlers/referrers.go
Outdated
} | ||
} | ||
|
||
func (h *referrersHandler) generateSortedReferrers(ctx context.Context, revision digest.Digest, artifactType string) ([]v1.Descriptor, error) { |
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.
Does OCI Referrer Spec specifies sorting? If so, please point out the spec reference.
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.
Removed sorting
registry/handlers/referrers.go
Outdated
func referrersDispatcher(ctx *Context, r *http.Request) http.Handler { | ||
dgst, err := getDigest(ctx) | ||
if err != nil { | ||
|
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.
nit: extra blank line?
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.
removed
registry/handlers/referrers.go
Outdated
} | ||
|
||
// ReferrersAPIResponse describes the response body of the referrers API. | ||
type ReferrersAPIResponse struct { |
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.
Looks like we are still missing annotations
field?
Btw, can we reuse v1.Index
?
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.
Agree that v1.Index
looks better.
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.
Yes, I forgot annotations
. Reused v1.Index
here instead and added annotations.
registry/handlers/referrers.go
Outdated
} | ||
|
||
nextTokens := query["nextToken"] | ||
TokenSet := make(map[string]string) |
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.
Why does this var start with capital letter?
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.
This variable is from the original rc2 branch implementation. Removed it now as it is for the paging feature, and paging will be implemented in a separate pr.
registry/handlers/referrers.go
Outdated
} | ||
|
||
func generateLinkHeader(repoName, subjectDigest, artifactType string, lastDigests []string, nPage int) string { | ||
url := fmt.Sprintf("/v2/%s/referrers/%s?nextToken=%s", |
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.
url.Values() may help here.
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.
removed this function as it is for paging and paging will be implemented in another pr.
registry/handlers/referrers.go
Outdated
dcontext.GetLogger(h).Debug("GetReferrers") | ||
|
||
query := r.URL.Query() | ||
n, nExists := query["n"] |
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.
Should use Values.Get() method to avoid array boundary issues.
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.
Replaced with Get()
.
registry/handlers/referrers.go
Outdated
} | ||
|
||
// ReferrersAPIResponse describes the response body of the referrers API. | ||
type ReferrersAPIResponse struct { |
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.
Agree that v1.Index
looks better.
registry/handlers/referrers.go
Outdated
} | ||
|
||
nextTokens := query["nextToken"] | ||
TokenSet := make(map[string]string) |
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.
Since it is a set, value is never used.
TokenSet := make(map[string]string) | |
tokenSet := make(map[string]struct{}) |
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.
removed this part as it is part of the paging feature.
registry/handlers/referrers.go
Outdated
func enumerateReferrerLinks(ctx context.Context, | ||
rootPath string, | ||
stDriver driver.StorageDriver, | ||
repository distribution.Repository, | ||
blobstatter distribution.BlobStatter, | ||
subjectRevision digest.Digest, | ||
artifactManifestIndex map[digest.Digest][]digest.Digest, | ||
ingestor func(ctx context.Context, | ||
digest digest.Digest, | ||
subjectRevision digest.Digest, | ||
artifactManifestIndex map[digest.Digest][]digest.Digest, | ||
repository distribution.Repository, | ||
blobstatter distribution.BlobStatter, | ||
storageDriver driver.StorageDriver) error) error { |
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.
This function declaration does not seem right.
It can be simplified as
func enumerateReferrerLinks(ctx context.Context, | |
rootPath string, | |
stDriver driver.StorageDriver, | |
repository distribution.Repository, | |
blobstatter distribution.BlobStatter, | |
subjectRevision digest.Digest, | |
artifactManifestIndex map[digest.Digest][]digest.Digest, | |
ingestor func(ctx context.Context, | |
digest digest.Digest, | |
subjectRevision digest.Digest, | |
artifactManifestIndex map[digest.Digest][]digest.Digest, | |
repository distribution.Repository, | |
blobstatter distribution.BlobStatter, | |
storageDriver driver.StorageDriver) error) error { | |
func enumerateReferrerLinks(ctx context.Context, | |
rootPath string, | |
stDriver driver.StorageDriver, | |
blobstatter distribution.BlobStatter, | |
ingestor func(digest digest.Digest) error) error { |
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.
Changed accordingly. Thanks for the work.
registry/handlers/referrers.go
Outdated
err = ingestor(ctx, | ||
digest, | ||
subjectRevision, | ||
artifactManifestIndex, | ||
repository, | ||
blobstatter, | ||
stDriver) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return nil |
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.
When you see the pattern
err = do()
if err != nil {
return err
}
return nil
it can be simplified as
return do()
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.
changed accordingly.
registry/handlers/referrers.go
Outdated
// only consider pagination if # of referrers is greater than page size | ||
if len(referrers) > nPage { | ||
startIndex := 0 | ||
if len(TokenSet) > 0 { | ||
for i, ref := range referrers { | ||
// check if ref matches a digest in nextToken list | ||
if _, ok := TokenSet[ref.Digest.String()]; ok { | ||
// set the starting index to the largest index | ||
if (i + 1) > startIndex { | ||
startIndex = i + 1 | ||
} | ||
} | ||
} | ||
} | ||
|
||
// only applicable if last item provided as nextToken | ||
if startIndex == len(referrers) { | ||
referrers = []v1.Descriptor{} | ||
} | ||
|
||
// if there's only 1 page of results left | ||
if len(referrers)-startIndex <= nPage { | ||
referrers = referrers[startIndex:] | ||
} else { | ||
referrers = referrers[startIndex:(startIndex + nPage)] | ||
// nextToken is a base64 encoded comma-separated string of the digests of the last three referrers in the response | ||
var nextDgsts []string | ||
for i := nPage - 1; i >= nPage-minPageSize; i-- { | ||
nextDgsts = append(nextDgsts, referrers[i].Digest.String()) | ||
} | ||
// if n was not provided in page, set nPage to a value so link header knows not to include n | ||
if nParseError != nil { | ||
nPage = -1 | ||
} | ||
// add the Link Header | ||
w.Header().Set("Link", generateLinkHeader(h.Repository.Named().Name(), h.Digest.String(), filterArtifactType, nextDgsts, nPage)) | ||
} | ||
} |
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.
The next token implementation is strange to me. Could you explain why you choose this algorithm?
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.
Removed the paging part, will do it in a future pr.
Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
Signed-off-by: wangxiaoxuan273 <[email protected]>
registry/handlers/referrers.go
Outdated
linked, err := digest.Parse(string(content)) | ||
if err != nil { | ||
return "", err | ||
} | ||
|
||
return linked, nil |
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.
code pattern can be applied.
linked, err := digest.Parse(string(content)) | |
if err != nil { | |
return "", err | |
} | |
return linked, nil | |
return digest.Parse(string(content)) |
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.
changed accordingly.
registry/handlers/referrers.go
Outdated
annotations := map[string]string{} | ||
query := r.URL.Query() | ||
artifactTypeFilter := query.Get("artifactType") | ||
if artifactTypeFilter != "" { | ||
annotations[v1.AnnotationReferrersFiltersApplied] = "artifactType" | ||
} |
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.
note that nil
map with omitempty
will be omitted. However, empty map will be encoded to {}
.
Here, if artifactType
is not there, we should use nil
map.
Consider this code pattern
annotations := map[string]string{} | |
query := r.URL.Query() | |
artifactTypeFilter := query.Get("artifactType") | |
if artifactTypeFilter != "" { | |
annotations[v1.AnnotationReferrersFiltersApplied] = "artifactType" | |
} | |
var annotations map[string]string | |
artifactTypeFilter := r.URL.Query().Get("artifactType") | |
if artifactTypeFilter != "" { | |
annotations = map[string]string{ | |
v1.AnnotationReferrersFiltersApplied: "artifactType", | |
} | |
} |
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.
changed accordingly.
registry/handlers/referrers.go
Outdated
|
||
func (h *referrersHandler) generateReferrersList(ctx context.Context, subjectDigest digest.Digest, artifactType string) ([]v1.Descriptor, error) { | ||
dcontext.GetLogger(ctx).Debug("(*referrersHandler).generateReferrersList") | ||
var referrers []v1.Descriptor |
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.
nit: code style: let's only declare variables when needed.
suggestion: move this line after line 99 but before line 102
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.
moved accordingly.
Signed-off-by: wangxiaoxuan273 <[email protected]>
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.
LGTM
This pr implements referrersHandler and its related operations. For this pr only artifact manifest referrers are supported. Support for image manifest referrers will be added in a future pr. The implementation is based on the existing rc2 branch, which implements referrersHandler as an extension. Changes the
registry/handler
package.Part 7 of #21.