Add suport for ECR repositorires #244
Conversation
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
pkg/registry/checker.go
Outdated
| keyChain, err := rc.providerRegistry.GetAuthKeychain(imageName) | ||
| if err != nil { | ||
| logrus.Warn("error while getting keychain for: ", err) | ||
| return store.UnknownError |
There was a problem hiding this comment.
I suggest returning a more specific authentication error, similar to the implementation here:
.This would help clarify the nature of the failure, but I’m open to feedback if there’s a better approach.
There was a problem hiding this comment.
I suppose you're right.
Changed in last commit.
Signed-off-by: alexey.komyakov <[email protected]>
pkg/providers/provider.go
Outdated
|
|
||
| func (p ProviderRegistry) GetAuthKeychain(registryStr string) (authn.Keychain, error) { | ||
| switch { | ||
| case strings.Contains(registryStr, "amazonaws.com"): |
There was a problem hiding this comment.
lets user url.Parse here and than check the URL with more accurate regex ^([0-9]{12})\.dkr\.ecr\.[a-z0-9-]+\.amazonaws\.com(\.cn)?$
pkg/providers/k8s/k8s.go
Outdated
| } | ||
| } | ||
|
|
||
| func (p Provider) GetAuthKeychain(registryStr string) (authn.Keychain, error) { |
There was a problem hiding this comment.
| func (p Provider) GetAuthKeychain(registryStr string) (authn.Keychain, error) { | |
| func (p Provider) GetAuthKeychain(registry string) (authn.Keychain, error) { |
NIT: you do not need to add a variable type to a name
There was a problem hiding this comment.
Yeah, i know, it's something like temp naming was but I couldn't think of a better name.
pkg/providers/amazon/amazon.go
Outdated
| return nil, err | ||
| } | ||
|
|
||
| client := ecr.NewFromConfig(cfg) |
There was a problem hiding this comment.
Can the client be cached in provider or do we need to reinitialize it on each execution?
pkg/providers/amazon/amazon.go
Outdated
| } | ||
|
|
||
| func awsRegionalClient(ctx context.Context, region string) (*ecr.Client, error) { | ||
| cfg, err := config.LoadDefaultConfig(ctx, config.WithRegion(region)) |
There was a problem hiding this comment.
I assume config also cannot be changed and thus be cached.
There was a problem hiding this comment.
I think we can init it only once in NewProvider block before caching client because it's needed only for it
pkg/providers/amazon/amazon.go
Outdated
| return &customKeychain{authenticator: auth}, nil | ||
| } | ||
|
|
||
| func parseECRDetails(registryStr string) string { |
There was a problem hiding this comment.
It is better to use regex with a capture group to highlight that we are extracting a region here.
Also please provide an example of an address of a ECR registry.
The name is also not good, lets use something like parseECRRegion.
pkg/providers/provider.go
Outdated
| func NewProviderChain(pullSecretsGetter func(image string) []corev1.Secret) ProviderRegistry { | ||
| return map[string]Provider{ | ||
| "amazon": amazon.NewProvider(), | ||
| "k8s": k8s.NewProvider(pullSecretsGetter), | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't like that the generic chain accepts kubernetes specific options Can we avoid it by initializing each provider separately?
| type customKeychain struct { | ||
| authenticator authn.Authenticator | ||
| } | ||
|
|
||
| func (kc *customKeychain) Resolve(_ authn.Resource) (authn.Authenticator, error) { | ||
| return kc.authenticator, nil | ||
| } |
There was a problem hiding this comment.
What is this? The name is too broad. Is this an authn.Keychain implementation?
There was a problem hiding this comment.
Is this an authn.Keychain implementation?
Yes it is
pkg/providers/amazon/amazon.go
Outdated
| return nil, fmt.Errorf("error loading AWS config: %w", err) | ||
| } | ||
|
|
||
| authTokenOutput, err := ecrClient.GetAuthorizationToken(context.TODO(), &ecr.GetAuthorizationTokenInput{}) |
There was a problem hiding this comment.
Is GetAuthorizationToken called for each image request?
AuthorizationData has the expiresAt field, so I assume we can cache the token till this time and avoid flooding AWS with requests.
pkg/registry/checker.go
Outdated
|
|
||
| pullSecretsGetter := func(image string) []corev1.Secret { | ||
| return rc.controllerIndexers.GetImagePullSecrets(image) | ||
| } | ||
| pc := providers.NewProviderChain(pullSecretsGetter) | ||
| rc.providerRegistry = pc | ||
|
|
There was a problem hiding this comment.
| pullSecretsGetter := func(image string) []corev1.Secret { | |
| return rc.controllerIndexers.GetImagePullSecrets(image) | |
| } | |
| pc := providers.NewProviderChain(pullSecretsGetter) | |
| rc.providerRegistry = pc | |
| rc.providerRegistry = providers.NewProviderChain(rc.controllerIndexers.GetImagePullSecrets) |
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Signed-off-by: alexey.komyakov <[email protected]>
Some users prefer to use in EKS clusters ECR without service accounts to authorize but with native support from ECR using IAM policies to provide credentials for their projects.
Exporter can't get any credentials from SA or from Secrets and falls with 401 Unauthorized exception.
This PR adds new libraries from AWS for work with their authorization methods.