Catalog content discovery #2782
Conversation
Signed-off-by: Eusebiu Petu <[email protected]>
Signed-off-by: Eusebiu Petu <[email protected]>
eusebiu-constantin-petu-dbk
requested review from
rchincha and
andaaron
as code owners
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
2 times, most recently
from
7216e94
to
e08544f
Compare
Signed-off-by: Eusebiu Petu <[email protected]>
eusebiu-constantin-petu-dbk
force-pushed
the
catalog_content_discovery
branch
from
e08544f
to
6619b7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2782 +/- ##
==========================================
- Coverage 92.07% 91.86% -0.22%
==========================================
Files 169 170 +1
Lines 30054 30260 +206
==========================================
+ Hits 27672 27797 +125
- Misses 1762 1833 +71
- Partials 620 630 +10 ☔ View full report in Codecov by Sentry. |
| return catalog.Repositories, nil | ||
| repos := catalog.Repositories | ||
|
|
||
| link := header.Get("Link") |
There was a problem hiding this comment.
So these repos return the "Link" even if we don't specify the query parameters in the request?
There was a problem hiding this comment.
According to the specification: https://github.com/opencontainers/distribution-spec/blob/v1.1.0/spec.md#content-discovery
When n is zero, this endpoint MUST return an empty list, and MUST NOT include a Link header.
Since we are not setting it to 0, its probably not an issue. However it also states further down that:
A Link header MUST be included in the response when the descriptor list cannot be returned in a single manifest. Each response is an image index with different descriptors in the manifests field. The Link header MUST be set according to RFC5988 with the Relation Type rel="next".
So technically if there is only 1 page, it is not required to be in the response if I get it right. But according to https://pkg.go.dev/net/http#Header.Get an empty string is returned when there is no value associated with the requested header - or it does not exist. So I kinda guess this is safe.
|
I gave it a try and found some problem I think. I updated the original issue: #2715 (comment) and will take a deeper look. |
| constants.RoutePrefix, constants.ExtCatalogPrefix) | ||
| if err != nil { | ||
| return []string{}, err | ||
| } | ||
|
|
||
| return catalog.Repositories, nil | ||
| repos := catalog.Repositories |
There was a problem hiding this comment.
I think the problem of duplicating the 2nd page in place of the 1st page in the final list starts here. Further down catalog is passed down as a reference as well and the Repositories property is overwritten with the 2nd page result, thus repos get "overwritten" as well. Further iterations in the loop work, because at append(...) the list capacity need to expand and a new list is reserved in memory after that.
This way the behaviour is flaky. Its just a matter of luck to some extent when the list gets expanded and which page get lost or overwritten.
| constants.RoutePrefix, constants.ExtCatalogPrefix) | ||
| if err != nil { | ||
| return []string{}, err | ||
| } | ||
|
|
||
| return catalog.Repositories, nil | ||
| repos := catalog.Repositories |
There was a problem hiding this comment.
I think simply constructing an empty array and keep appending to that should do the trick:
| repos := catalog.Repositories | |
| var repos []string | |
| repos = append(repos, catalog.Repositories...) |
|
Also be mindful about access control - one may not have read access to all repos. |
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades?
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.