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

Allows using robot accounts for Image replication #14905

Conversation

Vad1mo
Copy link
Member

@Vad1mo Vad1mo commented May 17, 2021

This patch (more of a bug fix) will allow using robot accounts for image replication.

Currently, it is not possible to create a robot account that can replicate images from one Harbor registry to another. (You need a System Admin account, big no go in corp environments)

The patch only changes getProject to call the API directly, instead of filtering the list of getProjects because getProjects needs admin level permission and hence is not possible to be used with robot accounts.

Side Effects

The change has no or littel side effects:
image

its only called by listProjects and is always providing and expect a single result,

This patch only works for Harbor => 2.2.0 and resolves a bunch of open issues.

resolves #14640, resolves #13384, resolves #13795
related #8723

How to creaet a Robot Accounts for Replication

In order to a replication with robot accounts, a robot account with those permissions needs to be created.

Permissions needed for robot accounts.

 [
        {
            'action': 'read',
            'resource': '?'
        },
        {
            'action': 'list',
            'resource': 'repository'
        },
        {
            'action': 'pull',
            'resource': 'repository'
        },
        {
            'action': 'list',
            'resource': 'artifact'
        },
        {
            'action': 'read',
            'resource': 'artifact'
        },
        {
            'action': 'list',
            'resource': 'helm-chart'
        },
        {
            'action': 'read',
            'resource': 'helm-chart'
        },
        {
            'action': 'list',
            'resource': 'helm-chart-version'
        },
        {
            'action': 'read',
            'resource': 'helm-chart-version'
        },
    ]

Signed-off-by: Vadim Bauer vb@container-registry.com

@codecov
Copy link

codecov bot commented May 17, 2021

Codecov Report

Merging #14905 (6aa517c) into master (c4f4e6e) will increase coverage by 0.01%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #14905      +/-   ##
==========================================
+ Coverage   66.99%   67.01%   +0.01%     
==========================================
  Files         930      930              
  Lines       76328    76327       -1     
  Branches     2233     2233              
==========================================
+ Hits        51139    51153      +14     
+ Misses      21278    21262      -16     
- Partials     3911     3912       +1     
Flag Coverage Δ
unittests 67.01% <75.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/pkg/reg/adapter/harbor/base/client.go 61.66% <75.00%> (+1.01%) ⬆️
...es/vulnerability/vulnerability-config.component.ts 48.71% <0.00%> (+1.70%) ⬆️
src/jobservice/worker/cworker/c_worker.go 68.42% <0.00%> (+1.91%) ⬆️
src/controller/event/topic.go 9.00% <0.00%> (+7.20%) ⬆️

@Vad1mo Vad1mo force-pushed the fix/replication_with_robot_accounts branch from 135a2e3 to b76a50c Compare May 17, 2021 21:39
@stonezdj stonezdj requested a review from wy65701436 May 24, 2021 08:43
@steven-zou steven-zou added the status/pending-on-voting The issues that need community decision. label May 31, 2021
@jonasrosland jonasrosland added the doc-impact Engineering issues that will require a change in user docs label Jun 2, 2021
Signed-off-by: Vadim Bauer <vb@container-registry.com>
Signed-off-by: Vadim Bauer <vb@container-registry.com>
@Vad1mo Vad1mo force-pushed the fix/replication_with_robot_accounts branch from e5549d3 to 6aa517c Compare June 2, 2021 15:56
@a-mccarthy
Copy link

@Vad1mo, I think adding docs around this scenario will be helpful for users. can you write up some more context here about this use case? Or create an issue so we can track adding this into the docs? thanks!

@OrlinVasilev
Copy link
Member

@reasonerjt can you check that please?

@defreng
Copy link

defreng commented Jun 24, 2021

Hi @wy65701436

I would be super interested in the issue with merging this bugfix? To me this seems to be a fairly obvious improvement of code quality and stability of Harbor.

The current code which first retrieves the unfiltered list of all projects and then gets the wanted one by looping over them in memory to me seems to be a clear antipattern. It will cause sluggishness - especially when a Harbor instance is managing more and more projects.

@wy65701436 wy65701436 added target/2.4.0 and removed status/pending-on-voting The issues that need community decision. labels Jun 29, 2021
@wy65701436
Copy link
Contributor

wy65701436 commented Jun 29, 2021

To make robot account support image replicaiton will in the next release, I'll have a review on the current design and find out the gaps.

There are some details that needs to be discussed.

@Vad1mo
Copy link
Member Author

Vad1mo commented Jun 29, 2021

@wy65701436 this PR makes Replication possible. We currently run replication like this. Let me know if I can be of any help

@defreng
Copy link

defreng commented Jun 29, 2021

@wy65701436 even though the title suggests otherwise, from my perspective this is really just a code quality improvement / bugfix.

The fact that it also fixes replication using robot accounts (which does work! We're using a version of Harbor with this patch in production), is merely a nice side-effect.

@phin1x
Copy link
Contributor

phin1x commented Jul 18, 2021

@Vad1mo do you allow every authenticated user to create projects?

@Vad1mo
Copy link
Member Author

Vad1mo commented Jul 18, 2021

I don't know, probably yes as this is the default setting. Does this make any difference.

@phin1x
Copy link
Contributor

phin1x commented Aug 8, 2021

harbor wants to create the project in the prepare method of the replication adpater, regadless if the project exists (it just ignores the error in this case). but if only admins are allowed to create projects, this api call will never succeed (old problem, mixing system and project level permissions in robot acocunts). we also had to patch out the code for creating the project in the prepare method and the replication succeeded (#14982).

@wy65701436
Copy link
Contributor

wy65701436 commented Aug 18, 2021

@Vad1mo this PR is for robot account to call list project & list repositories API. Thanks for the contributing, we can keep the client call here.

@wy65701436 wy65701436 closed this Aug 18, 2021
@Vad1mo
Copy link
Member Author

Vad1mo commented Aug 18, 2021

thank you @wy65701436,

Would this PR not still make sense? Instead of iterating over ListProjects and filtering the one project the is needed, it would make more sense to just call directly getProject API?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants