-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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(registry): support AssumeRoleWithWebIdentity for S3 #18686
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18686 +/- ##
==========================================
- Coverage 67.39% 67.39% -0.01%
==========================================
Files 981 981
Lines 107194 107194
Branches 2698 2698
==========================================
- Hits 72246 72241 -5
- Misses 31065 31069 +4
- Partials 3883 3884 +1
Flags with carried forward coverage won't be shown. Click here to find out more. |
Friendly ping @Vad1mo @wy65701436 @MinerYang |
@davidspek can you demo the setup and configuration in the next community meeting, we can use recording for documentation. |
@Vad1mo Sorry for the late response. When is the next community meeting? |
Hi @Vad1mo @wy65701436 @MinerYang - are we clear what the next steps are for getting this merged? It feels like we're really close to getting this long standing issue sorted. Thanks @davidspek for putting this in. |
@OrlinVasilev maybe you can also weigh in here. |
@wy65701436 can you check please! |
@davidspek https://goharbor.io/community/ here is the community page, tomorrow(28th June) we have community meeting here is the calendar link https://lists.cncf.io/g/harbor-users/viewevent?repeatid=51606&eventid=1944638&calstart=2023-06-28 also please add that to the agenda here : https://github.com/goharbor/community/wiki/Harbor-Community-Meetings#june-28-2023 |
@OrlinVasilev Thanks for the info. I've added the meeting to my calendar and added discussing this PR to the agenda. |
@davidspek Thank you :)) see you later :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
holding to discuss
I'm against this change because we do not want to maintain any forked or modified codebase that deviates from the original one. Instead, we should align with the upstream release cadence and pick up the next release that contains the two PRs. While we currently only have two or three backports that do not have any conflicts, we need to consider the long-term maintenance of our codebase. Furthermore, the redis patch is critical for users who want to enable redis high availability in sentinel. We only have one patch, which is not likely to conflict with other changes. |
Without this change Harbor is not suitable for most enterprise users using EKS.
|
Hi @peterellisjones can you join us later today so we can all discuss that, @wy65701436 can you please also join :) |
I would be in favour of merging this PR as there is no clear timeline in the upstream |
As discussed yesterday @wy65701436 and @davidspek will join forces to try reaching out to distribution maintainers to merge the fixes so we can have something tested and not customized on our side! you can find the recording here :) https://youtu.be/aMkzKf1vmbQ |
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
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. |
not stale |
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. |
not stale |
@wy65701436 and @davidspek any progress on that? |
Would love to see some progress on this. IMHO without support for assume role Harbor is not really an appropriate product for enterprise users |
@peterellisjones absolutely, that's the reason I'm trying to prioritize that :) @msha01 can you put that in the log please! |
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. |
We should close or update this PR. Harbor depends on upstream docker distribution, and we are not hard forking docker distribution. We only apply PRs that are in the main branch, but where a release is not published yet. It looks like that all of the upstream PRs have been closed
The consequence here is A to update this PR or B close this PR. |
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. |
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. |
Comprehensive Summary of your change
This PR adds patches to the building of the Harbor registry image that update the AWS SDK and fix how the AWS S3 client is created so that it can use
AssumeRoleWithWebIdentity
through IRSA.The upstream PRs used in the patches are distribution/distribution#3921 that change the way the client is created and distribution/distribution#3599 that updates the AWS SDK. I have built the image that results from this PR and verified I can push new images to Harbor while using IRSA. For testing the image
davidspek/registry-photon:v2.8.0-irsa-0.1.4
can be used.Issue being fixed
Fixes #16490 and #12888
Please indicate you've done the following: