From 5257fcc792d5c8a15aa99a1332686b049c882542 Mon Sep 17 00:00:00 2001 From: Andres Martinez Gotor Date: Thu, 30 May 2019 10:35:18 -0700 Subject: [PATCH] Add support for SVG icons (#631) --- cmd/chart-repo/utils.go | 33 ++++++++++++++++++++++++--------- cmd/chart-repo/utils_test.go | 26 +++++++++++++++++++++++++- cmd/chartsvc/handler.go | 8 +++++++- cmd/chartsvc/handler_test.go | 16 ++++++++++++---- cmd/chartsvc/main.go | 2 ++ cmd/chartsvc/main_test.go | 2 +- cmd/chartsvc/models/chart.go | 23 ++++++++++++----------- 7 files changed, 83 insertions(+), 27 deletions(-) diff --git a/cmd/chart-repo/utils.go b/cmd/chart-repo/utils.go index 969eae11f..1aa473549 100644 --- a/cmd/chart-repo/utils.go +++ b/cmd/chart-repo/utils.go @@ -310,20 +310,35 @@ func fetchAndImportIcon(dbSession datastore.Session, c chart) error { return fmt.Errorf("%d %s", res.StatusCode, c.Icon) } - orig, err := imaging.Decode(res.Body) - if err != nil { - return err - } + b := []byte{} + contentType := "" + if strings.Contains(res.Header.Get("Content-Type"), "image/svg") { + // if the icon is a SVG file simply read it + b, err = ioutil.ReadAll(res.Body) + if err != nil { + return err + } + contentType = res.Header.Get("Content-Type") + } else { + // if the icon is in any other format try to convert it to PNG + orig, err := imaging.Decode(res.Body) + if err != nil { + log.WithFields(log.Fields{"name": c.Name}).WithError(err).Error("failed to decode icon") + return err + } - // TODO: make this configurable? - icon := imaging.Fit(orig, 160, 160, imaging.Lanczos) + // TODO: make this configurable? + icon := imaging.Fit(orig, 160, 160, imaging.Lanczos) - var b bytes.Buffer - imaging.Encode(&b, icon, imaging.PNG) + var buf bytes.Buffer + imaging.Encode(&buf, icon, imaging.PNG) + b = buf.Bytes() + contentType = "image/png" + } db, closer := dbSession.DB() defer closer() - return db.C(chartCollection).UpdateId(c.ID, bson.M{"$set": bson.M{"raw_icon": b.Bytes()}}) + return db.C(chartCollection).UpdateId(c.ID, bson.M{"$set": bson.M{"raw_icon": b, "icon_content_type": contentType}}) } func fetchAndImportFiles(dbSession datastore.Session, name string, r repo, cv chartVersion) error { diff --git a/cmd/chart-repo/utils_test.go b/cmd/chart-repo/utils_test.go index e70ba4c97..cde6889e9 100644 --- a/cmd/chart-repo/utils_test.go +++ b/cmd/chart-repo/utils_test.go @@ -109,6 +109,16 @@ func (h *goodIconClient) Do(req *http.Request) (*http.Response, error) { return w.Result(), nil } +type svgIconClient struct{} + +func (h *svgIconClient) Do(req *http.Request) (*http.Response, error) { + w := httptest.NewRecorder() + w.Write([]byte("foo")) + res := w.Result() + res.Header.Set("Content-Type", "image/svg") + return res, nil +} + type goodTarballClient struct { c chart skipReadme bool @@ -360,7 +370,21 @@ func Test_fetchAndImportIcon(t *testing.T) { c := charts[0] m := mock.Mock{} dbSession := mockstore.NewMockSession(&m) - m.On("UpdateId", c.ID, bson.M{"$set": bson.M{"raw_icon": iconBytes()}}).Return(nil) + m.On("UpdateId", c.ID, bson.M{"$set": bson.M{"raw_icon": iconBytes(), "icon_content_type": "image/png"}}).Return(nil) + assert.NoErr(t, fetchAndImportIcon(dbSession, c)) + m.AssertExpectations(t) + }) + + t.Run("valid SVG icon", func(t *testing.T) { + netClient = &svgIconClient{} + c := chart{ + ID: "foo", + Icon: "https://foo/bar/logo.svg", + Repo: repo{}, + } + m := mock.Mock{} + dbSession := mockstore.NewMockSession(&m) + m.On("UpdateId", c.ID, bson.M{"$set": bson.M{"raw_icon": []byte("foo"), "icon_content_type": "image/svg"}}).Return(nil) assert.NoErr(t, fetchAndImportIcon(dbSession, c)) m.AssertExpectations(t) }) diff --git a/cmd/chartsvc/handler.go b/cmd/chartsvc/handler.go index 50f9993c2..da7f184d8 100644 --- a/cmd/chartsvc/handler.go +++ b/cmd/chartsvc/handler.go @@ -266,6 +266,12 @@ func getChartIcon(w http.ResponseWriter, req *http.Request, params Params) { return } + if chart.IconContentType != "" { + // Force the Content-Type header because the autogenerated type does not work for + // image/svg+xml. It is detected as plain text + w.Header().Set("Content-Type", chart.IconContentType) + } + w.Write(chart.RawIcon) } @@ -407,7 +413,7 @@ func chartVersionAttributes(cid string, cv models.ChartVersion) models.ChartVers func chartAttributes(c models.Chart) models.Chart { if c.RawIcon != nil { - c.Icon = pathPrefix + "/assets/" + c.ID + "/logo-160x160-fit.png" + c.Icon = pathPrefix + "/assets/" + c.ID + "/logo" } else { // If the icon wasn't processed, it is either not set or invalid c.Icon = "" diff --git a/cmd/chartsvc/handler_test.go b/cmd/chartsvc/handler_test.go index c380ed916..f9af3a371 100644 --- a/cmd/chartsvc/handler_test.go +++ b/cmd/chartsvc/handler_test.go @@ -64,7 +64,7 @@ func Test_chartAttributes(t *testing.T) { ID: "stable/wordpress", }}, {"chart has a icon", models.Chart{ - ID: "repo/mychart", RawIcon: iconBytes(), + ID: "repo/mychart", RawIcon: iconBytes(), IconContentType: "image/svg", }}, } @@ -76,7 +76,8 @@ func Test_chartAttributes(t *testing.T) { if len(tt.chart.RawIcon) == 0 { assert.Equal(t, len(c.Icon), 0, "icon url should be undefined") } else { - assert.Equal(t, c.Icon, pathPrefix+"/assets/"+tt.chart.ID+"/logo-160x160-fit.png", "the icon url should be the same") + assert.Equal(t, c.Icon, pathPrefix+"/assets/"+tt.chart.ID+"/logo", "the icon url should be the same") + assert.Equal(t, c.IconContentType, tt.chart.IconContentType, "the icon content type should be the same") } }) } @@ -551,7 +552,7 @@ func Test_getChartIcon(t *testing.T) { { "chart has icon", nil, - models.Chart{ID: "my-repo/my-chart", RawIcon: iconBytes()}, + models.Chart{ID: "my-repo/my-chart", RawIcon: iconBytes(), IconContentType: "image/png"}, http.StatusOK, }, { @@ -560,6 +561,12 @@ func Test_getChartIcon(t *testing.T) { models.Chart{ID: "my-repo/my-chart"}, http.StatusNotFound, }, + { + "chart has icon with custom type", + nil, + models.Chart{ID: "my-repo/my-chart", RawIcon: iconBytes(), IconContentType: "image/svg"}, + http.StatusOK, + }, } for _, tt := range tests { @@ -576,7 +583,7 @@ func Test_getChartIcon(t *testing.T) { } w := httptest.NewRecorder() - req := httptest.NewRequest("GET", "/assets/"+tt.chart.ID+"/logo-160x160-fit.png", nil) + req := httptest.NewRequest("GET", "/assets/"+tt.chart.ID+"/logo", nil) parts := strings.Split(tt.chart.ID, "/") params := Params{ "repo": parts[0], @@ -589,6 +596,7 @@ func Test_getChartIcon(t *testing.T) { assert.Equal(t, tt.wantCode, w.Code, "http status code should match") if tt.wantCode == http.StatusOK { assert.Equal(t, w.Body.Bytes(), tt.chart.RawIcon, "raw icon data should match") + assert.Equal(t, w.Header().Get("Content-Type"), tt.chart.IconContentType, "icon content type should match") } }) } diff --git a/cmd/chartsvc/main.go b/cmd/chartsvc/main.go index 48db2c25f..06450edc5 100644 --- a/cmd/chartsvc/main.go +++ b/cmd/chartsvc/main.go @@ -54,6 +54,8 @@ func setupRoutes() http.Handler { apiv1.Methods("GET").Path("/charts/{repo}/{chartName}").Handler(WithParams(getChart)) apiv1.Methods("GET").Path("/charts/{repo}/{chartName}/versions").Handler(WithParams(listChartVersions)) apiv1.Methods("GET").Path("/charts/{repo}/{chartName}/versions/{version}").Handler(WithParams(getChartVersion)) + apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/logo").Handler(WithParams(getChartIcon)) + // Maintain the logo-160x160-fit.png endpoint for backward compatibility /assets/{repo}/{chartName}/logo should be used instead apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/logo-160x160-fit.png").Handler(WithParams(getChartIcon)) apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/versions/{version}/README.md").Handler(WithParams(getChartVersionReadme)) apiv1.Methods("GET").Path("/assets/{repo}/{chartName}/versions/{version}/values.yaml").Handler(WithParams(getChartVersionValues)) diff --git a/cmd/chartsvc/main_test.go b/cmd/chartsvc/main_test.go index 69005dca7..c0c1c6afa 100644 --- a/cmd/chartsvc/main_test.go +++ b/cmd/chartsvc/main_test.go @@ -342,7 +342,7 @@ func Test_GetChartIcon(t *testing.T) { }) } - res, err := http.Get(ts.URL + pathPrefix + "/assets/" + tt.chart.ID + "/logo-160x160-fit.png") + res, err := http.Get(ts.URL + pathPrefix + "/assets/" + tt.chart.ID + "/logo") assert.NoError(t, err) defer res.Body.Close() diff --git a/cmd/chartsvc/models/chart.go b/cmd/chartsvc/models/chart.go index c0a2ba6d6..8879908cb 100644 --- a/cmd/chartsvc/models/chart.go +++ b/cmd/chartsvc/models/chart.go @@ -30,17 +30,18 @@ type Repo struct { // Chart is a higher-level representation of a chart package type Chart struct { - ID string `json:"-" bson:"_id"` - Name string `json:"name"` - Repo Repo `json:"repo"` - Description string `json:"description"` - Home string `json:"home"` - Keywords []string `json:"keywords"` - Maintainers []chart.Maintainer `json:"maintainers"` - Sources []string `json:"sources"` - Icon string `json:"icon"` - RawIcon []byte `json:"-" bson:"raw_icon"` - ChartVersions []ChartVersion `json:"-"` + ID string `json:"-" bson:"_id"` + Name string `json:"name"` + Repo Repo `json:"repo"` + Description string `json:"description"` + Home string `json:"home"` + Keywords []string `json:"keywords"` + Maintainers []chart.Maintainer `json:"maintainers"` + Sources []string `json:"sources"` + Icon string `json:"icon"` + RawIcon []byte `json:"-" bson:"raw_icon"` + IconContentType string `json:"-" bson:"icon_content_type,omitempty"` + ChartVersions []ChartVersion `json:"-"` } // ChartVersion is a representation of a specific version of a chart