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

feat: allow for configuring the docker hub registry endpoint via API #21385

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

Conversation

tuunit
Copy link

@tuunit tuunit commented Jan 7, 2025

Comprehensive Summary of your change

This PR adds the possibility to change the registry endpoint of DockerHub via API. This is useful for Harbor instances that run in airgapped environments and have to go through a DMZ or use a reverse proxy to access DockerHub.

Why not just use a normal Docker Registry type instead of the DockerHub adapter?

Because of the logic for library images which only applies to the Registry Type DockerHub:

func defaultLibrary(ctx context.Context, registryID int64, a lib.ArtifactInfo) (bool, string, error) {
if registryID <= 0 {
return false, "", nil
}
reg, err := registry.Ctl.Get(ctx, registryID)
if err != nil {
return false, "", err
}
if reg.Type != model.RegistryTypeDockerHub {
return false, "", err
}
name := strings.TrimPrefix(a.Repository, a.ProjectName+"/")
if strings.Contains(name, "/") {
return false, "", nil
}
return true, name, nil
}

By allowing to change the registry endpoint for DockerHub, those kinds of environments can still benefit from the library substitution logic.

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.

@tuunit tuunit force-pushed the feat/configurable-endpoint-for-dockerhub branch from a250a3e to 2df26b3 Compare January 7, 2025 11:27
@tuunit tuunit marked this pull request as ready for review January 7, 2025 11:33
@tuunit tuunit requested a review from a team as a code owner January 7, 2025 11:33
@tuunit
Copy link
Author

tuunit commented Jan 7, 2025

/label release-note/enhancement

@Vad1mo Vad1mo added the release-note/enhancement Label to mark PR to be added under release notes as enhancement label Jan 7, 2025
@tuunit tuunit force-pushed the feat/configurable-endpoint-for-dockerhub branch from 2df26b3 to d33cf33 Compare January 8, 2025 10:21
@tuunit
Copy link
Author

tuunit commented Jan 8, 2025

@Vad1mo any feedback on the approach? Do you want me to write an apitest?

@Vad1mo
Copy link
Member

Vad1mo commented Jan 8, 2025

Makes sense to me..

It might be the stupid questions. but why is the URL hardcoded and not changeable in the first place?
Why not add the convenience to the user to preset it and allow the user to enter what he wants.

@tuunit
Copy link
Author

tuunit commented Jan 8, 2025

I have no idea why it is hardcoded 😅 If you want me to change the UI as well to allow changing it. I can do it as part of this PR or another. Just let me know what you guys would prefer on how to continue

@wy65701436
Copy link
Contributor

wy65701436 commented Jan 9, 2025

We're doing this due to the behavior in the upstream. You can refer to the following link for more details: https://github.com/moby/moby/blob/master/registry/config.go#L43.

@tuunit, If you need to access Docker Hub through a proxy, you should configure the proxy for harbor-core and jobservice, rather than updating the Docker Hub URL.

@tuunit
Copy link
Author

tuunit commented Jan 9, 2025

@wy65701436 thanks for actually finding the original place in the moby code base 😅

Nevertheless, I already know that is the official registry URL of docker hub. But as explained in my PR description, it is sometimes necessary to run Harbor inside an air gapped / private environment without direct internet access and therefore replications / caching might need to go through a simple reverse proxy to communicate with the public web. For this purpose it would be great to keep the logic of the library completion if missing despite running behind a proxy. More details in the PR summary. (which is only done for the docker-hub adapter)

@Vad1mo
Copy link
Member

Vad1mo commented Jan 9, 2025

We're doing this due to the behavior in the upstream. You can refer to the following link for more details: https://github.com/moby/moby/blob/master/registry/config.go#L43.

The strategy Docker is following is for commercial reasons and backwards compatibility only. We should not limit ourselves to that.

If we ask ourselves why people user Harbor. One of the big arguments for Harbor is replication.

  1. On-Prem/air gapped environments
  2. No Cloud
  3. non Standards setups

Regarding this PR or its further thinking

  1. Will a prefilled and changeable URL set to registry-1.docker.io be backwards incompatible? → No
  2. Will it restrict users? → No
  3. Will it open up new possibilities and use cases? → yes

Personally, I don't see any strong argument against it, or any negative consequences.

@MinerYang
Copy link
Contributor

MinerYang commented Jan 13, 2025

Hi @tuunit
Would you help us to understand more why you need a reverse-proxy for dockerhub only instead of adding it into something like a while list in your air-gapped env?

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.

It looks good to me, but could you provide additional information from your side? For example, a screenshot and detailed logs after modifying the URL and successfully replicating images through the reverse proxy would be really helpful. Since we don't have a test case to validate this, it would give us more confidence in the implementation.

I have one concern: after updating the URL through the API, the UI still shows the default value. I believe we could either update the front-end code accordingly or fork Harbor on your side to address this.

@wy65701436 wy65701436 self-requested a review January 13, 2025 09:07
@reasonerjt
Copy link
Contributor

after updating the URL through the API, the UI still shows the default value.

This looks like a show-stopper...

@tuunit
Copy link
Author

tuunit commented Jan 13, 2025

Hi @tuunit Would you help us to understand more why you need a reverse-proxy for dockerhub only instead of adding it into something like a while list in your air-gapped env?

No offence but have you ever worked in government project? Or any air gap environments that doesn't allow ANY internet access except from one single proxy that is highly monitored and controlled?

I've seen setups like this often enough at my previous employer. Harbor might run inside an internal network without any internet access whatsoever so re-routing to a proxy is the only option which will most likely be behind multiple other hardware loadbalancers and firewalls.

@tuunit
Copy link
Author

tuunit commented Jan 13, 2025

It looks good to me, but could you provide additional information from your side? For example, a screenshot and detailed logs after modifying the URL and successfully replicating images through the reverse proxy would be really helpful. Since we don't have a test case to validate this, it would give us more confidence in the implementation.

I will provide some screenshots and logs later.

I have one concern: after updating the URL through the API, the UI still shows the default value. I believe we could either update the front-end code accordingly or fork Harbor on your side to address this.

This isn't correct, it's even worse right now. The UI shows a different URL after updating it via the API but the registry URL is not used inside the code. So it currently seems like you can change it via the API but it doesn't do anything.

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.22%. Comparing base (c8c11b4) to head (763e7bc).
Report is 352 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21385      +/-   ##
==========================================
+ Coverage   45.36%   46.22%   +0.86%     
==========================================
  Files         244      247       +3     
  Lines       13333    13883     +550     
  Branches     2719     2875     +156     
==========================================
+ Hits         6049     6418     +369     
- Misses       6983     7126     +143     
- Partials      301      339      +38     
Flag Coverage Δ
unittests 46.22% <ø> (+0.86%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 491 files with indirect coverage changes

@tuunit tuunit force-pushed the feat/configurable-endpoint-for-dockerhub branch from 763e7bc to 1133c64 Compare January 16, 2025 06:07
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
Development

Successfully merging this pull request may close these issues.

6 participants