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 3 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.Helm2Chart, 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"
helm3chart "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"
helm2chart "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 {
Helm2Chart *helm2chart.Chart
Helm3Chart *helm3chart.Chart
}

// LoadHelm2Chart should return a helm2 Chart struct from an IOReader
type LoadHelm2Chart func(in io.Reader) (*helm2chart.Chart, error)

// LoadHelm3Chart returns a helm3 Chart struct from an IOReader
type LoadHelm3Chart func(in io.Reader) (*helm3chart.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))
helm2Chart, err := helm2loader.LoadArchive(bytes.NewReader(data))
if err != nil {
return nil, err
}
helm3Chart, err := helm3loader.LoadArchive(bytes.NewReader(data))
if err != nil {
return nil, err
}
return &ChartMultiVersion{Helm2Chart: helm2Chart, Helm3Chart: helm3Chart}, nil
}

// 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.Helm2Chart.GetMetadata().GetName(), "nginx"; got != want {
t.Errorf("got: %q, want: %q", got, want)
}
if ch.Helm3Chart == nil {
t.Errorf("got: nil, want: non-nil")
} else if got, want := ch.Helm3Chart.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.

Helm2Chart: &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.