Tiller proxy basic functionality #357

andresmgot · 2018-06-12T10:44:35Z

Closes #351
Closes #352
Closes #353

This PR implements the basic functionality for the Tiller proxy discussed in #337. The proxy currently redirect requests to Tiller to list, create, get, update and delete releases. The commits has been split for an easier review. There is no need to review 3f0d6e7 since the code there has been already reviewed in the helm-crd project. Part of 22d2777 has been extracted from that project as well.

The proxy uses the same packages to serve the API than the chart service: gorilla/mux for the router and urfave/negroni as a middleware.

prydonius

This mostly LGTM, nice work.

Now that this is in this repo, there may be certain helpers we want to share with other services (e.g. chart-repo has helpers to deal with repo URLs and appending tarball URLs, downloading tarballs, etc.)

In addition to that, we should probably start putting packages in pkg directory and leave the cmd directory for main packages that import packages from pkg.

These are globally-scoped things that I don't want to scope-creep into this issue, so I'm okay with merging this without doing all that, but it would be good to tackle this at some point (or ideally at least some of it now before this gets merged).

prydonius · 2018-06-12T23:02:39Z

cmd/tiller-proxy/Dockerfile.dev

@@ -0,0 +1,4 @@
+FROM alpine:3.6


can we not run the binary directly, or with telepresence?

true, I'm just not that used to telepresence :)

prydonius · 2018-06-12T23:10:25Z

cmd/tiller-proxy/main.go

+
+var (
+	settings environment.EnvSettings
+	p        *proxy.Proxy


since this is a global var, can we call it proxy to make it more obvious what it is?

prydonius · 2018-06-12T23:11:57Z

cmd/tiller-proxy/main.go

+
+	config, err := rest.InClusterConfig()
+	if err != nil {
+		panic(err)


we should probably use log.Fatalf or similar instead of panic.

prydonius · 2018-06-12T23:24:30Z

cmd/tiller-proxy/proxy/proxy.go

+
+// GetRelease returns the info of a release
+func (p *Proxy) GetRelease(name, namespace string) (*release.Release, error) {
+	lock(name)


since this is read-only, do we need the lock? I think we only care about e.g. deleting a release whilst it's still being created?

it's not critical but it's something nice to have. We could be returning the wrong version or a missing release.

prydonius · 2018-06-12T23:26:59Z

cmd/tiller-proxy/proxy/proxy_test.go

+	return &chart.Chart{}, nil
+}
+
+func prepareTestProxy(hrs []helmRelease, existingTillerReleases []AppOverview) *Proxy {


I would name this newTestProxy or newFakeProxy - prepare doesn't sound like something that returns a *Proxy to me.

andresmgot · 2018-06-13T08:46:32Z

In addition to that, we should probably start putting packages in pkg directory and leave the cmd directory for main packages that import packages from pkg.

I agree with that, I just followed the existing pattern but it's true. I will move at least the current extra packages to the pkg folder and leave cmd just to the command line related package.

prydonius · 2018-06-13T17:19:33Z

I agree with that, I just followed the existing pattern but it's true. I will move at least the current extra packages to the pkg folder and leave cmd just to the command line related package.

Yeah that's a side effect of when we merged together the different repos, we didn't also move packages to the pkg folder, though we should have at the time. Moving the current extra packages sounds great, thanks!

prydonius

Can we move the packages from pkg/utils/proxy|chart to just pkg/proxy|chart?

Otherwise LGTM

andresmgot · 2018-06-18T10:57:30Z

@prydonius I have updated this PR to include the authorization feature (creating a new PR would be confusing since I needed to reorder the code of this PR). From the last bunch of commits the interesting ones for review are 6d2dc61 which contains all the auth related code and
0be05b7 that contains the wiring code for the REST server. Sorry for the big PR.

prydonius · 2018-06-18T18:37:32Z

cmd/tiller-proxy/README.md

+
+# Enabling authorization
+
+It is possible to enable authorization for helm releases setting the env var `ENABLE_AUTH`. If enabled, the client should have permissions to:


perhaps ENABLE_RBAC is more clear? Also, IMO this should be enabled by default. I also thing this should be a command line flag rather than an env var.

👍 I planned to do that the default when doing kubeapps up but it's true, a flag will be better. Also this will work (in theory) with any authorization system not just RBAC (e.g. ABAC): https://kubernetes.io/docs/reference/access-authn-authz/authorization/#authorization-modules

prydonius · 2018-06-18T18:38:44Z

cmd/tiller-proxy/README.md

@@ -35,3 +29,14 @@ This proxy provides 6 different routes:
 - `GET` `/namespaces/{namespace}/releases/{release}`: Get release info
 - `PUT` `/namespaces/{namespace}/releases/{release}`: Update release info
 - `DELETE` `/namespaces/{namespace}/releases/{release}`: Delete a release
+
+# Enabling authorization


We should add a caveat that we only support bearer token auth and not other methods atm.

prydonius · 2018-06-18T18:50:36Z

cmd/tiller-proxy/handler.go

-// WithParams can be used to wrap handlers to take an extra arg for path params
-type WithParams func(http.ResponseWriter, *http.Request, Params)
+// WithAuth can be used to wrap handlers to take an extra arg for path params
+type WithAuth func(http.ResponseWriter, *http.Request, Params, *auth.UserAuth)


IMO a more clean and modular way to implement this is to write middleware for Negroni and store the auth in the request context. The current approach ties together the auth and params. It's possible that WithParams could also be implemented as a middleware, but that's out of scope.

This is the middleware from Monocular: https://github.com/kubernetes-helm/monocular/blob/master/src/api/middleware/authgate.go, and this is an example of who it can be wrapped in a route: https://github.com/kubernetes-helm/monocular/blob/master/src/api/main.go#L102. The request context will need a key that can be used to read/set the auth data (https://github.com/kubernetes-helm/monocular/blob/master/src/api/models/user.go#L27), we may need a new package to share this.

prydonius · 2018-06-18T20:49:42Z

cmd/tiller-proxy/handler.go

+		return
+	}
+	action := "create"
+	if req.Method == "PUT" && params["releaseName"] != "" {


why not use a different handler for PUT?

it has just a couple of lines different but I guess it's better to have different handlers for readability

prydonius · 2018-06-18T20:54:45Z

cmd/tiller-proxy/handler.go

-	proxy.LogReleaseStatus(rel.Name)
+	status, err := proxy.GetReleaseStatus(rel.Name)
+	if err != nil {
+		log.Printf("Unable to fecth release status of %s: %v", rel.Name, err)


s/fecth/fetch

prydonius · 2018-06-18T20:59:33Z

pkg/auth/auth.go

+	Namespace  string
+}
+
+// NewAuth creates a auth agent


prydonius · 2018-06-18T21:53:44Z

pkg/auth/auth.go

+		if err != nil {
+			return err
+		}
+	default:


I think we can just have the default be err := u.isAllowed(action, resources and remove all other cases apart from upgrade. The SelfSubjectAccessReview will fail for unknown verbs anyway.

prydonius · 2018-06-18T21:56:15Z

pkg/auth/auth_test.go

+	if !strings.Contains(err.Error(), "Unauthorized to create v1/pods in the bar namespace") {
+		t.Errorf("Unexpected error: %v", err)
+	}
+}


any way to test a successful request?

I couldn't find a way of doing so without modify the functional code in a weird way. For being able to return a "success" we need to add to the fake client a SelfSubjectAccessReviews object but since the functional code does a Create (because a Get doesn't make sense in the real scenario) it returns an error "already exists".

prydonius · 2018-06-18T22:05:03Z

@andresmgot added some comments, some of which are hidden as "outdated" (although they aren't really)

andresmgot · 2018-06-19T10:15:20Z

@prydonius reviewed/replied

prydonius

lgtm

prydonius · 2018-06-19T23:07:03Z

cmd/tiller-proxy/handler.go

+func logStatus(name string) {
+	status, err := proxy.GetReleaseStatus(name)
+	if err != nil {
+		log.Printf("Unable to fecth release status of %s: %v", name, err)


s/fecth/fetch

Andres added 2 commits June 12, 2018 12:30

Vendor update

7c493ca

Docs, make tasks and dockerfiles

b225b5f

andresmgot requested review from sameersbn and prydonius June 12, 2018 10:44

Andres added 4 commits June 12, 2018 13:06

Copy chart logic from helm-crd

3f0d6e7

Proxy logic to communicate with tiller

22d2777

Proxy HTTP wrapper

e0bfeac

Main function

cbeb53c

andresmgot force-pushed the tillerProxy branch from 635e0d6 to cbeb53c Compare June 12, 2018 11:07

prydonius reviewed Jun 12, 2018

View reviewed changes

andresmgot force-pushed the tillerProxy branch from f4df818 to f22b7cf Compare June 13, 2018 10:29

prydonius reviewed Jun 15, 2018

View reviewed changes

Andres added 6 commits June 18, 2018 12:44

Minor review

4e02db6

CMD update

0be05b7

Chart utils update

aba1f3b

Update proxy pkg

3a970de

Add auth functionality

6d2dc61

Merge master

f0584d7

andresmgot force-pushed the tillerProxy branch from f22b7cf to f0584d7 Compare June 18, 2018 10:49

prydonius reviewed Jun 18, 2018

View reviewed changes

Review

4c08eb9

prydonius approved these changes Jun 19, 2018

View reviewed changes

Fix typo

ff9fe2a

andresmgot merged commit 2c70d7f into master Jun 21, 2018

prydonius deleted the tillerProxy branch June 21, 2018 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tiller proxy basic functionality #357

Tiller proxy basic functionality #357

andresmgot commented Jun 12, 2018 •

edited

Loading

prydonius left a comment •

edited

Loading

prydonius Jun 12, 2018

andresmgot Jun 13, 2018

prydonius Jun 12, 2018

prydonius Jun 12, 2018

prydonius Jun 12, 2018

andresmgot Jun 13, 2018

prydonius Jun 12, 2018

andresmgot commented Jun 13, 2018

prydonius commented Jun 13, 2018

prydonius left a comment

andresmgot commented Jun 18, 2018 •

edited

Loading

prydonius Jun 18, 2018

andresmgot Jun 19, 2018 •

edited

Loading

prydonius Jun 18, 2018

prydonius Jun 18, 2018

prydonius Jun 18, 2018

andresmgot Jun 19, 2018

prydonius Jun 18, 2018

prydonius Jun 18, 2018

prydonius Jun 18, 2018

prydonius Jun 18, 2018

andresmgot Jun 19, 2018

prydonius commented Jun 18, 2018

andresmgot commented Jun 19, 2018

prydonius left a comment

prydonius Jun 19, 2018


		# Enabling authorization

		It is possible to enable authorization for helm releases setting the env var `ENABLE_AUTH`. If enabled, the client should have permissions to:

Tiller proxy basic functionality #357

Tiller proxy basic functionality #357

Conversation

andresmgot commented Jun 12, 2018 • edited Loading

prydonius left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Jun 13, 2018

prydonius commented Jun 13, 2018

prydonius left a comment

Choose a reason for hiding this comment

andresmgot commented Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

andresmgot Jun 19, 2018 • 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

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

prydonius commented Jun 18, 2018

andresmgot commented Jun 19, 2018

prydonius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresmgot commented Jun 12, 2018 •

edited

Loading

prydonius left a comment •

edited

Loading

andresmgot commented Jun 18, 2018 •

edited

Loading

andresmgot Jun 19, 2018 •

edited

Loading