Issue #148: introduce cluster-scoped-rbac #149

dmartinol · 2023-03-03T12:52:39Z

Added ClusterScopeHandler struct to manage cluster scoped resources (initially with filterRbacResources function but later can be extended with other functions e.g. to export also CRD configurations).
This is triggered by the optional flag cluster-scoped-rbac in the crane export command.

Notes:

Updated Go version to 1.18 due to the golang.org/x/exp/slices package.
Since a new flag has been added to the export command, the documentation should also reflect the same. Please let me know how to do update the documentation as well

…he _cluster folder

… each exported namespace

shawn-hurley

I wonder if we can move this out of cmd package and into a pkg for re-use.

In the past, we would have put most of this in crane-lib but if this is going to work for your usecase, I don't have a huge problem.

The things that I would for sure change is that when listing ClusterScoped resources, we should only list the ones that we care about not all of them coming from discovery, I think that will put some strain on API servers if this is in an operator like git-ops primer.

cmd/export/cluster.go

shawn-hurley · 2023-03-03T17:44:50Z

cmd/export/discover.go

+		if g.APIResource.Namespaced {
+			return c.Namespace(namespace).List(context.Background(), opts)
+		} else {
+			return c.List(context.Background(), opts)


I wonder if you have a set of resources that you want it might be worth filtering the cluster scoped calls to just those resources

You mean adding ListOptions so that, for instance, when we collect the ServiceAccounts we also collect the meaningful ClusterRoleBinding and ClusterRole by using FieldSelector queries to fetch only the relevant instances?
If this is the purpose of the comment, it would require more changes to the discovery module because ATM if fetches only resources of a given, single kind. If we return resources of multiple kinds, instead, the code using getObjects function would need more changes.
Please let me know what was your actual goal.

FYI, not all the fields are eligible to be used in the ListOptions.FieldSelector, e.g. the following command:
oc get clusterrolebindings.rbac.authorization.k8s.io --field-selector roleRef.name=AAA
would actually return an error because of that:
Error from server (BadRequest): Unable to find "rbac.authorization.k8s.io/v1, Resource=clusterrolebindings" that match label selector "", field selector "roleRef.name=AAA": "roleRef.name" is not a known field selector: only "metadata.name", "metadata.namespace"

I think what I was asking, is that we look at the kind, and only retrieve the kinds of RBAC and SCC rather than filter here.

when it reaches this point in the getObjects function, line 226 can only collect the 3 modelled kinds of cluster-scoped RBAC resources, all other kinds were discarded before invoking getObjects

cmd/export/cluster.go

…ame in differernt groups Using range instead of reverse loop by index Reverted use of go 1.18 (removed slices package)

djzager

This is awesome work and thank you for putting it together. I do wonder though if it may be better for us to have the cluster-scoped-rbac flag (as you have included here) and when it is set to true, we go ahead and "GET" the admittedClusterScopeResources, write these resources out in the _cluster dir you create, leaving the filter logic you have in discover.go for the transform step.

My main concern here is that the filtering of RBAC resources looks a lot like a transform and I think it's wise for us to keep export and transform as distinct steps.

The part I have to apologize for is that I believe that means, in addition to changes here in crane, you'll need changes to:

The kubernetes plugin for ClusterRole(Binding)s https://github.com/migtools/crane-lib/blob/main/transform/kubernetes/kubernetes.go
The openshift plugin to handle security context constraints https://github.com/migtools/crane-plugin-openshift/blob/main/cmd.go

What do you think? I suspect the hardest thing will be related to the benefit you have here of being able to traverse all the resources and then pick the ones you want based on filter conditions. Might get tricky.

cmd/export/cluster.go

djzager · 2023-03-06T15:01:59Z

cmd/export/cluster.go

+	}
+}
+
+func (c *ClusterScopedRbacHandler) filteredResourcesOfKind(resource admittedResource) (*groupResource, bool) {


This filter logic, to me, seems more appropriate for the transform step.

Hey @djzager, I perfectly agree with you that this seems to be a transform step, and you almost anticipated some of the reasons why I haven't followed this path (e.g., complexity and more repos impacted), but at the moment this can't be done without adding any context related to the other exported resources, because the PluginRun interface works only on individual resources, but the decision whether to accept or whiteout an RBAC resource depends also on other resources (e.g., is there any ServiceAccount matching this ClusterRoleBinding?). In brief, we should pass some kind of "context" to the plugin execution which, in turn, for each and every RBAC resource, should unmarshal all the files in all the exported namespaces to look for these matches.
I'm totally worried by performance, as you can imagine, this is why the initial implementation, even if it broke the usual 3-steps approach, looked more promising.
WDYT?

Fair point. I believe the right answer for us in this scenario would be to articulate why we can't put this in transform plugin(s) via GitHub issue and proceed with this PR as it is currently implemented.

I am plus one, a follow up issue would be great

cmd/export/cluster.go

Co-authored-by: David Zager <[email protected]>

djzager

I believe I follow what's going on and why. Hope to give this a final read through before COB today.

djzager · 2023-03-06T16:46:50Z

cmd/export/cluster.go

+	return clusterScopeHandler
+}
+
+func isClusterScopedResource(apiGroup string, kind string) bool {


Suggested change

func isClusterScopedResource(apiGroup string, kind string) bool {

func isAdmittedClusterScopedResource(apiGroup string, kind string) bool {

Maybe isAdmitted is even better, currently there is a mismatch between the name of the function and what it's doing.

djzager · 2023-03-06T16:51:47Z

cmd/export/cluster.go

+	}
+}
+
+func (c *ClusterScopedRbacHandler) filteredResourcesOfKind(resource admittedResource) (*groupResource, bool) {


Fair point. I believe the right answer for us in this scenario would be to articulate why we can't put this in transform plugin(s) via GitHub issue and proceed with this PR as it is currently implemented.

shawn-hurley

I am good to go, @djzager want to merge after your read through?

shawn-hurley · 2023-03-07T13:57:56Z

/retest

dmartinol added 5 commits February 28, 2023 15:08

Added cluster=scoped-rbac flag and exporting all these resources in t…

89c7959

…he _cluster folder

Added ClusterScopeHandler and implemented filtering functions

619752a

Fixed wrong handling of optional flag and set default to false

94b1799

typo: Adding SCC when users match

8e3c57b

Moving _clusters under the namespace folder, to have one instance for…

35e4724

… each exported namespace

shawn-hurley requested changes Mar 3, 2023

View reviewed changes

Added admittedResource type to propertly manage resources with same n…

d0d930f

…ame in differernt groups Using range instead of reverse loop by index Reverted use of go 1.18 (removed slices package)

djzager requested changes Mar 6, 2023

View reviewed changes

dmartinol and others added 2 commits March 6, 2023 16:51

Update cmd/export/cluster.go

028306f

Co-authored-by: David Zager <[email protected]>

Update cmd/export/cluster.go

b0b8c80

Co-authored-by: David Zager <[email protected]>

djzager reviewed Mar 6, 2023

View reviewed changes

shawn-hurley approved these changes Mar 6, 2023

View reviewed changes

dmartinol mentioned this pull request Mar 7, 2023

Move exporter code into crane RHEcosystemAppEng/SaaSi#50

Closed

8 tasks

djzager merged commit d14f18b into migtools:main Mar 7, 2023

dmartinol mentioned this pull request Mar 7, 2023

[BUG] Export gets segmentation violation if access to cluster resources is forbidden #150

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #148: introduce cluster-scoped-rbac #149

Issue #148: introduce cluster-scoped-rbac #149

dmartinol commented Mar 3, 2023

shawn-hurley left a comment

shawn-hurley Mar 3, 2023

dmartinol Mar 6, 2023

dmartinol Mar 6, 2023

shawn-hurley Mar 6, 2023

dmartinol Mar 6, 2023

djzager left a comment

djzager Mar 6, 2023

dmartinol Mar 6, 2023

djzager Mar 6, 2023

shawn-hurley Mar 6, 2023

djzager left a comment

djzager Mar 6, 2023

djzager Mar 6, 2023

djzager Mar 6, 2023

shawn-hurley left a comment

shawn-hurley commented Mar 7, 2023

	func isClusterScopedResource(apiGroup string, kind string) bool {
	func isAdmittedClusterScopedResource(apiGroup string, kind string) bool {

Issue #148: introduce cluster-scoped-rbac #149

Issue #148: introduce cluster-scoped-rbac #149

Conversation

dmartinol commented Mar 3, 2023

shawn-hurley 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

djzager 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

djzager 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

shawn-hurley left a comment

Choose a reason for hiding this comment

shawn-hurley commented Mar 7, 2023