Add support for dockerhub rate-limiting

[Grimm75]

Add support for dockerhub API rate-limiting

Added function which wraps around client.Do() and:

• pauses when rate-limit is hit (HTTP err:429) for time given in retry-after header.
• checks the limit info from HTTP headers sent by API and will pause
  appropriately if rate-limit depletion is eminent.
• max. 2 attempts are made in addition to initial one before giving up.

And replaced calls to client.Do() with wrapped one.

See:
https://docs.docker.com/docker-hub/api/latest/#tag/rate-limiting

PS: Above docs are incorrect about x-retry-after header - name and content doesn't match observed headers from dockerhub, which are retry-after (without x-) and content is positive integer - number of seconds to wait, conforming to RFC9110 10.2.3.

Issue being fixed

Fixes #19967

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.

[Vad1mo]

this is a great contribution. thank you very much.. We need to give it a test

[Grimm75]

Thank you too, I just sqished some style related bugs, we'll see how it goes now.

[Grimm75]

fixed/explained in Fix: review - ... commits.

[MinerYang]

• add retry will increase your request to docker which actually would increase your risk of having rate limit error.
• response time will hang in there

[Grimm75]

• add retry will increase your request to docker which actually would increase your risk of having rate limit error.
• response time will hang in there

Fixed, 'retry' is now used ONLY when last response was 429 and we waited.

[wy65701436]

From the perspective of Harbor core, adding retry would result in more GET or HEAD requests to the upstream Docker registry, potentially increasing the chances of hitting rate limits.

Personally, I believe it's better to fail early and inform the client of the failure ASAP. This allows the client to decide whether to implement retry on their end. Hiding the retry functionality on the server side would inevitably increase response time, which could lead to timeouts for some clients. This approach doesn't seem practical to me.

[Grimm75]

Is there anybody to help me with this PR?

[codecov]

Codecov Report

Attention: Patch coverage is 27.27273% with 32 lines in your changes missing coverage. Please review.

Project coverage is 66.22%. Comparing base (b7b8847) to head (ed0bbf0).
Report is 216 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #19984      +/-   ##
==========================================
- Coverage   67.56%   66.22%   -1.34%     
==========================================
  Files         991     1045      +54     
  Lines      109181   113531    +4350     
  Branches     2719     2845     +126     
==========================================
+ Hits        73768    75188    +1420     
- Misses      31449    34234    +2785     
- Partials     3964     4109     +145     

Flag	Coverage Δ
unittests	66.22% <27.27%> (-1.34%)	⬇️

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

Files	Coverage Δ
src/pkg/reg/adapter/dockerhub/adapter.go	30.50% <27.27%> (-0.47%)	⬇️

... and 571 files with indirect coverage changes

[Vad1mo]

@wy65701436 can we get some indication for @Grimm75 in what direction to move?

[github-actions]

This PR is being marked stale due to a period of inactivty. If this PR is still relevant, please comment or remove the stale label. Otherwise, this PR will close in 30 days.

[OrlinVasilev]

Hey @wy65701436 @stonezdj can you check that please!

[Vad1mo]

@Grimm75, we are ready to go. Can you add a test to your function, to bump up the codecov?

[Vad1mo]

• The PR is not complete yet, as there are some missing test cases and the low test coverage. It is blocked from merging.
• In addition, I find there is too much logging into info, We can put all in debug, especially since we now can handle the rate limit there is nothing to info about.
• I would allow one info statement, when we reach/hit the limit. Other information is relevant for debut only.

[wy65701436]

I believe we should avoid implementing hidden retry logic on the server side, and I’ve shared my personal opinion on this matter.

@Vad1mo @OrlinVasilev We can discuss this in the community meeting if you think it's necessary.

Personally, I believe it's better to fail early and inform the client of the failure ASAP. This allows the client to decide whether to implement retry on their end. Hiding the retry functionality on the server side would inevitably increase response time, which could lead to timeouts for some clients. This approach doesn't seem practical to me.