Add regex to support ECR as OCI Helm registry #362
Conversation
bnsfrt
force-pushed
the
fix-ecr-oci-registry
branch
2 times, most recently
from
60f80da
to
58128d9
Compare
bnsfrt
force-pushed
the
fix-ecr-oci-registry
branch
from
58128d9
to
1a4b57a
Compare
oci/auth/aws/auth.go
Outdated
| registryParts = registryPartRe.FindAllStringSubmatch(image, -1) | ||
| if len(registryParts) < 1 || len(registryParts[0]) < 3 { | ||
| return "", "", false | ||
| } |
There was a problem hiding this comment.
I went on to find some background on why this function parses image and not registry like it's done for GCP and Azure.
The image parsing and the regex was introduced in fluxcd/image-reflector-controller#174 , which later was refactored into a package and moved to this repository.
Based on the above PR, I don't see any specific reason to have it different from the other providers. For GCP and Azure, only the registry host is checked to be valid. We should be able to do something similar for AWS.
I think the reason for the name to be specific to image is that it was added in image-reflector-controller, where the input is expected to be an image. Now that this is becoming more generic, we should be able to relax the parsing and make it about registry only. The information that's extracted after parsing are the account ID and region, which are still present in a registry URL.
Based on the above reasoning, I think we should rename the function to ParseRegistry(), then replace the old regex with the new ECR registry pattern, update login.go to use aws.ParseRegistry() and anywhere else where it's used, and update the old tests accordingly.
I think this change would be backwards compatible because we are relaxing the parser, shouldn't break any existing behavior.
There was a problem hiding this comment.
Also, with this ImageRegistryProvider() can just accept a name.Reference and the registry string can be passed to aws.ParseRegistry(), similar to how it's passed to the ValidHost() methods for other providers.
There was a problem hiding this comment.
Thanks for digging up that background, @darkowlzz . I went ahead and tried refactoring ParseImage into ParseRegistry and relaxed the parsing. I believe I've gotten all the pieces you've mentioned and the updated tests are passing locally. Would you give it another look?
There was a problem hiding this comment.
Thanks for updating. The changes look good to me. I'll run the integration tests with cloud providers to make sure it still works.
And please rebase and squash your commits.
There was a problem hiding this comment.
I ran the integration tests in this repo and in image-reflector-controller against AWS with these changes and they continue to work.
There was a problem hiding this comment.
We are about to do a release today. So, I'll just rebase and squash the commits myself. Hope that's okay. The new source-controller in this release will have this fix.
darkowlzz
force-pushed
the
fix-ecr-oci-registry
branch
from
b62fddb
to
c488c5d
Compare
This change replaces ParseImage with ParseRegistry to allow URLs that point to the root of an AWS account's ECR registry, instead of forcing all repositories to contain a "/" character. This enables support for using ECR repositories at root level to be used as OCI Helm Repository. Signed-off-by: Ben <benjamin.seifert@niche.com>
darkowlzz
force-pushed
the
fix-ecr-oci-registry
branch
from
c488c5d
to
f7c66eb
Compare
|
This fix did not work on our end, for the record, after bnsfrt@f7c66eb. Still unable to pull Helm charts from root ECR repo. |
|
@austinorth can you provide some more information? Error string, registry address, etc. Because this change also updates the test case for the code that was causing the failure for the initially reported issue. |
|
@austinorth @darkowlzz |
|
@cep21 you seem to have a different issue in fluxcd/source-controller#951 . This fix was for invalid chart reference, which you did had initially in your issue. But now, you've authentication issue, which is not related to this PR. |
|
@darkowlzz I added opened #429 to add context that @austinorth is pointing out. |
This change adds a new regex to parse AWS ECR OCI URLs when to allow URLs that point to the root of an AWS account's ECR registry, instead of forcing all repositories to contain a "/" character.
When accepted this will resolve fluxcd/source-controller#905.
I've verified this change resolves the issue by rebuilding the source-controller image and deploying it to a Kubernetes cluster and verifying it can properly authenticate and pull from AWS ECR when given a URL like
oci://<account>.dkr.ecr.<region>.amazonaws.comin a HelmRepository object.