harbor icon indicating copy to clipboard operation
harbor copied to clipboard

Support for Google Cloud Artifact Registry

Open mrbuk opened this issue 2 years ago • 3 comments

Implement an adapter for the Google Artifact Registry (src/pkg/reg/adapter/googlegar/adapter.go) based on the logic of the adapter for Google Container Registry (src/pkg/reg/adapter/googlegcr/adapter.go).

In contrast to Google Container Registry the native Authorizer can't be used for Google Artifact Registry. The native Authorizer only adds the Authentication Header to URLs that contain /v2/. In case of Google Artifact Registry the actual upload endpoint does not contain a /v2/ but instead /artifacts-uploads like showing below

18:30:35 HTTPS GET     europe-west1-docker.pkg.dev /v2/                                                                                                                  401   application/json
18:30:35 HTTPS HEAD    europe-west1-docker.pkg.dev /v2/artifact-registry-sync-121/sync-01/mrbuk/nginx/manifests/1.21                                                     404       [no content]
18:30:35 HTTPS HEAD    europe-west1-docker.pkg.dev /v2/artifact-registry-sync-121/sync-01/mrbuk/nginx/blobs/sha256:036ae8e42548a69ca1cd3f8862cc243dd90230258abecee936cd… 404       [no content]
18:30:35 HTTPS POST    europe-west1-docker.pkg.dev /v2/artifact-registry-sync-121/sync-01/mrbuk/nginx/blobs/uploads/                                                     202       [no content]
18:30:35 HTTPS PUT     europe-west1-docker.pkg.dev /artifacts-uploads/namespaces/artifact-registry-sync-121/repositories/sync-01/uploads/AB38jmJfA4_GYLVqot73sX7TCFk9AK… 401   application/json

The logic in the new Authorizer is adjusted so the Authorization Header is additionally added for a URL Path starting with /artifacts-uploads. The rest of the logic of the native Authorizer is preserved.

Tests for the change of the behaviour have been added (src/pkg/reg/adapter/googlegar/authorizer_test.go)

Instead of adding logic to existing code (e.g. googlegcr or native) the decision was made to introduce a new package googlegar instead. That way regressions/side-effects to existing code should be minimized.

Issue being fixed

Fixes #16973

Please indicate you've done the following:

  • [X] Well Written Title and Summary of the PR
  • [x] 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"
  • [X] Accepted the DCO. Commits without the DCO will delay acceptance.
  • [X] 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.

mrbuk avatar Jul 16 '22 10:07 mrbuk

Codecov Report

Merging #17179 (0916d44) into main (72cd65d) will decrease coverage by 0.02%. The diff coverage is 52.02%.

:exclamation: Current head 0916d44 differs from pull request most recent head 810d5b8. Consider uploading reports for the commit 810d5b8 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17179      +/-   ##
==========================================
- Coverage   67.30%   67.27%   -0.03%     
==========================================
  Files         992      995       +3     
  Lines       83934    84084     +150     
  Branches     2665     2665              
==========================================
+ Hits        56488    56570      +82     
- Misses      23567    23633      +66     
- Partials     3879     3881       +2     
Flag Coverage Δ
unittests 67.27% <52.02%> (-0.03%) :arrow_down:

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

Impacted Files Coverage Δ
src/pkg/reg/manager.go 48.11% <ø> (ø)
src/pkg/reg/model/registry.go 25.00% <ø> (ø)
...rojects/create-project/create-project.component.ts 50.42% <ø> (ø)
...portal/src/app/shared/services/endpoint.service.ts 16.98% <ø> (ø)
src/pkg/reg/adapter/googlegar/adapter.go 44.11% <44.11%> (ø)
src/pkg/reg/adapter/googlegar/authorizer.go 69.56% <69.56%> (ø)
...audit-log-purge/history/purge-history.component.ts 40.22% <0.00%> (-5.75%) :arrow_down:
src/portal/src/app/shared/units/shared.utils.ts 34.11% <0.00%> (-2.36%) :arrow_down:
src/controller/event/topic.go 9.00% <0.00%> (-1.81%) :arrow_down:
src/pkg/registry/auth/null/authorizer.go 100.00% <0.00%> (ø)
... and 4 more

codecov[bot] avatar Jul 18 '22 02:07 codecov[bot]

@mrbuk Thanks for your contribution! I'm not sure whether you have any test account or environment to share with me that I can verify this adapter work.

chlins avatar Jul 26 '22 01:07 chlins

@chlins I have added another commit to address the edge-case discussed in the issue.

I will be traveling tomorrow but am happy to support with testing from Thursdays (28.07.) if there is a need.

mrbuk avatar Jul 26 '22 21:07 mrbuk

@mrbuk Hi, mrbuk, the code looks good to me, but pending on the account for testing, thanks.

chlins avatar Sep 02 '22 07:09 chlins

From my perspective this PR can be closed as support was added in Artifact Registry. Artifact Registry now behaves like GCR and the GCR Adapter works out of the box (tested myself with Harbor 2.6.0).

See comment in #16973

mrbuk avatar Sep 13 '22 20:09 mrbuk