Add Helm 3 GetRelease #1406

SimonAlling · 2019-12-20T15:33:35Z

Description of the change

This PR implements GetRelease using the Helm 3 API.

The Dashboard doesn't understand the release format used by Helm 3, so we implemented a compatibility layer that translates Helm 3 releases to a Helm 2-similar format suitable for the Dashboard in its current form.

Benefits

By and large, it works.
The Dashboard need not be modified.

Possible drawbacks

The original values.yaml is not included in the response to the Dashboard, just a generated one without comments and formatting. The same goes for the actual config (values.yaml with release-specific modifications). We didn't know where to find the original files.
There are no tests yet (but there soon will be, thanks to @latiif).

Applicable issues

Based on discussions in Add Helm 3 support #1309.
The logical next step after Add Helm 3 CreateRelease #1399.
Kubeops doesn't work at all until Add readiness and liveness checks for Kubeops #1405 has been merged.

Additional information

Don't forget to set useHelm3 to true in values.yaml to see the changes.

Created in collaboration with @lindhe and @latiif.

As of this commit, the real values.yaml is NOT included in the response to the Dashboard, just a placeholder one.

lindhe · 2019-12-20T19:13:36Z

cmd/kubeops/internal/handler/dashboardcompat.go

+		Files:     compatibleFiles(h3c.Files),
+		Metadata:  *h3c.Metadata,
+		Templates: compatibleTemplates(h3c.Templates),
+		Values:    compatibleConfig(h3c),


According to comments in the Helm code, "Config is the set of extra Values added to the chart. These values override the default values inside of the chart.".

I guess that means that we should not use compatibleConfig to set Values. So Config should be the subset of Values that we saw somewhere (you know that yaml that was stripped of comments...), but we should also have a compatibleValues() that returns the actual full Values contents (wherever that is to be found).

https://github.com/helm/helm/blob/master/pkg/release/release.go#L29-L31

You're probably right.

It seems like config is the final, modified values. I deployed bitnami/apache with the replicaCount and metric.resources.limits properties manually modified in the GUI, and then compared values.raw with config.raw (both from the JSON response):

$ diff json-values.yaml json-config.yaml 27d26 < 51d49 < 55d52 < 58c55 < replicaCount: 1 --- > replicaCount: 5 86d82 < 90d85 < 94d88 < 98d91 < 164d156 < 170d161 < 179c170 < secrets: --- > secrets: 225,227c216,218 < limits: {} < # cpu: 100m < # memory: 128Mi --- > limits: > cpu: 100m > memory: 128Mi

Note that both values.raw and config.raw preserved all YAML comments.

lindhe · 2019-12-20T19:17:11Z

Could this be the function to use for getting the Values from a Release?

https://github.com/helm/helm/blob/master/pkg/chartutil/values.go#L137

They are not in their original raw YAML form, but instead generated from the internal map[string]interface{} representation.

SimonAlling · 2020-01-02T16:26:16Z

We've now implemented chart.values and config in the JSON response as best we can. We don't know where to find the original values.yaml (with comments etc), so we generate what YAML we can from the internal map[string]interface{} representation instead.

go.mod

SimonAlling · 2020-01-03T08:59:34Z

I've noticed that, when installing a chart, its Raw property contains the expected files, including values.yaml; however, when getting the resulting release, its Chart.Raw property is the empty list – at least as far as I can tell.

absoludity

Thanks Simon. I think the approach here is correct: dashboard expects a json-serialized helm2 release, so you're creating one manually. But it seems this could be simplified by just using the helm2 structs (rather than creating new ones that are compatible) and using the helm2 code (ie. for the yaml serialization for the Raw field).

See below for more details and let me know if I missed something?

absoludity · 2020-01-06T03:11:31Z

cmd/kubeops/internal/handler/dashboardcompat.go

+// generatedYamlHeader is prepended to YAML generated from the internal map[string]interface{} representation.
+const generatedYamlHeader = "# Not original YAML! Generated from parsed representation."
+
+type dashboardCompatibleRelease struct {


Is there an issue using the h2.Release instead of creating a new struct here? It appears to have the same fields (extra ones serialize with omitempty):

https://github.com/kubeapps/kubeapps/blob/master/pkg/agent/agent_test.go#L185

We originally wanted to write a function from h3.Release to h2.Release, but arrived at the conclusion that it's impossible in general. Can't remember exactly why, but I think there were several reasons. One might be that the respective Metadata types are not isomorphic. The Values property in the respective Chart types also seems problematic.

I've, however, made a best-effort attempt at using Helm 2 types instead of custom ones. It seems to work; see d67e136.

Right, here we're just looking at getting data back to the dashboard for presentation, rather than interacting with helm, so I don't think isomorphism is essential as it would be if we were generating this for use with helm itself. Thanks for the update!

absoludity · 2020-01-06T03:13:05Z

cmd/kubeops/internal/handler/dashboardcompat.go

+	Namespace string                    `json:"namespace,omitempty"`
+}
+
+type dashboardCompatibleChart struct {


Same here with https://github.com/helm/helm/blob/release-2.16/pkg/proto/hapi/chart/chart.pb.go#L24 (although I see you're using h3chart.Metadata below - are they the same?)

No, h2chart.Metadata and h3chart.Metadata are different (and not isomorphic).

absoludity · 2020-01-06T03:13:39Z

cmd/kubeops/internal/handler/dashboardcompat.go

+	Values    dashboardCompatibleValues `json:"values,omitempty"`
+}
+
+type dashboardCompatibleValues struct {


...and here https://github.com/helm/helm/blob/release-2.16/pkg/proto/hapi/chart/config.pb.go#L22

(in the helm2 code, the Values and Config appear to be the same thing, which looks reflected in you having the same type).

AFAIK, Values and Config are not the same thing: Values is the original values.yaml for the chart, whereas Config is the effective set of values used for the release – usually Values overridden with any custom values set by the user at install time.

Sorry - yes, I meant that they appear to use the same type in helm2 rather than requiring two different but identical types - so Chart.Values (at https://github.com/helm/helm/blob/release-2.16/pkg/proto/hapi/chart/chart.pb.go#L32) is actually of type *Config (whereas here we had two identical types with dashboardCompatibleValues and dashboardCompatibleConfig I think?)

absoludity · 2020-01-06T03:25:25Z

cmd/kubeops/internal/handler/dashboardcompat.go

+// valuesToYaml serializes to YAML and prepends an informative header.
+// It assumes that the serialization succeeds.
+func valuesToYaml(values map[string]interface{}) string {
+	marshaled, _ := yaml.Marshal(values)


Why do you want to ignore the error rather than handling it here? This could result in hard-to-discover bugs later (ie. getting an empty values without any known reason why, I think?)

Actually, this looks like another place where we could just use the helm2 pkg? See

https://github.com/helm/helm/blob/bbdfe5e7803a12bbdf97e94cd847859890cf4050/pkg/tiller/release_server.go#L157

where Raw is set using https://github.com/helm/helm/blob/release-2.16/pkg/chartutil/values.go#L45

I thought there would never occur any error there, and abandoning that assumption would contaminate the entire file with error and nil everywhere without adding any real value (or so I thought).

There should exist a total function from h3.Release to some Dashboard-compatible type, shouldn't there? (newDashboardCompatibleRelease is my attempt at constructing such a function.) Maybe @andresmgot's suggestion of storing the YAML separately is the best solution; then we wouldn't need to marshal anything at all.

Finally, I don't see how YAML could be used here. It works only on members of Values from Helm 2, but we have a map[string]interface{}.

In the unlikely case that the map[string]interface{} couldn't be marshaled, returning an error here would be helpful to identify the issue. I would either log the error (if you want this function to succeed in any case, even if the values cannot be marshalled) or return it and handle it in an upper level.

There should exist a total function from h3.Release to some Dashboard-compatible type, shouldn't there? (newDashboardCompatibleRelease is my attempt at constructing such a function.) Maybe @andresmgot's suggestion of storing the YAML separately is the best solution; then we wouldn't need to marshal anything at all.

Yes, I agree - since the raw config is not stored in the helm3 model at all, yet is an integral part of the Kubeapps UI (which uses the values.yaml as the default config which users update), Kubeapps needs some way to access the raw user config (comments and all) as it was able to with helm2. I'm just not yet sure what the sanest way to do so is. I'm +1 to land this (once conflicts are resolved) and perhaps Andres or I can work on this aspect separately so as to not slow you down?

Finally, I don't see how YAML could be used here. It works only on members of Values from Helm 2, but we have a map[string]interface{}.

Values in the chartutil pkg is exactly that - map[string]interface{}. Two lines above the definition of func (v Values) YAML() you'll see:

type Values map[string]interface{}

It's a bit confusing because it's a different type to that used in the struct field Chart.Values. But as above, this is moot since we really need the raw data with comments.

I'd suggest one of these options for now, to keep the code simple:

Panic if serialization failed.

Ignore any serialization error (current solution).

The rationale would be the same no matter which one we choose: This won't hit production anyway (because we need to figure out how to store/access values.yaml), and it's good enough™ for us to be able to keep working on Helm 3 support.

why don't you log the error instead? A panic can be disruptive in the whole process and you don't need to return the error and obscure the code if you just log the error.

Sounds reasonable. I'll log it instead.

andresmgot

Thanks for this PR! Apart from the comments of @absoludity I see an important issue: We have no way of restoring the original values used (only the map representation). This means that comments are lost so when comparing these values when upgrading (a feature added in the latest version) the user will see all the comments being deleted.

It seems that information is not being stored by helm so we may want to add that information when creating a release (either as another field of the Secret/ConfigMap) or in a different entity so here we can return that instead of the plain map.

In any case that should not block this PR so lets continue.

andresmgot · 2020-01-07T10:49:37Z

cmd/kubeops/internal/handler/dashboardcompat.go

+// generatedYamlHeader is prepended to YAML generated from the internal map[string]interface{} representation.
+const generatedYamlHeader = "# Not original YAML! Generated from parsed representation."


I would skip this header, then in the dashboard we can try to recover the comments

I think the header should be there for clarity until we can do something better, such as storing the YAML separately, as you suggested.

Yes, the problem is that it's giving to the user internal information about a problem they may not be aware of. It seems like an error. It's easier to understand that the parsed YAML is what it's stored in the cluster (with no comments).

andresmgot

At the end we have a translator package but we need to either do this translation here or in the dashboard so my +1 (with just one comment about the error). I would add a TODO to remove this package once we drop support for Helm v2.

Thanks! I will let @absoludity do a final review here as well.

andresmgot · 2020-01-07T14:43:36Z

cmd/kubeops/internal/handler/dashboardcompat.go

+// valuesToYaml serializes to YAML and prepends an informative header.
+// It assumes that the serialization succeeds.
+func valuesToYaml(values map[string]interface{}) string {
+	marshaled, _ := yaml.Marshal(values)


In the unlikely case that the map[string]interface{} couldn't be marshaled, returning an error here would be helpful to identify the issue. I would either log the error (if you want this function to succeed in any case, even if the values cannot be marshalled) or return it and handle it in an upper level.

absoludity · 2020-01-07T19:45:59Z

cmd/kubeops/internal/handler/dashboardcompat.go

+// valuesToYaml serializes to YAML and prepends an informative header.
+// It assumes that the serialization succeeds.
+func valuesToYaml(values map[string]interface{}) string {
+	marshaled, _ := yaml.Marshal(values)


There should exist a total function from h3.Release to some Dashboard-compatible type, shouldn't there? (newDashboardCompatibleRelease is my attempt at constructing such a function.) Maybe @andresmgot's suggestion of storing the YAML separately is the best solution; then we wouldn't need to marshal anything at all.

Yes, I agree - since the raw config is not stored in the helm3 model at all, yet is an integral part of the Kubeapps UI (which uses the values.yaml as the default config which users update), Kubeapps needs some way to access the raw user config (comments and all) as it was able to with helm2. I'm just not yet sure what the sanest way to do so is. I'm +1 to land this (once conflicts are resolved) and perhaps Andres or I can work on this aspect separately so as to not slow you down?

Finally, I don't see how YAML could be used here. It works only on members of Values from Helm 2, but we have a map[string]interface{}.

Values in the chartutil pkg is exactly that - map[string]interface{}. Two lines above the definition of func (v Values) YAML() you'll see:

type Values map[string]interface{}

It's a bit confusing because it's a different type to that used in the struct field Chart.Values. But as above, this is moot since we really need the raw data with comments.

andresmgot

Thanks for the changes!

Add Helm 3 GetRelease

ae2d738

As of this commit, the real values.yaml is NOT included in the response to the Dashboard, just a placeholder one.

lindhe reviewed Dec 20, 2019

View reviewed changes

latiif mentioned this pull request Jan 2, 2020

[kubeops] Implement framework for testing /kubeops/internal/handler #1408

Merged

SimonAlling added 2 commits January 2, 2020 16:04

Distinguish between Config and Values

4d4b463

Implement Config and Values

26b3f36

They are not in their original raw YAML form, but instead generated from the internal map[string]interface{} representation.

latiif reviewed Jan 2, 2020

View reviewed changes

go.mod Show resolved Hide resolved

SimonAlling mentioned this pull request Jan 3, 2020

Add readiness and liveness checks for Kubeops #1405

Merged

absoludity reviewed Jan 6, 2020

View reviewed changes

andresmgot reviewed Jan 7, 2020

View reviewed changes

SimonAlling added 2 commits January 7, 2020 12:57

Use Helm 2 types instead of custom ones

d67e136

Remove generated YAML header

f5c1d27

andresmgot approved these changes Jan 7, 2020

View reviewed changes

absoludity approved these changes Jan 7, 2020

View reviewed changes

SimonAlling added 3 commits January 8, 2020 08:36

Merge branch 'master' into helm3-get-release

a7ba07f

Log error if YAML serialization fails

770f4f6

Merge branch 'master' into helm3-get-release

5205fb5

andresmgot approved these changes Jan 8, 2020

View reviewed changes

andresmgot merged commit 4b4b14b into vmware-tanzu:master Jan 8, 2020

SimonAlling deleted the helm3-get-release branch January 8, 2020 13:51

SimonAlling mentioned this pull request Jan 8, 2020

Add Helm 3 DeleteRelease #1420

Merged

latiif mentioned this pull request Jan 10, 2020

[kubeops] Add tests for GetRelease #1434

Merged

SimonAlling mentioned this pull request Jan 22, 2020

Add Helm 3 support #1309

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Helm 3 GetRelease #1406

Add Helm 3 GetRelease #1406

SimonAlling commented Dec 20, 2019 •

edited

Loading

lindhe Dec 20, 2019 •

edited

Loading

SimonAlling Dec 20, 2019

SimonAlling Jan 2, 2020

lindhe commented Dec 20, 2019

SimonAlling commented Jan 2, 2020 •

edited

Loading

SimonAlling commented Jan 3, 2020 •

edited

Loading

absoludity left a comment •

edited

Loading

absoludity Jan 6, 2020

SimonAlling Jan 7, 2020

absoludity Jan 7, 2020

absoludity Jan 6, 2020

SimonAlling Jan 7, 2020

absoludity Jan 6, 2020

SimonAlling Jan 7, 2020

absoludity Jan 7, 2020

absoludity Jan 6, 2020

SimonAlling Jan 7, 2020

andresmgot Jan 7, 2020

absoludity Jan 7, 2020

SimonAlling Jan 8, 2020

andresmgot Jan 8, 2020

SimonAlling Jan 8, 2020

andresmgot left a comment

andresmgot Jan 7, 2020

SimonAlling Jan 7, 2020

andresmgot Jan 7, 2020

SimonAlling Jan 7, 2020

andresmgot left a comment

andresmgot Jan 7, 2020

absoludity Jan 7, 2020

andresmgot left a comment

		// generatedYamlHeader is prepended to YAML generated from the internal map[string]interface{} representation.
		const generatedYamlHeader = "# Not original YAML! Generated from parsed representation."

Add Helm 3 GetRelease #1406

Add Helm 3 GetRelease #1406

Conversation

SimonAlling commented Dec 20, 2019 • edited Loading

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

lindhe Dec 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindhe commented Dec 20, 2019

SimonAlling commented Jan 2, 2020 • edited Loading

SimonAlling commented Jan 3, 2020 • edited Loading

absoludity left a comment • edited Loading

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

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

andresmgot 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

andresmgot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot left a comment

Choose a reason for hiding this comment

SimonAlling commented Dec 20, 2019 •

edited

Loading

lindhe Dec 20, 2019 •

edited

Loading

SimonAlling commented Jan 2, 2020 •

edited

Loading

SimonAlling commented Jan 3, 2020 •

edited

Loading

absoludity left a comment •

edited

Loading