Load chart and provide access in both v2 and v3 structs. #1362

absoludity · 2019-12-11T05:05:32Z

Description of the change

In #1309 (comment) , related to the helm3 work, @latiif mentioned he was thinking about a function to convert between v2 and v3 chart structures. This PR is an alternative that relies on the respective v2/v3 loaders to ensure we always have access to the chart in both formats when working with Kubeapps' pkg/chart.

cc @SimonAlling

Benefits

I think this should be safer since we only require read-access to the chart structs (? I assume), so there should be no consistency issues, and we're relying on the actual helm loaders rather than our own conversion function.

It should still provide the benefits outlined by @latiif (re-use of existing code for repo management and syncing).

See what you think :)

Possible drawbacks

Applicable issues

Ref #1056

Additional information

absoludity

There were a couple of other drive-by improvements which I included, see inline comments.

absoludity · 2019-12-11T05:08:31Z

pkg/chart/chart.go

-// Chart struct contains the clients required to retrieve charts info
-type Chart struct {
+// ChartClient struct contains the clients required to retrieve charts info
+type ChartClient struct {


I think I've mentioned this before, but I found it confusing that we had a struct called Chart which wasn't a chart, but a struct containing clients for getting chart details. Hence the rename.

That's way less ambiguous

absoludity · 2019-12-11T05:10:59Z

pkg/chart/chart.go

 	kubeClient    kubernetes.Interface
 	appRepoClient appRepoClientSet.Interface
-	load          LoadChart


AFAICT, the LoadChart function was only injected into the struct so that tests could use a fake? At least, that was the only place a different LoadChart was used... the loader was hard-coded in the main.go. I've instead updated the tests so that they don't need a fake, but instead load a real chart from test_data (I took the current bitnami/nginx).

I initially packaged the nginx chart using both helm2 and 3, and although you end up with a different tarball, tests pass with both, so I've just left the helm2 packaged one.

Yes, it was only for testing purposes. What I don't like about using real data is that you cannot modify the input (so you are only testing the happy path). Since here we are only testing the happy path anyway I am okay with the change.

It is possible to both use real data and modify the input - I've just not done so here because it wasn't necessary for my simple test. But if we want to in the future, we can extend this with something like the helm tests for the archive loader, in https://github.com/helm/helm/blob/master/pkg/chart/loader/archive_test.go where you generate the archive with the setup for your test.

SimonAlling · 2019-12-11T07:41:44Z

pkg/chart/fake/chart.go

-		Values: &chart.Config{
-			Raw: details.Values,
+func (f *FakeChart) GetChart(details *chartUtils.Details, netClient chartUtils.HTTPClient) (*chartUtils.ChartMultiVersion, error) {
+	return &chartUtils.ChartMultiVersion{


Should there be no V3 in this ChartMultiVersion?

Currently the FakeChart is only used in a test which is testing the HelmV2 charts, so I didn't add dead code. We can add a V3 if/when the FakeChart[Client] is used with HelmV3 charts.

SimonAlling · 2019-12-11T07:53:16Z

That's a great job you've done there, @absoludity!

SimonAlling · 2019-12-11T07:59:03Z

pkg/chart/chart.go

+	chartV2, err := helm2loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	chartV3, err := helm3loader.LoadArchive(bytes.NewReader(data))


I'd probably do something like lol := bytes.NewReader(data) and then use lol twice (assuming it typechecks).

It does look like it should be DRYer, but bytes.NewReader() returns a Reader interface, which doesn't have a seek. So it can only be read once. I'd have to convert it to a SectionReader (so I can call Seek(0) after the first LoadArchive), but it ends up more complicated than this. Try it out (and let me know if you find a better option :)

Hehe, I suspected there could be some black magic like that going on. I'm absolutely fine with the current implementation. 😄

andresmgot

Thanks for the PR! I have a comment I would like to get your thoughts about:

andresmgot · 2019-12-11T09:47:07Z

pkg/chart/chart.go

 	kubeClient    kubernetes.Interface
 	appRepoClient appRepoClientSet.Interface
-	load          LoadChart


Yes, it was only for testing purposes. What I don't like about using real data is that you cannot modify the input (so you are only testing the happy path). Since here we are only testing the happy path anyway I am okay with the change.

andresmgot · 2019-12-11T10:14:21Z

pkg/chart/chart.go

+	chartV2, err := helm2loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	chartV3, err := helm3loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	return &ChartMultiVersion{V2: chartV2, V3: chartV3}, nil


I would assume this will bite us in the future. There may be cases in the future that a chart is only compatible with the v3 format. Trying to load as a v2 will likely fail. To avoid returning at that point, we can store the errors in the ChartMultiVersion struct so the caller can decide if the chart version that was expecting (if any) failed.

Also, another thing is that the naming can be a bit confusing (even though I think there is not much we can do). The chart APIVersion used in Helm v2 is v1 while the new version introduced in Helm 3 is v2. Because of that, a chartV2 may mean a chart for Helm v2 or a chart with an APIVersion v2 (used only by Helm 3).

Yeah, we'll definitely need to be able to handle Helm 3-only charts. Would it suffice to have V2 be nil in that case, in the (apparent) absence of type constructors like Maybe and Either?

Regarding the naming: Yes, that is a problem. One solution could be to use names like chartHelm2 instead of chartv2. Another solution, obviously, is to keep the current naming but with v1 and v2 instead of v2 and v3.

I would assume this will bite us in the future. There may be cases in the future that a chart is only compatible with the v3 format. Trying to load as a v2 will likely fail. To avoid returning at that point, we can store the errors in the ChartMultiVersion struct so the caller can decide if the chart version that was expecting (if any) failed.

Yeah, I'm assuming this is transition-only code. Right now they load for both. Later, if/when backwards incompatible changes are added to the chart format, then helm will be at a point where it only supports the new version, so we can update to do the same - I assume?

Also, another thing is that the naming can be a bit confusing (even though I think there is not much we can do). The chart APIVersion used in Helm v2 is v1 while the new version introduced in Helm 3 is v2. Because of that, a chartV2 may mean a chart for Helm v2 or a chart with an APIVersion v2 (used only by Helm 3).

Ah - I assumed in this test in helm https://github.com/helm/helm/blob/master/pkg/chart/loader/load_test.go#L101 that the V1 was an older format, but from what you're saying it's the format used with helm v2? What do you recommend naming-wise? I could use either V1 and V2, or use HelmV2 and HelmV3 to be explicit? Let me know.

Yeah, we'll definitely need to be able to handle Helm 3-only charts.

Yep, but that's in the future, right? (I know there are currently different hooks supported, but I didn't think there was a different format right now, as in a chart which wouldn't load in helm2 or the other way around).

Would it suffice to have V2 be nil in that case, in the (apparent) absence of type constructors like Maybe and Either?

Yes, in the future, it'll be no problem for V2 to be nil if it doesn't load (once we've also transition the repo sync code etc. to use the v3 one instead). But I'd think at that point we'd just be removing it.

Regarding the naming: Yes, that is a problem. One solution could be to use names like chartHelm2 instead of chartv2. Another solution, obviously, is to keep the current naming but with v1 and v2 instead of v2 and v3.

Yep, so what we're referring to is the struct for a chart in helm2 or the struct for a chart in helm3, so I've updated to use helm2chart and helm3chart respectively. See what you think.

Yep, but that's in the future, right?

Shouldn't we support v2 of the chart API (i.e. Helm 3) as soon as we claim to have "Helm 3 support"? Such charts are not supported by Helm 2 AFAIK.

I would still do that now. Mostly because this will hit us for sure (and better for us to fix it in advance) and also because we may not notice and users could find it instead. It's also an easy fix.

Regarding the naming, it's less important. To be explicit we can say chartHelm2 and chartHelm3 but up to you if you want to leave it as it is. Note that chartHelm3 (at least from what I can see in the code`) has support for charts v1 and v2.

Yep, but that's in the future, right?

Shouldn't we support v2 of the chart API (i.e. Helm 3) as soon as we claim to have "Helm 3 support"? Such charts are not supported by Helm 2 AFAIK.

Yes, we definitely need to support the v2 charts as soon as we claim to have "Helm3 support", and you're right: if I update the apiVersion of my test nginx chart to v2 I can trigger an error during my TestGetChart test, because the helm2 (v1) loader refuses to load a chart with apiVersion: v2. (I had previously only packaged the chart with helm3 without updating the apiVersion in the Chart.yaml). I've updated and added tests showing this (more below)

But I don't think we can support v2 charts in one hit here, we need to (a) unblock the helm3 work, and (b) plan steps towards supporting both v1 and v2 charts. There are currently no v2 charts in stable, incubator or bitnami (but yes, it's only a matter of time).

I would still do that now. Mostly because this will hit us for sure (and better for us to fix it in advance) and also because we may not notice and users could find it instead. It's also an easy fix.

I've updated to handle the case (see the extra tests). Rather than storing the error, I've added a flag requireV1Support to GetChart so that the caller can choose to require and handle errors (ie. tiller proxy), or not require v1 support (and not have an error to handle - this will be what the kubeops will use eventually, once we've removed the dependence on the v1 chart for other operations in a later branch).

This should unblock the helm3 work for now where v1 support is not required (though can still be used so that existing versions of proxy.ResolveManifest etc. functions can used until their helm3 versions are created bit-by-bit). Note: it's not clear to me we've also lost the original intention of @latiif's question: I don't see how we can the chart management/syncing code is relevant here, but assume you meant the existing code in the proxy library? Let me know.

Regarding the naming, it's less important. To be explicit we can say chartHelm2 and chartHelm3 but up to you if you want to leave it as it is. Note that chartHelm3 (at least from what I can see in the code`) has support for charts v1 and v2.

Yep, I'd already updated to helm2chart and helm3chart respectively.

Thanks!

absoludity

Thanks for the input people (comments inline). @latiif does this fix/unblock you though? I think it does based on what you said, but want to be sure before I land once reviewed.

absoludity · 2019-12-11T11:09:14Z

pkg/chart/chart.go

 	kubeClient    kubernetes.Interface
 	appRepoClient appRepoClientSet.Interface
-	load          LoadChart


It is possible to both use real data and modify the input - I've just not done so here because it wasn't necessary for my simple test. But if we want to in the future, we can extend this with something like the helm tests for the archive loader, in https://github.com/helm/helm/blob/master/pkg/chart/loader/archive_test.go where you generate the archive with the setup for your test.

absoludity · 2019-12-11T11:15:16Z

pkg/chart/chart.go

+	chartV2, err := helm2loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	chartV3, err := helm3loader.LoadArchive(bytes.NewReader(data))


It does look like it should be DRYer, but bytes.NewReader() returns a Reader interface, which doesn't have a seek. So it can only be read once. I'd have to convert it to a SectionReader (so I can call Seek(0) after the first LoadArchive), but it ends up more complicated than this. Try it out (and let me know if you find a better option :)

absoludity · 2019-12-11T11:19:50Z

pkg/chart/chart.go

+	chartV2, err := helm2loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	chartV3, err := helm3loader.LoadArchive(bytes.NewReader(data))
+	if err != nil {
+		return nil, err
+	}
+	return &ChartMultiVersion{V2: chartV2, V3: chartV3}, nil


I would assume this will bite us in the future. There may be cases in the future that a chart is only compatible with the v3 format. Trying to load as a v2 will likely fail. To avoid returning at that point, we can store the errors in the ChartMultiVersion struct so the caller can decide if the chart version that was expecting (if any) failed.

Yeah, I'm assuming this is transition-only code. Right now they load for both. Later, if/when backwards incompatible changes are added to the chart format, then helm will be at a point where it only supports the new version, so we can update to do the same - I assume?

Also, another thing is that the naming can be a bit confusing (even though I think there is not much we can do). The chart APIVersion used in Helm v2 is v1 while the new version introduced in Helm 3 is v2. Because of that, a chartV2 may mean a chart for Helm v2 or a chart with an APIVersion v2 (used only by Helm 3).

Ah - I assumed in this test in helm https://github.com/helm/helm/blob/master/pkg/chart/loader/load_test.go#L101 that the V1 was an older format, but from what you're saying it's the format used with helm v2? What do you recommend naming-wise? I could use either V1 and V2, or use HelmV2 and HelmV3 to be explicit? Let me know.

absoludity · 2019-12-11T11:21:02Z

pkg/chart/fake/chart.go

-		Values: &chart.Config{
-			Raw: details.Values,
+func (f *FakeChart) GetChart(details *chartUtils.Details, netClient chartUtils.HTTPClient) (*chartUtils.ChartMultiVersion, error) {
+	return &chartUtils.ChartMultiVersion{


Currently the FakeChart is only used in a test which is testing the HelmV2 charts, so I didn't add dead code. We can add a V3 if/when the FakeChart[Client] is used with HelmV3 charts.

SimonAlling · 2019-12-11T11:30:29Z

@latiif does this fix/unblock you though?

Should. But hard to say for sure before we have tried building on top of your code, which we'll do ASAP.

andresmgot

Giving my +1. Thanks!

Load chart and provide access in both v2 and v3 structs.

755257d

absoludity requested a review from andresmgot December 11, 2019 05:05

absoludity self-assigned this Dec 11, 2019

Merge branch 'master' into chart-v2-v3

fc975bb

absoludity commented Dec 11, 2019

View reviewed changes

absoludity mentioned this pull request Dec 11, 2019

Add Helm 3 support #1309

Closed

absoludity force-pushed the chart-v2-v3 branch from 0a46815 to fc975bb Compare December 11, 2019 05:27

SimonAlling reviewed Dec 11, 2019

View reviewed changes

andresmgot reviewed Dec 11, 2019

View reviewed changes

absoludity commented Dec 11, 2019

View reviewed changes

absoludity added 3 commits December 11, 2019 13:01

Switch to clearer naming

62746f9

Enable requiring v1 chart support or not (for error handling).

5dd2caa

Merge branch 'master' into chart-v2-v3

287d876

andresmgot approved these changes Dec 12, 2019

View reviewed changes

Merge branch 'master' into chart-v2-v3

22164c7

absoludity merged commit b3e0d07 into vmware-tanzu:master Dec 13, 2019

absoludity deleted the chart-v2-v3 branch December 13, 2019 00:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load chart and provide access in both v2 and v3 structs. #1362

Load chart and provide access in both v2 and v3 structs. #1362

absoludity commented Dec 11, 2019

absoludity left a comment

absoludity Dec 11, 2019

latiif Dec 11, 2019

absoludity Dec 11, 2019

andresmgot Dec 11, 2019

absoludity Dec 11, 2019

SimonAlling Dec 11, 2019

absoludity Dec 11, 2019

SimonAlling commented Dec 11, 2019

SimonAlling Dec 11, 2019

absoludity Dec 11, 2019

SimonAlling Dec 11, 2019

andresmgot left a comment

andresmgot Dec 11, 2019

andresmgot Dec 11, 2019 •

edited

Loading

SimonAlling Dec 11, 2019

SimonAlling Dec 11, 2019

absoludity Dec 11, 2019

absoludity Dec 11, 2019

SimonAlling Dec 11, 2019

andresmgot Dec 11, 2019 •

edited

Loading

absoludity Dec 12, 2019 •

edited

Loading

absoludity left a comment

absoludity Dec 11, 2019

absoludity Dec 11, 2019

absoludity Dec 11, 2019

absoludity Dec 11, 2019

SimonAlling commented Dec 11, 2019

andresmgot left a comment

Load chart and provide access in both v2 and v3 structs. #1362

Load chart and provide access in both v2 and v3 structs. #1362

Conversation

absoludity commented Dec 11, 2019

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

absoludity 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

SimonAlling commented Dec 11, 2019

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

andresmgot Dec 11, 2019 • 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

andresmgot Dec 11, 2019 • edited Loading

Choose a reason for hiding this comment

absoludity Dec 12, 2019 • edited Loading

Choose a reason for hiding this comment

absoludity 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

SimonAlling commented Dec 11, 2019

andresmgot left a comment

Choose a reason for hiding this comment

andresmgot Dec 11, 2019 •

edited

Loading

andresmgot Dec 11, 2019 •

edited

Loading

absoludity Dec 12, 2019 •

edited

Loading