[Feat]: Support for AWS ECR Authentication with Temporary Tokens

[tamilhce]

What type of PR is this?
feature
Which issue does this PR fix:
#2650

What does this PR do / Why do we need it:
This PR adds support for temporary credentials for upstream registries, specifically focusing on AWS ECR. Since ECR credentials are not permanent and need to be rotated periodically, this enhancement enables Zot to dynamically obtain and refresh valid usernames and passwords when the CredentialHelper is configured for the registry

If an issue # is not available please add repro steps and logs showing the issue:
N/A

Testing done on this change:

During initialization, the logs confirm that ECR credentials have been updated:

{"level":"info","goroutine":1,"caller":"zotregistry.dev/zot/pkg/extensions/sync/service.go:77","time":"2024-10-18T23:15:48.269973286+05:30","message":"Using credentials helper, because CredentialHelper is set to ecr"}
{"level":"info","goroutine":1,"caller":"zotregistry.dev/zot/pkg/extensions/sync/service.go:81","time":"2024-10-18T23:15:48.269998807+05:30","message":"Fetch the credentials using AWS ECR Auth Token."}

During credential expiry, the following log entries are generated:

{"level":"info","url":"accountid.dkr.ecr.us-east-1.amazonaws.com","goroutine":593,"caller":"zotregistry.dev/zot/pkg/extensions/sync/ecr_credential_helper.go:126","time":"2024-10-19T00:43:06.320476296+05:30","message":"The credentials are close to expiring"}
{"level":"info","url":"accountid.dkr.ecr.us-east-1.amazonaws.com","goroutine":593,"caller":"zotregistry.dev/zot/pkg/extensions/sync/ecr_credential_helper.go:135","time":"2024-10-19T00:43:06.320498388+05:30","message":"Refreshing the ECR credentials"}
{"level":"info","url":"accountid.dkr.ecr.us-east-1.amazonaws.com","goroutine":593,"caller":"zotregistry.dev/zot/pkg/extensions/sync/service.go:162","time":"2024-10-19T00:43:07.009198386+05:30","message":"Refreshing the upstream remote registry credentials"}

These logs verify that the credentials are nearing the expiry window of one hour and have been successfully refreshed.
Automation added to e2e:
Added TestECRCredentialsHelper in sync_internal_test

Will this break upgrades or downgrades?
No

Does this PR introduce any user-facing change?:
No

release-note
With this PR, users can configure AWS ECR as an upstream registry for on-demand or periodic sync by setting CredentialHelper: ecr in the extension sync configuration. This change eliminates the need for users to manually add usernames and passwords in the credentialsFile; instead, credentials will be stored in memory and automatically rotated as they approach expiry. An example configuration is available in examples/config-sync-ecr-credential-helper.json.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[tamilhce]

I have verified that this PR works as expected in my environment and has been running without issues in our dev/UAT clusters for the past two months.

For reference, related discussions can be found in my personal repo: zot1 PR #13.

Based on the discussion in the linked thread, the PR looks good to @rchincha. @andaaron/ others, please review this as well.

I was on a break due to medical reasons for the last few weeks. Apologies for keeping this thread open for so long. Let's close this ASAP. Let me know if you have any suggestions or comments

[rchincha]

@tamilhce your PR has passed all tests. Pls fix review comments.

In general, everything is under pkg/extensions/sync, so your methods can all be foo..() instead of Foo..() so they are scoped only in that pkg right?

[andaaron]

We'll also need tests. Is it possible to use localstack to test this functionality? We have examples of using localstack for S3 and DynamoDB (ofc we'd need to check if localstack is available locally and only run those tests if available).

[tamilhce]

Thanks, @rchincha and @andaaron, for your valuable comments.
I have addressed all the review comments.

@andaaron, regarding this:

We'll also need tests. Is it possible to use LocalStack to test this functionality? We have examples of using LocalStack for S3 and DynamoDB (of course, we'd need to check if LocalStack is available locally and only run those tests if available).

LocalStack does not support ECR authentication, so it cannot be used to test ECR temporary tokens. However, I can add an example for ECR with LocalStack separately later.

Please review this MR once , and let me know if you have any other suggestions

[andaaron]

@eusebiu-constantin-petu-dbk , @tamilhce we need to sync about this.
There's another PR rewriting sync to use the regctl library: #2903

I took a brief look at regctl, and it looks like the way they solve the issue is using an external binary: https://github.com/regclient/regclient/blob/main/build/Dockerfile.regsync#L30 and https://github.com/regclient/regclient/blob/d1b36cec797378d9974dcf0b7035be077e56abac/config/credhelper.go

I assume is this the same feature we are discussing in this PR? If it is, can we have our own logic outside of the regctl to handled this? And do we want to have our own logic to handle this outside regctl?

[tamilhce]

@andaaron Regarding this #2907 (comment)
Initially, we discussed about using the external binary amazon-ecr-credential-helper.
As discussed in the initial request, we decided to go with this approach
FYI,
#2650 (comment)
#2650 (comment)

[andaaron]

@andaaron Regarding this #2907 (comment) Initially, we discussed about using the external binary amazon-ecr-credential-helper. As discussed in the initial request, we decided to go with this approach FYI, #2650 (comment) #2650 (comment)

@eusebiu-constantin-petu-dbk ^^, what is your take on this?

[tamilhce]

took a brief look at regctl, and it looks like the way they solve the issue is using an external binary:

When running Zot as a deployment in a K8s cluster, using an external binary for the credential-helper introduces additional dependencies and complexities:

• It requires an external binary like amazon-ecr-credential-helper.
• Zot would need access to the root filesystem as well

Moreover, the external binary periodically updates the credentials regardless of whether the user is actively interacting with the configured upstream registry.

In our implemented approach, we retrieve the ECR authentication token only when it's needed. We avoid unnecessary periodic updates and instead validate the token's expiry whenever an upstream request is made.

If the token has expired, we refresh it (the token is typically valid for 12 hours). For instance, if there are no on-demand sync requests for 48 hours, this approach avoids unnecessary calls to the AWS API to refresh the token.

Additionally, relying on an external binary for ECR/registry authentication can lead to issues if the external system encounters problems, such as expired tokens or connectivity failures. With this approach, we retain control and can independently validate token expiry and refresh it as needed.

since we added the CredentialHelper as an interface, which can easily extend the support other token-based registries like GCR by integrating the necessary SDK. This method is straightforward, minimizes external dependencies, avoids introducing too many components, and provides more control overall.

[eusebiu-constantin-petu-dbk]

@eusebiu-constantin-petu-dbk , @tamilhce we need to sync about this. There's another PR rewriting sync to use the regctl library: #2903

I assume is this the same feature we are discussing in this PR? If it is, can we have our own logic outside of the regctl to handled this? And do we want to have our own logic to handle this outside regctl?

Yes, we can have our own logic, we will do it exactly the same like in this PR.
Credentials are exported vars in the host configuration in regclient, which we have access from sync.
No need to change much in the current PR.

I would say let's merge this one first and I'll rebase.

[tamilhce]

@andaaron All the review comments have been addressed. If everything looks good and there are no further comments, please trigger the remaining test suite and consider approve/merge this MR.

[codecov]

Codecov Report

Attention: Patch coverage is 41.61850% with 101 lines in your changes missing coverage. Please review.

Project coverage is 91.60%. Comparing base (cf8b20d) to head (c1a9230).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/extensions/sync/ecr_credential_helper.go	50.00%	48 Missing and 5 partials ⚠️
pkg/extensions/sync/service.go	30.64%	42 Missing and 1 partial ⚠️
pkg/extensions/sync/remote.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2907      +/-   ##
==========================================
- Coverage   91.90%   91.60%   -0.30%     
==========================================
  Files         170      171       +1     
  Lines       30293    30456     +163     
==========================================
+ Hits        27841    27900      +59     
- Misses       1825     1922      +97     
- Partials      627      634       +7     

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

[andaaron]

@andaaron All the review comments have been addressed. If everything looks good and there are no further comments, please trigger the remaining test suite and consider approve/merge this MR.

Sign your commit (git commit --amend -S), and you should be good to go.

[tamilhce]

@andaaron Done, I have signed the commit now. Could you please review it once.