-
Notifications
You must be signed in to change notification settings - Fork 5.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: use oras-go library to enable more complex OCI Helm authentication #12554
Conversation
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #12554 +/- ##
==========================================
+ Coverage 49.19% 49.22% +0.03%
==========================================
Files 248 248
Lines 42909 42830 -79
==========================================
- Hits 21110 21084 -26
+ Misses 19684 19648 -36
+ Partials 2115 2098 -17
☔ View full report in Codecov by Sentry. |
tested it in our org, and it works as expected @alexmt can you have a look? |
Okay, after 12 hours, I'm getting this:
|
Thank you @alexef @crenshaw-dev can we merge before 2.7 release |
this is because token expired, need to provide new token |
@amohamedhey yes, the problem is that between also note, that when I got into this state, not event a "Refresh (hard)" force refresh helped. the only thing that helped was a reposerver restart |
That sounds familiar @alexef... |
Any update on this? |
Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Thanks for the catch @blakepettersson . I'm sorry for the lag here, I couldn't get time to work on it. It works, but the cache issue prevented us from using it for more than 12h. Hopefully with @blakepettersson's change we can give it another try |
@pasha-codefresh we've tested it on our infrastructure, without observing service interruption (after @blakepettersson's changes). can you have another look so we can land it in master for wider testing? |
e2e seem to be flaky, getting another master merge build |
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@crenshaw-dev / @leoluz ping. I'm confident this change won't break anything, as we tested it for more than a week now. can we merge it to master? |
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.
Thanks, @alexef!!
how did you fix this isuse? we have the same problem with ECR i did try restarting reposerver but it doesnt help. we have ECR token refresh with external-secrets and it has work with 2.6.x |
that's weird. I tried the rc1 image and i still got this issue with ECR. I rollback to 2.7.x for now |
Still seems to be an issue with the latest build on Quay from July 14. The latest version has better error reporting then 2.7.x, but the error in the Harbor log seems to be the same. Works with specific tag - fails with Argo v2.9.0+9bf5e50 ArgoCD error:
Harbor logs:
|
@hcsaustrup maybe it's an issue with splitting repoURL and repo name? I can't find the ticket, but in the past, keeping only the domain as repoURL and putting the entire path as repo name helped (maybe related: #12436) |
…ion (argoproj#12554) * feat: use oras-go library to enable more complex OCI Helm authentication Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com> * Update util/helm/client.go Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com> * Ran make mod-vendor-local Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com> --------- Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
fixes: #12392
a follow up of: #10641
The existing GetTags implementation supports only
"Authorization: Basic ..."
, as implemented by AWS ECR or Azure CR.By switching to the
oras-go
library (used by helm, harbour, etc), we're now supporting"Authorization: Bearer ..."
as well, as required by ghcr.io and others.Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: