From a67d25256c913a33e017005089ad0f1097d4b4d5 Mon Sep 17 00:00:00 2001 From: Andrew Lavery Date: Fri, 3 May 2024 23:41:40 +0800 Subject: [PATCH] add protected/dynamic values to charts before reconcile (#163) * add protected/dynamic values to charts before reconcile * remove protected values handling * update unit tests * use logger --- controllers/helm.go | 143 ++++----- controllers/helm_test.go | 312 ++++++-------------- controllers/installation_controller.go | 8 +- controllers/installation_controller_test.go | 60 +++- 4 files changed, 192 insertions(+), 331 deletions(-) diff --git a/controllers/helm.go b/controllers/helm.go index b31910b75..db0055a8d 100644 --- a/controllers/helm.go +++ b/controllers/helm.go @@ -1,6 +1,7 @@ package controllers import ( + "context" "fmt" "path/filepath" @@ -8,8 +9,8 @@ import ( k0shelm "github.com/k0sproject/k0s/pkg/apis/helm/v1beta1" k0sv1beta1 "github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1" "github.com/ohler55/ojg/jp" - "github.com/ohler55/ojg/oj" ectypes "github.com/replicatedhq/embedded-cluster-kinds/types" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/yaml" "github.com/replicatedhq/embedded-cluster-kinds/apis/v1beta1" @@ -17,55 +18,31 @@ import ( const DEFAULT_VENDOR_CHART_ORDER = 10 -// MergeValues takes two helm values in the form of dig.Mapping{} and a list of values (in jsonpath notation) to not override -// and combines the values. it returns the resultant yaml string -func MergeValues(oldValues, newValues string, protectedValues []string) (string, error) { - +func setHelmValue(valuesYaml, path, newValue string) (string, error) { newValuesMap := dig.Mapping{} - if err := yaml.Unmarshal([]byte(newValues), &newValuesMap); err != nil { - return "", fmt.Errorf("failed to unmarshal new chart values: %w", err) + if err := yaml.Unmarshal([]byte(valuesYaml), &newValuesMap); err != nil { + return "", fmt.Errorf("failed to unmarshal initial values: %w", err) } - // merge the known fields from the current chart values to the new chart values - for _, path := range protectedValues { - x, err := jp.ParseString(path) - if err != nil { - return "", fmt.Errorf("failed to parse json path: %w", err) - } - - valuesJson, err := yaml.YAMLToJSON([]byte(oldValues)) - if err != nil { - return "", fmt.Errorf("failed to convert yaml to json: %w", err) - } - - obj, err := oj.ParseString(string(valuesJson)) - if err != nil { - return "", fmt.Errorf("failed to parse json: %w", err) - } - - value := x.Get(obj) - - // if the value is empty, skip it - if len(value) < 1 { - continue - } + x, err := jp.ParseString(path) + if err != nil { + return "", fmt.Errorf("failed to parse json path %q: %w", path, err) + } - err = x.Set(newValuesMap, value[0]) - if err != nil { - return "", fmt.Errorf("failed to set json path: %w", err) - } + err = x.Set(newValuesMap, newValue) + if err != nil { + return "", fmt.Errorf("failed to set json path %q to %q: %w", path, newValue, err) } newValuesYaml, err := yaml.Marshal(newValuesMap) if err != nil { - return "", fmt.Errorf("failed to marshal new chart values: %w", err) + return "", fmt.Errorf("failed to marshal updated values: %w", err) } return string(newValuesYaml), nil - } // merge the default helm charts and repositories (from meta.Configs) with vendor helm charts (from in.Spec.Config.Extensions.Helm) -func mergeHelmConfigs(meta *ectypes.ReleaseMetadata, in *v1beta1.Installation) *k0sv1beta1.HelmExtensions { +func mergeHelmConfigs(ctx context.Context, meta *ectypes.ReleaseMetadata, in *v1beta1.Installation) *k0sv1beta1.HelmExtensions { // merge default helm charts (from meta.Configs) with vendor helm charts (from in.Spec.Config.Extensions.Helm) combinedConfigs := &k0sv1beta1.HelmExtensions{ConcurrencyLevel: 1} if meta != nil { @@ -100,6 +77,9 @@ func mergeHelmConfigs(meta *ectypes.ReleaseMetadata, in *v1beta1.Installation) * combinedConfigs.Repositories = append(combinedConfigs.Repositories, meta.BuiltinConfigs["velero"].Repositories...) } + // update the infrastructure charts from the install spec + combinedConfigs.Charts = updateInfraChartsFromInstall(ctx, in, combinedConfigs.Charts) + // k0s sorts order numbers alphabetically because they're used in file names, // which means double digits can be sorted before single digits (e.g. "10" comes before "5"). // We add 100 to the order of each chart to work around this. @@ -109,6 +89,51 @@ func mergeHelmConfigs(meta *ectypes.ReleaseMetadata, in *v1beta1.Installation) * return combinedConfigs } +// update the 'admin-console' and 'embedded-cluster-operator' charts to add cluster ID, binary name, and airgap status +func updateInfraChartsFromInstall(ctx context.Context, in *v1beta1.Installation, charts k0sv1beta1.ChartsSettings) k0sv1beta1.ChartsSettings { + log := ctrl.LoggerFrom(ctx) + + if in == nil { + return charts + } + + for i, chart := range charts { + if chart.Name == "admin-console" { + // admin-console has "embeddedClusterID" and "isAirgap" as dynamic values + newVals, err := setHelmValue(chart.Values, "embeddedClusterID", in.Spec.ClusterID) + if err != nil { + log.Info("failed to set embeddedClusterID for %s: %v", chart.Name, err) + continue + } + + newVals, err = setHelmValue(newVals, "isAirgap", fmt.Sprintf("%t", in.Spec.AirGap)) + if err != nil { + log.Info("failed to set isAirgap for %s: %v", chart.Name, err) + continue + } + + charts[i].Values = newVals + } + if chart.Name == "embedded-cluster-operator" { + // embedded-cluster-operator has "embeddedBinaryName" and "embeddedClusterID" as dynamic values + newVals, err := setHelmValue(chart.Values, "embeddedBinaryName", in.Spec.BinaryName) + if err != nil { + log.Info("failed to set embeddedBinaryName for %s: %v", err) + continue + } + + newVals, err = setHelmValue(newVals, "embeddedClusterID", in.Spec.ClusterID) + if err != nil { + log.Info("failed to set embeddedClusterID for %s: %v", err) + continue + } + + charts[i].Values = newVals + } + } + return charts +} + // detect if the charts currently installed in the cluster (currentConfigs) match the desired charts (combinedConfigs) func detectChartDrift(combinedConfigs, currentConfigs *k0sv1beta1.HelmExtensions) (bool, []string, error) { chartDrift := false @@ -250,50 +275,6 @@ func applyUserProvidedAddonOverrides(in *v1beta1.Installation, combinedConfigs * return patchedConfigs, nil } -// merge the helmcharts in the cluster with the charts we desire to be in the cluster -// if the chart is already in the cluster, merge the values -func generateDesiredCharts(meta *ectypes.ReleaseMetadata, clusterconfig k0sv1beta1.ClusterConfig, combinedConfigs *k0sv1beta1.HelmExtensions) ([]k0sv1beta1.Chart, error) { - // get the protected values from the release metadata - protectedValues := map[string][]string{} - if meta != nil && meta.Protected != nil { - protectedValues = meta.Protected - } - - // TODO - apply unsupported override from installation config - finalConfigs := map[string]k0sv1beta1.Chart{} - // include charts in the final spec that are already in the cluster (with merged values) - for _, chart := range clusterconfig.Spec.Extensions.Helm.Charts { - for _, newChart := range combinedConfigs.Charts { - // check if we can skip this chart - _, ok := protectedValues[chart.Name] - if chart.Name != newChart.Name || !ok { - continue - } - // if we have known fields, we need to merge them forward - newValuesYaml, err := MergeValues(chart.Values, newChart.Values, protectedValues[chart.Name]) - if err != nil { - return nil, fmt.Errorf("failed to merge chart values: %w", err) - } - newChart.Values = newValuesYaml - finalConfigs[newChart.Name] = newChart - break - } - } - // include new charts in the final spec that are not yet in the cluster - for _, newChart := range combinedConfigs.Charts { - if _, ok := finalConfigs[newChart.Name]; !ok { - finalConfigs[newChart.Name] = newChart - } - } - - // flatten chart map - finalChartList := []k0sv1beta1.Chart{} - for _, chart := range finalConfigs { - finalChartList = append(finalChartList, chart) - } - return finalChartList, nil -} - // patchExtensionsForAirGap makes sure we do not have any external repository reference and also makes // sure that all helm charts point to a chart stored on disk as a tgz file. These files are already // expected to be present on the disk and, during an upgrade, are laid down on disk by the artifact diff --git a/controllers/helm_test.go b/controllers/helm_test.go index f8c6c3456..2728c445d 100644 --- a/controllers/helm_test.go +++ b/controllers/helm_test.go @@ -1,114 +1,16 @@ package controllers import ( + "context" "testing" - "github.com/k0sproject/dig" k0shelm "github.com/k0sproject/k0s/pkg/apis/helm/v1beta1" k0sv1beta1 "github.com/k0sproject/k0s/pkg/apis/k0s/v1beta1" "github.com/replicatedhq/embedded-cluster-kinds/apis/v1beta1" ectypes "github.com/replicatedhq/embedded-cluster-kinds/types" "github.com/stretchr/testify/require" - "sigs.k8s.io/yaml" ) -func Test_mergeValues(t *testing.T) { - tests := []struct { - name string - oldValues string - newValues string - protectedValues []string - want string - }{ - { - name: "combined test", - oldValues: ` - password: "foo" - someField: "asdf" - other: "text" - overridden: "abcxyz" - nested: - nested: - protect: "testval" -`, - newValues: ` - someField: "newstring" - other: "text" - overridden: "this is new" - nested: - nested: - newkey: "newval" - protect: "newval" -`, - protectedValues: []string{"password", "overridden", "nested.nested.protect"}, - want: ` - password: "foo" - someField: "newstring" - nested: - nested: - newkey: "newval" - protect: "testval" - other: "text" - overridden: "abcxyz" -`, - }, - { - name: "empty old values", - oldValues: ``, - newValues: ` - someField: "newstring" - other: "text" - overridden: "this is new" - nested: - nested: - newkey: "newval" - protect: "newval" -`, - protectedValues: []string{"password", "overridden", "nested.nested.protect"}, - want: ` - someField: "newstring" - overridden: "this is new" - nested: - nested: - newkey: "newval" - protect: "newval" - other: "text" -`, - }, - { - name: "no protected values", - oldValues: ` - password: "foo" - someField: "asdf" -`, - newValues: ` -password: "newpassword" -`, - protectedValues: []string{}, - want: ` -password: "newpassword" -`, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - req := require.New(t) - got, err := MergeValues(tt.oldValues, tt.newValues, tt.protectedValues) - req.NoError(err) - - targetDataMap := dig.Mapping{} - err = yaml.Unmarshal([]byte(tt.want), &targetDataMap) - req.NoError(err) - - mergedDataMap := dig.Mapping{} - err = yaml.Unmarshal([]byte(got), &mergedDataMap) - req.NoError(err) - - req.Equal(targetDataMap, mergedDataMap) - }) - } -} - func Test_mergeHelmConfigs(t *testing.T) { type args struct { meta *ectypes.ReleaseMetadata @@ -336,7 +238,7 @@ func Test_mergeHelmConfigs(t *testing.T) { } req := require.New(t) - got := mergeHelmConfigs(tt.args.meta, &installation) + got := mergeHelmConfigs(context.TODO(), tt.args.meta, &installation) req.Equal(tt.want, got) }) } @@ -653,134 +555,6 @@ func Test_detectChartCompletion(t *testing.T) { } } -func Test_generateDesiredCharts(t *testing.T) { - type args struct { - meta *ectypes.ReleaseMetadata - clusterconfig k0sv1beta1.ClusterConfig - combinedConfigs *k0sv1beta1.HelmExtensions - } - tests := []struct { - name string - args args - want []k0sv1beta1.Chart - }{ - { - name: "no meta/configs", - args: args{ - meta: nil, - clusterconfig: k0sv1beta1.ClusterConfig{ - Spec: &k0sv1beta1.ClusterSpec{ - Extensions: &k0sv1beta1.ClusterExtensions{ - Helm: &k0sv1beta1.HelmExtensions{}, - }, - }, - }, - combinedConfigs: &k0sv1beta1.HelmExtensions{}, - }, - want: []k0sv1beta1.Chart{}, - }, - { - name: "add new chart, change chart values, change chart versions, remove old chart", - args: args{ - meta: &ectypes.ReleaseMetadata{ - Protected: map[string][]string{ - "changethis": {"abc"}, - }, - }, - clusterconfig: k0sv1beta1.ClusterConfig{ - Spec: &k0sv1beta1.ClusterSpec{ - Extensions: &k0sv1beta1.ClusterExtensions{ - Helm: &k0sv1beta1.HelmExtensions{ - Charts: []k0sv1beta1.Chart{ - { - Name: "removethis", - }, - { - Name: "changethis", - Values: "abc: xyz", - }, - { - Name: "newversion", - Version: "1.0.0", - }, - { - Name: "change2", - Values: "", - Version: "1.0.0", - }, - { - Name: "change3", - Values: "", - Version: "1.0.0", - }, - }, - }, - }, - }, - }, - combinedConfigs: &k0sv1beta1.HelmExtensions{ - Charts: []k0sv1beta1.Chart{ - { - Name: "addthis", - Values: "addedval: xyz", - }, - { - Name: "changethis", - Values: "key2: val2", - }, - { - Name: "newversion", - Version: "2.0.0", - }, - { - Name: "change2", - Values: "abc: 2", - Version: "1.0.2", - }, - { - Name: "change3", - Version: "1.0.3", - Values: "abc: 3", - }, - }, - }, - }, - want: []k0sv1beta1.Chart{ - { - Name: "addthis", - Values: "addedval: xyz", - }, - { - Name: "changethis", - Values: "abc: xyz\nkey2: val2\n", - }, - { - Name: "newversion", - Version: "2.0.0", - }, - { - Name: "change2", - Values: "abc: 2", - Version: "1.0.2", - }, - { - Name: "change3", - Version: "1.0.3", - Values: "abc: 3", - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - req := require.New(t) - got, err := generateDesiredCharts(tt.args.meta, tt.args.clusterconfig, tt.args.combinedConfigs) - req.NoError(err) - req.ElementsMatch(tt.want, got) - }) - } -} - func Test_detectChartDrift2(t *testing.T) { tests := []struct { name string @@ -1303,3 +1077,85 @@ func Test_applyUserProvidedAddonOverrides(t *testing.T) { }) } } + +func Test_updateInfraChartsFromInstall(t *testing.T) { + type args struct { + in *v1beta1.Installation + charts []k0sv1beta1.Chart + } + tests := []struct { + name string + args args + want []k0sv1beta1.Chart + }{ + { + name: "other chart", + args: args{ + in: &v1beta1.Installation{ + Spec: v1beta1.InstallationSpec{ + ClusterID: "abc", + }, + }, + charts: []k0sv1beta1.Chart{ + { + Name: "test", + Values: "abc: xyz", + }, + }, + }, + want: []k0sv1beta1.Chart{ + { + Name: "test", + Values: "abc: xyz", + }, + }, + }, + { + name: "admin console and operator", + args: args{ + in: &v1beta1.Installation{ + Spec: v1beta1.InstallationSpec{ + ClusterID: "testid", + BinaryName: "testbin", + AirGap: true, + }, + }, + charts: []k0sv1beta1.Chart{ + { + Name: "test", + Values: "abc: xyz", + }, + { + Name: "admin-console", + Values: "abc: xyz", + }, + { + Name: "embedded-cluster-operator", + Values: "this: that", + }, + }, + }, + want: []k0sv1beta1.Chart{ + { + Name: "test", + Values: "abc: xyz", + }, + { + Name: "admin-console", + Values: "abc: xyz\nembeddedClusterID: testid\nisAirgap: \"true\"\n", + }, + { + Name: "embedded-cluster-operator", + Values: "embeddedBinaryName: testbin\nembeddedClusterID: testid\nthis: that\n", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := require.New(t) + got := updateInfraChartsFromInstall(context.TODO(), tt.args.in, tt.args.charts) + req.ElementsMatch(tt.want, got) + }) + } +} diff --git a/controllers/installation_controller.go b/controllers/installation_controller.go index 48346d9f1..67cbb0db6 100644 --- a/controllers/installation_controller.go +++ b/controllers/installation_controller.go @@ -630,7 +630,7 @@ func (r *InstallationReconciler) ReconcileHelmCharts(ctx context.Context, in *v1 return nil } - combinedConfigs := mergeHelmConfigs(meta, in) + combinedConfigs := mergeHelmConfigs(ctx, meta, in) // fetch the current clusterConfig var clusterConfig k0sv1beta1.ClusterConfig @@ -638,12 +638,6 @@ func (r *InstallationReconciler) ReconcileHelmCharts(ctx context.Context, in *v1 return fmt.Errorf("failed to get cluster config: %w", err) } - finalChartList, err := generateDesiredCharts(meta, clusterConfig, combinedConfigs) - if err != nil { - return err - } - combinedConfigs.Charts = finalChartList - if in.Spec.AirGap { // if in airgap mode then all charts are already on the node's disk. we just need to // make sure that the helm charts are pointing to the right location on disk and that diff --git a/controllers/installation_controller_test.go b/controllers/installation_controller_test.go index 3bacb6519..1cf6985de 100644 --- a/controllers/installation_controller_test.go +++ b/controllers/installation_controller_test.go @@ -410,10 +410,13 @@ func TestInstallationReconciler_ReconcileHelmCharts(t *testing.T) { }, }, { - name: "k8s install completed, good version, overridden values, both types of charts, no drift", + name: "k8s install completed, good version, admin console and operator values, both types of charts, no drift", in: v1beta1.Installation{ Status: v1beta1.InstallationStatus{State: v1beta1.InstallationStateKubernetesInstalled}, Spec: v1beta1.InstallationSpec{ + ClusterID: "test cluster ID", + BinaryName: "test-binary-name", + AirGap: false, Config: &v1beta1.ConfigSpec{ Version: "goodver", Extensions: v1beta1.Extensions{ @@ -437,30 +440,49 @@ func TestInstallationReconciler_ReconcileHelmCharts(t *testing.T) { Configs: k0sv1beta1.HelmExtensions{ Charts: []k0sv1beta1.Chart{ { - Name: "metachart", + Name: "admin-console", Version: "1", Values: ` abc: xyz -password: overridden`, +password: frommeta`, + }, + { + Name: "embedded-cluster-operator", + Version: "1", + Values: ` +abc: xyz +password: frommeta`, }, }, }, - Protected: map[string][]string{ - "metachart": {"password"}, - }, }, fields: fields{ State: []runtime.Object{ &k0shelmv1beta1.Chart{ ObjectMeta: metav1.ObjectMeta{ - Name: "metachart", + Name: "admin-console", }, Spec: k0shelmv1beta1.ChartSpec{ - ReleaseName: "metachart", + ReleaseName: "admin-console", Values: `abc: xyz -password: original`, +embeddedClusterID: test cluster ID +isAirgap: "false" +password: frommeta`, + }, + Status: k0shelmv1beta1.ChartStatus{Version: "1", ValuesHash: "84bc4f42c99204aeafa5349f6b0852a14a169db54082566ccd679ddbe49e27bb"}, + }, + &k0shelmv1beta1.Chart{ + ObjectMeta: metav1.ObjectMeta{ + Name: "embedded-cluster-operator", }, - Status: k0shelmv1beta1.ChartStatus{Version: "1", ValuesHash: "eec6dc8e36073ed2211154bca2d54cdc01acba8f512d46c095c3d7a1ede4b0d6"}, + Spec: k0shelmv1beta1.ChartSpec{ + ReleaseName: "embedded-cluster-operator", + Values: `abc: xyz +embeddedBinaryName: test-binary-name +embeddedClusterID: test cluster ID +password: frommeta`, + }, + Status: k0shelmv1beta1.ChartStatus{Version: "1", ValuesHash: "2b3f4301ee3da37c75b573e12fc8e0cb0dc81ec1fbf5a1084b9adc198f06bbb0"}, }, &k0shelmv1beta1.Chart{ ObjectMeta: metav1.ObjectMeta{ @@ -479,11 +501,22 @@ password: original`, Helm: &k0sv1beta1.HelmExtensions{ Charts: []k0sv1beta1.Chart{ { - Name: "metachart", + Name: "admin-console", Version: "1", Values: ` abc: xyz -password: original`, +embeddedClusterID: test cluster ID +isAirgap: "false" +password: frommeta`, + }, + { + Name: "embedded-cluster-operator", + Version: "1", + Values: ` +abc: xyz +embeddedBinaryName: test-binary-name +embeddedClusterID: test cluster ID +password: frommeta`, }, { Name: "extchart", @@ -533,9 +566,6 @@ password: overridden`, }, }, }, - Protected: map[string][]string{ - "metachart": {"password"}, - }, }, fields: fields{ State: []runtime.Object{