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

Enable Umbrella Chart with dependencies from OCI repositories #770

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

souleb
Copy link
Member

@souleb souleb commented Jun 7, 2022

fixes #722

new feature

When using a gitrepository source, it is possible to declare Umbrella Charts with a dependencies section like:

dependencies:
  - name: helmchart
    version: "0.1.0"
    repository: "file://../helmchart"
  - name: my-chart
    version: "1.1.0"
    repository: "https://my-helm-repository/helm-charts"

A depended helm chart can be resolved from a local path with file:// prefix or from a helm repository with https:// prefix in the repository field.

This pull request adds a new option for resolving depended helm chart, OCI helm repositories. If implemented, user will be able to declare three type of repositories like:

dependencies:
  - name: helmchart
    version: "0.1.0"
    repository: "file://../helmchart"
  - name: my-chart
    version: "1.1.0"
    repository: "https://my-helm-repository/helm-charts"
  - name: my-oci-chart
    version: "1.1.0"
    repository: "oci://my-oci-helm-repository/helm-charts"

code changes

Introduces a new interface that permits downloading Helm Charts from Helm HTTP/S repositories and Helm OCi repositories.

// Downloader is used to download a chart from a remote Helm repository or OCI registry.
type Downloader interface {
	// GetChartVersion returns the repo.ChartVersion for the given name and version
	// from the remote repository.ChartRepository.
	GetChartVersion(name, version string) (*repo.ChartVersion, error)
	// DownloadChart downloads a chart from the remote Helm repository or OCI registry.
	DownloadChart(chart *repo.ChartVersion) (*bytes.Buffer, error)
	Clear() error
}

The dependency manager is refactored to accept those interfaces instead of a repository struct. It enables using both repository types to resolve chart dependencies.

The remote builder has been refactored as part of this work.

@souleb souleb marked this pull request as draft June 7, 2022 22:14
@darkowlzz darkowlzz added area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests labels Jun 8, 2022
@souleb souleb marked this pull request as ready for review June 24, 2022 10:51
@souleb souleb requested a review from hiddeco June 24, 2022 10:55
internal/helm/chart/dependency_manager.go Outdated Show resolved Hide resolved
internal/helm/chart/dependency_manager.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
@darkowlzz
Copy link
Contributor

I did some manual testing in my development environment with harbor for storing chart dependencies as OCI artifacts.
Posting some results so that any possible weirdness can be noticed and highlighted.

Without this change, using a GitRepo as the chart source with dependencies:

dependencies:
- name: hello-helm
  version: "0.1.4"
  repository: "oci://harbor.example.com/helm-test"

Got DependencyBuildError:

{"level":"error","ts":"2022-06-28T20:54:51.494Z","logger":"controller.helmchart","msg":"Reconciler error","reconciler group":"source.toolkit.fluxcd.io","reconciler kind":"HelmChart","name":"default-greetings","namespace":"default","error":"dependency build error: failed to add remote dependency 'hello-helm': failed to load index for 'hello-helm': failed to strategically load index: failed to cache index to temporary file: object required"}

HelmChart status:

status:
  conditions:
  - lastTransitionTime: "2022-06-28T20:51:40Z"
    message: no artifact for resource in storage
    observedGeneration: 1
    reason: NoArtifact
    status: "True"
    type: Reconciling
  - lastTransitionTime: "2022-06-28T20:51:40Z"
    message: 'dependency build error: failed to add remote dependency ''hello-helm'':
      failed to load index for ''hello-helm'': failed to strategically load index:
      failed to cache index to temporary file: object required'
    observedGeneration: 1
    reason: DependencyBuildError
    status: "False"
    type: Ready
  - lastTransitionTime: "2022-06-28T20:51:40Z"
    message: 'dependency build error: failed to add remote dependency ''hello-helm'':
      failed to load index for ''hello-helm'': failed to strategically load index:
      failed to cache index to temporary file: object required'
    observedGeneration: 1
    reason: DependencyBuildError
    status: "True"
    type: BuildFailed
  observedGeneration: -1
  observedSourceArtifactRevision: main/4c5b64ddfec12f8aaafa3594af544385acacbd7f

Using this change, the build just succeeded and the HelmRelease was successful.

HelmChart status:

status:
  artifact:
    checksum: a8556733c0a2cc960c04d97432c8f7119b878026be65111437a00ca0e26c0f8f
    lastUpdateTime: "2022-06-28T21:15:26Z"
    path: helmchart/default/default-greetings/greetings-helm-0.1.1.tgz
    revision: 0.1.1
    size: 49332
    url: http://source-controller.flux-system.svc.cluster.local./helmchart/default/default-greetings/greetings-helm-0.1.1.tgz
  conditions:
  - lastTransitionTime: "2022-06-28T21:15:26Z"
    message: packaged 'greetings-helm' chart with version '0.1.1'
    observedGeneration: 1
    reason: ChartPackageSucceeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2022-06-28T21:15:26Z"
    message: packaged 'greetings-helm' chart with version '0.1.1'
    observedGeneration: 1
    reason: ChartPackageSucceeded
    status: "True"
    type: ArtifactInStorage
  observedChartName: greetings-helm
  observedGeneration: 1
  observedSourceArtifactRevision: main/4c5b64ddfec12f8aaafa3594af544385acacbd7f
  url: http://source-controller.flux-system.svc.cluster.local./helmchart/default/default-greetings/latest.tar.gz

controllers/helmchart_controller.go Outdated Show resolved Hide resolved
internal/helm/chart/dependency_manager.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
internal/helm/chart/dependency_manager.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
internal/helm/chart/dependency_manager_test.go Outdated Show resolved Hide resolved
internal/helm/chart/dependency_manager_test.go Outdated Show resolved Hide resolved
internal/helm/registry/client.go Outdated Show resolved Hide resolved
internal/helm/repository/chart_repository.go Outdated Show resolved Hide resolved
internal/helm/repository/oci_chart_repository.go Outdated Show resolved Hide resolved
@souleb souleb force-pushed the oci-for-deps-manager branch 5 times, most recently from 53c814c to ecec000 Compare July 4, 2022 13:47
@souleb souleb requested a review from darkowlzz July 4, 2022 15:30
@darkowlzz
Copy link
Contributor

I think this is looking very good now.
Can you do something about the last commit fixup! Enable remote dependencies from OCI repositories. Maybe amend it with the previous commits or make it independent with proper commit message.

internal/helm/repository/repository.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
internal/helm/chart/dependency_manager_test.go Outdated Show resolved Hide resolved
internal/helm/chart/builder_remote.go Show resolved Hide resolved
controllers/helmchart_controller.go Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
controllers/helmchart_controller.go Outdated Show resolved Hide resolved
Copy link
Contributor

@darkowlzz darkowlzz left a comment

Choose a reason for hiding this comment

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

LGTM!

@darkowlzz
Copy link
Contributor

Need rebasing.

// or a shim with defaults if no object could be found.
func (r *HelmChartReconciler) namespacedChartRepositoryCallback(ctx context.Context, name, namespace string) chart.GetChartRepositoryCallback {
Copy link
Member

Choose a reason for hiding this comment

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

This func has very much overlap with buildFromHelmRepository and I wonder whether we can converge some of it into a common func. Not a blocker, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we can reuse that much with all the serror.Event in buildFromHelmRepository.

@souleb
Copy link
Member Author

souleb commented Jul 6, 2022

Thanks all for the reviews. Can someone merge this please?

@hiddeco
Copy link
Member

hiddeco commented Jul 6, 2022

Requires another rebase.

Signed-off-by: Soule BA <soule@weave.works>
If implemented, the source controller will be able to resolve charts
dependencies from OCI repositories.

The remote builder has been refactored as part of this work.

Signed-off-by: Soule BA <soule@weave.works>
@darkowlzz darkowlzz merged commit 0219905 into fluxcd:main Jul 6, 2022
@souleb souleb deleted the oci-for-deps-manager branch July 7, 2022 06:54
@pjbgf pjbgf mentioned this pull request Jul 15, 2022
pjbgf pushed a commit that referenced this pull request Jul 15, 2022
This reverts commit 0219905, reversing
changes made to f7006e9.
pjbgf pushed a commit that referenced this pull request Jul 15, 2022
This reverts commit 0219905, reversing
changes made to f7006e9.
pjbgf pushed a commit that referenced this pull request Jul 15, 2022
This reverts commit 0219905, reversing
changes made to f7006e9.
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for OCI Helm Artifacts when building remote dependencies
5 participants