From 0fb8c723fec977f91a4bca696a325e3995403722 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Charles-Edouard=20Br=C3=A9t=C3=A9ch=C3=A9?= Date: Fri, 10 Mar 2023 10:09:19 +0100 Subject: [PATCH] refactor: reduce userinfos deps and add unit tests (#6524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Charles-Edouard Brétéché --- pkg/userinfo/roleRef.go | 29 +- pkg/userinfo/roleRef_test.go | 295 +++++++++++++++++++ pkg/webhooks/handlers/dump.go | 2 +- pkg/webhooks/utils/policy_context_builder.go | 2 +- 4 files changed, 319 insertions(+), 9 deletions(-) diff --git a/pkg/userinfo/roleRef.go b/pkg/userinfo/roleRef.go index 24a8b61715..84b0049f31 100644 --- a/pkg/userinfo/roleRef.go +++ b/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 { diff --git a/pkg/userinfo/roleRef_test.go b/pkg/userinfo/roleRef_test.go index a172dcc58c..85fb225903 100644 --- a/pkg/userinfo/roleRef_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/webhooks/handlers/dump.go b/pkg/webhooks/handlers/dump.go index 461703fb46..22dc613a66 100644 --- a/pkg/webhooks/handlers/dump.go +++ b/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 diff --git a/pkg/webhooks/utils/policy_context_builder.go b/pkg/webhooks/utils/policy_context_builder.go index 14f950128a..51ad42d0d3 100644 --- a/pkg/webhooks/utils/policy_context_builder.go +++ b/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