Support for rbac v1beta1 on kubernetes v1.7.5+ #682

nrb · 2018-07-18T13:36:29Z

Fixes #660
Signed-off-by: Nolan Brubaker [email protected]

nrb · 2018-07-18T13:38:13Z

pkg/backup/service_account_action.go

 	"github.com/heptio/ark/pkg/kuberesource"
 )

+type clusterRoleBindingVersionWrapper struct {


Would like some guidance here - I don't think this struct is the way to go, but I was struggling coming up with an interface, given the ClusterRoleBinding is just a struct w/o methods.

skriss · 2018-07-18T21:00:37Z

pkg/backup/rbac.go

+	// Name returns the name of a ClusterRoleBinding.
+	Name() string
+	// Subjects translates a ClusterRoleBindings Subjects slice into a slice of subject wrapper objects.
+	Subjects() []subject


One thing you could do is instead of exposing Subjects() and needing the wrapper type for that, just add a method to the ClusterRoleBinding interface called HasSubject that takes a subject kind, namespace, and name, and returns true or false -- since that's all we really need. That would minimize the amount of this wrapper code that we need

Actually may need to be HasServiceAccountSubject so it doesn't have to take the kind (since that's part of the API group/version), but same idea.

skriss

Haven't gone through units in detail yet

skriss · 2018-07-19T21:10:34Z

pkg/backup/rbac.go

+	rbacclientbeta "k8s.io/client-go/kubernetes/typed/rbac/v1beta1"
+)
+
+type CRBLister interface {


I think I'd prefer to spell out ClusterRoleBinding in the type names (interface + 4 structs). I'm fine using abbreviations for local variable names though

skriss · 2018-07-19T21:13:38Z

pkg/backup/rbac.go

+	List() ([]ClusterRoleBinding, error)
+}
+
+type V1CRBLister struct {


this and the V1beta1CRBLister1 don't need to be exported, just the interface

skriss · 2018-07-19T21:14:00Z

pkg/backup/rbac.go

+}
+
+// NewCRBLister wraps the v1beta1 and v1 RBAC client's List with a common interface.
+func NewCRBLister(log logrus.FieldLogger, clientset kubernetes.Interface) (CRBLister, error) {


spell out ClusterRoleBinding in the func name

skriss · 2018-07-19T21:14:24Z

pkg/backup/rbac.go

+
+// NewCRBLister wraps the v1beta1 and v1 RBAC client's List with a common interface.
+func NewCRBLister(log logrus.FieldLogger, clientset kubernetes.Interface) (CRBLister, error) {
+	var lister CRBLister


usually declare local vars closer to their usage (unlike a bunch of other languages)

skriss · 2018-07-19T21:15:21Z

pkg/backup/rbac.go

+		return nil, errors.WithStack(err)
+	}
+	var supportedAPI metav1.GroupVersionForDiscovery
+	apigroups := discoveryHelper.APIGroups()


no real need to declare apigroups - can just range over discoveryHelper.APIGroups() in the next line

skriss · 2018-07-19T21:17:29Z

pkg/backup/rbac.go

+	var supportedAPI metav1.GroupVersionForDiscovery
+	apigroups := discoveryHelper.APIGroups()
+	for _, ag := range apigroups {
+		if ag.Name == "rbac.authorization.k8s.io" {


s/"rbac.authorization.k8s.io"/rbac.GroupName

skriss · 2018-07-19T21:18:23Z

pkg/backup/rbac.go

+
+	switch supportedAPI.Version {
+	case "v1":
+		lister = V1CRBLister{client: clientset.RbacV1().ClusterRoleBindings()}


I'd just directly return the lister, nil

skriss · 2018-07-19T21:18:28Z

pkg/backup/rbac.go

+	case "v1":
+		lister = V1CRBLister{client: clientset.RbacV1().ClusterRoleBindings()}
+	case "v1beta1":
+		lister = V1beta1CRBLister{client: clientset.RbacV1beta1().ClusterRoleBindings()}


I'd just directly return the lister, nil

skriss · 2018-07-19T21:18:54Z

pkg/backup/rbac.go

+	case "v1beta1":
+		lister = V1beta1CRBLister{client: clientset.RbacV1beta1().ClusterRoleBindings()}
+	// Return an empty client if we don't have a known version.
+	default:


I'd just directly return nil, nil for the default (or get rid of default and return it outside the switch)

skriss · 2018-07-19T21:20:43Z

pkg/backup/service_account_action_test.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2018 the Heptio Ark contributors.
+


undo deletion of copyright

skriss · 2018-07-19T21:24:02Z

pkg/backup/rbac.go

+	}
+
+	switch supportedAPI.Version {
+	case "v1":


could use rbac.SchemeGroupVersion.Version rather than "v1" here, and rbacbeta.... for the v1beta1 case

carlisia

I had a minor style comment only. There are some exported methods that don't have a description, some are obvious and others not so obvious to me, so my additional comment is to add moar description if it makes sense. Other than this, lgtm as far as I can tell.

carlisia · 2018-07-19T21:46:22Z

pkg/backup/rbac.go

+}
+
+func (v1beta1 V1beta1CRBLister) List() ([]ClusterRoleBinding, error) {
+	var crbs []ClusterRoleBinding


Wanted to extend on what @skriss said to make sure this and I think one additional case of this is also changed: I'd move this crbs var to right above the for loop where it's used. It's a common thing in Go so it's expected but also avoids scrolling all over for where it's used if we expect its first use to happen right after where it's declared.

nrb · 2018-07-20T01:54:15Z

There are some exported methods that don't have a description

Which ones in particular? Happy to add more comments, though I'm not sure what's missing. The ones exposed via interfaces should be commented on the interfaces themselves.

carlisia · 2018-07-20T14:55:39Z

The ones exposed via interfaces should be commented on the interfaces themselves.

My bad, @nrb, those were precisely the already commented methods of interfaces :) 👍

nrb · 2018-07-20T16:43:56Z

Tested this with Kubernetes v1.7.16. The server started (where it would previously fail), and the cluster role bindings we create with the Ark installation were backed up.

% tar xzvf arktest-data.tar.gz
resources/namespaces/cluster/heptio-ark.json
resources/secrets/namespaces/heptio-ark/ark-token-rd7w2.json
resources/secrets/namespaces/heptio-ark/cloud-credentials.json
resources/secrets/namespaces/heptio-ark/default-token-lr9x1.json
resources/clusterrolebindings.rbac.authorization.k8s.io/cluster/ark.json
resources/clusterroles.rbac.authorization.k8s.io/cluster/cluster-admin.json
resources/serviceaccounts/namespaces/heptio-ark/ark.json
resources/serviceaccounts/namespaces/heptio-ark/default.json
resources/configs.ark.heptio.com/namespaces/heptio-ark/default.json
resources/backups.ark.heptio.com/namespaces/heptio-ark/arktest.json
resources/backups.ark.heptio.com/namespaces/heptio-ark/nginx.json

% cat resources/clusterrolebindings.rbac.authorization.k8s.io/cluster/ark.json
{"apiVersion":"rbac.authorization.k8s.io/v1beta1","kind":"ClusterRoleBinding","metadata":{"annotations":{"kubectl.kubernetes.io/last-applied-configuration":"{\"apiVersion\":\"rbac.authorization.k8s.io/v1beta1\",\"kind\":\"ClusterRoleBinding\",\"metadata\":{\"annotations\":{},\"labels\":{\"component\":\"ark\"},\"name\":\"ark\",\"namespace\":\"\"},\"roleRef\":{\"apiGroup\":\"rbac.authorization.k8s.io\",\"kind\":\"ClusterRole\",\"name\":\"cluster-admin\"},\"subjects\":[{\"kind\":\"ServiceAccount\",\"name\":\"ark\",\"namespace\":\"heptio-ark\"}]}\n"},"creationTimestamp":"2018-07-20T16:30:02Z","labels":{"component":"ark"},"name":"ark","resourceVersion":"1348","selfLink":"/apis/rbac.authorization.k8s.io/v1beta1/clusterrolebindings/ark","uid":"2721ffd3-8c3a-11e8-9722-0a1f5e4b1da4"},"roleRef":{"apiGroup":"rbac.authorization.k8s.io","kind":"ClusterRole","name":"cluster-admin"},"subjects":[{"kind":"ServiceAccount","name":"ark","namespace":"heptio-ark"}]}%

skriss · 2018-07-20T17:00:39Z

pkg/backup/rbac.go

+}
+
+func (v1beta1 v1beta1ClusterRoleBindingLister) List() ([]ClusterRoleBinding, error) {
+


rm blank line

skriss · 2018-07-20T17:01:38Z

pkg/backup/rbac.go

+func NewClusterRoleBindingLister(log logrus.FieldLogger, clientset kubernetes.Interface) (ClusterRoleBindingLister, error) {
+	discoveryHelper, err := arkdiscovery.NewHelper(clientset.Discovery(), log)
+	if err != nil {
+		return nil, errors.WithStack(err)


don't need a errors.WithStack here since the error is coming from ark code so should already have a stack

skriss · 2018-07-20T17:02:01Z

pkg/backup/rbac.go

+}
+
+// v1 implementation
+type v1crb struct {


i'd probably spell out ClusterRoleBinding in the type name here

skriss · 2018-07-20T17:02:09Z

pkg/backup/rbac.go

+
+// v1beta1 implementation
+
+type v1beta1crb struct {


i'd probably spell out ClusterRoleBinding in the type name here

skriss · 2018-07-20T17:02:31Z

pkg/backup/rbac.go

+	return saSubjects
+}
+
+// v1beta1 implementation


I'd remove this, put a godoc comment on the struct if you want but not necessary

skriss · 2018-07-20T17:03:37Z

pkg/backup/service_account_action.go

-				roles.Insert(clusterRoleBinding.RoleRef.Name)
-				break
+				bindings.Insert(crb.Name())
+				roles.Insert(crb.RoleRefName())


you can still break once you've found a matching subject

Is it not valid to have multiple service account subjects? Wouldn't we want to back them all up?

We are trying to back up CRBs/CRs that have the current service account item as a subject. Once we know that a given CRB does have this SA as a subject, we're going to back it up. No need to look at all the other service accounts that this CRB applies to at that point.

skriss · 2018-07-20T17:07:08Z

pkg/discovery/helper.go

@@ -45,6 +45,8 @@ type Helper interface {
 	// Refresh pulls an updated set of Ark-backuppable resources from the
 	// discovery API.
 	Refresh() error
+
+	APIGroups() []metav1.APIGroup


nrb · 2018-07-20T17:16:55Z

Double-checking with a 1.9 cluster on GKE, ClusterRoleBindings still back up as expected with this change.

skriss · 2018-07-20T19:04:20Z

pkg/cmd/server/plugin/plugin.go

 					cmd.CheckError(err)
+
+					if crbLister != nil {


What happens if crbLister is null (i.e. RBAC is not enabled on this cluster)? We still go through the code paths of setting up an action item plugin and serving it - seems like there may be a problem there.

Good catch - I had intended to address this and forgot.

Would it make sense to check if the action is nil before adding to the plugin configuration? Or maybe move the NewClusterRoleBindingLister logic into the NewServiceAccount action function?

Also, do we support clusters without RBAC? I didn't think we did, but even if that's the case this behavior needs to be fixed.

I don't know if we've ever really tried on clusters without RBAC. Hopefully shouldn't be much of a use case going fwd, but who knows.

I'd say it makes sense to always have a non-nil action. We can move the NewClusterRoleBindingLister into the NewServiceAccount func. If no API group is found, just leave the item action's clusterRoleBindings field as nil, and return the action/no error. Then Execute() becomes a no-op (more or less) in that scenario.

Sorry I didn't notice this earlier

np, better now than after tagging!

That logic makes sense. I'll have to modify the signature and the testing fakes a bit, but shouldn't be too bad.

skriss · 2018-07-20T19:05:10Z

Apart from the question re: plugin serving, the rest LGTM.

skriss · 2018-07-20T21:59:04Z

pkg/backup/rbac.go

+}
+
+// NewClusterRoleBindingLister wraps the v1beta1 and v1 RBAC client's List with a common interface.
+func NewClusterRoleBindingLister(log logrus.FieldLogger, clientset kubernetes.Interface, discoveryHelper arkdiscovery.Helper) (ClusterRoleBindingLister, error) {


We could change the signature here so instead of taking a clientset, it takes a map[string]ListerConstructor, where the keys are RBAC API versions, and the values are functions that return a ClusterRoleBindingLister. Then, instead of having a switch statement on concrete versions, we just do a lookup into the map for the server's preferred version.

This logic would be easily testable without actually requiring a kubernetes.Interface, and the correctness of the map (which would be defined in plugin.go could be determined by inspection.

Food for thought

I like that idea - are you thinking of pairing that with a no-op lister then?

Yeah, I think that'd be the way to go. So NewServiceAccountAction would go back to taking a logger and a CRBLister, NewClusterRoleBindingLister would take a logger, a discovery helper, and that map of constructor funcs, and return a no-op lister if there was no preferred version or no constructor for the preferred version, and then plugin.go would call NewClusterRoleBindingLister then pass the result to NewServiceAccountAction.

Sketching this out, it might look something like

// In rbac.go type ListerConstructor func() ClusterRoleBindingLister NewV1clusterrolebindinglister() NewV1beta1clusterrolebindinglister() NewRBACDisabledLister // In plugins.go listerFuncs := map[string]ListerConstructor { rbac.SchemeGroupVersion.Version: NewV1clusterrolebindinglister, rbacbeta.SchemeGroupVersion.Version: NewV1betaclusterrolebindinglister, "": NewRBACDisabledLister // returns an empty list } action, err := NewServiceAccountAction(log, discoveryHelper, listerFuncs) // In service_item_action.go/NewServiceAccountAction // apigroup logic moved from NewClusterRoleBindingLister crbLister := listerFuncs[apigroup.Version]() crbs, err := crbLister.List() if err != nil { return nil, err } return &serviceAccountAction{ log: log, clusterRoleBindings: crbs,

I think it works; the listerFuncs map may also be specified in rbac.go and exported, looking at it here. WDYT?

yep, looks good!

skriss · 2018-07-22T23:59:48Z

pkg/backup/service_account_action.go

 }

 // NewServiceAccountAction creates a new ItemAction for service accounts.
-func NewServiceAccountAction(log logrus.FieldLogger, client rbacclient.ClusterRoleBindingInterface) (ItemAction, error) {
-	clusterRoleBindings, err := client.List(metav1.ListOptions{})
+func NewServiceAccountAction(log logrus.FieldLogger, clusterRoleBindingListers map[string]ClusterRoleBindingLister, discoveryHelper arkdiscovery.Helper) (ItemAction, error) {


It would be nice to have a unit test on this now that it doesn't depend a full clientset. You could fake/mock a couple of ClusterRoleBindingListers to return a specific list of CRBs each, put them in the clusterRoleBindingListers arg, then set up a fake discoveryHelper with the relevant API groups that correspond to what's in the map (easiest way to set up the fake discoveryHelper is probably by not using the constructor function, but just instantiating directly and setting the apiGroups to what you need). Then you can verify that the service account action's clusterRoleBindings match what the expected lister had.

skriss · 2018-07-23T00:01:52Z

pkg/backup/rbac.go

+// NewClusterRoleBindingListerMap creates a map of RBAC version strings to their associated
+// ClusterRoleBindingLister structs.
+// Necessary so that callers to the ClusterRoleBindingLister interfaces don't need the kubernetes.Interface.
+func NewClusterRoleBindingListerMap(clientset kubernetes.Interface) map[string]ClusterRoleBindingLister {


I think it would be slightly better if this returned a map[string]ListerConstructor, so that you don't have to construct all of them upfront. This way, only the one that's actually selected is instantiated. I would just inline the constructors as anonymous functions/closures of the clientset argument.

Is there a problem or cost with instantiating them all, given we have 3? To me, returning them directly simplifies reading the code and reduces the number of types we have to carry around.

ok, not a big deal.

skriss · 2018-07-23T00:02:08Z

pkg/backup/rbac.go

+}
+
+// NewV1beta1ClusterRoleBindingLister returns a ClusterRoleBindingLister for RBAC v1beta1.
+func NewV1beta1ClusterRoleBindingLister() ClusterRoleBindingLister {


this should be removed

skriss · 2018-07-23T00:02:16Z

pkg/backup/rbac.go

+}
+
+// NewV1ClusterRoleBindingLister returns a ClusterRoleBindingLister for RBAC v1.
+func NewV1ClusterRoleBindingLister() ClusterRoleBindingLister {


this should be removed

skriss

Looking good, just one unused type def to remove.

skriss · 2018-07-23T18:01:24Z

pkg/backup/rbac.go

+}
+
+// ListerConstructor specifies a function which can return a ClusterRoleBindingLister.
+type ListerConstructor func() ClusterRoleBindingLister


can remove this since it's not being used

Signed-off-by: Nolan Brubaker <[email protected]>

skriss

LGTM.

skriss · 2018-07-23T18:56:52Z

@nrb all set to merge?

nrb · 2018-07-23T18:58:51Z

Yep, ready to merge from my POV. @carlisia do you want to do another pass?

skriss · 2018-07-23T19:00:43Z

I'm gonna go ahead and merge so we can get the patch release out. Can always follow up with additional PRs if necessary,

nrb commented Jul 18, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch 2 times, most recently from 9ce51eb to 0a66211 Compare July 18, 2018 20:24

nrb added the Bug label Jul 18, 2018

skriss reviewed Jul 18, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch from 0a66211 to fe2dd12 Compare July 19, 2018 20:43

nrb changed the title ~~WIP support for rbac v1beta1 on kubernetes v1.7.5+~~ Support for rbac v1beta1 on kubernetes v1.7.5+ Jul 19, 2018

nrb requested a review from carlisia July 19, 2018 20:45

nrb added this to the v0.9.1 milestone Jul 19, 2018

skriss suggested changes Jul 19, 2018

View reviewed changes

skriss reviewed Jul 19, 2018

View reviewed changes

carlisia reviewed Jul 19, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch from fe2dd12 to 42dc865 Compare July 20, 2018 02:04

skriss reviewed Jul 20, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch from 42dc865 to 99264c5 Compare July 20, 2018 18:51

skriss reviewed Jul 20, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch 4 times, most recently from dbf87d2 to 6b2f56f Compare July 20, 2018 21:46

skriss reviewed Jul 20, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch from 6b2f56f to 7fdb079 Compare July 20, 2018 23:54

skriss reviewed Jul 22, 2018

View reviewed changes

skriss reviewed Jul 23, 2018

View reviewed changes

nrb force-pushed the support-rbac-1.7 branch from 7fdb079 to 3b9b762 Compare July 23, 2018 17:59

skriss reviewed Jul 23, 2018

View reviewed changes

Add RBAC support for 1.7 clusters

dd1e150

Signed-off-by: Nolan Brubaker <[email protected]>

nrb force-pushed the support-rbac-1.7 branch from 3b9b762 to dd1e150 Compare July 23, 2018 18:19

skriss approved these changes Jul 23, 2018

View reviewed changes

skriss merged commit e063b79 into vmware-tanzu:master Jul 23, 2018

skriss mentioned this pull request Jul 23, 2018

cherry-pick commits for v0.9.1 #695

Merged

		@@ -1,5 +1,5 @@
		/*
		Copyright 2018 the Heptio Ark contributors.

		}

		func (v1beta1 v1beta1ClusterRoleBindingLister) List() ([]ClusterRoleBinding, error) {

Support for rbac v1beta1 on kubernetes v1.7.5+ #682

Support for rbac v1beta1 on kubernetes v1.7.5+ #682

Conversation

nrb commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss 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

skriss Jul 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

carlisia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrb commented Jul 20, 2018

carlisia commented Jul 20, 2018

nrb commented Jul 20, 2018

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

skriss Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrb commented Jul 20, 2018

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

nrb Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

skriss commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss Jul 20, 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

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

skriss commented Jul 23, 2018

nrb commented Jul 23, 2018

skriss commented Jul 23, 2018

skriss Jul 19, 2018 •

edited

Loading

skriss Jul 20, 2018 •

edited

Loading

nrb Jul 20, 2018 •

edited

Loading

skriss Jul 20, 2018 •

edited

Loading