Utilize user credentials to invoke the new kubeapps-api #2908

antgamdia · 2021-05-30T20:02:13Z

Description of the change

This PR tries to replace the current hardcoded SA with the actual user's credentials, including the proper configuration for the Pinniped scenario.
Currently, it also adds a new chart flag for dev purposes (kubeappsapis.UnsafeUseDemoSA: true) that allow having the same behavior prior to this PR.

Benefits

A consistent auth approach with the rest of the Kubeapps components.

Possible drawbacks

~~No SA for quick testing, but it will be get solved with a flag soon.~~

Applicable issues

fixes [kubeapps-apis] Switch to use user credentials from request #2851

Additional information

WIP, there is a couple of things I wanna re-check and thoughts:

(outdated) As it is a sort of transversal functionality, we should provide a unified auth experience for plugin developers. Can plugins invoke main program functions? Another way round: can the main program share info with the plugins?
Currently, I just came with using envars to share the config so that it can be accessed by the plugins.
(outdated) Now we are passing the parsed config back to the plugins. Ideally, I'd like to pass the whole k8s client, but we still the token in each request (plus ... caching? we are creating the config each time the method is invoked :S).
(outdated) Testing is still pending, I'll do it tomorrow

For subsequent PRs

Use shared directory for manifests: Move cm used by kubeops and kubeappsapis to a shared folder #2926
Move the parser under pkg/.. to avoid duplicated code: Extract ParseClusterConfig fn to /pkg #2958
(future) Improve the clientGetter testing: Utilize user credentials to invoke the new kubeapps-api #2908 (comment)

Still a hardcoded config, pending using the existing config file

absoludity

WIP, there is a couple of things I wanna re-check and thoughts:

* As it is a sort of transversal functionality, we should provide a unified auth experience for plugin developers. Can plugins invoke main program functions?

No - I don't think they should be able to do so, but open to suggestions.

Another way round: can the main program share info with the plugins?

Yes, we can definitely do this. For example, we could include configuration data in the signature of the RegisterWithGRPCServer function, or we could define a set of env vars that are available. Or, it may be useful (as I think you suggested) to even pass a function or something to the plugins when registering that returns a k8s config (so each plugin doesn't have to do this, or care about pinniped etc.)

  Currently, I just came with using envars to share the config so that it can be accessed by the plugins.

One thing: I didn't expect this initial PR to require cluster config or anything to do with pinniped etc. Is that just because your local dev server is setup with pinniped? I'd recommend first just using a normal non-pinniped setup and verifying the way to ensure plugins have a valid k8s config. Then we can update to support pinniped etc. But up to you.

Glad the crux of it is straight-forward (extracting the token from the context, which is translated from the http header).

chart/kubeapps/templates/kubeappsapis/config.yaml

chart/kubeapps/templates/kubeappsapis/deployment.yaml

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

absoludity · 2021-05-31T01:59:53Z

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

+func extractToken(ctx context.Context) (string, error) {
+	md, ok := metadata.FromIncomingContext(ctx)
+	if !ok {
+		return "", status.Errorf(codes.Unauthenticated, "no authorization metadata found")


We may have some anonymous endpoionts? Why not defer to the endpoint (which will fail if RBAC doesn't allow it).

Ooops, that's true. I've updated that function accordingly.

However, I still have a doubt here: do anonymous endpoints (eg getConfiguredPlugins) still require the k8s authentication? In this PR I just added that logic in those which are not anonymous.

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

gfichtenholt · 2021-05-31T10:26:36Z

I am not 100% sure about this, but I think I am hitting some RBAC-related issues, possibly related to this PR
with the current plugin code in fluxv2 server.go. I notice that
func clientForRequestContext(ctx context.Context) (dynamic.Interface, error)
currently isn't making use of ctx argument with the "to be updated" notice
The issue I am hitting is I can programmatically list existing flux CRDs with something like this:
chartList, err := client.Resource(chartsResource).List(ctx, metav1.ListOptions{})
just fine. But creating a CRD via the same resource
newChart, err := client.Resource(chartsResource).Create(ctx, &unstructuredChart, metav1.CreateOptions{})
results in
0531 06:36:34.695099 1 server.go:235] error creating chart: the server could not find the requested resource
I have done some digging and its looking like the issue is likely to be due to some RBAC settings that somehow
allow read-only operations but not write operations for the user identity on behalf of which the code is being executed.
BTW, related question - is there a call in go I can make to log the identity?
Also, interestingly, just as an experiment I added a similar piece of code into kapp_controller plug in
(i.e. use the resource to programmatically create a packageRepository)
and it worked there. I can't quite explain that one yet. To be continued...

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

absoludity · 2021-05-31T22:56:51Z

I am not 100% sure about this, but I think I am hitting some RBAC-related issues, possibly related to this PR
with the current plugin code in fluxv2 server.go. I notice that
func clientForRequestContext(ctx context.Context) (dynamic.Interface, error)
currently isn't making use of ctx argument with the "to be updated" notice

Correct, yes that's the bit that Antonio is working on in this branch, but it won't be affecting your experience yet (nor when it lands, I hope :) ).

The issue I am hitting is I can programmatically list existing flux CRDs with something like this:
chartList, err := client.Resource(chartsResource).List(ctx, metav1.ListOptions{})
just fine. But creating a CRD via the same resource
newChart, err := client.Resource(chartsResource).Create(ctx, &unstructuredChart, metav1.CreateOptions{})
results in
0531 06:36:34.695099 1 server.go:235] error creating chart: the server could not find the requested resource

My first thought was that our development RBAC may not have you covered, but it seems to (since all the flux CRDs appear to be for source.toolkit.fluxcd.io/v1beta1 which that RBAC covers).

My next guess is that it's a namespace-scoped object (a HelmChart)? So perhaps you need to do client.Resource(chartsResource).Namespace('foo').Create(...), otherwise the client may be looking for a cluster-scoped resource (which doesn't exist). But that's just a guess.

BTW, related question - is there a call in go I can make to log the identity?

Currently for the ease of development, all calls are using the in-cluster config which is the service account linked to the above RBAC. Once Antonio finishes, we'll additionally have the option of switching off the development-only RBAC so the user (JWT) identity is used, which I guess we could parse and use the username. The auth-proxy logs on the frontend already do something like this for us, but if you think it's needed for this service, it should be straight forward to add once Antonio is finished.

Also, interestingly, just as an experiment I added a similar piece of code into kapp_controller plug in
(i.e. use the resource to programmatically create a packageRepository)
and it worked there. I can't quite explain that one yet. To be continued...

Right, that might be more evidence for my hypothesis above, given the PackageRepository is a cluster-scoped object currently.

chart/kubeapps/templates/kubeappsapis/config.yaml

chart/kubeapps/templates/kubeappsapis/deployment.yaml

chart/kubeapps/templates/kubeappsapis/rbac.yaml

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/main.go

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

gfichtenholt · 2021-06-01T04:38:41Z

I am not 100% sure about this, but I think I am hitting some RBAC-related issues, possibly related to this PR
with the current plugin code in fluxv2 server.go. I notice that
func clientForRequestContext(ctx context.Context) (dynamic.Interface, error)
currently isn't making use of ctx argument with the "to be updated" notice

Correct, yes that's the bit that Antonio is working on in this branch, but it won't be affecting your experience yet (nor when it lands, I hope :) ).

The issue I am hitting is I can programmatically list existing flux CRDs with something like this:
chartList, err := client.Resource(chartsResource).List(ctx, metav1.ListOptions{})
just fine. But creating a CRD via the same resource
newChart, err := client.Resource(chartsResource).Create(ctx, &unstructuredChart, metav1.CreateOptions{})
results in
0531 06:36:34.695099 1 server.go:235] error creating chart: the server could not find the requested resource

My first thought was that our development RBAC may not have you covered, but it seems to (since all the flux CRDs appear to be for source.toolkit.fluxcd.io/v1beta1 which that RBAC covers).

My next guess is that it's a namespace-scoped object (a HelmChart)? So perhaps you need to do client.Resource(chartsResource).Namespace('foo').Create(...), otherwise the client may be looking for a cluster-scoped resource (which doesn't exist). But that's just a guess.

BTW, related question - is there a call in go I can make to log the identity?

Currently for the ease of development, all calls are using the in-cluster config which is the service account linked to the above RBAC. Once Antonio finishes, we'll additionally have the option of switching off the development-only RBAC so the user (JWT) identity is used, which I guess we could parse and use the username. The auth-proxy logs on the frontend already do something like this for us, but if you think it's needed for this service, it should be straight forward to add once Antonio is finished.

Also, interestingly, just as an experiment I added a similar piece of code into kapp_controller plug in
(i.e. use the resource to programmatically create a packageRepository)
and it worked there. I can't quite explain that one yet. To be continued...

Right, that might be more evidence for my hypothesis above, given the PackageRepository is a cluster-scoped object currently.

Yep, that was exactly it, I needed to set the namespace, as I was dealing with Namespaced-scoped object. Thanks, Michael!. Your help is always spot on and very much appreciated. Still would like to see an example of piece of go code that logs the user identity on behalf of which the code is executed. I know how to do this in Java (using thread-locals) in spring or EE container but no clue in go

It holds the cluster info so that other components can read it. 6f6e39

Conflicts: cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/main.go cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/main.go cmd/kubeapps-apis/server/plugins.go

antgamdia · 2021-06-02T19:04:42Z

I've tried to add as many unit tests as possible, but some functions are really coupled to a k8s cluster, so, even if I tried to mock some objects, the coverage is far from being ideal.

fwiw, I've tested it locally with a single cluster using pinniped and csp, so... let's mark it as ready for review (finally... 😁 )

absoludity

Yep, I got it. After refactoring a little bit for writing the tests, I just improved the b) option, creating two functions w/ and w/o the required params (still don't know why golang is against overloading functions...)

The (b) option is using a global var I think, see inline comments about using function closures to avoid the globals. Regarding the two functions w/ and w/o the required params, I didn't see them? Perhaps you figured that out? (as I see you're using the raw signature now rather than a type alias defined in a package).

chart/kubeapps/Chart.yaml

cmd/kubeapps-apis/cmd/root.go

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

absoludity · 2021-06-03T04:32:08Z

cmd/kubeapps-apis/server/plugins.go

@@ -232,3 +253,140 @@ func listSOFiles(fsys fs.FS, pluginDirs []string) ([]string, error) {
 	}
 	return matches, nil
 }
+
+// parseClusterConfig returns a kube.ClustersConfig struct after parsing the raw `clusters` object provided by the user
+// TODO(agamez): this fn is the same as in kubeapps/cmd/kubeops/main.go, export it and use it instead (unit test available at: cmd/kubeops/main_test.go)


+1, let's move it into pkg/kube and re-use from there (guessing that's what you intended, just being explicit).

But let's do it in another PR, no? I already have it planned once we got this PR landed

Yes, I didn't mean it needs to be moved here.

cmd/kubeapps-apis/server/plugins.go

absoludity · 2021-06-03T04:37:49Z

cmd/kubeapps-apis/server/plugins.go

+
+// dynClientGetterForContext returns a k8s client for use during interactions with the cluster.
+// It is invoked by dynClientGetterForContext and unit tests passing the appropriate configuration
+func dynClientGetterForContextWithConfig(ctx context.Context, inClusterConfig *rest.Config, serveOpts ServeOptions, config kube.ClustersConfig) (dynamic.Interface, error) {


Nice, so this function is not only testable, but doesn't require any global vars... great, as this enables us to avoid using global vars altogether...

cmd/kubeapps-apis/server/plugins.go

absoludity · 2021-06-03T05:58:07Z

cmd/kubeapps-apis/server/plugins.go

 		return fmt.Errorf("unable to use %q in plugin %v due to mismatched signature.\nwant: %T\ngot: %T", grpcRegisterFunction, pluginDetail, dummyFn, grpcRegFn)
 	}
-	server := grpcFn(registrar)
+
+	server := grpcFn(registrar, dynClientGetterForContext)


So I think ^^ is the reason that you've needed global variables: you're passing in a static function to each plugin and so that statically defined function needs access to the server options, without them being passed in (due to the function signature)?

The solution is to not use a static function, but instead, we can define a function closure (a function which captures state from the scope where it is defined), probably not here in registerGRPC as then we'd be doing it for every plugin, but above in registerPlugins where we can create it once for all plugins:

var dynClientGetterForContext := func(ctx context.Context) (dynamic.Interfoce, error) { ... }

You should only need to ensure that the server options are passed down to registerPlugins. Then you can pass the closure to each plugin.

Or if you don't want to define the function closure within the registerPlugins, you can instead define a function that takes the server options as an arg and returns the function closure, such as:

func createClientGetter(serveOpts ServeOptions) func(context.Context) (dynamic.Client, error) { // parse the cluster config etc. return func(ctx context.Context) (dynamic.Client, error) { // In here you can refer to both serveOpts and the parsed cluster config ... } }

You can then call this function once in registerPlugins to create the dynamic client getter, then pass that down to each registerGRPC call.

cmd/kubeapps-apis/server/plugins.go

absoludity

I've tried to add as many unit tests as possible, but some functions are really coupled to a k8s cluster, so, even if I tried to mock some objects, the coverage is far from being ideal.

Yep, you've got the main thing tested (the extraction of the token from context) - great. Testing the function for the dynamic client is tricky, what you've done is fine for now. I've left an idea for a future improvement that we can try at some point.

fwiw, I've tested it locally with a single cluster using pinniped and csp, so... let's mark it as ready for review (finally... grin )

Great. I'll hold off approving until you've had a chance to look at the function closure to get rid of the global vars.

absoludity · 2021-06-03T06:22:38Z

cmd/kubeapps-apis/server/plugins_test.go

+		contextKey    string
+		contextValue  string
+		expectedToken string
+		expectedErr   string


You may also be able to use the errors package to instead define an error type here (ie. nil, for no error, or an actual error if it is, then use errors.Is. But fine as is.

I've changed it to fmt.Errorf(...) and status.Errorf (passing the expected codes.Unauthenticated instead of relying on the string msg). Thanks!

absoludity · 2021-06-03T06:25:18Z

cmd/kubeapps-apis/server/plugins_test.go

+		expectedErr   string
+	}{
+		{
+			name:          "Good token",


So this function is an easy one to test - great. I try to make the name of the test follow the test spec, like "it returns the expected token without error for a valid context value", but that's just a personal preference.

Yup, totally right. I also like having meaningfulñ names for the tests, but I was too focused on writing them that I forgot to edit them before pushing. Thanks for the heads up!

absoludity · 2021-06-03T06:45:40Z

cmd/kubeapps-apis/server/plugins_test.go

+			}
+			dynamicInterface, err := dynClientGetterForContextWithConfig(context, inClusterConfig, serveOpts, config)
+
+			if tc.shouldCreate != (dynamicInterface != nil) {


So right, this one is harder to test. Worth asking what is the purpose of the test? I assume we want to ensure that the returned client is valid when it should be, but it's tricky to do so, since the returned result doesn't have much to check in the way of public fields.

One thing that may be possible (not recommending you do this, just a thought) would be to use the http_test package to create a fake http server and use it's address as the endpoint you expect (such as the pinnipedProxyURL). It's possible then that you could actually use the client to request something (anything) and verify that the token was sent with the request to the expected IP. But again, no need to do this, just thinking out aloud. I'd be happy to give it a go as a follow-up.

Exactly, I initally was trying to -use- the dynamic interface, but it was more tricky than expected, so I just checked if the creation was being performed.

I've added a //TODO there, but I'd rather leave the current test. At least we are checking the closure fn is being called with the proper params so that it doesn't throw any error.

Co-authored-by: Michael Nelson <absoludity@gmail.com>

Conflicts: chart/kubeapps/Chart.yaml

antgamdia · 2021-06-07T20:22:02Z

I don't know why, but the e2e tests are not passing because of a CI issue 😭 . Why it's happening here? Dunno, the changes herein are totally unrelated... I've sent a fix in #2950

absoludity

Great work Antonio! Small note inline about hard-coding the "default" cluster, but otherwise +1

absoludity · 2021-06-08T00:54:37Z

cmd/kubeapps-apis/server/plugins.go

+
+		var client dynamic.Interface
+		if !serveOpts.UnsafeUseDemoSA {
+			restConfig, err := kube.NewClusterConfig(inClusterConfig, token, "default", config)


Guessing you didn't mean to hard-code the "default" cluster here?

Oops, leftover I didn't notice! It should retrieve the cluster from ClustersConfig.KubeappsClusterName (since we are not using cluster-scoped endpoints yet, afaik). Changing it straight away. Thanks!

Conflicts: cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

antgamdia · 2021-06-08T13:49:59Z

I don't know why, but the e2e tests are not passing because of a CI issue . Why it's happening here? Dunno, the changes herein are totally unrelated... I've sent a fix in #2950

Well, it was not the CI who blame 100%, but a minor kind-of syntax bug (always forget Helm doesn't have lazy eval...). The issue is that it should have been caught by the test chart render CI step.
I've sent another PR addressing it #2957 , but now this one is passing finally. So... let's merge! (and create the one for extracting the shared function to /pkg)

Thanks again for your valuable suggestions and ideas!

antgamdia added 5 commits May 28, 2021 17:52

Add log traces

44fb45e

Initial add authentication version

580e6e2

Still a hardcoded config, pending using the existing config file

Merge branch 'master' into 2851-addUserAuth

0db6c84

Add env vars with cluster config in chart

11c0833

Build config based on env vars

6ceef18

absoludity reviewed May 31, 2021

View reviewed changes

antgamdia added 3 commits May 31, 2021 23:05

Add unsafeUseDemoSA flag

c36e6a6

Pass parsed ClustersConfig to the plugins

c9d7e31

Fix syntax errors in tests

4b5b013

antgamdia commented May 31, 2021

View reviewed changes

cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go Outdated Show resolved Hide resolved

absoludity reviewed Jun 1, 2021

View reviewed changes

antgamdia added 6 commits June 2, 2021 01:35

Remove "get client" logic from the plugins

ea6e4c1

Add get token logic in the plugins.go

cb26816

Fix tests

735e4c0

Create a cluster-config cm under 'shared' folder

4d7ff5a

It holds the cluster info so that other components can read it. 6f6e39

Modify the file with the extra value fields

47bd116

Remove todos from rbac

466478c

antgamdia force-pushed the 2851-addUserAuth branch from 5c4e05b to 466478c Compare June 2, 2021 00:36

antgamdia added 2 commits June 2, 2021 19:44

Refactor for testing

5462234

Add tests

d7357ea

antgamdia mentioned this pull request Jun 2, 2021

Move cm used by kubeops and kubeappsapis to a shared folder #2926

Merged

antgamdia added 2 commits June 2, 2021 20:36

Prepare changes before merging

5332e60

Merge branch 'master'

e5f977c

Conflicts: cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/main.go cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go cmd/kubeapps-apis/plugins/kapp_controller/packages/v1alpha1/main.go cmd/kubeapps-apis/server/plugins.go

antgamdia marked this pull request as ready for review June 2, 2021 19:05

absoludity reviewed Jun 3, 2021

View reviewed changes

antgamdia and others added 3 commits June 7, 2021 12:46

Apply suggestions from code review

3519133

Co-authored-by: Michael Nelson <absoludity@gmail.com>

Apply pr review feedback

7fe7337

Merge branch 'master'

422216f

Conflicts: chart/kubeapps/Chart.yaml

This was referenced Jun 7, 2021

[kubeapps-apis] Switch to use user credentials from request #2851

Closed

Improve k8s_wait_for_deployment #2950

Merged

absoludity approved these changes Jun 8, 2021

View reviewed changes

antgamdia added 3 commits June 8, 2021 09:08

Remove leftover "default" hardcoded cluster

b264481

Merge branch 'master'

26ddde4

Conflicts: cmd/kubeapps-apis/plugins/fluxv2/packages/v1alpha1/server.go

Add nested if to avoid null references

78d4677

antgamdia merged commit 6340f26 into vmware-tanzu:master Jun 8, 2021

antgamdia deleted the 2851-addUserAuth branch June 8, 2021 13:57

This was referenced Jun 8, 2021

Extract ParseClusterConfig fn to /pkg #2958

Merged

Minor apidocs update #2964

Merged

[kubeapps-api] Fix "make run" to locally test the changes w/o passing cluster config #2969

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Utilize user credentials to invoke the new kubeapps-api #2908

Utilize user credentials to invoke the new kubeapps-api #2908

antgamdia commented May 30, 2021 •

edited

absoludity left a comment

absoludity May 31, 2021

antgamdia May 31, 2021

gfichtenholt commented May 31, 2021

absoludity commented May 31, 2021

gfichtenholt commented Jun 1, 2021

antgamdia commented Jun 2, 2021

absoludity left a comment

absoludity Jun 3, 2021

antgamdia Jun 7, 2021

absoludity Jun 7, 2021

absoludity Jun 3, 2021

absoludity Jun 3, 2021 •

edited

absoludity left a comment

absoludity Jun 3, 2021

antgamdia Jun 7, 2021

absoludity Jun 3, 2021

antgamdia Jun 7, 2021

absoludity Jun 3, 2021

antgamdia Jun 7, 2021

antgamdia commented Jun 7, 2021

absoludity left a comment

absoludity Jun 8, 2021

antgamdia Jun 8, 2021

antgamdia commented Jun 8, 2021

Utilize user credentials to invoke the new kubeapps-api #2908

Utilize user credentials to invoke the new kubeapps-api #2908

Conversation

antgamdia commented May 30, 2021 • edited

Description of the change

Benefits

Possible drawbacks

Applicable issues

Additional information

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

gfichtenholt commented May 31, 2021

absoludity commented May 31, 2021

gfichtenholt commented Jun 1, 2021

antgamdia commented Jun 2, 2021

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

absoludity Jun 3, 2021 • edited

Choose a reason for hiding this comment

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

antgamdia commented Jun 7, 2021

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

antgamdia commented Jun 8, 2021

antgamdia commented May 30, 2021 •

edited

absoludity Jun 3, 2021 •

edited