refactor: reduce userinfos deps and add unit tests (#6524)

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

pkg/userinfo/roleRef.go

@@ -3,11 +3,10 @@ package userinfo
 import (
 	"fmt"
 
-	admissionv1 "k8s.io/api/admission/v1"
 	authenticationv1 "k8s.io/api/authentication/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	labels "k8s.io/apimachinery/pkg/labels"
-	rbacv1listers "k8s.io/client-go/listers/rbac/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 const (
@@ -15,24 +14,39 @@ const (
 	roleKind        = "Role"
 )
 
+type RoleBindingLister interface {
+	List(labels.Selector) ([]*rbacv1.RoleBinding, error)
+}
+
+type ClusterRoleBindingLister interface {
+	List(labels.Selector) ([]*rbacv1.ClusterRoleBinding, error)
+}
+
 // GetRoleRef gets the list of roles and cluster roles for the incoming api-request
-func GetRoleRef(rbLister rbacv1listers.RoleBindingLister, crbLister rbacv1listers.ClusterRoleBindingLister, request *admissionv1.AdmissionRequest) ([]string, []string, error) {
+func GetRoleRef(rbLister RoleBindingLister, crbLister ClusterRoleBindingLister, userInfo authenticationv1.UserInfo) ([]string, []string, error) {
 	// rolebindings
 	roleBindings, err := rbLister.List(labels.Everything())
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to list rolebindings: %v", err)
 	}
-	rs, crs := getRoleRefByRoleBindings(roleBindings, request.UserInfo)
+	rs, crs := getRoleRefByRoleBindings(roleBindings, userInfo)
 	// clusterrolebindings
 	clusterroleBindings, err := crbLister.List(labels.Everything())
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to list clusterrolebindings: %v", err)
 	}
-	crs = append(crs, getRoleRefByClusterRoleBindings(clusterroleBindings, request.UserInfo)...)
+	crs = append(crs, getRoleRefByClusterRoleBindings(clusterroleBindings, userInfo)...)
+	if rs != nil {
+		rs = sets.List(sets.New(rs...))
+	}
+	if crs != nil {
+		crs = sets.List(sets.New(crs...))
+	}
 	return rs, crs, nil
 }
 
-func getRoleRefByRoleBindings(roleBindings []*rbacv1.RoleBinding, userInfo authenticationv1.UserInfo) (roles []string, clusterRoles []string) {
+func getRoleRefByRoleBindings(roleBindings []*rbacv1.RoleBinding, userInfo authenticationv1.UserInfo) ([]string, []string) {
+	var roles, clusterRoles []string
 	for _, rolebinding := range roleBindings {
 		if matchBindingSubjects(rolebinding.Subjects, userInfo, rolebinding.Namespace) {
 			switch rolebinding.RoleRef.Kind {
@@ -46,7 +60,8 @@ func getRoleRefByRoleBindings(roleBindings []*rbacv1.RoleBinding, userInfo authe
 	return roles, clusterRoles
 }
 
-func getRoleRefByClusterRoleBindings(clusterroleBindings []*rbacv1.ClusterRoleBinding, userInfo authenticationv1.UserInfo) (clusterRoles []string) {
+func getRoleRefByClusterRoleBindings(clusterroleBindings []*rbacv1.ClusterRoleBinding, userInfo authenticationv1.UserInfo) []string {
+	var clusterRoles []string
 	for _, clusterRoleBinding := range clusterroleBindings {
 		if matchBindingSubjects(clusterRoleBinding.Subjects, userInfo, "") {
 			if clusterRoleBinding.RoleRef.Kind == clusterroleKind {

pkg/userinfo/roleRef_test.go

@@ -1,12 +1,14 @@
 package userinfo
 
 import (
+	"errors"
 	"reflect"
 	"testing"
 
 	authenticationv1 "k8s.io/api/authentication/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	labels "k8s.io/apimachinery/pkg/labels"
 )
 
 func Test_getRoleRefByRoleBindings(t *testing.T) {
@@ -407,3 +409,296 @@ func Test_getRoleRefByClusterRoleBindings(t *testing.T) {
 		})
 	}
 }
+
+type roleBindingLister struct {
+	ret []*rbacv1.RoleBinding
+	err error
+}
+
+func (l roleBindingLister) List(labels.Selector) ([]*rbacv1.RoleBinding, error) {
+	return l.ret, l.err
+}
+
+type clusterRoleBindingLister struct {
+	ret []*rbacv1.ClusterRoleBinding
+	err error
+}
+
+func (l clusterRoleBindingLister) List(labels.Selector) ([]*rbacv1.ClusterRoleBinding, error) {
+	return l.ret, l.err
+}
+
+func TestGetRoleRef(t *testing.T) {
+	type args struct {
+		rbLister  RoleBindingLister
+		crbLister ClusterRoleBindingLister
+		userInfo  authenticationv1.UserInfo
+	}
+	type want struct {
+		roles        []string
+		clusterRoles []string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    want
+		wantErr bool
+	}{
+		{
+			args: args{
+				rbLister: roleBindingLister{
+					ret: []*rbacv1.RoleBinding{{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "same-ns",
+							Namespace: "ns-1",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind: "ServiceAccount",
+							Name: "sa",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "same-ns",
+							Namespace: "ns-1",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "ns-1",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "different-ns",
+							Namespace: "ns-2",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "ns-1",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "different-ns",
+							Namespace: "ns-2",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "ns-1",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-1",
+						},
+					}},
+				},
+				crbLister: clusterRoleBindingLister{},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:ns-1:sa",
+				},
+			},
+			want: want{
+				roles:        []string{"ns-1:role-1", "ns-2:role-1"},
+				clusterRoles: []string{"role-1"},
+			},
+		}, {
+			args: args{
+				rbLister: roleBindingLister{
+					err: errors.New("error"),
+				},
+				crbLister: clusterRoleBindingLister{},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:ns-1:sa",
+				},
+			},
+			wantErr: true,
+		}, {
+			args: args{
+				rbLister: roleBindingLister{},
+				crbLister: clusterRoleBindingLister{
+					err: errors.New("error"),
+				},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:ns-1:sa",
+				},
+			},
+			wantErr: true,
+		}, {
+			args: args{
+				rbLister: roleBindingLister{
+					err: errors.New("error"),
+				},
+				crbLister: clusterRoleBindingLister{
+					err: errors.New("error"),
+				},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:ns-1:sa",
+				},
+			},
+			wantErr: true,
+		}, {
+			args: args{
+				rbLister: roleBindingLister{},
+				crbLister: clusterRoleBindingLister{
+					ret: []*rbacv1.ClusterRoleBinding{{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "cluster-role",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "foo",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "cluster-role",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "bar",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-2",
+						},
+					}},
+				},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:foo:sa",
+				},
+			},
+			want: want{
+				clusterRoles: []string{"role-1"},
+			},
+		}, {
+			args: args{
+				rbLister: roleBindingLister{
+					ret: []*rbacv1.RoleBinding{{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "same-ns",
+							Namespace: "foo",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind: "ServiceAccount",
+							Name: "sa",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "same-ns",
+							Namespace: "foo",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "foo",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "different-ns",
+							Namespace: "ns-2",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "foo",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "Role",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "different-ns",
+							Namespace: "ns-2",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "foo",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-1",
+						},
+					}},
+				},
+				crbLister: clusterRoleBindingLister{
+					ret: []*rbacv1.ClusterRoleBinding{{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "cluster-role",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "foo",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-1",
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "cluster-role",
+						},
+						Subjects: []rbacv1.Subject{{
+							Kind:      "ServiceAccount",
+							Name:      "sa",
+							Namespace: "bar",
+						}},
+						RoleRef: rbacv1.RoleRef{
+							Kind: "ClusterRole",
+							Name: "role-2",
+						},
+					}},
+				},
+				userInfo: authenticationv1.UserInfo{
+					Username: "system:serviceaccount:foo:sa",
+				},
+			},
+			want: want{
+				roles:        []string{"foo:role-1", "ns-2:role-1"},
+				clusterRoles: []string{"role-1"},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			roles, clusterRoles, err := GetRoleRef(tt.args.rbLister, tt.args.crbLister, tt.args.userInfo)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("GetRoleRef() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(roles, tt.want.roles) {
+				t.Errorf("GetRoleRef() roles = %v, want %v", roles, tt.want.roles)
+			}
+			if !reflect.DeepEqual(clusterRoles, tt.want.clusterRoles) {
+				t.Errorf("GetRoleRef() clusterRoles = %v, want %v", clusterRoles, tt.want.clusterRoles)
+			}
+		})
+	}
+}

pkg/webhooks/handlers/dump.go

@@ -96,7 +96,7 @@ func newAdmissionRequestPayload(
 	}
 	var roles, clusterRoles []string
 	if rbLister != nil && crbLister != nil {
-		if r, cr, err := userinfo.GetRoleRef(rbLister, crbLister, request); err != nil {
+		if r, cr, err := userinfo.GetRoleRef(rbLister, crbLister, request.UserInfo); err != nil {
 			return nil, err
 		} else {
 			roles = r

pkg/webhooks/utils/policy_context_builder.go

@@ -39,7 +39,7 @@ func (b *policyContextBuilder) Build(request *admissionv1.AdmissionRequest) (*en
 	userRequestInfo := kyvernov1beta1.RequestInfo{
 		AdmissionUserInfo: *request.UserInfo.DeepCopy(),
 	}
-	if roles, clusterRoles, err := userinfo.GetRoleRef(b.rbLister, b.crbLister, request); err != nil {
+	if roles, clusterRoles, err := userinfo.GetRoleRef(b.rbLister, b.crbLister, request.UserInfo); err != nil {
 		return nil, fmt.Errorf("failed to fetch RBAC information for request: %w", err)
 	} else {
 		userRequestInfo.Roles = roles