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

feat: use oras-go library to enable more complex OCI Helm authentication #12554

Merged
merged 12 commits into from
May 11, 2023

Conversation

alexef
Copy link
Member

@alexef alexef commented Feb 21, 2023

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:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@alexef alexef requested review from alexmt, crenshaw-dev and blakepettersson and removed request for alexmt February 21, 2023 08:02
@alexef alexef added this to the v2.7 milestone Feb 21, 2023
@alexef alexef added enhancement New feature or request component:config-management Tools specific issues (helm, kustomize etc) labels Feb 21, 2023
@codecov
Copy link

codecov bot commented Feb 21, 2023

Codecov Report

Patch coverage: 66.66% and project coverage change: +0.03 🎉

Comparison is base (0b12edd) 49.19% compared to head (b472b82) 49.22%.

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     
Impacted Files Coverage Δ
util/helm/client.go 50.32% <66.66%> (+2.80%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@alexef
Copy link
Member Author

alexef commented Mar 9, 2023

tested it in our org, and it works as expected

@alexmt can you have a look?

@alexef
Copy link
Member Author

alexef commented Mar 10, 2023

Okay, after 12 hours, I'm getting this:

rpc error: code = Unknown desc = unable to get tags: failed to get tags: GET "https://redacted.dkr.ecr.eu-central-1.amazonaws.com/v2/subgraph-user/tags/list": response status code 403: denied: Your authorization token has expired. Reauthenticate and try again.```

@amohamedhey
Copy link

Thank you @alexef @crenshaw-dev can we merge before 2.7 release

@amohamedhey
Copy link

Okay, after 12 hours, I'm getting this:

rpc error: code = Unknown desc = unable to get tags: failed to get tags: GET "https://redacted.dkr.ecr.eu-central-1.amazonaws.com/v2/subgraph-user/tags/list": response status code 403: denied: Your authorization token has expired. Reauthenticate and try again.```

this is because token expired, need to provide new token

@alexef
Copy link
Member Author

alexef commented Mar 17, 2023

@amohamedhey yes, the problem is that between reposerver and oras, reposerver fails to re-initialize oras with a new token (we refresh the token using a cronjob)
so I need to figure out how to tell oras to always use the latest username/password combination the reposerver provides.

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

@blakepettersson
Copy link
Member

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...

@Danielkem
Copy link

Any update on this?

Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
Signed-off-by: Alex Eftimie <alex.eftimie@getyourguide.com>
@alexef
Copy link
Member Author

alexef commented Apr 21, 2023

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

@alexef
Copy link
Member Author

alexef commented May 3, 2023

@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?

@alexef
Copy link
Member Author

alexef commented May 4, 2023

e2e seem to be flaky, getting another master merge build

@alexef
Copy link
Member Author

alexef commented May 11, 2023

@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?

Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @alexef!!

@crenshaw-dev crenshaw-dev merged commit a08282b into argoproj:master May 11, 2023
24 checks passed
@alexef alexef deleted the oci-use-oras-go branch May 11, 2023 14:10
@rouke-broersma
Copy link
Contributor

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...

Sounds like this maybe? #7673

@tuananh
Copy link
Contributor

tuananh commented Jun 30, 2023

@amohamedhey yes, the problem is that between reposerver and oras, reposerver fails to re-initialize oras with a new token (we refresh the token using a cronjob) so I need to figure out how to tell oras to always use the latest username/password combination the reposerver provides.

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

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

@alexef
Copy link
Member Author

alexef commented Jun 30, 2023

@tuananh the fix was to disable cache, see: 82428b2

it was only an issue during the development of this PR

@tuananh
Copy link
Contributor

tuananh commented Jun 30, 2023

that's weird. I tried the rc1 image and i still got this issue with ECR. I rollback to 2.7.x for now

@hcsaustrup
Copy link

hcsaustrup commented Jul 15, 2023

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:

rpc error: code = Unknown desc = unable to get tags: failed to get tags: GET "https://xxx/v2/xxx-charts/xxx-api/tags/list": response status code 401: unauthorized: unauthorized to access repository: xxx-charts/xxx-api, action: pull: unauthorized to access repository: xxx-charts/xxx-api, action: pull

Harbor logs:

2023-07-15T20:29:25Z [DEBUG] [/server/middleware/log/log.go:30]: attach request id 56cba1335b4d4eb6165af5584f5fd35a to the logger for the request GET /v2/xxx-charts/xxx-api/tags/list
2023-07-15T20:29:25Z [DEBUG] [/server/middleware/artifactinfo/artifact_info.go:55]: In artifact info middleware, url: /v2/xxx-charts/xxx-api/tags/list
2023-07-15T20:29:25Z [DEBUG] [/server/middleware/security/unauthorized.go:28][requestID="56cba1335b4d4eb6165af5584f5fd35a"]: an unauthorized security context generated for request GET /v2/xxx-charts/xxx-api/tags/list
2023-07-15T20:29:25Z [DEBUG] [/lib/http/error.go:62]: {"errors":[{"code":"UNAUTHORIZED","message":"authorize header needed to send HEAD to repository: authorize header needed to send HEAD to repository"}]}

@alexef
Copy link
Member Author

alexef commented Jul 18, 2023

@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)

yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:config-management Tools specific issues (helm, kustomize etc) enhancement New feature or request ready-for-review
Projects
Archived in project
Status: Completed
Development

Successfully merging this pull request may close these issues.

Wildcard support in OCI Helm repositories does not work
9 participants