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

Add manifestcache push for tag and digest to local repository #21141

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

raphaelzoellner
Copy link

@raphaelzoellner raphaelzoellner commented Nov 5, 2024

Comprehensive Summary of your change

Harbor Proxies will push a manifest referencing a digest and tag to the local repo if both are known.

Issue being fixed

Fixes #21122

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

Signed-off-by: Raphael Zöllner <raphael.zoellner@regiocom.com>
@stonezdj
Copy link
Contributor

Thanks for your contribution! because the current code change doesn't fix the issue #21122, and might result in regression issue, so we will close this PR.

@stonezdj stonezdj closed this Nov 11, 2024
@raphaelzoellner
Copy link
Author

Hello @stonezdj,

could you give a hint why this code change (creating the manifest referenced by the tag) wouldn't solve or at least improve the described behavior in #21122, so one can think about another solution?

Is this maybe a misunderstanding with the PR #21123, which is already closed?

}
}

func (m *ManifestCache) push(art lib.ArtifactInfo, man distribution.Manifest) error {
Copy link
Contributor

@stonezdj stonezdj Nov 27, 2024

Choose a reason for hiding this comment

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

You have replace the m.local.PushManifest(art.Repository, getReference(art), man) with the m.push(art, man),
the intention of this method is push the manifest to the proxy cache project if there is any tag exist in the art, current the art is parsing from the pull request, for a normal image pull request, it is pulled either by digest or by tag. it can't be both.
The previous getReference(art) will return tag if it pull by tag, and return digest if it is pull by digest.
Current code change has no difference with the previous implementation.

Copy link
Author

Choose a reason for hiding this comment

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

You have replace the m.local.PushManifest(art.Repository, getReference(art), man) with the m.push(art, man)
Yes my intention is to push the manifest to the local proxy cache project with any reference known (tag, digest or both).

for a normal image pull request, it is pulled either by digest or by tag. it can't be both.
I agree the GET manifest request either uses a tag or digest as reference, but during the storage of the proxied manifest the digest might be looked up in the remote repository and the manifest is then only stored under the digest in the local proxy cache project.

The previous getReference(art) will return tag if it pull by tag, and return digest if it is pull by digest.
I think there are cases where art will contain both a digest and tag here. In this case the getReference(art) will favor digest over tag, ultimately leading to the manifest being stored in the local proxy cache project only under the digest.

This happens during the proxy controller's ProxyManifest call.

For a manifest pull referencing a tag the digest will be looked up in the remote repository.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L224

Then the digest will be added to the copied artInfo which should until then contain the tag parsed from the the original pull by tag.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L249

This artInfo containing both tag and digest will then be used in the original call to m.local.PushManifest(art.Repository, getReference(art), man), where getReference(art) leads to the digest being favored over the tag.
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L251C1-L251C69
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L318
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/manifestcache.go#L201
https://github.com/goharbor/harbor/blob/v2.10.2/src/controller/proxy/controller.go#L327-L330

My proposal is to push the manifest with both references (digest and tag) if both are known. This should lead to subsequent manifest pulls referencing the tag being served from the local proxy cache project if the digest has not changed to the remote manifest digest for said tag.

@Vad1mo Vad1mo added the release-note/enhancement Label to mark PR to be added under release notes as enhancement label Dec 1, 2024
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.

Approve, that we need that functionality. I didn't try it out on the technical side yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/enhancement Label to mark PR to be added under release notes as enhancement
Projects
None yet
6 participants