Change for helm v3 client #643

mattfarina · 2019-09-26T19:37:28Z

This PR contains two changes to support helm v3.

First, helm search will let one search the Helm Hub. But, the response has URLs which aren't installable. So, we want to make the monocular URLs installable.

This change makes it so that one can run a command like...

$ helm install https://monocular.example.com/charts/foo/bar

Monocular will redirect to the URL with the tgz file. Handles calls for provenance files as well

Second, Helm search is calling to the url at /api/chartsvc/v1. This exposes the internal architecture of Monocular. We'd prefer to have a public URL. So, this exposes the search at /api/v1 as well. We want to keep both so that we could someday treat public calls differently from the UI calls if needed.

mattfarina · 2019-09-26T19:39:06Z

Note, not sure of the right way to bump the version as this needs to go into a new monocular release for the full package to work.

andresmgot

The code mostly looks good to me. Letting @prydonius do another review just in case.

andresmgot · 2019-09-30T10:08:06Z

cmd/chartsvc/handler.go

+	p := strings.Split(ct, "/")
+
+	// Not enough parts passed in to the path
+	if len(p) < 2 || len(p) > 3 {


To improve readability of the code I would substitute p for a meaningful name. Also an example of what's expected would help

Updated to make it more clear

andresmgot · 2019-09-30T14:09:14Z

Note, not sure of the right way to bump the version as this needs to go into a new monocular release for the full package to work.

It's a new feature, in theory the minor version should be bumped.

prydonius

thanks for working on this @mattfarina - sorry for the late review (just got back from holiday).

prydonius · 2019-10-03T18:18:53Z

cmd/chartsvc/handler.go

@@ -444,3 +445,75 @@ func newChartVersionListResponse(c *models.Chart) apiListResponse {

 	return cvl
 }
+
+func handleRedirect(w http.ResponseWriter, req *http.Request) {
+


nit: empty line

prydonius · 2019-10-03T18:21:16Z

cmd/chartsvc/handler.go

@@ -444,3 +445,75 @@ func newChartVersionListResponse(c *models.Chart) apiListResponse {

 	return cvl
 }
+
+func handleRedirect(w http.ResponseWriter, req *http.Request) {


perhaps this should be named getChartVersionPackage? Looks like most of the handlers in this file start with get or list. Another option is redirectToChartVersionPackage.

I used redirectToChartVersionPackage because it's not getting the version package... it's more getting a redirect to the package.

prydonius · 2019-10-03T18:24:29Z

cmd/chartsvc/handler.go

+
+	// The URL can be in one of two forms:
+	// - /v1/redirect/charts/stable/aerospike
+	// - /v1/redirect/charts/stable/aerospike/v1.2.3


I would split the handlers up and extract the shared logic to other functions. We should try to use the router as much as possible to handle path parameters.

So... here is my problem and maybe you can help with it. Let's say I have the path my-repo/my-chart/0.1.0/. Now we also need to get the provenance file for that. This would be my-repo/my-chart/0.1.0/.prov. Notice the placements of the path separators. Or, what if we have the path my-repo/my-chart/0.1.0 and need to get my-repo/my-chart/0.1.0.prov. How do we separate the chart tarball from the provenance one with mux?

There is some variation to the possible inputs which makes handling some of this in the router difficult. Any pointers on that?

prydonius · 2019-10-03T18:25:25Z

cmd/chartsvc/handler.go

+	ct := strings.TrimPrefix(req.URL.Path, "/v1/redirect/charts/")
+
+	// check if URL for provenance
+	prov := strings.HasSuffix(ct, ".prov")


I think we may also want to consider handling the provenance file in a separate handler

I'm not sure how to handle that with mux (see setupRoutes). PathPrefix is used because the redirect URL is going to have multiple path parts to it. In the router there is not method for filtering based on suffix, that I'm aware of. Do you have a suggestion?

prydonius · 2019-10-03T18:26:30Z

cmd/chartsvc/handler.go

+	chartID := fmt.Sprintf("%s/%s", p[0], p[1])
+
+	if version == "" {
+		if err := db.C(chartCollection).FindId(chartID).One(&chart); err != nil {


Since we're starting to repeat these MongoDB calls, we may want to extract them out to separate functions and call them from here and in getChart (and the one below in getChartVersion)

Can we do this as part of a separate change? This query is used 4 times by 4 different callbacks. If the logic were to be extracted I would suggest doing it uniformly and updating all of them. It's worth being tested separately from this feature addition IMHO. Repeated the same database query is common here as it stands.

prydonius · 2019-10-03T18:28:26Z

cmd/chartsvc/handler.go

+		cv := chart.ChartVersions[0]
+		if len(cv.URLs) > 0 {
+			if prov {
+				http.Redirect(w, req, cv.URLs[0]+".prov", http.StatusTemporaryRedirect)


Just a thought, instead of redirecting, it might make sense to stream the file directly back to the client. I know the intention of this is for public repositories, but this could be useful for instances of Monocular pointing to private repositories that might not be accessible by the client.

I thought about that but if we do that for the Helm Hub it means we are eating a lot more egress cost as we stream all the packages out from the hub. It also means more system resources are required. I was attempting to be cost sensitive with this implementation.

The search URL is going public as part of Helm. The Helm CLI calls it for search. The name of the internal service should not be part of the semi-public API in case we make changes in the future. This change makes both the old and new paths accessible via ingress. The Helm v3 betas are calling the existing path so we do not want to remove it and break their experience. Signed-off-by: Matt Farina <[email protected]>

This will enable helm to be used in a manner like: helm install https://monocular.example.com/charts/foo/bar Monocular will redirect to the URL with the tarball for the chart or the provenance file if the call to monocular ends in .prov Signed-off-by: Matt Farina <[email protected]>

mattfarina requested a review from prydonius September 26, 2019 19:37

helm-bot added the size/L label Sep 26, 2019

karuppiah7890 mentioned this pull request Sep 29, 2019

Helm 3 install command Error: gzip: invalid header helm/helm#6393

Closed

andresmgot approved these changes Sep 30, 2019

View reviewed changes

prydonius reviewed Oct 3, 2019

View reviewed changes

mattfarina added 2 commits November 13, 2019 10:32

mattfarina force-pushed the public-search-api branch from a5faf9a to 6b4a98d Compare November 13, 2019 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change for helm v3 client #643

Change for helm v3 client #643

mattfarina commented Sep 26, 2019

mattfarina commented Sep 26, 2019

andresmgot left a comment

andresmgot Sep 30, 2019

mattfarina Nov 13, 2019

andresmgot commented Sep 30, 2019

prydonius left a comment

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

prydonius Oct 3, 2019

mattfarina Nov 13, 2019

Change for helm v3 client #643

Are you sure you want to change the base?

Change for helm v3 client #643

Conversation

mattfarina commented Sep 26, 2019

mattfarina commented Sep 26, 2019

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 commented Sep 30, 2019

prydonius 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

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