Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Dec 13, 2019
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
755257d
Load chart and provide access in both v2 and v3 structs.
absoludity Dec 11, 2019
fc975bb
Merge branch 'master' into chart-v2-v3
absoludity Dec 11, 2019
62746f9
Switch to clearer naming
absoludity Dec 11, 2019
5dd2caa
Enable requiring v1 chart support or not (for error handling).
absoludity Dec 12, 2019
287d876
Merge branch 'master' into chart-v2-v3
absoludity Dec 12, 2019
22164c7
Merge branch 'master' into chart-v2-v3
absoludity Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/tiller-proxy/internal/handler/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func getChart(req *http.Request, cu chartUtils.Resolver) (*chartUtils.Details, *
if err != nil {
return nil, nil, err
}
return chartDetails, ch, nil
return chartDetails, ch.V2, nil
}

func returnForbiddenActions(forbiddenActions []auth.Action, w http.ResponseWriter) {
Expand Down
5 changes: 2 additions & 3 deletions cmd/tiller-proxy/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ import (
"github.com/urfave/negroni"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"
helmChartUtil "k8s.io/helm/pkg/chartutil"
"k8s.io/helm/pkg/helm"
"k8s.io/helm/pkg/helm/environment"
"k8s.io/helm/pkg/tlsutil"
Expand Down Expand Up @@ -135,7 +134,7 @@ func main() {
}

proxy = tillerProxy.NewProxy(kubeClient, helmClient, timeout)
chartutils := chartUtils.NewChart(kubeClient, appRepoClient, helmChartUtil.LoadArchive, userAgent())
chartClient := chartUtils.NewChartClient(kubeClient, appRepoClient, userAgent())

r := mux.NewRouter()

Expand All @@ -150,7 +149,7 @@ func main() {
h := handler.TillerProxy{
DisableAuth: disableAuth,
ListLimit: listLimit,
ChartClient: chartutils,
ChartClient: chartClient,
ProxyClient: proxy,
}

Expand Down
57 changes: 38 additions & 19 deletions pkg/chart/chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,12 @@ import (
"github.com/ghodss/yaml"
appRepov1 "github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/apis/apprepository/v1alpha1"
appRepoClientSet "github.com/kubeapps/kubeapps/cmd/apprepository-controller/pkg/client/clientset/versioned"
chartv3 "helm.sh/helm/v3/pkg/chart"
helm3loader "helm.sh/helm/v3/pkg/chart/loader"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/helm/pkg/proto/hapi/chart"
helm2loader "k8s.io/helm/pkg/chartutil"
chartv2 "k8s.io/helm/pkg/proto/hapi/chart"
"k8s.io/helm/pkg/repo"
)

Expand Down Expand Up @@ -78,31 +81,37 @@ type HTTPClient interface {
Do(req *http.Request) (*http.Response, error)
}

// LoadChart should return a Chart struct from an IOReader
type LoadChart func(in io.Reader) (*chart.Chart, error)
type ChartMultiVersion struct {
V2 *chartv2.Chart
V3 *chartv3.Chart
}

// LoadChartV2 should return a V2 Chart struct from an IOReader
type LoadChartV2 func(in io.Reader) (*chartv2.Chart, error)

// LoadChartV3 returns a V3 Chart struct from an IOReader
type LoadChartV3 func(in io.Reader) (*chartv3.Chart, error)

// Resolver for exposed funcs
type Resolver interface {
ParseDetails(data []byte) (*Details, error)
GetChart(details *Details, netClient HTTPClient) (*chart.Chart, error)
GetChart(details *Details, netClient HTTPClient) (*ChartMultiVersion, error)
InitNetClient(details *Details) (HTTPClient, error)
}

// 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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's way less ambiguous

kubeClient kubernetes.Interface
appRepoClient appRepoClientSet.Interface
load LoadChart
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

userAgent string
appRepo *appRepov1.AppRepository
}

// NewChart returns a new Chart
func NewChart(kubeClient kubernetes.Interface, appRepoClient appRepoClientSet.Interface, load LoadChart, userAgent string) *Chart {
return &Chart{
// NewChartClient returns a new ChartClient
func NewChartClient(kubeClient kubernetes.Interface, appRepoClient appRepoClientSet.Interface, userAgent string) *ChartClient {
return &ChartClient{
kubeClient: kubeClient,
appRepoClient: appRepoClient,
load: load,
userAgent: userAgent,
}
}
Expand Down Expand Up @@ -225,7 +234,7 @@ func findChartInRepoIndex(repoIndex *repo.IndexFile, repoURL, chartName, chartVe
}

// fetchChart returns the Chart content given an URL
func fetchChart(netClient *HTTPClient, chartURL string, load LoadChart) (*chart.Chart, error) {
func fetchChart(netClient *HTTPClient, chartURL string) (*ChartMultiVersion, error) {
req, err := getReq(chartURL)
if err != nil {
return nil, err
Expand All @@ -239,11 +248,19 @@ func fetchChart(netClient *HTTPClient, chartURL string, load LoadChart) (*chart.
if err != nil {
return nil, err
}
return load(bytes.NewReader(data))
chartV2, err := helm2loader.LoadArchive(bytes.NewReader(data))
if err != nil {
return nil, err
}
chartV3, err := helm3loader.LoadArchive(bytes.NewReader(data))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return nil, err
}
return &ChartMultiVersion{V2: chartV2, V3: chartV3}, nil
Copy link
Contributor

@andresmgot andresmgot Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@andresmgot andresmgot Dec 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@absoludity absoludity Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}

// ParseDetails return Chart details
func (c *Chart) ParseDetails(data []byte) (*Details, error) {
func (c *ChartClient) ParseDetails(data []byte) (*Details, error) {
details := &Details{}
err := json.Unmarshal(data, details)
if err != nil {
Expand Down Expand Up @@ -278,7 +295,7 @@ func (c *clientWithDefaultHeaders) Do(req *http.Request) (*http.Response, error)

// InitNetClient returns an HTTP client based on the chart details loading a
// custom CA if provided (as a secret)
func (c *Chart) InitNetClient(details *Details) (HTTPClient, error) {
func (c *ChartClient) InitNetClient(details *Details) (HTTPClient, error) {
// Require the SystemCertPool unless the env var is explicitly set.
caCertPool, err := x509.SystemCertPool()
if err != nil {
Expand Down Expand Up @@ -342,8 +359,9 @@ func (c *Chart) InitNetClient(details *Details) (HTTPClient, error) {
}, nil
}

// GetChart retrieves and loads a Chart from a registry
func (c *Chart) GetChart(details *Details, netClient HTTPClient) (*chart.Chart, error) {
// GetChart retrieves and loads a Chart from a registry in both
// v2 and v3 formats.
func (c *ChartClient) GetChart(details *Details, netClient HTTPClient) (*ChartMultiVersion, error) {
repoURL := c.appRepo.Spec.URL
if repoURL == "" {
// FIXME: Make configurable
Expand All @@ -363,9 +381,10 @@ func (c *Chart) GetChart(details *Details, netClient HTTPClient) (*chart.Chart,
}

log.Printf("Downloading %s ...", chartURL)
chartRequested, err := fetchChart(&netClient, chartURL, c.load)
chart, err := fetchChart(&netClient, chartURL)
if err != nil {
return nil, err
}
return chartRequested, nil

return chart, nil
}
44 changes: 28 additions & 16 deletions pkg/chart/chart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"io"
"io/ioutil"
"net/http"
"os"
"testing"
"time"

Expand All @@ -34,10 +35,12 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
fakeK8s "k8s.io/client-go/kubernetes/fake"
"k8s.io/helm/pkg/proto/hapi/chart"
chartv2 "k8s.io/helm/pkg/proto/hapi/chart"
"k8s.io/helm/pkg/repo"
)

const testChartArchive = "./testdata/nginx-helm2pkg-5.1.1.tgz"

func Test_resolveChartURL(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -86,7 +89,7 @@ func TestFindChartInRepoIndex(t *testing.T) {
repoURL := "http://charts.example.com/repo/"
expectedURL := fmt.Sprintf("%s%s", repoURL, chartURL)

chartMeta := chart.Metadata{Name: name, Version: version}
chartMeta := chartv2.Metadata{Name: name, Version: version}
chartVersion := repo.ChartVersion{URLs: []string{chartURL}}
chartVersion.Metadata = &chartMeta
chartVersions := []*repo.ChartVersion{&chartVersion}
Expand Down Expand Up @@ -152,7 +155,7 @@ func TestParseDetails(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ch := Chart{}
ch := ChartClient{}
details, err := ch.ParseDetails([]byte(tc.data))

if tc.err {
Expand All @@ -173,9 +176,9 @@ func TestParseDetails(t *testing.T) {
}
}

// fakeLoadChart implements LoadChart interface.
func fakeLoadChart(in io.Reader) (*chart.Chart, error) {
return &chart.Chart{}, nil
// fakeLoadChartV2 implements LoadChartV2 interface.
func fakeLoadChartV2(in io.Reader) (*chartv2.Chart, error) {
return &chartv2.Chart{}, nil
}

const pem_cert = `
Expand Down Expand Up @@ -373,10 +376,9 @@ func TestInitNetClient(t *testing.T) {
}
appRepoClient := fakeAppRepo.NewSimpleClientset(expectedAppRepo)

chUtils := Chart{
chUtils := ChartClient{
kubeClient: kubeClient,
appRepoClient: appRepoClient,
load: fakeLoadChart,
}

t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -455,7 +457,11 @@ func (f *fakeHTTPClient) Do(h *http.Request) (*http.Response, error) {
for _, chartURL := range f.chartURLs {
if h.URL.String() == chartURL {
// Fake chart response
return &http.Response{StatusCode: 200, Body: ioutil.NopCloser(bytes.NewReader([]byte{}))}, nil
f, err := os.Open(testChartArchive)
if err != nil {
return nil, fmt.Errorf("unable to open test chart archive: %q", testChartArchive)
}
return &http.Response{StatusCode: 200, Body: f}, nil
}
}
// Unexpected path
Expand All @@ -467,7 +473,7 @@ func newHTTPClient(repoURL string, charts []Details, userAgent string) HTTPClien
entries := map[string]repo.ChartVersions{}
// Populate Chart registry with content of the given helmReleases
for _, ch := range charts {
chartMeta := chart.Metadata{Name: ch.ChartName, Version: ch.Version}
chartMeta := chartv2.Metadata{Name: ch.ChartName, Version: ch.Version}
chartURL := fmt.Sprintf("%s%s-%s.tgz", repoURL, ch.ChartName, ch.Version)
chartURLs = append(chartURLs, chartURL)
chartVersion := repo.ChartVersion{Metadata: &chartMeta, URLs: []string{chartURL}}
Expand Down Expand Up @@ -521,14 +527,13 @@ func TestGetChart(t *testing.T) {
},
}

const repoURL = "http://foo.com/"
const repoURL = "http://example.com/"
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
httpClient := newHTTPClient(repoURL, []Details{target}, tc.userAgent)
kubeClient := fakeK8s.NewSimpleClientset()
chUtils := Chart{
chUtils := ChartClient{
kubeClient: kubeClient,
load: fakeLoadChart,
userAgent: tc.userAgent,
appRepo: &appRepov1.AppRepository{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -543,11 +548,18 @@ func TestGetChart(t *testing.T) {
ch, err := chUtils.GetChart(&target, httpClient)

if err != nil {
t.Errorf("requests were %v", httpClient.(*clientWithDefaultHeaders).client.(*fakeHTTPClient).requests[1])
t.Fatalf("Unexpected error: %v", err)
}
// Currently tests return an empty chart object.
if got, want := ch, &(chart.Chart{}); !cmp.Equal(got, want) {
t.Errorf("got: %v, want: %v", got, want)
// Currently tests return an nginx chart from ./testdata
// We need to ensure it got loaded in both version formats.
if got, want := ch.V2.GetMetadata().GetName(), "nginx"; got != want {
t.Errorf("got: %q, want: %q", got, want)
}
if ch.V3 == nil {
t.Errorf("got: nil, want: non-nil")
} else if got, want := ch.V3.Name(), "nginx"; got != want {
t.Errorf("got: %q, want: %q", got, want)
}

requests := getFakeClientRequests(t, httpClient)
Expand Down
16 changes: 9 additions & 7 deletions pkg/chart/fake/chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,15 @@ func (f *FakeChart) ParseDetails(data []byte) (*chartUtils.Details, error) {
return details, err
}

func (f *FakeChart) GetChart(details *chartUtils.Details, netClient chartUtils.HTTPClient) (*chart.Chart, error) {
return &chart.Chart{
Metadata: &chart.Metadata{
Name: details.ChartName,
},
Values: &chart.Config{
Raw: details.Values,
func (f *FakeChart) GetChart(details *chartUtils.Details, netClient chartUtils.HTTPClient) (*chartUtils.ChartMultiVersion, error) {
return &chartUtils.ChartMultiVersion{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there be no V3 in this ChartMultiVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

V2: &chart.Chart{
Metadata: &chart.Metadata{
Name: details.ChartName,
},
Values: &chart.Config{
Raw: details.Values,
},
},
}, nil
}
Expand Down
Binary file added pkg/chart/testdata/nginx-helm2pkg-5.1.1.tgz
View file
Open in desktop
Binary file not shown.