Skip to content

Conversation

@stonezdj
Copy link
Contributor

@stonezdj stonezdj commented Dec 1, 2025

No description provided.

@stonezdj stonezdj force-pushed the 25nov12_proxycache_referers branch from a842058 to 44fac81 Compare December 29, 2025 07:50
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@stonezdj stonezdj force-pushed the 25nov12_proxycache_referers branch from 44fac81 to 92fda34 Compare January 5, 2026 07:40
Copy link
Contributor

@bupd bupd left a comment

Choose a reason for hiding this comment

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

@stonezdj added suggestions.

Thanks

Copy link
Member

@Vad1mo Vad1mo left a comment

Choose a reason for hiding this comment

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

The proposal should be compliant with the OCI spec. The proposal should support OCI registries and not only proxying from Harbor.

@reasonerjt
Copy link
Contributor

reasonerjt commented Jan 8, 2026

@Vad1mo
The intention of the proposal is to enhance the proxy cache feature, which proxies data from upstream registries, so it doesn't break OCI compliance Harbor.

I'm not against the idea of proxying referrers from other upstream registries. I also believe the implementation supports other upstream registries that are OCI-compliant, but I don't think we have the bandwidth to test other registries.

@stonezdj
Copy link
Contributor Author

stonezdj commented Jan 8, 2026

The proposal should be compliant with the OCI spec. The proposal should support OCI registries and not only proxying from Harbor.

@Vad1mo For bandwith limitation, we only test/verify it on Harbor, it doesn't mean it is not comply with OCI spec. any registry follow the OCI spec should work this proxy cache feature.

@Vad1mo Vad1mo self-requested a review January 14, 2026 10:27
Copy link
Member

@Vad1mo Vad1mo left a comment

Choose a reason for hiding this comment

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

I find the proposal incomplete, and from what I understand, it is contradictory to OCI spec.

1. Missing: Referrers Tag Schema Fallback

OCI Spec Requirement:

"If the referrers API returns a 404, the client MUST fallback to pulling the referrers tag schema."

The tag schema format is <alg>-<ref> (e.g., sha256-aaaa... with first 64 chars of digest).

Proposal Gap: The proposal doesn't address this fallback mechanism. When proxying from an upstream registry that doesn't support the referrers API (returns 404), Harbor should:

  1. Attempt the referrers tag schema fallback on the upstream
  2. Return appropriate response to the client

Impact: OCI 1.0 registries and 1.1 registries without referrers API support won't work correctly. This includes many production registries (Docker Hub, older Harbor versions, ECR, GCR, etc.).

2. Missing: Pagination Support

OCI Spec Requirement:

"A Link header MUST be included in the response when the descriptor list cannot be returned in a single manifest."

Proposal Gap: No mention of pagination handling. Questions:

  • How will Harbor handle paginated responses from upstream?
  • Will Harbor aggregate all pages before caching?
  • Will Harbor pass through Link headers to clients?

Impact: Large referrer lists may be truncated or cause errors.

3. Missing: artifactType Filtering

OCI Spec Requirement:

"The registry SHOULD support filtering on artifactType."

Request: GET /v2/<name>/referrers/<digest>?artifactType=<mediaType>
Response header: OCI-Filters-Applied: artifactType

Proposal Gap: The rawQuery parameter in the proposed interface hints at filter support, but no explicit handling is described:

ListReferrers(repository, ref string, rawQuery string) (*v1.Index, error)

@stonezdj stonezdj force-pushed the 25nov12_proxycache_referers branch from a101f0c to cedba72 Compare January 15, 2026 07:46
@stonezdj
Copy link
Contributor Author

stonezdj commented Jan 19, 2026

@Vad1mo

  1. Missing: Referrers Tag Schema Fallback
    Answer: It is the client must fallback, not the proxy cache, the proxy should act like a server, it has no responsibility to faill back to the tag schema format.
  2. Missing: Pagination Support
    Answer: the rawQuery can handle the Paging parameter to the upstream server, will include the 'Link' Header in the response, because it is a common sense, will implement in the code but not mentioned in the proposal specially)
  3. Missing: artifactType Filtering
    Answer: the rawQuery parameter can handle the artifactType Filtering, it passes the artifactType parameter.

@Vad1mo
Copy link
Member

Vad1mo commented Jan 23, 2026

"Because it is complex to merge the referrers from upstream registry and local storage, the first stage implementation only return the referrers from upstream registry. merging the referrers can be considered in future enhancements."

  1. Will the referrer artifacts from upstream be cached locally?
  2. This is not right; we should either make it impossible to add referrers to a proxy cache project or have the merge strategy. The current suggestion breaks the current user workflow and is a breaking change, making local signatures in Harbor-B becomes invisible. The proposal should either implement the merge strategy from the start or explicitly block local referrer creation on proxy cache projects when this option is enabled.

"Only Harbor is officially supported as the upstream registry for proxy cache projects. verification for other registries can be considered in future enhancements."
"The registry client provide a default implementation of the ListReferrers method, any registry support /v2//referrers/ API can use this default implementation in theory."

These two statements contradict each other. - If the implementation will be truly OCI compliant, this should support at least two to three widely used OCI-compliant registries like ghcr/dockerhub/gitlab.
At least one other registry from Harbor should be tested and verified.

"Cache referrers list in redis for future use (in background)."
"If the response code is 200, cache the referrers list in the redis by URL for future use. when upstream registry is not available, the cached referrers list can be returned, if no cached referrers list, return empty referrers."

There are missing technical details on caching, such as
Importantly, how are cache writes handled under concurrent requests?
What is the TTL? for the cached referrers
How is cache invalidated when upstream referrers change?
What is the maximum cache size
What happens if cached data becomes corrupt?

Since this proposal is touching one of the core harbor features. This proposal should be clear on what it adds/solves

@bupd
Copy link
Contributor

bupd commented Jan 25, 2026

I think we can defer this feature to v2.16 given that we are already closer to release date for v2.15.

And unaddressed comments / need for potential testing across different registries for this feature.

Thanks

@stonezdj
Copy link
Contributor Author

stonezdj commented Jan 26, 2026

@Vad1mo

Will the referrer artifacts from upstream be cached locally?

[Answer] Yes, cached in redis

This is not right; we should either make it impossible to add referrers to a proxy cache project or have the merge strategy.
The current suggestion breaks the current user workflow and is a breaking change, making local signatures in Harbor-B becomes invisible. The proposal should either implement the merge strategy from the start or explicitly block local referrer creation on proxy cache projects when this option is enabled.

[Answer] It is not a break change in current implementation, there is an check option "Proxy Referrer API", if the check option is enabled, It will proxy the request to the upstream registry, if the user don't want to proxy to the upstream registry, it will return the local generated referrers list. Merging the referrers can be considered in future enhancements. we have already change the table schema for future use. but we don't have bandwith to implement in v2.15.

These two statements contradict each other. - If the implementation will be truly OCI compliant, this should support at least two to three widely used OCI-compliant registries like ghcr/dockerhub/gitlab.
At least one other registry from Harbor should be tested and verified.

[Answer] statement changed to: 1. Only Harbor will be verified as the upstream registry for proxy cache projects. verification for other registries can be considered in future enhancements.

Importantly, how are cache writes handled under concurrent requests?

[Answer] It is cached in redis, each success request to the upstream is write to redis.

What is the TTL? for the cached referrers

[Answer] TTL is 1 week by default.

How is cache invalidated when upstream referrers change?

[Answer] check by json marshal

What is the maximum cache size

[Answer] the maximun cache size is depends on the memory of the redis.

What happens if cached data becomes corrupt?

[Answer] Return error and will return http 500 error.

Signed-off-by: stonezdj <stonezdj@gmail.com>
@stonezdj stonezdj force-pushed the 25nov12_proxycache_referers branch from 599faa2 to 5bc5050 Compare January 26, 2026 06:21
@bupd
Copy link
Contributor

bupd commented Jan 26, 2026

[Answer] Merging the referrers can be considered in future enhancements. we have already change the table schema for future use. but we don't have bandwith to implement in v2.15

Okay then let's do it 2.16

Don't wanna have a half done feature in harbor

[Answer] statement changed to: 1. Only Harbor will be verified as the upstream registry for proxy cache projects. verification for other registries can be considered in future enhancements.

The answer doesn't actually makes sense

Since the question specifically requested

At least one other registry from Harbor should be tested and verified.

I believe this is oci compliant so atleast one other registry should be verified.

Thanks @stonezdj

@OrlinVasilev
Copy link
Member

@stonezdj @wy65701436 Before merging that we should have agreement from all parties not only Broadcom maintainers.
Also we have merged the goharbor/harbor#22746 which is the actual implementation, before the proposal is approved.
Please follow the governance and respect the maintainers team! Let's reverse the goharbor/harbor#22746

@reasonerjt
Copy link
Contributor

@OrlinVasilev
I agree with you that the code change should be reverted.

@bupd @stonezdj
Let's defer this to 2.16 to make sure merging the referrer list is incorporated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants