Add support for `OCIRepository` as chartRef #905

souleb · 2024-03-05T14:44:49Z

api/v2beta2/reference_types.go

config/testdata/install-from-ocirepo-source/test.yaml

hiddeco · 2024-03-17T14:35:41Z

internal/reconcile/helmchart_template.go

@@ -95,6 +95,20 @@ func (r *HelmChartTemplate) Reconcile(ctx context.Context, req *Request) error {
 		}
 	}

+	if mustCleanDeployedChart(obj) {


I would simply let the existence of a reference take precedence over the template existing.

My assumption is that if a user switch to a chartRef and wants to use an existing ociRepo it is because he wants to reduce the number of existing helmChart resources. So if we don't clean up during the reconciliation, the user will likely perform the clean up by other means. That would leave a status that does not reflect the actual state.

I think it's a must to cleanup the old Charts from storage and etcd.

api/v2beta2/helmrelease_types.go

api/v2beta2/reference_types.go

darkowlzz · 2024-03-25T19:27:16Z

internal/controller/helmrelease_controller.go

+	if !isValidChartRef(obj) {
+		return ctrl.Result{}, reconcile.TerminalError(fmt.Errorf("invalid Chart reference"))
+	}


This is expected to be an impossible scenario, right? Because the API server validation won't allow such object to be created.
But if there's a possibility for such object to be reconciled, ensuring that the object status reflects a stalled condition would be better. That is, moving to reconcileRelease(), marking the object with Ready=False and Stalled=True, and then returning the terminal error would be ideal.

But it can be left as it is if we will never process such object and the API server will let the user know about the invalid object.

We use CEL validation to have a oneOf validation behaviour. Quoting here "Kubernetes CRD Validation Using CEL has been turned on by default since Kubernetes 1.25 and graduated to GA in Kubernetes 1.29." As of now we support k8s from 1.27 so most likely it is safe, but I assume that it is still a possible scenario.

internal/controller/helmrelease_controller.go

api/v2beta2/helmrelease_types.go

darkowlzz

I fixed the broken code references from the last commit locally and did some manual testing and it works nicely.
Left some minor suggestions.

internal/controller/helmrelease_controller.go

internal/controller/helmrelease_controller_test.go

api/v2beta2/helmrelease_types.go

stefanprodan

It is not clear from this PR if a mutable tag will trigger a Helm upgrade when the upstream digest changes. Can we have a test for this?

config/testdata/upgrade-from-ocirepo-source/upgrade.yaml

config/testdata/upgrade-from-ocirepo-source/install.yaml

api/v2beta2/helmrelease_types.go

stefanprodan · 2024-04-12T17:12:26Z

Stuck helm-controller, the OCIRepo predicate is buggy:

$ flux -n podinfo get all
NAME                 	REVISION             	SUSPENDED	READY	MESSAGE                                            
ocirepository/podinfo	6.6.2@sha256:83295d47	False    	True 	stored artifact for digest '6.6.2@sha256:83295d47'	

NAME               	REVISION	SUSPENDED	READY	MESSAGE                                                                                           
helmrelease/podinfo	        	False    	False	OCIRepository 'podinfo/podinfo' is not ready: latest generation of object has not been reconciled

The OCIRepo observedGeneration is up to date:

apiVersion: source.toolkit.fluxcd.io/v1beta2
kind: OCIRepository
metadata:
  annotations:
    reconcile.fluxcd.io/requestedAt: "2024-04-12T20:09:15.148746+03:00"
  creationTimestamp: "2024-04-12T17:02:24Z"
  finalizers:
  - finalizers.fluxcd.io
  generation: 2
  name: podinfo
  namespace: podinfo
  resourceVersion: "5910"
  uid: d2464665-8e4c-4845-a449-2e7a384c3292
spec:
  interval: 1m
  layerSelector:
    mediaType: application/vnd.cncf.helm.chart.content.v1.tar+gzip
    operation: copy
  provider: generic
  ref:
    semver: '>=5.0.0'
  timeout: 60s
  url: oci://ghcr.io/stefanprodan/charts/podinfo
  verify:
    provider: notation
    secretRef:
      name: podinfo-notation
status:
  artifact:
    digest: sha256:9db648076519fe8ae594182fe0bb74bceb3bd516407d5da6577216d12a1f6625
    lastUpdateTime: "2024-04-12T17:07:06Z"
    metadata:
      org.opencontainers.image.authors: stefanprodan ([email protected])
      org.opencontainers.image.created: "2024-04-10T11:07:52Z"
      org.opencontainers.image.description: Podinfo Helm chart for Kubernetes
      org.opencontainers.image.source: https://github.com/stefanprodan/podinfo
      org.opencontainers.image.title: podinfo
      org.opencontainers.image.url: https://github.com/stefanprodan/podinfo
      org.opencontainers.image.version: 6.6.2
    path: ocirepository/podinfo/podinfo/sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154.tar.gz
    revision: 6.6.2@sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154
    size: 14905
    url: http://source-controller.flux-system.svc.cluster.local./ocirepository/podinfo/podinfo/sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154.tar.gz
  conditions:
  - lastTransitionTime: "2024-04-12T17:02:45Z"
    message: stored artifact for digest '6.6.2@sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154'
    observedGeneration: 2
    reason: Succeeded
    status: "True"
    type: Ready
  - lastTransitionTime: "2024-04-12T17:02:45Z"
    message: stored artifact for digest '6.6.2@sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154'
    observedGeneration: 2
    reason: Succeeded
    status: "True"
    type: ArtifactInStorage
  - lastTransitionTime: "2024-04-12T17:02:45Z"
    message: verified signature of revision 6.6.2@sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154
    observedGeneration: 2
    reason: Succeeded
    status: "True"
    type: SourceVerified
  lastHandledReconcileAt: "2024-04-12T20:09:15.148746+03:00"
  observedGeneration: 2
  observedLayerSelector:
    mediaType: application/vnd.cncf.helm.chart.content.v1.tar+gzip
    operation: copy
  url: http://source-controller.flux-system.svc.cluster.local./ocirepository/podinfo/podinfo/latest.tar.gz

stefanprodan · 2024-04-12T17:16:51Z

The test succeded event does not contain the oci-digest annotation and breaks the promotion workflow:

Helm test succeeded for release podinfo/podinfo.v2 with chart [email protected]+83295d47de6d: 3 test hooks completed successfully
revision
6.6.2+83295d47de6d

The upgrade event has it:

Helm upgrade succeeded for release podinfo/podinfo.v2 with chart [email protected]+83295d47de6d
oci-digest
sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154
revision
6.6.2+83295d47de6d

souleb · 2024-04-16T08:53:50Z

Just tested, test events show the digest as expected now:

- apiVersion: v1
  count: 1
  eventTime: null
  firstTimestamp: "2024-04-16T08:48:41Z"
  involvedObject:
    apiVersion: helm.toolkit.fluxcd.io/v2beta2
    kind: HelmRelease
    name: podinfo
    namespace: default
    resourceVersion: "3928"
    uid: 280e469f-e21e-4edd-b577-8d6ead548396
  kind: Event
  lastTimestamp: "2024-04-16T08:48:41Z"
  message: 'Helm test succeeded for release default/podinfo.v2 with chart [email protected]+83295d47de6d:
    3 test hooks completed successfully'
  metadata:
    annotations:
      helm.toolkit.fluxcd.io/oci-digest: sha256:83295d47de6d6ca634ed4b952a7572fc176bcc38854d0c11ca0fa197bc5f1154
      helm.toolkit.fluxcd.io/revision: 6.6.2+83295d47de6d
      helm.toolkit.fluxcd.io/token: sha256:5ee5478adca0d5fa927d1ebff4714f8eea683389ac0e21489b5572806e3ab307
    creationTimestamp: "2024-04-16T08:48:41Z"
    name: podinfo.17c6b674cd8c4571
    namespace: default
    resourceVersion: "4061"
    uid: 15a0533d-68d0-43ae-b019-ff3dd26ddbd2
  reason: TestSucceeded
  reportingComponent: helm-controller
  reportingInstance: ""
  source:
    component: helm-controller
  type: Normal
kind: List
metadata:
  resourceVersion: ""

souleb · 2024-04-16T08:55:11Z

internal/reconcile/rollback_remediation.go

@@ -182,7 +182,7 @@ func observeRollback(obj *v2.HelmRelease) storage.ObserveFunc {
 		for i := range obj.Status.History {
 			snap := obj.Status.History[i]
 			if snap.Targets(rls.Name, rls.Namespace, rls.Version) {
-				newSnap := release.ObservedToSnapshot(release.ObserveRelease(rls))
+				newSnap := release.ObservedToSnapshot(releaseToObservation(rls, snap))


note that if no matching snapshot is found, we have no way of retrieving the corresponding oci digest.

stefanprodan

LGTM

Awesome contribution! Thanks @souleb 🥇

PS. I have run extensive tests and all the issues I found are now fixed.

gabrielqs · 2024-04-16T20:57:48Z

is this ready to go live? can't wait to use it

stefanprodan · 2024-04-16T21:05:26Z

@gabrielqs this will be released in Flux v2.3, see fluxcd/flux2#4712

souleb · 2024-04-17T10:01:59Z

I plan to merge this tomorrow, so late reviews can still happen.

It takes into account switching from a chart template to a referenced source (garbage collection). Signed-off-by: Soule BA <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]> Signed-off-by: souleb <[email protected]>

Signed-off-by: Soule BA <[email protected]>

Co-authored-by: Stefan Prodan <[email protected]> Signed-off-by: souleb <[email protected]>

This is needed for an OCIRepository source in order to detect change for mutable tags. Signed-off-by: Soule BA <[email protected]>

Signed-off-by: Soule BA <[email protected]>

This commit add the oci artifact digest into the release observed snapshot. This is used to later to add that value as an annotation. Signed-off-by: Soule BA <[email protected]>

The test case successfully upgrade with the same chart because version is not computed the same way (12 digits of digest appended for OCIRepository source). Signed-off-by: Soule BA <[email protected]>

Signed-off-by: Soule BA <[email protected]>

Use artifact digest instead of revision to validate whether to trigger a new reconciliation Signed-off-by: Soule BA <[email protected]>

Add support for `OCIRepository` as chartRef Signed-off-by: Sean Eagan <[email protected]>

Add support for `OCIRepository` as chartRef

souleb changed the title ~~enable reusing an existing OCIRepo as chartRef~~ Enable reusing an existing OCIRepo as chartRef Mar 5, 2024

souleb marked this pull request as draft March 5, 2024 14:45

souleb added the enhancement New feature or request label Mar 5, 2024

stefanprodan reviewed Mar 5, 2024

View reviewed changes

api/v2beta2/reference_types.go Outdated Show resolved Hide resolved

souleb force-pushed the enable-ocirepo-sources branch 2 times, most recently from 35da8cd to 87fd317 Compare March 9, 2024 21:21

souleb changed the title ~~Enable reusing an existing OCIRepo as chartRef~~ Enable reusing an existing Source as chartRef Mar 9, 2024

souleb changed the title ~~Enable reusing an existing Source as chartRef~~ Enable reusing an existing OCIRepo as chartRef Mar 11, 2024

souleb force-pushed the enable-ocirepo-sources branch 2 times, most recently from 3a93f37 to 5cf3d4d Compare March 12, 2024 10:00

stefanprodan reviewed Mar 12, 2024

View reviewed changes

config/testdata/install-from-ocirepo-source/test.yaml Outdated Show resolved Hide resolved

souleb force-pushed the enable-ocirepo-sources branch 3 times, most recently from 2ddf5c2 to 07c3cc4 Compare March 12, 2024 15:34

souleb marked this pull request as ready for review March 12, 2024 15:44

souleb requested a review from darkowlzz March 12, 2024 15:56

souleb force-pushed the enable-ocirepo-sources branch from 976ee54 to 071fbef Compare March 13, 2024 16:19

hiddeco reviewed Mar 17, 2024

View reviewed changes

api/v2beta2/helmrelease_types.go Outdated Show resolved Hide resolved

hiddeco reviewed Mar 17, 2024

View reviewed changes

api/v2beta2/helmrelease_types.go Outdated Show resolved Hide resolved

darkowlzz reviewed Mar 25, 2024

View reviewed changes

darkowlzz reviewed Mar 26, 2024

View reviewed changes

api/v2beta2/helmrelease_types.go Show resolved Hide resolved

stefanprodan reviewed Apr 1, 2024

View reviewed changes

stefanprodan reviewed Apr 2, 2024

View reviewed changes

config/testdata/upgrade-from-ocirepo-source/upgrade.yaml Outdated Show resolved Hide resolved

config/testdata/upgrade-from-ocirepo-source/install.yaml Outdated Show resolved Hide resolved

stefanprodan added this to the Helm GA milestone Apr 2, 2024

souleb force-pushed the enable-ocirepo-sources branch from b40425d to d875211 Compare April 3, 2024 10:10

stefanprodan changed the title ~~Enable reusing an existing OCIRepo as chartRef~~ Add support for OCIRepository as chartRef Apr 3, 2024

stefanprodan reviewed Apr 3, 2024

View reviewed changes

api/v2beta2/helmrelease_types.go Outdated Show resolved Hide resolved

souleb force-pushed the enable-ocirepo-sources branch from d875211 to 4bdc4c3 Compare April 4, 2024 09:34

souleb force-pushed the enable-ocirepo-sources branch from 0cfb143 to 48fa9eb Compare April 12, 2024 15:42

souleb force-pushed the enable-ocirepo-sources branch from 4ce7e3c to 94ccc4f Compare April 15, 2024 09:58

souleb commented Apr 16, 2024

View reviewed changes

souleb force-pushed the enable-ocirepo-sources branch from 453b631 to 2da06bc Compare April 16, 2024 09:47

stefanprodan approved these changes Apr 16, 2024

View reviewed changes

souleb and others added 12 commits April 18, 2024 13:05

This commit enable reusing an existing OCIRepo as chartRef.

20e14fe

It takes into account switching from a chart template to a referenced source (garbage collection). Signed-off-by: Soule BA <[email protected]>

Update api/v2beta2/helmrelease_types.go

350accf

Co-authored-by: Hidde Beydals <[email protected]> Signed-off-by: souleb <[email protected]>

fix methods names

157f806

Signed-off-by: Soule BA <[email protected]>

address review comments

686fe58

Signed-off-by: Soule BA <[email protected]>

adding an upgrade from ocirepo test

d1d2d00

Signed-off-by: Soule BA <[email protected]>

Apply suggestions from code review

7864e3a

Co-authored-by: Stefan Prodan <[email protected]> Signed-off-by: souleb <[email protected]>

Adding 12 first character of digest to chart version

aeac55d

This is needed for an OCIRepository source in order to detect change for mutable tags. Signed-off-by: Soule BA <[email protected]>

adding a section for chartRef in the doc

49b47d4

Signed-off-by: Soule BA <[email protected]>

Take into account the oci-digest

edec322

This commit add the oci artifact digest into the release observed snapshot. This is used to later to add that value as an annotation. Signed-off-by: Soule BA <[email protected]>

Add a test when switching from chart template to chartRef

65a02c8

The test case successfully upgrade with the same chart because version is not computed the same way (12 digits of digest appended for OCIRepository source). Signed-off-by: Soule BA <[email protected]>

All observeFuncs make take into account existing OCI Digest in snapshots

f5447b4

Signed-off-by: Soule BA <[email protected]>

fix requestForOCIRepository change

a98d957

Use artifact digest instead of revision to validate whether to trigger a new reconciliation Signed-off-by: Soule BA <[email protected]>

souleb force-pushed the enable-ocirepo-sources branch from 2da06bc to a98d957 Compare April 18, 2024 11:10

souleb merged commit 5e760db into main Apr 18, 2024
6 checks passed

souleb deleted the enable-ocirepo-sources branch April 18, 2024 11:39

seaneagan pushed a commit to seaneagan/helm-controller that referenced this pull request Apr 18, 2024

Merge pull request fluxcd#905 from fluxcd/enable-ocirepo-sources

8bb19ea

Add support for `OCIRepository` as chartRef Signed-off-by: Sean Eagan <[email protected]>

seaneagan pushed a commit to seaneagan/helm-controller that referenced this pull request Apr 18, 2024

Merge pull request fluxcd#905 from fluxcd/enable-ocirepo-sources

9c1c670

Add support for `OCIRepository` as chartRef

stefanprodan mentioned this pull request Apr 22, 2024

Add support for HelmChart as chartRef #945

Merged

stefanprodan mentioned this pull request May 9, 2024

Prevent snapshot drift when upgrading to API v2 #975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `OCIRepository` as chartRef #905

Add support for `OCIRepository` as chartRef #905

souleb commented Mar 5, 2024 •

edited

Loading

hiddeco Mar 17, 2024

souleb Mar 26, 2024

stefanprodan Apr 11, 2024

darkowlzz Mar 25, 2024

souleb Mar 25, 2024

darkowlzz left a comment •

edited

Loading

stefanprodan left a comment

stefanprodan commented Apr 12, 2024 •

edited

Loading

stefanprodan commented Apr 12, 2024 •

edited

Loading

souleb commented Apr 16, 2024

souleb Apr 16, 2024 •

edited

Loading

stefanprodan left a comment

gabrielqs commented Apr 16, 2024

stefanprodan commented Apr 16, 2024 •

edited

Loading

souleb commented Apr 17, 2024

Add support for OCIRepository as chartRef #905

Add support for OCIRepository as chartRef #905

Conversation

souleb commented Mar 5, 2024 • edited Loading

hiddeco Mar 17, 2024

Choose a reason for hiding this comment

souleb Mar 26, 2024

Choose a reason for hiding this comment

stefanprodan Apr 11, 2024

Choose a reason for hiding this comment

darkowlzz Mar 25, 2024

Choose a reason for hiding this comment

souleb Mar 25, 2024

Choose a reason for hiding this comment

darkowlzz left a comment • edited Loading

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

stefanprodan commented Apr 12, 2024 • edited Loading

stefanprodan commented Apr 12, 2024 • edited Loading

souleb commented Apr 16, 2024

souleb Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

stefanprodan left a comment

Choose a reason for hiding this comment

gabrielqs commented Apr 16, 2024

stefanprodan commented Apr 16, 2024 • edited Loading

souleb commented Apr 17, 2024

Add support for `OCIRepository` as chartRef #905

Add support for `OCIRepository` as chartRef #905

souleb commented Mar 5, 2024 •

edited

Loading

darkowlzz left a comment •

edited

Loading

stefanprodan commented Apr 12, 2024 •

edited

Loading

stefanprodan commented Apr 12, 2024 •

edited

Loading

souleb Apr 16, 2024 •

edited

Loading

stefanprodan commented Apr 16, 2024 •

edited

Loading