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

fix(storage): make OCI reauthentication with AWS ECR #3044

Merged
merged 7 commits into from
May 10, 2024

Conversation

erka
Copy link
Collaborator

@erka erka commented May 3, 2024

Few observations about AWS ECR. It looks like there is a big difference between private and public AWS ECR.

Public ECR uses Bearer authorization and returns errors json encoded.

> GET /datadog/datadog HTTP/2
> Host: public.ecr.aws
>
* Request completely sent off
< HTTP/2 401
< content-type: application/json; charset=utf-8
< docker-distribution-api-version: registry/2.0
< www-authenticate: Bearer realm="https://public.ecr.aws/token/",service="public.ecr.aws",scope="aws"
<
{"errors":[{"code":"DENIED","message":"Not Authorized"}]}

The private ECR uses Basic auth and returns errors in plain text.

> GET / HTTP/1.1
> Host: 0.dkr.ecr.us-west-2.amazonaws.com
> Authorization: Basic asdfasfasfd
>
* Request completely sent off
< HTTP/1.1 401 Unauthorized
< Docker-Distribution-Api-Version: registry/2.0
< Www-Authenticate: Basic realm="https://0.dkr.ecr.us-west-2.amazonaws.com/",service="ecr.amazonaws.com"
< Content-Type: text/plain; charset=utf-8
<
Not Authorized

@erka erka mentioned this pull request May 3, 2024
1 task
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 79.51807% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 72.15%. Comparing base (f997fb9) to head (dd608f4).
Report is 301 commits behind head on main.

Files Patch % Lines
internal/oci/ecr/ecr.go 68.29% 8 Missing and 5 partials ⚠️
internal/oci/ecr/credentials_store.go 88.57% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3044      +/-   ##
==========================================
+ Coverage   70.78%   72.15%   +1.36%     
==========================================
  Files          91       99       +8     
  Lines        8729     7502    -1227     
==========================================
- Hits         6179     5413     -766     
+ Misses       2165     1684     -481     
- Partials      385      405      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

erka added 4 commits May 7, 2024 15:58
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
@erka
Copy link
Collaborator Author

erka commented May 7, 2024

I'm concerned about the complexity of my implementation. In oci/Store func Fetch calls getTarget which creates new oras repo with a new auth client. As the result it requires to cache internal ecr client and cached credential with expiration time. It's hard to understand and hard to maintain.

If anyone has ideas they are welcome here.

@thepabloaguilar
Copy link
Contributor

Sure @erka, I'll take some time to think about an alternative implementation!

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
erka added 2 commits May 8, 2024 20:04
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
@erka erka marked this pull request as ready for review May 9, 2024 19:01
@erka erka requested a review from a team as a code owner May 9, 2024 19:01
@erka erka requested a review from thepabloaguilar May 9, 2024 19:02
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

looks good to me! thanks @erka

wdyt @GeorgeMac ?

@thepabloaguilar are you able to build off of this branch to see if it fixes #3043

Copy link
Contributor

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

Yep, looks great 👏 Nice one @erka

@markphelps markphelps added the automerge Used by Kodiak bot to automerge PRs label May 10, 2024
@kodiakhq kodiakhq bot merged commit 96820c3 into flipt-io:main May 10, 2024
28 checks passed
@erka erka deleted the oci-aws-ecr-reauth branch May 10, 2024 17:26
markphelps pushed a commit that referenced this pull request May 13, 2024
* fix(storage): make OCI reauthentication with AWS ECR

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* rework

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* address PR feedback and added tests

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* update cache

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* rework

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* cleanup

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

* add more tests and cleanup

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>

---------

Signed-off-by: Roman Dmytrenko <rdmytrenko@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Used by Kodiak bot to automerge PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants