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

[RFC 0002] Flux OCI support for Helm #690

Merged
merged 7 commits into from
May 19, 2022
Merged

[RFC 0002] Flux OCI support for Helm #690

merged 7 commits into from
May 19, 2022

Conversation

makkes
Copy link
Member

@makkes makkes commented Apr 27, 2022

This is an implementation of RFC 0002.

Closes #669

If implemented:

  • users will be able to declare OCI HelmRepository by using
    the .Spec.Type field of the HelmRepository API. Contrary to the HTTP/S HelmRepository no index.yaml is reconciled from source, instead a simple url and credentials validation is performed.

  • users will be able to declare the new OCI HelmRepository type as source using the .Spec.SourceRef field of the HelmChart API. This will result in reconciling a chart from an OCI repository.

TODOS:

  • reconciliation from public OCI repositories
  • reconciliation from private OCI repositories
  • generate new registryClient for everyReconciliation
  • documentation
  • unit tests
  • e2e tests

not covered by this pull request

  • dependencies from OCI repositories. This is a follow up work
  • custom certs for tls. This is a follow up work

Tests

deployed on AWS EKS

> ~/f/test-oci k describe helmrepository podinfo                                                                                                                                                   16:26:02
Name:         podinfo
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  source.toolkit.fluxcd.io/v1beta2
Kind:         HelmRepository
Metadata:
  Creation Timestamp:  2022-05-10T14:18:39Z
  Finalizers:
    finalizers.fluxcd.io
  Generation:  1
  Managed Fields:
    API Version:  source.toolkit.fluxcd.io/v1beta2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:interval:
        f:timeout:
        f:type:
        f:url:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-05-10T14:18:39Z
    API Version:  source.toolkit.fluxcd.io/v1beta2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"finalizers.fluxcd.io":
      f:status:
        f:conditions:
        f:observedGeneration:
    Manager:         source-controller
    Operation:       Update
    Time:            2022-05-10T14:18:40Z
  Resource Version:  20096738
  UID:               f74a18bc-df7e-4421-97f3-8ad7e7d280e3
Spec:
  Interval:  10m0s
  Timeout:   60s
  Type:      oci
  URL:       oci://ghcr.io/stefanprodan/charts
Status:
  Conditions:
    Last Transition Time:  2022-05-10T14:18:40Z
    Message:               Helm repository "podinfo" is valid
    Observed Generation:   1
    Reason:                Succeeded
    Status:                True
    Type:                  Ready
    Last Transition Time:  2022-05-10T14:18:40Z
    Message:               Helm repository "podinfo" is valid
    Observed Generation:   1
    Reason:                Succeeded
    Status:                True
    Type:                  SourceValid
  Observed Generation:     1
Events:                    <none>> ~/f/test-oci k describe helmreleases podinfo                                                                                                                                                     16:25:29
Name:         podinfo
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  helm.toolkit.fluxcd.io/v2beta1
Kind:         HelmRelease
Metadata:
  Creation Timestamp:  2022-05-10T14:20:31Z
  Finalizers:
    finalizers.fluxcd.io
  Generation:  1
  Managed Fields:
    API Version:  helm.toolkit.fluxcd.io/v2beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:chart:
          .:
          f:spec:
            .:
            f:chart:
            f:reconcileStrategy:
            f:sourceRef:
              .:
              f:kind:
              f:name:
            f:version:
        f:interval:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-05-10T14:20:31Z
    API Version:  helm.toolkit.fluxcd.io/v2beta1
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"finalizers.fluxcd.io":
      f:status:
        f:conditions:
        f:helmChart:
        f:lastAppliedRevision:
        f:lastAttemptedRevision:
        f:lastAttemptedValuesChecksum:
        f:lastReleaseRevision:
        f:observedGeneration:
    Manager:         helm-controller
    Operation:       Update
    Time:            2022-05-10T14:20:34Z
  Resource Version:  20097278
  UID:               2c953d63-c794-4092-afa4-5c70051c983d
Spec:
  Chart:
    Spec:
      Chart:               podinfo
      Reconcile Strategy:  ChartVersion
      Source Ref:
        Kind:   HelmRepository
        Name:   podinfo
      Version:  *
  Interval:     1m0s
Status:
  Conditions:
    Last Transition Time:          2022-05-10T14:20:34Z
    Message:                       Release reconciliation succeeded
    Reason:                        ReconciliationSucceeded
    Status:                        True
    Type:                          Ready
    Last Transition Time:          2022-05-10T14:20:34Z
    Message:                       Helm install succeeded
    Reason:                        InstallSucceeded
    Status:                        True
    Type:                          Released
  Helm Chart:                      default/default-podinfo
  Last Applied Revision:           6.1.4
  Last Attempted Revision:         6.1.4
  Last Attempted Values Checksum:  da39a3ee5e6b4b0d3255bfef95601890afd80709
  Last Release Revision:           1
  Observed Generation:             1
Events:
  Type    Reason  Age    From             Message
  ----    ------  ----   ----             -------
  Normal  info    4m59s  helm-controller  HelmChart 'default/default-podinfo' is not ready
  Normal  info    4m56s  helm-controller  Helm install has started
  Normal  info    4m56s  helm-controller  Helm install succeeded


⋊> ~/f/test-oci k describe  helmcharts default-podinfo                                                                                                                                              16:25:56
Name:         default-podinfo
Namespace:    default
Labels:       <none>
Annotations:  <none>
API Version:  source.toolkit.fluxcd.io/v1beta2
Kind:         HelmChart
Metadata:
  Creation Timestamp:  2022-05-10T14:20:31Z
  Finalizers:
    finalizers.fluxcd.io
  Generation:  1
  Managed Fields:
    API Version:  source.toolkit.fluxcd.io/v1beta2
    Fields Type:  FieldsV1
    fieldsV1:
      f:spec:
        .:
        f:chart:
        f:interval:
        f:reconcileStrategy:
        f:sourceRef:
          .:
          f:kind:
          f:name:
        f:version:
    Manager:      helm-controller
    Operation:    Update
    Time:         2022-05-10T14:20:31Z
    API Version:  source.toolkit.fluxcd.io/v1beta2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:finalizers:
          .:
          v:"finalizers.fluxcd.io":
      f:status:
        f:artifact:
          .:
          f:checksum:
          f:lastUpdateTime:
          f:path:
          f:revision:
          f:size:
          f:url:
        f:conditions:
        f:observedChartName:
        f:observedGeneration:
        f:url:
    Manager:         source-controller
    Operation:       Update
    Time:            2022-05-10T14:20:34Z
  Resource Version:  20097262
  UID:               254bd56e-3420-408b-ac3d-44d8f5b07f55
Spec:
  Chart:               podinfo
  Interval:            1m0s
  Reconcile Strategy:  ChartVersion
  Source Ref:
    Kind:   HelmRepository
    Name:   podinfo
  Version:  *
Status:
  Artifact:
    Checksum:          63f416297202c14e8b0c2ddafbcc153b67ede5fa252e7e67299d22dca5e6dd7b
    Last Update Time:  2022-05-10T14:20:34Z
    Path:              helmchart/default/default-podinfo/podinfo-6.1.4.tgz
    Revision:          6.1.4
    Size:              13548
    URL:               http://source-controller.flux-system.svc.cluster.local./helmchart/default/default-podinfo/podinfo-6.1.4.tgz
  Conditions:
    Last Transition Time:  2022-05-10T14:20:34Z
    Message:               pulled 'podinfo' chart with version '6.1.4'
    Observed Generation:   1
    Reason:                ChartPullSucceeded
    Status:                True
    Type:                  Ready
    Last Transition Time:  2022-05-10T14:20:34Z
    Message:               pulled 'podinfo' chart with version '6.1.4'
    Observed Generation:   1
    Reason:                ChartPullSucceeded
    Status:                True
    Type:                  ArtifactInStorage
  Observed Chart Name:     podinfo
  Observed Generation:     1
  URL:                     http://source-controller.flux-system.svc.cluster.local./helmchart/default/default-podinfo/latest.tar.gz
Events:
  Type    Reason                      Age                  From               Message
  ----    ------                      ----                 ----               -------
  Normal  ChartPullSucceeded          5m28s                source-controller  pulled 'podinfo' chart with version '6.1.4'
  Normal  GarbageCollectionSucceeded  4m28s                source-controller  garbage collected 1 artifacts
  Normal  ArtifactUpToDate            26s (x5 over 4m28s)  source-controller  artifact up-to-date with remote revision: '6.1.4'

@stefanprodan stefanprodan added enhancement New feature or request area/helm Helm related issues and pull requests labels Apr 27, 2022
@stefanprodan stefanprodan added this to the GA milestone Apr 27, 2022
@souleb
Copy link
Member

souleb commented Apr 27, 2022

This is still on-going, and we will continue refining most of the code. We want to have early feedback so we can address any red flag.

I would like to draw your attention on the fact that the HelmRepositoryOCIReconciler does not store any artifact, but does validate the repository. We felt that it was the way to go ...

@hiddeco
Copy link
Member

hiddeco commented Apr 27, 2022

I am still frowning when I see Default as the "default" value, as it isn't really explanatory (compared to OCI), but I do not have a better alternative to suggest at the moment. As a nitpick, I would expect the value to be lowercase (which would match the e.g. Git implementation enums we have for repositories).

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Please add the copyright header to all new files and set the year to 2022 instead of 2020.

go.mod Outdated Show resolved Hide resolved
@souleb souleb force-pushed the rfc-0002-helm-oci branch 3 times, most recently from c921cbd to 21bdf70 Compare May 10, 2022 08:20
@souleb souleb force-pushed the rfc-0002-helm-oci branch 2 times, most recently from a81acae to eea3c22 Compare May 10, 2022 16:06
@makkes makkes marked this pull request as ready for review May 11, 2022 07:15
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

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

Please add an OCI section to the HelmRepository API docs and explain how people can use it.

api/v1beta2/condition_types.go Outdated Show resolved Hide resolved
api/v1beta2/helmrepository_types.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
controllers/helmchart_controller_test.go Outdated Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Outdated Show resolved Hide resolved
controllers/helmrepository_controller.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
internal/helm/repository/oci_chart_repository.go Outdated Show resolved Hide resolved
internal/helm/repository/oci_chart_repository.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
Copy link
Member

@pjbgf pjbgf left a comment

Choose a reason for hiding this comment

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

Great job getting the credential store to be unique per reconciliation. 👏

controllers/helmchart_controller.go Outdated Show resolved Hide resolved
controllers/helmchart_controller_test.go Outdated Show resolved Hide resolved
@makkes makkes deleted the rfc-0002-helm-oci branch May 19, 2022 12:50
@hiddeco
Copy link
Member

hiddeco commented May 19, 2022

I would still like @darkowlzz to do a review, even post-merge.

main.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
Get(name, version string) (*repo.ChartVersion, error)
// GetChartVersion returns a chart.ChartVersion from the remote repository.
DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error)
}
Copy link
Contributor

@darkowlzz darkowlzz May 20, 2022

Choose a reason for hiding this comment

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

This interface represents a Repository, maybe it doesn't matter if it's remote or local, download could just be a copy for local repositories. Being in chart package, it could be confused with a remote chart due to calling it just Remote.
Based on the implementations of this, it seems to fit better in the internal/helm/repository package with a generic name like just Repository.
Also, the method descriptions seem to be outdated with some previous names.

We may also need to add an Unload() method to this interface. The various implementation can implement it in their own appropriate ways, no-op for some and GC signal for others. I believe this would improve some usage of this interface below for repository download. downloadFromOCIRepository() and downloadFromRepository() could be unified by moving StrategicallyLoadIndex() for non-OCI repository into the implementation of Get(). The existing Get() can be renamed to something else to not remove the current behavior option if needed. Or the Get() interface method could be renamed to GetChart() which would be more appropriate as the interface is a repository and GetChart() describes exactly what's being fetched, and aligns with DownloadChart().

Copy link
Member

Choose a reason for hiding this comment

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

Remote and local chartBuilder have quite different code base, and a Repository in our code always download from a remote url.

I created the interface here because it is the only place where we need this abstraction for now.

I agree that the name needs to be changed and will propose a new one in the follow-up pr. I will also update the method description, thanks.

I agree to renaming Get to GetChart, that makes perfect sense. I'm not quite sure about having the Get method do more that what it does.

About the Unload() method in the interface, my feeling is that we should stick to having the minimum set of methods in the interface, instead of having no-op implementations.

@@ -171,6 +152,121 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o
return result, nil
}

func (b *remoteChartBuilder) downloadFromOCIRepository(remote *repository.OCIChartRepository, remoteRef RemoteReference, buildResult *Build, opts BuildOptions) (*bytes.Buffer, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The function arguments can be simplified by removing remote repository to be passed, as remoteChartBuilder embeds the remote. If the interface is expanded as suggested above, Get(), Unload() and DownloadChart() can be directly called on the remote without the need of any type casting.
If the remote interface is not expanded to have Unload(), as this function is aware of when it's called, instead of passing a typed remote, it can internally do the type casting. I think expanding the remote interface would help not have two separate functions for downloading, as most of the code is exactly the same.

Also, the buildResult argument can just become a returned value. Don't see any specific reason to accept it as a pointer. Whenever there's an error and buildResult is not assigned, the caller always ends further building and stops any usage of buildResult.

All these comments apply to downloadFromRepository() as well.

Copy link
Member

Choose a reason for hiding this comment

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

Now that I read this, an Unload() method in the interface makes more sense 👍

internal/helm/chart/builder_remote.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
@makkes makkes added the area/oci OCI related issues and pull requests label May 23, 2022
controllers/suite_test.go Show resolved Hide resolved

// Registry config
config := &configuration.Configuration{}
port, err := freeport.GetFreePort()
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's not worth importing a new dependency just to get a free port.
We have just done it simply in some tests like

l, err := net.Listen("tcp", ":0")
g.Expect(err).ToNot(HaveOccurred())
proxyAddr := fmt.Sprintf("localhost:%d", l.Addr().(*net.TCPAddr).Port)
which is what freeport does.

Copy link
Member

@souleb souleb May 24, 2022

Choose a reason for hiding this comment

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

see #728

Edit:
This causes error on the e2e tests. keeping the current code.

controllers/suite_test.go Show resolved Hide resolved
controllers/suite_test.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
HelmRepository_OCI reconcilers to make to retry on failure

The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
HelmRepository_OCI reconcilers to make to retry on failure

The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
HelmRepository_OCI reconcilers to make to retry on failure

The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
HelmRepository_OCI reconcilers to make to retry on failure

The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
HelmRepository_OCI reconcilers to make to retry on failure

The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 24, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
controllers/helmrepository_controller_oci.go Show resolved Hide resolved
func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error {
if !obj.DeletionTimestamp.IsZero() {
if !obj.DeletionTimestamp.IsZero() || (obj.Spec.Type != "" && obj.Spec.Type != sourcev1.HelmRepositoryTypeDefault) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about introducing a method on HelmRepositoryReconciler, say isDifferentType(obj), which does this non empty and not default type check? I believe that'd make it more readable and less error prone when writing the same conditional check multiple times, here and above in Reconcile().

souleb added a commit to souleb/source-controller that referenced this pull request May 25, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 25, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 25, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 25, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
souleb added a commit to souleb/source-controller that referenced this pull request May 27, 2022
The setupRegistryServer has been refactored to take into account fluxcd#690
reviews.

Signed-off-by: Soule BA <[email protected]>
@sandrotaje
Copy link

Hi, do you know when this feature will be released?

@pjbgf
Copy link
Member

pjbgf commented Jun 7, 2022

@sandrotaje this was released a few days ago on version v0.25.0. Since then there were some minor fixes, so please use the latest version of the source-controller.

Here's the official documentation on how to use it: https://fluxcd.io/docs/components/source/helmrepositories/#helm-oci-repository

@jobinjosem1
Copy link

Hi, Does that means we can now use helm repositories on Azure Container Registry? This document says using Charts from OCI Registries is not yet supported.
https://fluxcd.io/docs/use-cases/azure/#helm-repositories-on-azure-container-registry

@souleb
Copy link
Member

souleb commented Jun 10, 2022

@jobinjosem1 we shall update the doc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[RFC-0002] Implement Flux OCI support for Helm
8 participants