Initial proposal of the new Helmops controller. #3092

0xavi0 · 2024-11-22T17:33:43Z

This adds a new custom resource HelmApp (resource name open to debate) that describes a helm chart to be deployed.

The resource contains all the fields from the classic fleet.yaml file plus a few new from the GitRepo resource.

HelmApp yaml example:

apiVersion: fleet.cattle.io/v1alpha1
kind: HelmApp
metadata:
  name: sample1
  namespace: fleet-local
spec:
  helm:
    releaseName: testhelm
    repo: https://charts.bitnami.com/bitnami
    chart: postgresql
    version: 16.2.1
  insecureSkipTLSVerify: true

The implementation tries to share as much as possible from a Bundle spec inside the new resource, because it helps to "transform" the HelmApp into a deployment (no conversion is needed for most of the spec).

The new controller was also implemented splitting the functionality into 2 controllers (similar to what we did for the GitRepo controller). This allows us to reuse most of the status handling code, as display fields in the status of the new resource are as similar as possible to have consistent user experience and to integrate with the UI in the same way the GitRepo does.

When a new HelmApp resource is applied it is transformed into a single Bundle, adding some extra fields to let the Bundle reconciler know that this is not a regular Bundle coming from a GitRepo.

Similar as we did for OCI storage, the Bundle created from a HelmApp does not contain resources. The helm chart to be deployed is downloaded by the agent.

Code for downloading the helm chart is reused from gitops, so the same formats are supported. Insecure TLS skipping was added the the ChartURL and LoadDirectory functions in order to support this for gitops and helmops.

If we need a secret to access the helm repository we can use the helmSecretName field. This secret will be cloned to secrets under the BundleDeployment namespace (same as we did for the OCI storage secret handling).

The PR includes unit, integration (most of code is tested this way) and just one single e2e test so far just to test the whole feature together in a real cluster.

The following image describes how Spec fields are shared between Bundle and HelmApp and how the Status fields are shared between GitRepo and HelmApp:

Note: This is an experimental feature. In order to activate the HelmApp reconciling and Bundle deployment you need to set the following environment variable: EXPERIMENTAL_HELM_OPS=true

Refers to: #2962

It adds a new custom resource `HelmApp` (resource name open to debate) that describes a helm chart to be deployed. The resource contains all the fealds from the classic `fleet.yaml` file plus a few new from the `GitRepo` resource. `HelmApp` yaml example: ```yaml apiVersion: fleet.cattle.io/v1alpha1 kind: HelmApp metadata: name: sample1 namespace: fleet-local spec: helm: releaseName: testhelm repo: https://charts.bitnami.com/bitnami chart: postgresql version: 16.2.1 insecureSkipTLSVerify: true ``` The implementation tries to share as much as possible from a `Bundle` spec inside the new resource, because it helps to "transform" the `HelmApp` into a deployment (no coversion is needed for most of the spec). The new controller was also implemented splitting the functionality into 2 controllers (similar to what we did for the `GitRepo` controller). This allows us to reuse most of the status handling code, as display fields in the status of the new resource are as similar as possible to have consistent user experience and to integrate with the UI in the same way the `GitRepo` does. When a new `HelmApp` resource is applied it is transformed into a single `Bundle`, adding some extra fields to let the `Bundle` reconciler know that this is not a regular `Bundle` coming from a `GitRepo`. Similar as we did for OCI storage, the `Bundle` created from a `HelmApp` does not contain resources. The helm chart to be deployed is downloaded by the agent. Code for downloading the helm chart is reused from gitops, so the same formats are supported. Insecure TLS skipping was added the the ChartURL and LoadDirectory functions in order to support this for gitops and helmops. If we need a secret to access the helm repository we can use the `helmSecretName` field. This secret will be cloned to secrets under the `BundleDeployment` namespace (same as we did for the OCI storage secret handling). The PR includes unit, integration (most of code is tested this way) and just one single e2e test so far just to test the whole feature together in a real cluster. Note: This is an experimental feature. In order to activate the `HelmApp` reconciling and `Bundle` deployment you need to the the environment variable: `EXPERIMENTAL_HELM_OPS=true` Refers to: rancher#2962

Signed-off-by: Xavi Garcia <[email protected]>

This is so we can enable the UI and browse artifacts Signed-off-by: Xavi Garcia <[email protected]>

Signed-off-by: Xavi Garcia <[email protected]>

weyfonk

Thanks for this! Leaving ~~a few~~ comments, many of which are just nitpicks.
This new feature creates bundles synchronously, which may be fine for now as they are more lightweight, without resources, than in the gitOps case. But I wonder if that may become an issue on larger setups (although worker counts will soon be configurable event in the agent).

weyfonk · 2024-11-28T12:15:13Z

charts/fleet/templates/deployment_helmops.yaml

+{{- end -}}
+
+{{ range $shard := $shards }}
+{{- if has (dict "name" "EXPERIMENTAL_HELM_OPS" "value" "true") $.Values.extraEnv }}


nit: As discussed offline, this condition could be moved out of the loop over shards, and something similar could be done to the check on $.Values.gitops.enabled in the gitjob deployment.

weyfonk · 2024-11-28T12:32:01Z

charts/fleet/templates/rbac_helmops.yaml

+    resources:
+      - 'events'
+    verbs:
+      - '*'


nit: do we need all verbs here? Wouldn't create be enough?

(I'm aware that this probably applies to gitjob and perhaps other RBAC definitions in the fleet chart as well)

weyfonk · 2024-11-28T15:08:25Z

internal/bundlereader/loaddirectory.go

+	} else {
+		if auth.InsecureSkipVerify {


nit:

Suggested change

} else {

if auth.InsecureSkipVerify {

} else if auth.InsecureSkipVerify {

// one fewer level of indentation

weyfonk · 2024-11-28T15:10:55Z

internal/bundlereader/loaddirectory.go

@@ -166,7 +166,7 @@ func readFleetIgnore(path string) ([]string, error) {
 	return ignored, nil
 }

-func loadDirectory(ctx context.Context, compress bool, disableDepsUpdate bool, prefix, base, source, version string, auth Auth) ([]fleet.BundleResource, error) {
+func LoadDirectory(ctx context.Context, compress bool, disableDepsUpdate bool, prefix, base, source, version string, auth Auth) ([]fleet.BundleResource, error) {


Why does this function need to be exported?

weyfonk · 2024-12-02T11:56:16Z

internal/bundlereader/charturl.go

 // inspecting the repo's index.yaml
-func chartURL(location *fleet.HelmOptions, auth Auth) (string, error) {
-	// repos are not supported in case of OCI Charts
+func ChartURLVersion(location fleet.HelmOptions, auth Auth) (string, string, error) {


I like that we're able to return both URL and version, that seems useful. However, do we ever need to fetch both at once? A brief look at calls suggests that we may not.

weyfonk · 2024-12-03T10:48:12Z

internal/bundlereader/helm_test.go

+func checksumPrefix(helm *fleet.HelmOptions) string {
+	if helm == nil {
+		return "none"
+	}
+	return fmt.Sprintf(".chart/%x", sha256.Sum256([]byte(helm.Chart + ":" + helm.Repo + ":" + helm.Version)[:]))
+}


Why reimplement this logic, instead of exporting and using bundlereader.checksum?

weyfonk · 2024-12-03T10:55:15Z

integrationtests/helmops/controller/controller_test.go

+}
+
+func randHelmOptions() *fleet.HelmOptions {
+	// release name "mqMitPLkEI"


äh? Is this a leftover from a debugging session? :)

weyfonk · 2024-12-03T11:05:09Z

integrationtests/helmops/controller/controller_test.go

+			}
+
+			DeferCleanup(func() {
+				Expect(k8sClient.Delete(ctx, nsSpec)).ToNot(HaveOccurred())


nit: since not all test cases do it:

Suggested change

Expect(k8sClient.Delete(ctx, nsSpec)).ToNot(HaveOccurred())

Expect(k8sClient.Delete(ctx, nsSpec)).ToNot(HaveOccurred())

_ = k8sClient.Delete(ctx, helmApp)

weyfonk · 2024-12-03T11:10:55Z

integrationtests/helmops/controller/controller_test.go

+				}
+			})
+
+			It("creates a bundle with the expected spec and the targets and customization merged", func() {


If we keep this behaviour, we'll want to document it. Fleet's docs website currently states that:

The targetCustomizations: in fleet.yaml override Helm values only and do not change targeting.

Granted, that's for fleet.yaml, hence GitRepos, but we may end up with 2 config directives named identically but which behave differently depending on the source of truth (git vs Helm).

weyfonk · 2024-12-03T11:14:06Z

integrationtests/helmops/controller/controller_test.go

+				helmapp.Spec.Helm.Chart = "alpine"
+			})
+
+			It("creates a bundle with the latest version it got from the index", func() {


Nice one! Removing my comment on unit tests about the need to cover this use case 😁

Also: do we want a similar test case for a wildcard version?

Signed-off-by: Xavi Garcia <[email protected]>

0xavi0 self-assigned this Nov 22, 2024

0xavi0 force-pushed the 2962-helm-charts branch 6 times, most recently from b8eac37 to 5324f8f Compare November 27, 2024 08:30

0xavi0 force-pushed the 2962-helm-charts branch from 5324f8f to 4ad653a Compare November 27, 2024 09:15

0xavi0 marked this pull request as ready for review November 28, 2024 16:47

0xavi0 requested a review from a team as a code owner November 28, 2024 16:47

0xavi0 added 3 commits December 2, 2024 19:11

Add Insecure TLS option when downloading from OCI registry

b797805

Signed-off-by: Xavi Garcia <[email protected]>

Upgrades zot to version 2.1.1

32ad8ee

This is so we can enable the UI and browse artifacts Signed-off-by: Xavi Garcia <[email protected]>

Fix resource names unit test

abf1868

Signed-off-by: Xavi Garcia <[email protected]>

weyfonk reviewed Dec 3, 2024

View reviewed changes

Changes after 1st review

acc2d99

Signed-off-by: Xavi Garcia <[email protected]>

0xavi0 force-pushed the 2962-helm-charts branch from eb1967d to acc2d99 Compare December 4, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial proposal of the new Helmops controller. #3092

Initial proposal of the new Helmops controller. #3092

0xavi0 commented Nov 22, 2024 •

edited by weyfonk

Loading

weyfonk left a comment

weyfonk Nov 28, 2024

weyfonk Nov 28, 2024

weyfonk Nov 28, 2024

weyfonk Nov 28, 2024

weyfonk Dec 2, 2024

weyfonk Dec 3, 2024

weyfonk Dec 3, 2024

weyfonk Dec 3, 2024

weyfonk Dec 3, 2024

weyfonk Dec 3, 2024

	Expect(k8sClient.Delete(ctx, nsSpec)).ToNot(HaveOccurred())
	Expect(k8sClient.Delete(ctx, nsSpec)).ToNot(HaveOccurred())
	_ = k8sClient.Delete(ctx, helmApp)

Initial proposal of the new Helmops controller. #3092

Are you sure you want to change the base?

Initial proposal of the new Helmops controller. #3092

Conversation

0xavi0 commented Nov 22, 2024 • edited by weyfonk Loading

weyfonk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xavi0 commented Nov 22, 2024 •

edited by weyfonk

Loading