OCI HelmRepository doesn't support charts at the root of AWS ECR

[bnsfrt]

Hello,

This issue is related to #896 and addresses what I believe may be a more proper fix for the problem.

As noted in #896, with v0.29.0 deployed and a HelmRepository like the following created:

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: HelmRepository
metadata:
  name: xxx
  namespace: flux-system
spec:
  interval: 10m
  provider: aws
  type: oci
  url: oci://<account>.dkr.ecr.<region>.amazonaws.com

the behavior we see is that the service panics due to an unhandled nil return, resulting from the failed parsing of the URL with the regex here.

According to the documentation for HelmRepository objects of type oci, the URL is "expected to point to a registry repository." Based on this, I would expect that the proper configuration for a HelmRelease and HelmRepository referencing a chart in AWS ECR at oci://<account>.dkr.ecr.<region>.amazonaws.com/<service> would look something like this:

---
apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: HelmRepository
metadata:
  name: xxx
  namespace: flux-system
spec:
  interval: 10m
  provider: aws
  type: oci
  url: oci://<account>.dkr.ecr.<region>.amazonaws.com/<service>
---
apiVersion: helm.toolkit.fluxcd.io/v2beta1
kind: HelmRelease
metadata:
  name: xxx
  namespace: flux-system
spec:
  interval: 10m
  targetNamespace: default
  chart:
    spec:
      chart: <service>
      sourceRef:
        kind: HelmRepository
        name: xxx
        namespace: flux-system
  releaseName: xxx

In the above configuration the HelmRepository object is correctly pointing to the Helm repository where the chart is stored in ECR, and the .spec.chart in the HelmRelease object should effectively be redundant, as within a Helm repository there can be only one uniquely named chart. However, with this configuration, the source-controller appears to attempt to look up the <service> chart using the URL from the HelmRepository object as a base, generating an error like the following:

invalid chart reference: failed to get chart version for remote reference: could not get tags for "<service>": could not fetch tags for "oci://<account>.dkr.ecr.<region>.amazonaws.com/<service>/<service>"

The chart in question is located in AWS ECR at oci://<account>.dkr.ecr.<region>.amazonaws.com/<service>, so it is no surprise that the source-controller is unable to find any tags with the additional /<service> appended on the path. This suggests that the HelmRepository object is not actually referring, then, to a Helm repository properly, but rather to either an Helm registry or to a subpath within a Helm registry underneath which specific Helm repositories can be located. In either case, it is clear that the source-controller is expecting to be able to find the specific Helm charts in ECR at a path that involves appending /<.spec.chart> to the URL from the HelmRepository object.

As a result of this finding, it appears that the only valid way to support a chart located at a path like oci://<account>.dkr.ecr.<region>.amazonaws.com/<service> would be for the URL in the HelmRepository object to be set to oci://<account>.dkr.ecr.<region>.amazonaws.com. However, as noted in #896 and as patched in #897 this URL is treated as invalid, because it does not successfully parse according to the regex built to parse and validate ECR images. The patch in #897 handles this case by returning an explicit error message declaring the failure to parse, effectively establishing that Helm charts housed in an AWS ECR repository that does not contain a / character in the name are not to be supported by the source-controller.

My request is one of the following (with a preference for the option number 1):

1. Could the regex used to validate the HelmRepository URL be updated to make the /([^:]+) component optional? This would allow a URL like oci://<account>.dkr.ecr.<region>.amazonaws.com to pass and return the account and region properly to authenticate to AWS ECR. That enables the use of Helm charts located in ECR repositories without a / character in their names, which I believe is a use case that should be supported.
2. Could the documentation be updated to reflect the fact that HelmRepository objects of type oci do not properly correspond to a Helm repository, but rather a Helm registry or subpath within a Helm registry where Helm repositories may be located?

I would be happy to contribute and test a solution if option number 1 is deemed desirable.

Thanks!

[souleb]

Thanks @bnsfrt for opening this PR. I think that you are correct in your analysis and we need to implement the missing parts to make this works for AWS ECR whether we provide a registry or registry/subpath url. We should also update the related documentation as mentioned above.

For the implementation part, it would be best to update the following function, so it can fallback to parsing a registry.

https://github.com/fluxcd/pkg/blob/dbad05cf95b380c6f619a9bf76dc755c6ff6e3cc/oci/auth/aws/auth.go#L41

The fallback option is preferred because Helmrepository is the only reconciler that deals with registry urls, other consumer s provide images repositories urls.

[bnsfrt]

Great! I'm glad to know I'm barking up the right tree. Would you like me to go ahead and take a crack at opening a PR for this change myself?

[souleb]

that would be much appreciated.

[bnsfrt]

I've raised fluxcd/pkg#362 to address the issue and tested that the source-controller works with the change.