Move all k8s resources operations to pkg/apprepo #1518

andresmgot · 2020-02-13T16:55:03Z

Description of the change

Requires #1517

The goal of this PR is to complete the migration to the existing operations over k8s resources to the package pkg/apprepo (the name of the package is to be changed). The advantage of this package is that it allow callers to set a token. If set, it uses that token to do requests, if not it uses the backend serviceaccount.

Now the package pkg/chart uses this package thanks to the implementation of the methods GetAppRepository and GetSecret that use the same approach for the token.

The fake Handler has been moved to its own package so it can be reused.

absoludity · 2020-02-13T23:05:04Z

The advantage of this package is that it allow callers to set a token. If set, it uses that token to do requests, if not it uses the backend serviceaccount.

This is only for the GetNamespaces method, right? The way you've stated it here sounds like it's general functionality for the package (ie. including other methods like CreateAppRepository etc.)

absoludity

I'm keen for any call-sites to be super clear as to whether they're getting user resources (app repos or secrets) using the user token or the service account, so any future reviews or changes can't let something slip in that is incorrectly using the service account token.

For the namespaces this is not important, imo, as we explicitly want to enable exposing just the namespaces to which the user has access. But for these general getters on actual user resources, it's much more important, imo.

absoludity · 2020-02-13T23:15:58Z

cmd/kubeops/internal/handler/handler.go

-				return
+			kubeappsNamespace := os.Getenv("POD_NAMESPACE")
+			if namespace == "" {
+				namespace = defaultNamespace


Why are we defaulting/guessing that the Kubeapps namespace is the kube-system here (metav1.NamespaceSystem)?

We should now be able to fail to start, given that POD_NAMESPACE is set for both tiller-proxy and kubeops? EDIT: Ah, because this is within a function which has no option of returning an error :/ . If WithHandlerConfig requires knowing what the kubeapps namespace is, let's make it a required arg?
EDIT2: And because you've not changed this, it was already like this in pkg/chart from where it was moved. It seems like a bad guess to make, potentially a security issue.

My vote would be to pass it as a required arg (and in the main.go, fail if it's not defined). Let me know if you've got other ideas.

yes, I just moved code around. It doesn't really make sense for this to be kube-system in any case so I am fine moving this to the main.go file

absoludity · 2020-02-13T23:23:05Z

pkg/apprepo/apprepos_handler.go

-	configCopy.BearerToken = token
+	if token != "" {
+		configCopy.BearerToken = token
+	}


This looks dangerous - as though it implicitly returns a service account config if you pass in an empty token? ConfigForToken should only ever return a config for the specified token, whether or not the token is passed with bad information.

absoludity · 2020-02-13T23:27:32Z

pkg/apprepo/apprepos_handler.go

+// GetAppRepository returns an AppRepository resource from a namespace.
+// Optionally set a token to get the AppRepository using a custom serviceaccount
+func (a *AppRepositoriesHandler) GetAppRepository(repoName, repoNamespace, token string) (*v1alpha1.AppRepository, error) {
+	clientset, err := a.clientsetForRequest(token)


Right, this looks like we're depending on clientsetForRequest("") returning the service account clientset. I'm -1 on that, it's too easy to mistakenly end up with a service account client set. I think we should remove that and do something like:

if token != "" { clientset, err := a.clientsetForToken(token) } else { clientset = svcKubeClient }

In fact, even then the optional token on this public method is very implicit at call-sites. We could additionally make this fn private (getAppRepository) and instead expose two very small (single-line) wrappers:

func (a *AppRepositoriesHandler) GetAppRepositoryForUser(repoName, repoNamespace, token string) ...

and

func (a *AppRepositoriesHandler) GetAppRepositoryForServiceAccount(repoName, repoNamespace string) ...

calling the private method.

My plan was to standardize this library to have always this token property to be able to reuse all the methods (even most of them are currently just used with or without token).

Another possibility, to keep having the flexibility that the code have now and being explicit we can add another interface to make sure that the caller is using the desired service account:

handler.AsUser(token).CreateAppRepository(...) handler.AsSVC().GetSecret(...)

absoludity · 2020-02-13T23:30:04Z

pkg/apprepo/apprepos_handler.go

+
+// GetSecret return the a secret from a namespace using a token if given
+func (a *AppRepositoriesHandler) GetSecret(name, namespace, token string) (*corev1.Secret, error) {
+	userClientset, err := a.clientsetForRequest(token)


absoludity · 2020-02-13T23:51:08Z

pkg/chart/chart.go

 	// We grab the specified app repository (for later access to the repo URL, as well as any specified
 	// auth).
-	appRepo, err := c.appRepoClient.KubeappsV1alpha1().AppRepositories(namespace).Get(details.AppRepositoryResourceName, metav1.GetOptions{})
+	appRepo, err := c.appRepoHandler.GetAppRepository(details.AppRepositoryResourceName, c.kubeappsNamespace, "")


So this (and the calls below) would use .GetAppRepositoryForServiceAccount(details.AppRepositoryResourceName, c.kubeappsNamespace)

andresmgot · 2020-02-14T15:40:16Z

Thanks for the review @absoludity. Check 7a839b5, I think we can have both flexibility and explicit token usage as I explain in a inline comment. Let me know what you think about it.

absoludity

Thanks Andres. I'm adding a +1, as this is much nicer with the explicit handlers. I do have a couple of concerns inline. You may be able to resolve them with your current approach using AsUser() and AsSVC, or it may be simpler to just have two explicit methods GetNamespaces and GetNamespacesAsSVC for just the methods which require auth with the SVC account. Hopefully you can find a way with your current approach, as it is nicer :)

absoludity · 2020-02-17T05:19:24Z

cmd/tiller-proxy/main.go

+	kubeappsNamespace := os.Getenv("POD_NAMESPACE")
+	if kubeappsNamespace == "" {
+		log.Fatalf("POD_NAMESPACE should be defined")
+	}


Much better, thanks for moving it out to main.go even though it wasn't a change you'd added.

absoludity · 2020-02-17T05:21:21Z

pkg/apprepo/apprepos_handler.go

+// AuthHandler exposes handler functionality as a user or the current serviceaccount
+type AuthHandler interface {
+	AsUser(token string) handler
+	AsSVC() handler


Excellent, much more explicit while still being easy to use for call-sites. Thanks!

The only down-side I can think of here is that it now means that all the methods on the interface can be used with the service account, rather than just GetNamespaces and GetAppRepository. We could fix that by composing the interface and having AsSVC() return the more limited interface? Not sure. See what you think.

That's what I wanted, if in the future we need to use this Handler with any the svc account or the user account we don't need to modify the library. Since it's quite explicit now, I don't think there is risk for a developer to misuse the method (nor a reviewer to miss it).

absoludity · 2020-02-17T05:40:45Z

pkg/apprepo/apprepos_handler.go

+	a.clientset = clientset
+	return a
+}
+
+func (a *appRepositoriesHandler) AsSVC() handler {
+	a.clientset = a.svcClientset
+	return a


These don't look thread safe? Would it be better to return a copy configured with the correct clientset, rather than updating the internal a.clientset for the call-site?

Or, stepping back, it may even make more sense if the current appRepositoryHandler was something like a handler factory, that only created a handler when you call AsUser or AsSVC. Not sure about needing a factory, but the main thing is that it seems in its current form here to be mixing responsibilities... more below.

that's a good point, let me check the possibility of modifying the interfaces to fix this and you next comment.

absoludity · 2020-02-17T05:43:52Z

pkg/apprepo/apprepos_handler.go

 		config:            *config,
 		kubeappsNamespace: kubeappsNamespace,
 		// See comment in the struct defn above.
 		clientsetForConfig: clientsetForConfig,
-		svcKubeClient:      svcKubeClient,
+		svcClientset:       &combinedClientset{svcAppRepoClient, svcKubeClient},


So a.clientset isn't yet defined (as no one has called AsUser() or AsSVC() yet), so if a call-site now called a.CreateAppRepository(..) they'd get a run-time panic, I think? This is what I meant above about mixing responsibilities. Perhaps what is returned here should not implement the interface, and only the handler returned by AsUser() or AsSVC() should implement the interface. So the object returned here wouldn't be a handler, but enables you to create a handler.

andresmgot · 2020-02-17T12:14:20Z

Merging! let me know if you want to discuss something further

absoludity · 2020-02-17T22:34:00Z

Merging! let me know if you want to discuss something further

LGTM, thanks!

absoludity reviewed Feb 13, 2020

View reviewed changes

Andres Martinez Gotor added 2 commits February 14, 2020 16:14

Move all k8s resources operations to pkg/apprepo

79c3621

Minor review

4d434d6

andresmgot force-pushed the apprepoRefactor branch from 9391ef1 to d6234fd Compare February 14, 2020 15:30

andresmgot changed the base branch from remoteHTTP to master February 14, 2020 15:30

Apply review

7a839b5

andresmgot force-pushed the apprepoRefactor branch from d6234fd to 7a839b5 Compare February 14, 2020 15:39

absoludity approved these changes Feb 17, 2020

View reviewed changes

Review

f73a062

andresmgot merged commit ee150e6 into master Feb 17, 2020

andresmgot deleted the apprepoRefactor branch February 17, 2020 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all k8s resources operations to pkg/apprepo #1518

Move all k8s resources operations to pkg/apprepo #1518

andresmgot commented Feb 13, 2020

absoludity commented Feb 13, 2020

absoludity left a comment

absoludity Feb 13, 2020

andresmgot Feb 14, 2020

absoludity Feb 13, 2020

absoludity Feb 13, 2020

andresmgot Feb 14, 2020

absoludity Feb 13, 2020

absoludity Feb 13, 2020

andresmgot commented Feb 14, 2020 •

edited

Loading

absoludity left a comment

absoludity Feb 17, 2020

absoludity Feb 17, 2020

andresmgot Feb 17, 2020

absoludity Feb 17, 2020

andresmgot Feb 17, 2020

absoludity Feb 17, 2020

andresmgot Feb 17, 2020

andresmgot commented Feb 17, 2020

absoludity commented Feb 17, 2020

Move all k8s resources operations to pkg/apprepo #1518

Move all k8s resources operations to pkg/apprepo #1518

Conversation

andresmgot commented Feb 13, 2020

Description of the change

absoludity commented Feb 13, 2020

absoludity 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

andresmgot commented Feb 14, 2020 • edited Loading

absoludity 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

andresmgot commented Feb 17, 2020

absoludity commented Feb 17, 2020

andresmgot commented Feb 14, 2020 •

edited

Loading