From 53f3c3419e708d588bfebc413fdaab92789731de Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Mon, 13 Apr 2026 01:38:00 +0200 Subject: [PATCH 1/4] feat: support admin & global permissions instance serviceaccount Signed-off-by: BoxBoxJason --- .../v1alpha1/zz_generated.deepcopy.go | 38 ++- .../v1alpha1/zz_serviceaccount_type.go | 19 ++ apis/common/v1alpha1/serviceaccount.go | 2 + .../instance/v1alpha1/serviceaccount_type.go | 19 ++ .../v1alpha1/zz_generated.deepcopy.go | 38 ++- examples/instance/serviceaccount.yaml | 4 +- ....gitlab.crossplane.io_serviceaccounts.yaml | 5 + ...itlab.m.crossplane.io_serviceaccounts.yaml | 5 + ....gitlab.crossplane.io_serviceaccounts.yaml | 35 +++ ...itlab.m.crossplane.io_serviceaccounts.yaml | 35 +++ pkg/cluster/clients/groups/fake/zz_fake.go | 6 + pkg/cluster/clients/groups/zz_group.go | 1 + .../clients/groups/zz_serviceaccount.go | 1 + .../clients/instance/zz_serviceaccount.go | 19 +- .../instance/serviceaccounts/zz_controller.go | 117 ++++++- .../serviceaccounts/zz_controller_helpers.go | 235 ++++++++++++++ .../zz_controller_helpers_test.go | 297 ++++++++++++++++++ .../serviceaccounts/zz_controller_test.go | 3 +- pkg/namespaced/clients/groups/fake/fake.go | 6 + pkg/namespaced/clients/groups/group.go | 1 + .../clients/groups/serviceaccount.go | 1 + .../clients/instance/serviceaccount.go | 19 +- .../instance/serviceaccounts/controller.go | 117 ++++++- .../serviceaccounts/controller_helpers.go | 233 ++++++++++++++ .../controller_helpers_test.go | 295 +++++++++++++++++ .../serviceaccounts/controller_test.go | 3 +- 26 files changed, 1493 insertions(+), 61 deletions(-) create mode 100644 pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers.go create mode 100644 pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers_test.go create mode 100644 pkg/namespaced/controller/instance/serviceaccounts/controller_helpers.go create mode 100644 pkg/namespaced/controller/instance/serviceaccounts/controller_helpers_test.go diff --git a/apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go b/apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go index b8a640f3..20c6fcf2 100644 --- a/apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go +++ b/apis/cluster/instance/v1alpha1/zz_generated.deepcopy.go @@ -3166,6 +3166,31 @@ func (in *ServiceAccount) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ServiceAccountBaselinePermissionsObservation) DeepCopyInto(out *ServiceAccountBaselinePermissionsObservation) { + *out = *in + if in.MissingMemberShipGroups != nil { + in, out := &in.MissingMemberShipGroups, &out.MissingMemberShipGroups + *out = make([]int64, len(*in)) + copy(*out, *in) + } + if in.WrongPermissionsGroups != nil { + in, out := &in.WrongPermissionsGroups, &out.WrongPermissionsGroups + *out = make([]int64, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountBaselinePermissionsObservation. +func (in *ServiceAccountBaselinePermissionsObservation) DeepCopy() *ServiceAccountBaselinePermissionsObservation { + if in == nil { + return nil + } + out := new(ServiceAccountBaselinePermissionsObservation) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceAccountList) DeepCopyInto(out *ServiceAccountList) { *out = *in @@ -3202,6 +3227,7 @@ func (in *ServiceAccountList) DeepCopyObject() runtime.Object { func (in *ServiceAccountObservation) DeepCopyInto(out *ServiceAccountObservation) { *out = *in out.CommonServiceAccountObservation = in.CommonServiceAccountObservation + in.ServiceAccountBaselinePermissionsObservation.DeepCopyInto(&out.ServiceAccountBaselinePermissionsObservation) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountObservation. @@ -3218,6 +3244,16 @@ func (in *ServiceAccountObservation) DeepCopy() *ServiceAccountObservation { func (in *ServiceAccountParameters) DeepCopyInto(out *ServiceAccountParameters) { *out = *in in.CommonServiceAccountParameters.DeepCopyInto(&out.CommonServiceAccountParameters) + if in.Admin != nil { + in, out := &in.Admin, &out.Admin + *out = new(bool) + **out = **in + } + if in.BaselinePermissions != nil { + in, out := &in.BaselinePermissions, &out.BaselinePermissions + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountParameters. @@ -3251,7 +3287,7 @@ func (in *ServiceAccountSpec) DeepCopy() *ServiceAccountSpec { func (in *ServiceAccountStatus) DeepCopyInto(out *ServiceAccountStatus) { *out = *in in.ResourceStatus.DeepCopyInto(&out.ResourceStatus) - out.AtProvider = in.AtProvider + in.AtProvider.DeepCopyInto(&out.AtProvider) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountStatus. diff --git a/apis/cluster/instance/v1alpha1/zz_serviceaccount_type.go b/apis/cluster/instance/v1alpha1/zz_serviceaccount_type.go index 438244e2..20f96ad5 100644 --- a/apis/cluster/instance/v1alpha1/zz_serviceaccount_type.go +++ b/apis/cluster/instance/v1alpha1/zz_serviceaccount_type.go @@ -30,11 +30,30 @@ import ( // ServiceAccountParameters defines the desired state of Gitlab Instance ServiceAccount type ServiceAccountParameters struct { commonv1alpha1.CommonServiceAccountParameters `json:",inline"` + + // Admin represents whether the service account has admin privileges. + // +optional + Admin *bool `json:"admin,omitempty"` + // BaselinePermissions represents the minimal permissions level for all top level groups. + // WARNING: If this field is set to a value other than "no-access", the service account will be added to all groups with at least the specified access level. This can lead to unintended consequences if not used carefully. + // WARNING: This DOES NOT remove the service account from groups if changed from a higher access level to a lower access level. It only adds the service account to groups if changed from a lower access level to a higher access level. + // +optional + // +kubebuilder:validation:Enum=no-access;minimal-access;guest;planner;reporter;security-manager;developer;maintainer;owner + BaselinePermissions *string `json:"baselinePermissions,omitempty"` } // ServiceAccountObservation represents the observed state of the Gitlab Instance ServiceAccount type ServiceAccountObservation struct { commonv1alpha1.CommonServiceAccountObservation `json:",inline"` + + // ServiceAccountBaselinePermissionsObservation represents the observed state of the service account's baseline permissions, which is used to determine if the service account is missing permissions for any top level groups or has the wrong permissions for any top level groups. + ServiceAccountBaselinePermissionsObservation `json:",inline"` +} + +// ServiceAccountBaselinePermissionsObservation represents the observed state of the service account's baseline permissions, which is used to determine if the service account is missing permissions for any top level groups or has the wrong permissions for any top level groups. +type ServiceAccountBaselinePermissionsObservation struct { + MissingMemberShipGroups []int64 `json:"missingMembershipGroups,omitempty"` + WrongPermissionsGroups []int64 `json:"wrongPermissionsGroups,omitempty"` } // A ServiceAccountSpec defines the desired state of a GitLab instance service account. diff --git a/apis/common/v1alpha1/serviceaccount.go b/apis/common/v1alpha1/serviceaccount.go index d494f9b4..7e5bc99f 100644 --- a/apis/common/v1alpha1/serviceaccount.go +++ b/apis/common/v1alpha1/serviceaccount.go @@ -48,4 +48,6 @@ type CommonServiceAccountObservation struct { Username string `json:"username"` // Email represents the email of the service account. Email string `json:"email"` + // Admin represents whether the service account has admin privileges. + Admin bool `json:"admin"` } diff --git a/apis/namespaced/instance/v1alpha1/serviceaccount_type.go b/apis/namespaced/instance/v1alpha1/serviceaccount_type.go index 711a43e2..4eda275d 100644 --- a/apis/namespaced/instance/v1alpha1/serviceaccount_type.go +++ b/apis/namespaced/instance/v1alpha1/serviceaccount_type.go @@ -33,11 +33,30 @@ import ( // ServiceAccountParameters defines the desired state of Gitlab Instance ServiceAccount type ServiceAccountParameters struct { commonv1alpha1.CommonServiceAccountParameters `json:",inline"` + + // Admin represents whether the service account has admin privileges. + // +optional + Admin *bool `json:"admin,omitempty"` + // BaselinePermissions represents the minimal permissions level for all top level groups. + // WARNING: If this field is set to a value other than "no-access", the service account will be added to all groups with at least the specified access level. This can lead to unintended consequences if not used carefully. + // WARNING: This DOES NOT remove the service account from groups if changed from a higher access level to a lower access level. It only adds the service account to groups if changed from a lower access level to a higher access level. + // +optional + // +kubebuilder:validation:Enum=no-access;minimal-access;guest;planner;reporter;security-manager;developer;maintainer;owner + BaselinePermissions *string `json:"baselinePermissions,omitempty"` } // ServiceAccountObservation represents the observed state of the Gitlab Instance ServiceAccount type ServiceAccountObservation struct { commonv1alpha1.CommonServiceAccountObservation `json:",inline"` + + // ServiceAccountBaselinePermissionsObservation represents the observed state of the service account's baseline permissions, which is used to determine if the service account is missing permissions for any top level groups or has the wrong permissions for any top level groups. + ServiceAccountBaselinePermissionsObservation `json:",inline"` +} + +// ServiceAccountBaselinePermissionsObservation represents the observed state of the service account's baseline permissions, which is used to determine if the service account is missing permissions for any top level groups or has the wrong permissions for any top level groups. +type ServiceAccountBaselinePermissionsObservation struct { + MissingMemberShipGroups []int64 `json:"missingMembershipGroups,omitempty"` + WrongPermissionsGroups []int64 `json:"wrongPermissionsGroups,omitempty"` } // A ServiceAccountSpec defines the desired state of a GitLab instance service account. diff --git a/apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go b/apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go index 933f5d97..0dbb175f 100644 --- a/apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go +++ b/apis/namespaced/instance/v1alpha1/zz_generated.deepcopy.go @@ -3166,6 +3166,31 @@ func (in *ServiceAccount) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ServiceAccountBaselinePermissionsObservation) DeepCopyInto(out *ServiceAccountBaselinePermissionsObservation) { + *out = *in + if in.MissingMemberShipGroups != nil { + in, out := &in.MissingMemberShipGroups, &out.MissingMemberShipGroups + *out = make([]int64, len(*in)) + copy(*out, *in) + } + if in.WrongPermissionsGroups != nil { + in, out := &in.WrongPermissionsGroups, &out.WrongPermissionsGroups + *out = make([]int64, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountBaselinePermissionsObservation. +func (in *ServiceAccountBaselinePermissionsObservation) DeepCopy() *ServiceAccountBaselinePermissionsObservation { + if in == nil { + return nil + } + out := new(ServiceAccountBaselinePermissionsObservation) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ServiceAccountList) DeepCopyInto(out *ServiceAccountList) { *out = *in @@ -3202,6 +3227,7 @@ func (in *ServiceAccountList) DeepCopyObject() runtime.Object { func (in *ServiceAccountObservation) DeepCopyInto(out *ServiceAccountObservation) { *out = *in out.CommonServiceAccountObservation = in.CommonServiceAccountObservation + in.ServiceAccountBaselinePermissionsObservation.DeepCopyInto(&out.ServiceAccountBaselinePermissionsObservation) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountObservation. @@ -3218,6 +3244,16 @@ func (in *ServiceAccountObservation) DeepCopy() *ServiceAccountObservation { func (in *ServiceAccountParameters) DeepCopyInto(out *ServiceAccountParameters) { *out = *in in.CommonServiceAccountParameters.DeepCopyInto(&out.CommonServiceAccountParameters) + if in.Admin != nil { + in, out := &in.Admin, &out.Admin + *out = new(bool) + **out = **in + } + if in.BaselinePermissions != nil { + in, out := &in.BaselinePermissions, &out.BaselinePermissions + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountParameters. @@ -3251,7 +3287,7 @@ func (in *ServiceAccountSpec) DeepCopy() *ServiceAccountSpec { func (in *ServiceAccountStatus) DeepCopyInto(out *ServiceAccountStatus) { *out = *in in.ResourceStatus.DeepCopyInto(&out.ResourceStatus) - out.AtProvider = in.AtProvider + in.AtProvider.DeepCopyInto(&out.AtProvider) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ServiceAccountStatus. diff --git a/examples/instance/serviceaccount.yaml b/examples/instance/serviceaccount.yaml index e26e3920..33d2ed17 100644 --- a/examples/instance/serviceaccount.yaml +++ b/examples/instance/serviceaccount.yaml @@ -10,6 +10,8 @@ spec: kind: ProviderConfig forProvider: name: Example Service Account - username: example-service-account + username: example-service-account-2 # WARNING: field is immutable after creation # email: example@example.com + admin: true + baselinePermissions: developer diff --git a/package/crds/groups.gitlab.crossplane.io_serviceaccounts.yaml b/package/crds/groups.gitlab.crossplane.io_serviceaccounts.yaml index b75661c6..1e373204 100644 --- a/package/crds/groups.gitlab.crossplane.io_serviceaccounts.yaml +++ b/package/crds/groups.gitlab.crossplane.io_serviceaccounts.yaml @@ -258,6 +258,10 @@ spec: atProvider: description: Represents the observed state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean email: description: Email represents the email of the service account. type: string @@ -273,6 +277,7 @@ spec: account. type: string required: + - admin - email - id - name diff --git a/package/crds/groups.gitlab.m.crossplane.io_serviceaccounts.yaml b/package/crds/groups.gitlab.m.crossplane.io_serviceaccounts.yaml index b1d0d8df..c9778ced 100644 --- a/package/crds/groups.gitlab.m.crossplane.io_serviceaccounts.yaml +++ b/package/crds/groups.gitlab.m.crossplane.io_serviceaccounts.yaml @@ -222,6 +222,10 @@ spec: atProvider: description: Represents the observed state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean email: description: Email represents the email of the service account. type: string @@ -237,6 +241,7 @@ spec: account. type: string required: + - admin - email - id - name diff --git a/package/crds/instance.gitlab.crossplane.io_serviceaccounts.yaml b/package/crds/instance.gitlab.crossplane.io_serviceaccounts.yaml index 659df92c..28ab2614 100644 --- a/package/crds/instance.gitlab.crossplane.io_serviceaccounts.yaml +++ b/package/crds/instance.gitlab.crossplane.io_serviceaccounts.yaml @@ -73,6 +73,26 @@ spec: forProvider: description: Defines the desired state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean + baselinePermissions: + description: |- + BaselinePermissions represents the minimal permissions level for all top level groups. + WARNING: If this field is set to a value other than "no-access", the service account will be added to all groups with at least the specified access level. This can lead to unintended consequences if not used carefully. + WARNING: This DOES NOT remove the service account from groups if changed from a higher access level to a lower access level. It only adds the service account to groups if changed from a lower access level to a higher access level. + enum: + - no-access + - minimal-access + - guest + - planner + - reporter + - security-manager + - developer + - maintainer + - owner + type: string email: description: email represents the email of the service account. type: string @@ -177,6 +197,10 @@ spec: atProvider: description: Represents the observed state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean email: description: Email represents the email of the service account. type: string @@ -184,6 +208,11 @@ spec: description: ID is the unique identifier of the service account. format: int64 type: integer + missingMembershipGroups: + items: + format: int64 + type: integer + type: array name: description: Name represents the display name of the service account. type: string @@ -191,7 +220,13 @@ spec: description: Username represents the user @ name of the service account. type: string + wrongPermissionsGroups: + items: + format: int64 + type: integer + type: array required: + - admin - email - id - name diff --git a/package/crds/instance.gitlab.m.crossplane.io_serviceaccounts.yaml b/package/crds/instance.gitlab.m.crossplane.io_serviceaccounts.yaml index 944d7d23..cdcd289e 100644 --- a/package/crds/instance.gitlab.m.crossplane.io_serviceaccounts.yaml +++ b/package/crds/instance.gitlab.m.crossplane.io_serviceaccounts.yaml @@ -59,6 +59,26 @@ spec: forProvider: description: Defines the desired state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean + baselinePermissions: + description: |- + BaselinePermissions represents the minimal permissions level for all top level groups. + WARNING: If this field is set to a value other than "no-access", the service account will be added to all groups with at least the specified access level. This can lead to unintended consequences if not used carefully. + WARNING: This DOES NOT remove the service account from groups if changed from a higher access level to a lower access level. It only adds the service account to groups if changed from a lower access level to a higher access level. + enum: + - no-access + - minimal-access + - guest + - planner + - reporter + - security-manager + - developer + - maintainer + - owner + type: string email: description: email represents the email of the service account. type: string @@ -135,6 +155,10 @@ spec: atProvider: description: Represents the observed state of the ServiceAccount. properties: + admin: + description: Admin represents whether the service account has + admin privileges. + type: boolean email: description: Email represents the email of the service account. type: string @@ -142,6 +166,11 @@ spec: description: ID is the unique identifier of the service account. format: int64 type: integer + missingMembershipGroups: + items: + format: int64 + type: integer + type: array name: description: Name represents the display name of the service account. type: string @@ -149,7 +178,13 @@ spec: description: Username represents the user @ name of the service account. type: string + wrongPermissionsGroups: + items: + format: int64 + type: integer + type: array required: + - admin - email - id - name diff --git a/pkg/cluster/clients/groups/fake/zz_fake.go b/pkg/cluster/clients/groups/fake/zz_fake.go index c957523b..ded4c1e9 100644 --- a/pkg/cluster/clients/groups/fake/zz_fake.go +++ b/pkg/cluster/clients/groups/fake/zz_fake.go @@ -36,6 +36,7 @@ type MockClient struct { MockDeleteGroup func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) MockShareGroupWithGroup func(gid interface{}, opt *gitlab.ShareGroupWithGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Group, *gitlab.Response, error) MockUnshareGroupFromGroup func(gid interface{}, groupID int64, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + MockListGroups func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) MockGetMember func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) MockAddMember func(gid interface{}, opt *gitlab.AddGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) @@ -97,6 +98,11 @@ func (c *MockClient) UnshareGroupFromGroup(gid interface{}, groupID int64, optio return c.MockUnshareGroupFromGroup(gid, groupID, options...) } +// ListGroups calls the underlying MockListGroups method. +func (c *MockClient) ListGroups(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + return c.MockListGroups(opt, options...) +} + // GetGroupMember calls the underlying MockGetMember method. func (c *MockClient) GetGroupMember(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { return c.MockGetMember(gid, user) diff --git a/pkg/cluster/clients/groups/zz_group.go b/pkg/cluster/clients/groups/zz_group.go index 1f645054..3c1d8d91 100644 --- a/pkg/cluster/clients/groups/zz_group.go +++ b/pkg/cluster/clients/groups/zz_group.go @@ -41,6 +41,7 @@ type Client interface { DeleteGroup(gid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) ShareGroupWithGroup(gid interface{}, opt *gitlab.ShareGroupWithGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Group, *gitlab.Response, error) UnshareGroupFromGroup(gid interface{}, groupID int64, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + ListGroups(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) } // NewGroupClient returns a new Gitlab Group service diff --git a/pkg/cluster/clients/groups/zz_serviceaccount.go b/pkg/cluster/clients/groups/zz_serviceaccount.go index b3163ceb..f192113f 100644 --- a/pkg/cluster/clients/groups/zz_serviceaccount.go +++ b/pkg/cluster/clients/groups/zz_serviceaccount.go @@ -60,6 +60,7 @@ func GenerateServiceAccountObservationFromUser(u *gitlab.User) v1alpha1.ServiceA Name: u.Name, Username: u.Username, Email: u.Email, + Admin: u.IsAdmin, }, } } diff --git a/pkg/cluster/clients/instance/zz_serviceaccount.go b/pkg/cluster/clients/instance/zz_serviceaccount.go index ea379b8e..8b798c31 100644 --- a/pkg/cluster/clients/instance/zz_serviceaccount.go +++ b/pkg/cluster/clients/instance/zz_serviceaccount.go @@ -49,6 +49,7 @@ func GenerateServiceAccountObservation(u *gitlab.User) v1alpha1.ServiceAccountOb Name: u.Name, Username: u.Username, Email: u.Email, + Admin: u.IsAdmin, }, } } @@ -59,6 +60,7 @@ func GenerateUpdateServiceAccountOptions(p *v1alpha1.ServiceAccountParameters) * Name: p.Name, Username: p.Username, Email: p.Email, + Admin: p.Admin, } } @@ -81,17 +83,8 @@ func IsServiceAccountUpToDate(p *v1alpha1.ServiceAccountParameters, u *gitlab.Us return false } - if !clients.IsComparableEqualToComparablePtr(p.Name, u.Name) { - return false - } - - if !clients.IsComparableEqualToComparablePtr(p.Username, u.Username) { - return false - } - - if !clients.IsComparableEqualToComparablePtr(p.Email, u.Email) { - return false - } - - return true + return clients.IsComparableEqualToComparablePtr(p.Name, u.Name) && + clients.IsComparableEqualToComparablePtr(p.Username, u.Username) && + clients.IsComparableEqualToComparablePtr(p.Email, u.Email) && + clients.IsComparableEqualToComparablePtr(p.Admin, u.IsAdmin) } diff --git a/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go b/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go index cd01161a..dd33d36f 100644 --- a/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go +++ b/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go @@ -21,6 +21,7 @@ package serviceaccounts import ( "context" "strconv" + "sync" xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1" "github.com/crossplane/crossplane-runtime/v2/pkg/controller" @@ -32,11 +33,13 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/resource" "github.com/crossplane/crossplane-runtime/v2/pkg/statemetrics" gitlab "gitlab.com/gitlab-org/api/client-go" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane-contrib/provider-gitlab/apis/cluster/instance/v1alpha1" "github.com/crossplane-contrib/provider-gitlab/pkg/cluster/clients" + "github.com/crossplane-contrib/provider-gitlab/pkg/cluster/clients/groups" "github.com/crossplane-contrib/provider-gitlab/pkg/cluster/clients/instance" "github.com/crossplane-contrib/provider-gitlab/pkg/common" ) @@ -48,6 +51,25 @@ const ( errUpdateFailed = "cannot update Gitlab service account" errDeleteFailed = "cannot delete Gitlab service account" errIDNotInt = "specified ID is not an integer" + + accessLevelNoAccess = "no-access" + accessLevelNoAccessValue = 0 + accessLevelMinimalAccess = "minimal-access" + accessLevelMinimalAccessValue = 5 + accessLevelGuest = "guest" + accessLevelGuestValue = 10 + accessLevelPlanner = "planner" + accessLevelPlannerValue = 15 + accessLevelReporter = "reporter" + accessLevelReporterValue = 20 + accessLevelSecurityManager = "security-manager" + accessLevelSecurityManagerValue = 25 + accessLevelDeveloper = "developer" + accessLevelDeveloperValue = 30 + accessLevelMaintainer = "maintainer" + accessLevelMaintainerValue = 40 + accessLevelOwner = "owner" + accessLevelOwnerValue = 50 ) // SetupServiceAccount adds a controller that reconciles GitLab Service Accounts. @@ -109,13 +131,20 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E if err != nil { return nil, err } - return &external{kube: c.kube, client: c.newGitlabClientFn(*cfg)}, nil + return &external{ + kube: c.kube, + client: c.newGitlabClientFn(*cfg), + groupsClient: groups.NewGroupClient(*cfg), + groupMemberClient: groups.NewMemberClient(*cfg), + }, nil } // external represents the external client for Gitlab Service Accounts type external struct { - kube client.Client - client instance.ServiceAccountClient + kube client.Client + client instance.ServiceAccountClient + groupsClient groups.Client + groupMemberClient groups.MemberClient } // Observe checks if the Gitlab Service Account external resource exists and whether @@ -147,9 +176,26 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) cr.Status.SetConditions(xpv1.Available()) + minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) + + groupsUpToDate := true + + if minPermission != accessLevelNoAccess { + minPermissionValue := getAccessLevelValue(minPermission) + + notInGroups, wrongPermsGroups, err := e.fetchTopLevelGroupsMissingPermissions(minPermissionValue, serviceAccount.ID) + if err != nil { + return managed.ExternalObservation{}, errors.Wrap(err, "cannot fetch Gitlab groups missing permissions for service account") + } + + groupsUpToDate = len(notInGroups) == 0 && len(wrongPermsGroups) == 0 + cr.Status.AtProvider.MissingMemberShipGroups = notInGroups + cr.Status.AtProvider.WrongPermissionsGroups = wrongPermsGroups + } + return managed.ExternalObservation{ ResourceExists: true, - ResourceUpToDate: instance.IsServiceAccountUpToDate(&cr.Spec.ForProvider, serviceAccount), + ResourceUpToDate: instance.IsServiceAccountUpToDate(&cr.Spec.ForProvider, serviceAccount) && groupsUpToDate, ResourceLateInitialized: false, }, nil } @@ -179,6 +225,8 @@ func (e *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext } // Update updates the external resource to match the desired state. +// +//nolint:gocyclo func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.ExternalUpdate, error) { cr, ok := mg.(*v1alpha1.ServiceAccount) if !ok { @@ -196,19 +244,60 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext if err != nil { return managed.ExternalUpdate{}, errors.New(errIDNotInt) } + updateWaitGroup := sync.WaitGroup{} + errorsChannel := make(chan error, 3) // buffer size of 3 to avoid blocking if all operations fail + + // Reconcile service account fields + updateWaitGroup.Go(func() { + serviceAccount, _, err := e.client.ModifyUser( + serviceAccountID, + instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), + gitlab.WithContext(ctx)) + if err != nil { + errorsChannel <- errors.Wrap(err, errUpdateFailed) + return + } - // Call GitLab Users API - serviceAccount, _, err := e.client.ModifyUser( - serviceAccountID, - instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), - gitlab.WithContext(ctx)) - if err != nil { - return managed.ExternalUpdate{}, errors.Wrap(err, errUpdateFailed) - } + cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) + }) - cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) + minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) + minPermissionValue := getAccessLevelValue(minPermission) + + // Reconcile service account group memberships + updateWaitGroup.Go(func() { + if cr.Status.AtProvider.MissingMemberShipGroups != nil { + + errorsChannel <- e.addServiceAccountToGroups(ctx, serviceAccountID, cr.Status.AtProvider.MissingMemberShipGroups, minPermissionValue) + } + }) + + // Reconcile service account group permissions + updateWaitGroup.Go(func() { + if cr.Status.AtProvider.WrongPermissionsGroups != nil { - return managed.ExternalUpdate{}, nil + errorsChannel <- e.updateServiceAccountGroupPermissions(ctx, serviceAccountID, cr.Status.AtProvider.WrongPermissionsGroups, minPermissionValue) + } + }) + + updateWaitGroup.Wait() + close(errorsChannel) + + errorsSlice := make([]error, 0) + for err := range errorsChannel { + if err != nil { + errorsSlice = append(errorsSlice, err) + } + } + + switch len(errorsSlice) { + case 0: + return managed.ExternalUpdate{}, nil + case 1: + return managed.ExternalUpdate{}, errorsSlice[0] + default: + return managed.ExternalUpdate{}, errors.Join(errorsSlice...) + } } // Delete removes the service account resource. diff --git a/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers.go b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers.go new file mode 100644 index 00000000..fb794544 --- /dev/null +++ b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers.go @@ -0,0 +1,235 @@ +/* +Copyright 2021 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Code generated by hack/generate-cluster-scope.go - DO NOT EDIT. + +package serviceaccounts + +import ( + "context" + "sync" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + gitlab "gitlab.com/gitlab-org/api/client-go" + "k8s.io/utils/ptr" + + "github.com/crossplane-contrib/provider-gitlab/pkg/cluster/clients" +) + +// addServiceAccountToGroups is a helper function that adds the service account to the specified groups with the specified access level. This is used to satisfy the BaselinePermissions field by adding the service account to all groups that have at least the specified access level. +func (e *external) addServiceAccountToGroups(ctx context.Context, serviceAccountID int64, groupIDs []int64, accessLevel int) error { + var addToGroupsWaitGroup sync.WaitGroup + var requestErr error + var requestErrMu sync.Mutex + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for _, groupID := range groupIDs { + addToGroupsWaitGroup.Add(1) + go func(groupID int64) { + defer addToGroupsWaitGroup.Done() + + _, _, err := e.groupMemberClient.AddGroupMember( + groupID, + &gitlab.AddGroupMemberOptions{ + UserID: ptr.To(serviceAccountID), + AccessLevel: ptr.To(gitlab.AccessLevelValue(accessLevel)), + }, + gitlab.WithContext(ctx), + ) + if err != nil { + setRequestErr(errors.Wrapf(err, "cannot add service account to Gitlab group with ID %d", groupID)) + return + } + }(groupID) + } + + addToGroupsWaitGroup.Wait() + return requestErr +} + +// updateServiceAccountGroupPermissions is a helper function that updates the service account's permissions for the specified groups to the specified access level. This is used to satisfy the BaselinePermissions field by updating the service account's permissions for all groups that have at least the specified access level. +func (e *external) updateServiceAccountGroupPermissions(ctx context.Context, serviceAccountID int64, groupIDs []int64, accessLevel int) error { + var updateGroupsWaitGroup sync.WaitGroup + var requestErr error + var requestErrMu sync.Mutex + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for _, groupID := range groupIDs { + updateGroupsWaitGroup.Add(1) + go func(groupID int64) { + defer updateGroupsWaitGroup.Done() + + _, _, err := e.groupMemberClient.EditGroupMember( + groupID, + serviceAccountID, + &gitlab.EditGroupMemberOptions{ + AccessLevel: ptr.To(gitlab.AccessLevelValue(accessLevel)), + }, + gitlab.WithContext(ctx), + ) + if err != nil { + setRequestErr(errors.Wrapf(err, "cannot update service account permissions for Gitlab group with ID %d", groupID)) + return + } + }(groupID) + } + + updateGroupsWaitGroup.Wait() + return requestErr +} + +// fetchTopLevelGroupsMissingPermissions is a helper function that returns a list of top level group IDs that the service account is missing permissions for, based on the specified minimum permission level. This is used to determine which groups the service account needs to be added to in order to satisfy the BaselinePermissions field. +// +//nolint:gocyclo +func (e *external) fetchTopLevelGroupsMissingPermissions(minPermission int, serviceAccountID int64) (notInGroups []int64, wrongPermsGroups []int64, err error) { + var ( + permissionsCheckWaitGroup sync.WaitGroup + groupsMissingPermissionsMu sync.Mutex + requestErr error + requestErrMu sync.Mutex + ) + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for page := int64(1); ; { + groups, resp, err := e.fetchTopLevelGroupsPage(page) + if err != nil { + return nil, nil, err + } + + for _, group := range groups { + permissionsCheckWaitGroup.Add(1) + go func(groupID int64) { + defer permissionsCheckWaitGroup.Done() + + isMissingMembership, hasInsufficientPermissions, err := e.getGroupPermissionStatus(groupID, serviceAccountID, minPermission) + if err != nil { + setRequestErr(err) + return + } + + groupsMissingPermissionsMu.Lock() + if isMissingMembership { + notInGroups = append(notInGroups, groupID) + } + if hasInsufficientPermissions { + wrongPermsGroups = append(wrongPermsGroups, groupID) + } + groupsMissingPermissionsMu.Unlock() + }(group.ID) + } + + if resp.NextPage == 0 { + break + } + page = resp.NextPage + } + + permissionsCheckWaitGroup.Wait() + if requestErr != nil { + return nil, nil, requestErr + } + + return notInGroups, wrongPermsGroups, nil +} + +func (e *external) fetchTopLevelGroupsPage(page int64) ([]*gitlab.Group, *gitlab.Response, error) { + groups, resp, err := e.groupsClient.ListGroups(&gitlab.ListGroupsOptions{ + TopLevelOnly: ptr.To(true), + ListOptions: gitlab.ListOptions{ + PerPage: 100, + Page: page, + }, + }) + if err != nil { + return nil, nil, errors.Wrap(err, "cannot list Gitlab groups") + } + if resp == nil { + return nil, nil, errors.New("no response received when listing Gitlab groups") + } + + return groups, resp, nil +} + +func (e *external) getGroupPermissionStatus(groupID int64, serviceAccountID int64, minPermission int) (isMissingMembership bool, hasInsufficientPermissions bool, err error) { + groupMember, resp, err := e.groupMemberClient.GetGroupMember(groupID, serviceAccountID) + if err != nil { + if clients.IsResponseNotFound(resp) { + return true, false, nil + } + return false, false, errors.Wrap(err, "cannot check Gitlab group member") + } + + if groupMember == nil { + return false, true, nil + } + + if int(groupMember.AccessLevel) < minPermission { + return false, true, nil + } + + return false, false, nil +} + +// getAccessLevelValue is a helper function that returns the integer value of the given access level string. This is used to convert the BaselinePermissions field from a string to an integer that can be used with the Gitlab API. +func getAccessLevelValue(accessLevel string) int { + accessLevelsMapping := map[string]int{ + accessLevelNoAccess: accessLevelNoAccessValue, + accessLevelMinimalAccess: accessLevelMinimalAccessValue, + accessLevelGuest: accessLevelGuestValue, + accessLevelPlanner: accessLevelPlannerValue, + accessLevelReporter: accessLevelReporterValue, + accessLevelSecurityManager: accessLevelSecurityManagerValue, + accessLevelDeveloper: accessLevelDeveloperValue, + accessLevelMaintainer: accessLevelMaintainerValue, + accessLevelOwner: accessLevelOwnerValue, + } + + value, ok := accessLevelsMapping[accessLevel] + if !ok { + return accessLevelNoAccessValue + } + return value +} diff --git a/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers_test.go b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers_test.go new file mode 100644 index 00000000..a6de926b --- /dev/null +++ b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_helpers_test.go @@ -0,0 +1,297 @@ +/* +Copyright 2021 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Code generated by hack/generate-cluster-scope.go - DO NOT EDIT. + +package serviceaccounts + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + + xerrors "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + gitlab "gitlab.com/gitlab-org/api/client-go" + + groupsfake "github.com/crossplane-contrib/provider-gitlab/pkg/cluster/clients/groups/fake" +) + +func TestGetAccessLevelValue(t *testing.T) { + tests := map[string]struct { + accessLevel string + want int + }{ + "NoAccess": {accessLevel: accessLevelNoAccess, want: accessLevelNoAccessValue}, + "MinimalAccess": {accessLevel: accessLevelMinimalAccess, want: accessLevelMinimalAccessValue}, + "Guest": {accessLevel: accessLevelGuest, want: accessLevelGuestValue}, + "Planner": {accessLevel: accessLevelPlanner, want: accessLevelPlannerValue}, + "Reporter": {accessLevel: accessLevelReporter, want: accessLevelReporterValue}, + "SecurityManager": {accessLevel: accessLevelSecurityManager, want: accessLevelSecurityManagerValue}, + "Developer": {accessLevel: accessLevelDeveloper, want: accessLevelDeveloperValue}, + "Maintainer": {accessLevel: accessLevelMaintainer, want: accessLevelMaintainerValue}, + "Owner": {accessLevel: accessLevelOwner, want: accessLevelOwnerValue}, + "UnknownDefaultsToNoAccess": {accessLevel: "unknown", want: accessLevelNoAccessValue}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := getAccessLevelValue(tc.accessLevel) + if got != tc.want { + t.Fatalf("getAccessLevelValue(%q) = %d, want %d", tc.accessLevel, got, tc.want) + } + }) + } +} + +func TestFetchTopLevelGroupsPage(t *testing.T) { + client := &groupsfake.MockClient{ + MockListGroups: func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + if opt.TopLevelOnly == nil || !*opt.TopLevelOnly { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups without TopLevelOnly") + } + if opt.PerPage != 100 { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups with PerPage = %d, want 100", opt.PerPage) + } + if opt.Page != 2 { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups with Page = %d, want 2", opt.Page) + } + return []*gitlab.Group{{ID: 123}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 3}, nil + }, + } + + e := &external{groupsClient: client} + + got, resp, err := e.fetchTopLevelGroupsPage(2) + if err != nil { + t.Fatalf("fetchTopLevelGroupsPage() unexpected error: %v", err) + } + if resp == nil { + t.Fatal("fetchTopLevelGroupsPage() returned nil response") + } + if resp.NextPage != 3 { + t.Fatalf("fetchTopLevelGroupsPage() NextPage = %d, want 3", resp.NextPage) + } + if diff := cmp.Diff([]int64{123}, []int64{got[0].ID}); diff != "" { + t.Fatalf("fetchTopLevelGroupsPage() unexpected groups (-want +got):\n%s", diff) + } +} + +func TestGetGroupPermissionStatus(t *testing.T) { + tests := map[string]struct { + memberClient *groupsfake.MockClient + wantMissing bool + wantWrong bool + wantErr error + }{ + "NotFound": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusNotFound}}, errors.New("not found") + }}, + wantMissing: true, + }, + "NilMember": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + wantWrong: true, + }, + "LowAccess": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelReporterValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + wantWrong: true, + }, + "EnoughAccess": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelMaintainerValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + }, + "UnexpectedError": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + }}, + wantErr: xerrors.New("cannot check Gitlab group member: boom"), + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + e := &external{groupMemberClient: tc.memberClient} + missing, wrong, err := e.getGroupPermissionStatus(42, 99, accessLevelDeveloperValue) + if tc.wantErr == nil { + if err != nil { + t.Fatalf("getGroupPermissionStatus() unexpected error: %v", err) + } + } else if err == nil || err.Error() != tc.wantErr.Error() { + t.Fatalf("getGroupPermissionStatus() error = %v, want %v", err, tc.wantErr) + } + if missing != tc.wantMissing { + t.Fatalf("getGroupPermissionStatus() missing = %v, want %v", missing, tc.wantMissing) + } + if wrong != tc.wantWrong { + t.Fatalf("getGroupPermissionStatus() wrong = %v, want %v", wrong, tc.wantWrong) + } + }) + } +} + +func TestFetchTopLevelGroupsMissingPermissions(t *testing.T) { + var ( + pagesMu sync.Mutex + pages []int64 + ) + + client := &groupsfake.MockClient{ + MockListGroups: func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + pagesMu.Lock() + pages = append(pages, opt.Page) + pagesMu.Unlock() + + switch opt.Page { + case 1: + return []*gitlab.Group{{ID: 1}, {ID: 2}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 2}, nil + case 2: + return []*gitlab.Group{{ID: 3}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 0}, nil + default: + t.Fatalf("unexpected page %d", opt.Page) + return nil, nil, nil + } + }, + MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + switch gid.(int64) { + case 1: + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusNotFound}}, errors.New("not found") + case 2: + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + case 3: + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelReporterValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + default: + t.Fatalf("unexpected group id %v", gid) + return nil, nil, nil + } + }, + } + + e := &external{groupsClient: client, groupMemberClient: client} + + notIn, wrong, err := e.fetchTopLevelGroupsMissingPermissions(accessLevelDeveloperValue, 99) + if err != nil { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() unexpected error: %v", err) + } + if diff := cmp.Diff([]int64{1}, notIn, cmpopts.SortSlices(func(a, b int64) bool { return a < b })); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() notInGroups mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff([]int64{2, 3}, wrong, cmpopts.SortSlices(func(a, b int64) bool { return a < b })); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() wrongPermsGroups mismatch (-want +got):\n%s", diff) + } + + pagesMu.Lock() + defer pagesMu.Unlock() + if diff := cmp.Diff([]int64{1, 2}, pages); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() page sequence mismatch (-want +got):\n%s", diff) + } +} + +func TestAddServiceAccountToGroups(t *testing.T) { + var ( + callsMu sync.Mutex + calls = map[int64]*gitlab.AddGroupMemberOptions{} + ) + + client := &groupsfake.MockClient{ + MockAddMember: func(gid interface{}, opt *gitlab.AddGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + groupID := gid.(int64) + callsMu.Lock() + calls[groupID] = opt + callsMu.Unlock() + + if groupID == 12 { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + } + + return &gitlab.GroupMember{}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusCreated}}, nil + }, + } + + e := &external{groupMemberClient: client} + err := e.addServiceAccountToGroups(context.Background(), 99, []int64{11, 12}, accessLevelMaintainerValue) + if err == nil { + t.Fatal("addServiceAccountToGroups() expected an error") + } + + callsMu.Lock() + defer callsMu.Unlock() + if len(calls) != 2 { + t.Fatalf("addServiceAccountToGroups() called AddGroupMember %d times, want 2", len(calls)) + } + for groupID, call := range calls { + if call == nil { + t.Fatalf("addServiceAccountToGroups() nil call for group %d", groupID) + } + if call.UserID == nil || *call.UserID != 99 { + t.Fatalf("addServiceAccountToGroups() UserID for group %d = %v, want 99", groupID, call.UserID) + } + if call.AccessLevel == nil || int(*call.AccessLevel) != accessLevelMaintainerValue { + t.Fatalf("addServiceAccountToGroups() AccessLevel for group %d = %v, want %d", groupID, call.AccessLevel, accessLevelMaintainerValue) + } + } +} + +func TestUpdateServiceAccountGroupPermissions(t *testing.T) { + var ( + callsMu sync.Mutex + calls = map[int64]*gitlab.EditGroupMemberOptions{} + ) + + client := &groupsfake.MockClient{ + MockEditMember: func(gid interface{}, user int64, opt *gitlab.EditGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + groupID := gid.(int64) + callsMu.Lock() + calls[groupID] = opt + callsMu.Unlock() + + if groupID == 12 { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + } + + return &gitlab.GroupMember{}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }, + } + + e := &external{groupMemberClient: client} + err := e.updateServiceAccountGroupPermissions(context.Background(), 99, []int64{11, 12}, accessLevelMaintainerValue) + if err == nil { + t.Fatal("updateServiceAccountGroupPermissions() expected an error") + } + + callsMu.Lock() + defer callsMu.Unlock() + if len(calls) != 2 { + t.Fatalf("updateServiceAccountGroupPermissions() called EditGroupMember %d times, want 2", len(calls)) + } + for groupID, call := range calls { + if call == nil { + t.Fatalf("updateServiceAccountGroupPermissions() nil call for group %d", groupID) + } + if call.AccessLevel == nil || int(*call.AccessLevel) != accessLevelMaintainerValue { + t.Fatalf("updateServiceAccountGroupPermissions() AccessLevel for group %d = %v, want %d", groupID, call.AccessLevel, accessLevelMaintainerValue) + } + } +} diff --git a/pkg/cluster/controller/instance/serviceaccounts/zz_controller_test.go b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_test.go index 76545c58..8e78d19b 100644 --- a/pkg/cluster/controller/instance/serviceaccounts/zz_controller_test.go +++ b/pkg/cluster/controller/instance/serviceaccounts/zz_controller_test.go @@ -52,8 +52,7 @@ const ( testServiceAccountEmail = "sa@example.org" ) -// MockClient is a small, purpose-built mock for instance.ServiceAccountClient. -// We keep it local to this test file to avoid adding any non-test code. +// MockClient is a small mock for instance.ServiceAccountClient used by controller tests. type MockClient struct { MockGetUser func(user int64, opt gitlab.GetUsersOptions, options ...gitlab.RequestOptionFunc) (*gitlab.User, *gitlab.Response, error) MockCreateServiceAccountUser func(opts *gitlab.CreateServiceAccountUserOptions, options ...gitlab.RequestOptionFunc) (*gitlab.User, *gitlab.Response, error) diff --git a/pkg/namespaced/clients/groups/fake/fake.go b/pkg/namespaced/clients/groups/fake/fake.go index 6e50c5fd..5419cc52 100644 --- a/pkg/namespaced/clients/groups/fake/fake.go +++ b/pkg/namespaced/clients/groups/fake/fake.go @@ -34,6 +34,7 @@ type MockClient struct { MockDeleteGroup func(pid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) MockShareGroupWithGroup func(gid interface{}, opt *gitlab.ShareGroupWithGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Group, *gitlab.Response, error) MockUnshareGroupFromGroup func(gid interface{}, groupID int64, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + MockListGroups func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) MockGetMember func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) MockAddMember func(gid interface{}, opt *gitlab.AddGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) @@ -95,6 +96,11 @@ func (c *MockClient) UnshareGroupFromGroup(gid interface{}, groupID int64, optio return c.MockUnshareGroupFromGroup(gid, groupID, options...) } +// ListGroups calls the underlying MockListGroups method. +func (c *MockClient) ListGroups(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + return c.MockListGroups(opt, options...) +} + // GetGroupMember calls the underlying MockGetMember method. func (c *MockClient) GetGroupMember(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { return c.MockGetMember(gid, user) diff --git a/pkg/namespaced/clients/groups/group.go b/pkg/namespaced/clients/groups/group.go index b45cd676..16e3e861 100644 --- a/pkg/namespaced/clients/groups/group.go +++ b/pkg/namespaced/clients/groups/group.go @@ -39,6 +39,7 @@ type Client interface { DeleteGroup(gid interface{}, opt *gitlab.DeleteGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) ShareGroupWithGroup(gid interface{}, opt *gitlab.ShareGroupWithGroupOptions, options ...gitlab.RequestOptionFunc) (*gitlab.Group, *gitlab.Response, error) UnshareGroupFromGroup(gid interface{}, groupID int64, options ...gitlab.RequestOptionFunc) (*gitlab.Response, error) + ListGroups(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) } // NewGroupClient returns a new Gitlab Group service diff --git a/pkg/namespaced/clients/groups/serviceaccount.go b/pkg/namespaced/clients/groups/serviceaccount.go index 9ad82da6..358bfeaa 100644 --- a/pkg/namespaced/clients/groups/serviceaccount.go +++ b/pkg/namespaced/clients/groups/serviceaccount.go @@ -58,6 +58,7 @@ func GenerateServiceAccountObservationFromUser(u *gitlab.User) v1alpha1.ServiceA Name: u.Name, Username: u.Username, Email: u.Email, + Admin: u.IsAdmin, }, } } diff --git a/pkg/namespaced/clients/instance/serviceaccount.go b/pkg/namespaced/clients/instance/serviceaccount.go index 21cc5a55..2602e71a 100644 --- a/pkg/namespaced/clients/instance/serviceaccount.go +++ b/pkg/namespaced/clients/instance/serviceaccount.go @@ -47,6 +47,7 @@ func GenerateServiceAccountObservation(u *gitlab.User) v1alpha1.ServiceAccountOb Name: u.Name, Username: u.Username, Email: u.Email, + Admin: u.IsAdmin, }, } } @@ -57,6 +58,7 @@ func GenerateUpdateServiceAccountOptions(p *v1alpha1.ServiceAccountParameters) * Name: p.Name, Username: p.Username, Email: p.Email, + Admin: p.Admin, } } @@ -79,17 +81,8 @@ func IsServiceAccountUpToDate(p *v1alpha1.ServiceAccountParameters, u *gitlab.Us return false } - if !clients.IsComparableEqualToComparablePtr(p.Name, u.Name) { - return false - } - - if !clients.IsComparableEqualToComparablePtr(p.Username, u.Username) { - return false - } - - if !clients.IsComparableEqualToComparablePtr(p.Email, u.Email) { - return false - } - - return true + return clients.IsComparableEqualToComparablePtr(p.Name, u.Name) && + clients.IsComparableEqualToComparablePtr(p.Username, u.Username) && + clients.IsComparableEqualToComparablePtr(p.Email, u.Email) && + clients.IsComparableEqualToComparablePtr(p.Admin, u.IsAdmin) } diff --git a/pkg/namespaced/controller/instance/serviceaccounts/controller.go b/pkg/namespaced/controller/instance/serviceaccounts/controller.go index 82528b63..06ab2ccf 100644 --- a/pkg/namespaced/controller/instance/serviceaccounts/controller.go +++ b/pkg/namespaced/controller/instance/serviceaccounts/controller.go @@ -19,6 +19,7 @@ package serviceaccounts import ( "context" "strconv" + "sync" xpv1 "github.com/crossplane/crossplane-runtime/v2/apis/common/v1" "github.com/crossplane/crossplane-runtime/v2/pkg/controller" @@ -30,12 +31,14 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/resource" "github.com/crossplane/crossplane-runtime/v2/pkg/statemetrics" gitlab "gitlab.com/gitlab-org/api/client-go" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "github.com/crossplane-contrib/provider-gitlab/apis/namespaced/instance/v1alpha1" "github.com/crossplane-contrib/provider-gitlab/pkg/common" "github.com/crossplane-contrib/provider-gitlab/pkg/namespaced/clients" + "github.com/crossplane-contrib/provider-gitlab/pkg/namespaced/clients/groups" "github.com/crossplane-contrib/provider-gitlab/pkg/namespaced/clients/instance" ) @@ -46,6 +49,25 @@ const ( errUpdateFailed = "cannot update Gitlab service account" errDeleteFailed = "cannot delete Gitlab service account" errIDNotInt = "specified ID is not an integer" + + accessLevelNoAccess = "no-access" + accessLevelNoAccessValue = 0 + accessLevelMinimalAccess = "minimal-access" + accessLevelMinimalAccessValue = 5 + accessLevelGuest = "guest" + accessLevelGuestValue = 10 + accessLevelPlanner = "planner" + accessLevelPlannerValue = 15 + accessLevelReporter = "reporter" + accessLevelReporterValue = 20 + accessLevelSecurityManager = "security-manager" + accessLevelSecurityManagerValue = 25 + accessLevelDeveloper = "developer" + accessLevelDeveloperValue = 30 + accessLevelMaintainer = "maintainer" + accessLevelMaintainerValue = 40 + accessLevelOwner = "owner" + accessLevelOwnerValue = 50 ) // SetupServiceAccount adds a controller that reconciles GitLab Service Accounts. @@ -107,13 +129,20 @@ func (c *connector) Connect(ctx context.Context, mg resource.Managed) (managed.E if err != nil { return nil, err } - return &external{kube: c.kube, client: c.newGitlabClientFn(*cfg)}, nil + return &external{ + kube: c.kube, + client: c.newGitlabClientFn(*cfg), + groupsClient: groups.NewGroupClient(*cfg), + groupMemberClient: groups.NewMemberClient(*cfg), + }, nil } // external represents the external client for Gitlab Service Accounts type external struct { - kube client.Client - client instance.ServiceAccountClient + kube client.Client + client instance.ServiceAccountClient + groupsClient groups.Client + groupMemberClient groups.MemberClient } // Observe checks if the Gitlab Service Account external resource exists and whether @@ -145,9 +174,26 @@ func (e *external) Observe(ctx context.Context, mg resource.Managed) (managed.Ex cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) cr.Status.SetConditions(xpv1.Available()) + minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) + + groupsUpToDate := true + + if minPermission != accessLevelNoAccess { + minPermissionValue := getAccessLevelValue(minPermission) + + notInGroups, wrongPermsGroups, err := e.fetchTopLevelGroupsMissingPermissions(minPermissionValue, serviceAccount.ID) + if err != nil { + return managed.ExternalObservation{}, errors.Wrap(err, "cannot fetch Gitlab groups missing permissions for service account") + } + + groupsUpToDate = len(notInGroups) == 0 && len(wrongPermsGroups) == 0 + cr.Status.AtProvider.MissingMemberShipGroups = notInGroups + cr.Status.AtProvider.WrongPermissionsGroups = wrongPermsGroups + } + return managed.ExternalObservation{ ResourceExists: true, - ResourceUpToDate: instance.IsServiceAccountUpToDate(&cr.Spec.ForProvider, serviceAccount), + ResourceUpToDate: instance.IsServiceAccountUpToDate(&cr.Spec.ForProvider, serviceAccount) && groupsUpToDate, ResourceLateInitialized: false, }, nil } @@ -177,6 +223,8 @@ func (e *external) Create(ctx context.Context, mg resource.Managed) (managed.Ext } // Update updates the external resource to match the desired state. +// +//nolint:gocyclo func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.ExternalUpdate, error) { cr, ok := mg.(*v1alpha1.ServiceAccount) if !ok { @@ -194,19 +242,60 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext if err != nil { return managed.ExternalUpdate{}, errors.New(errIDNotInt) } + updateWaitGroup := sync.WaitGroup{} + errorsChannel := make(chan error, 3) // buffer size of 3 to avoid blocking if all operations fail + + // Reconcile service account fields + updateWaitGroup.Go(func() { + serviceAccount, _, err := e.client.ModifyUser( + serviceAccountID, + instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), + gitlab.WithContext(ctx)) + if err != nil { + errorsChannel <- errors.Wrap(err, errUpdateFailed) + return + } - // Call GitLab Users API - serviceAccount, _, err := e.client.ModifyUser( - serviceAccountID, - instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), - gitlab.WithContext(ctx)) - if err != nil { - return managed.ExternalUpdate{}, errors.Wrap(err, errUpdateFailed) - } + cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) + }) - cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) + minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) + minPermissionValue := getAccessLevelValue(minPermission) + + // Reconcile service account group memberships + updateWaitGroup.Go(func() { + if cr.Status.AtProvider.MissingMemberShipGroups != nil { + + errorsChannel <- e.addServiceAccountToGroups(ctx, serviceAccountID, cr.Status.AtProvider.MissingMemberShipGroups, minPermissionValue) + } + }) + + // Reconcile service account group permissions + updateWaitGroup.Go(func() { + if cr.Status.AtProvider.WrongPermissionsGroups != nil { - return managed.ExternalUpdate{}, nil + errorsChannel <- e.updateServiceAccountGroupPermissions(ctx, serviceAccountID, cr.Status.AtProvider.WrongPermissionsGroups, minPermissionValue) + } + }) + + updateWaitGroup.Wait() + close(errorsChannel) + + errorsSlice := make([]error, 0) + for err := range errorsChannel { + if err != nil { + errorsSlice = append(errorsSlice, err) + } + } + + switch len(errorsSlice) { + case 0: + return managed.ExternalUpdate{}, nil + case 1: + return managed.ExternalUpdate{}, errorsSlice[0] + default: + return managed.ExternalUpdate{}, errors.Join(errorsSlice...) + } } // Delete removes the service account resource. diff --git a/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers.go b/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers.go new file mode 100644 index 00000000..6addfade --- /dev/null +++ b/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers.go @@ -0,0 +1,233 @@ +/* +Copyright 2021 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package serviceaccounts + +import ( + "context" + "sync" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + gitlab "gitlab.com/gitlab-org/api/client-go" + "k8s.io/utils/ptr" + + "github.com/crossplane-contrib/provider-gitlab/pkg/namespaced/clients" +) + +// addServiceAccountToGroups is a helper function that adds the service account to the specified groups with the specified access level. This is used to satisfy the BaselinePermissions field by adding the service account to all groups that have at least the specified access level. +func (e *external) addServiceAccountToGroups(ctx context.Context, serviceAccountID int64, groupIDs []int64, accessLevel int) error { + var addToGroupsWaitGroup sync.WaitGroup + var requestErr error + var requestErrMu sync.Mutex + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for _, groupID := range groupIDs { + addToGroupsWaitGroup.Add(1) + go func(groupID int64) { + defer addToGroupsWaitGroup.Done() + + _, _, err := e.groupMemberClient.AddGroupMember( + groupID, + &gitlab.AddGroupMemberOptions{ + UserID: ptr.To(serviceAccountID), + AccessLevel: ptr.To(gitlab.AccessLevelValue(accessLevel)), + }, + gitlab.WithContext(ctx), + ) + if err != nil { + setRequestErr(errors.Wrapf(err, "cannot add service account to Gitlab group with ID %d", groupID)) + return + } + }(groupID) + } + + addToGroupsWaitGroup.Wait() + return requestErr +} + +// updateServiceAccountGroupPermissions is a helper function that updates the service account's permissions for the specified groups to the specified access level. This is used to satisfy the BaselinePermissions field by updating the service account's permissions for all groups that have at least the specified access level. +func (e *external) updateServiceAccountGroupPermissions(ctx context.Context, serviceAccountID int64, groupIDs []int64, accessLevel int) error { + var updateGroupsWaitGroup sync.WaitGroup + var requestErr error + var requestErrMu sync.Mutex + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for _, groupID := range groupIDs { + updateGroupsWaitGroup.Add(1) + go func(groupID int64) { + defer updateGroupsWaitGroup.Done() + + _, _, err := e.groupMemberClient.EditGroupMember( + groupID, + serviceAccountID, + &gitlab.EditGroupMemberOptions{ + AccessLevel: ptr.To(gitlab.AccessLevelValue(accessLevel)), + }, + gitlab.WithContext(ctx), + ) + if err != nil { + setRequestErr(errors.Wrapf(err, "cannot update service account permissions for Gitlab group with ID %d", groupID)) + return + } + }(groupID) + } + + updateGroupsWaitGroup.Wait() + return requestErr +} + +// fetchTopLevelGroupsMissingPermissions is a helper function that returns a list of top level group IDs that the service account is missing permissions for, based on the specified minimum permission level. This is used to determine which groups the service account needs to be added to in order to satisfy the BaselinePermissions field. +// +//nolint:gocyclo +func (e *external) fetchTopLevelGroupsMissingPermissions(minPermission int, serviceAccountID int64) (notInGroups []int64, wrongPermsGroups []int64, err error) { + var ( + permissionsCheckWaitGroup sync.WaitGroup + groupsMissingPermissionsMu sync.Mutex + requestErr error + requestErrMu sync.Mutex + ) + + setRequestErr := func(err error) { + if err == nil { + return + } + requestErrMu.Lock() + if requestErr == nil { + requestErr = err + } + requestErrMu.Unlock() + } + + for page := int64(1); ; { + groups, resp, err := e.fetchTopLevelGroupsPage(page) + if err != nil { + return nil, nil, err + } + + for _, group := range groups { + permissionsCheckWaitGroup.Add(1) + go func(groupID int64) { + defer permissionsCheckWaitGroup.Done() + + isMissingMembership, hasInsufficientPermissions, err := e.getGroupPermissionStatus(groupID, serviceAccountID, minPermission) + if err != nil { + setRequestErr(err) + return + } + + groupsMissingPermissionsMu.Lock() + if isMissingMembership { + notInGroups = append(notInGroups, groupID) + } + if hasInsufficientPermissions { + wrongPermsGroups = append(wrongPermsGroups, groupID) + } + groupsMissingPermissionsMu.Unlock() + }(group.ID) + } + + if resp.NextPage == 0 { + break + } + page = resp.NextPage + } + + permissionsCheckWaitGroup.Wait() + if requestErr != nil { + return nil, nil, requestErr + } + + return notInGroups, wrongPermsGroups, nil +} + +func (e *external) fetchTopLevelGroupsPage(page int64) ([]*gitlab.Group, *gitlab.Response, error) { + groups, resp, err := e.groupsClient.ListGroups(&gitlab.ListGroupsOptions{ + TopLevelOnly: ptr.To(true), + ListOptions: gitlab.ListOptions{ + PerPage: 100, + Page: page, + }, + }) + if err != nil { + return nil, nil, errors.Wrap(err, "cannot list Gitlab groups") + } + if resp == nil { + return nil, nil, errors.New("no response received when listing Gitlab groups") + } + + return groups, resp, nil +} + +func (e *external) getGroupPermissionStatus(groupID int64, serviceAccountID int64, minPermission int) (isMissingMembership bool, hasInsufficientPermissions bool, err error) { + groupMember, resp, err := e.groupMemberClient.GetGroupMember(groupID, serviceAccountID) + if err != nil { + if clients.IsResponseNotFound(resp) { + return true, false, nil + } + return false, false, errors.Wrap(err, "cannot check Gitlab group member") + } + + if groupMember == nil { + return false, true, nil + } + + if int(groupMember.AccessLevel) < minPermission { + return false, true, nil + } + + return false, false, nil +} + +// getAccessLevelValue is a helper function that returns the integer value of the given access level string. This is used to convert the BaselinePermissions field from a string to an integer that can be used with the Gitlab API. +func getAccessLevelValue(accessLevel string) int { + accessLevelsMapping := map[string]int{ + accessLevelNoAccess: accessLevelNoAccessValue, + accessLevelMinimalAccess: accessLevelMinimalAccessValue, + accessLevelGuest: accessLevelGuestValue, + accessLevelPlanner: accessLevelPlannerValue, + accessLevelReporter: accessLevelReporterValue, + accessLevelSecurityManager: accessLevelSecurityManagerValue, + accessLevelDeveloper: accessLevelDeveloperValue, + accessLevelMaintainer: accessLevelMaintainerValue, + accessLevelOwner: accessLevelOwnerValue, + } + + value, ok := accessLevelsMapping[accessLevel] + if !ok { + return accessLevelNoAccessValue + } + return value +} diff --git a/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers_test.go b/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers_test.go new file mode 100644 index 00000000..769a8fba --- /dev/null +++ b/pkg/namespaced/controller/instance/serviceaccounts/controller_helpers_test.go @@ -0,0 +1,295 @@ +/* +Copyright 2021 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package serviceaccounts + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + + xerrors "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + gitlab "gitlab.com/gitlab-org/api/client-go" + + groupsfake "github.com/crossplane-contrib/provider-gitlab/pkg/namespaced/clients/groups/fake" +) + +func TestGetAccessLevelValue(t *testing.T) { + tests := map[string]struct { + accessLevel string + want int + }{ + "NoAccess": {accessLevel: accessLevelNoAccess, want: accessLevelNoAccessValue}, + "MinimalAccess": {accessLevel: accessLevelMinimalAccess, want: accessLevelMinimalAccessValue}, + "Guest": {accessLevel: accessLevelGuest, want: accessLevelGuestValue}, + "Planner": {accessLevel: accessLevelPlanner, want: accessLevelPlannerValue}, + "Reporter": {accessLevel: accessLevelReporter, want: accessLevelReporterValue}, + "SecurityManager": {accessLevel: accessLevelSecurityManager, want: accessLevelSecurityManagerValue}, + "Developer": {accessLevel: accessLevelDeveloper, want: accessLevelDeveloperValue}, + "Maintainer": {accessLevel: accessLevelMaintainer, want: accessLevelMaintainerValue}, + "Owner": {accessLevel: accessLevelOwner, want: accessLevelOwnerValue}, + "UnknownDefaultsToNoAccess": {accessLevel: "unknown", want: accessLevelNoAccessValue}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := getAccessLevelValue(tc.accessLevel) + if got != tc.want { + t.Fatalf("getAccessLevelValue(%q) = %d, want %d", tc.accessLevel, got, tc.want) + } + }) + } +} + +func TestFetchTopLevelGroupsPage(t *testing.T) { + client := &groupsfake.MockClient{ + MockListGroups: func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + if opt.TopLevelOnly == nil || !*opt.TopLevelOnly { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups without TopLevelOnly") + } + if opt.PerPage != 100 { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups with PerPage = %d, want 100", opt.PerPage) + } + if opt.Page != 2 { + t.Fatalf("fetchTopLevelGroupsPage() called ListGroups with Page = %d, want 2", opt.Page) + } + return []*gitlab.Group{{ID: 123}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 3}, nil + }, + } + + e := &external{groupsClient: client} + + got, resp, err := e.fetchTopLevelGroupsPage(2) + if err != nil { + t.Fatalf("fetchTopLevelGroupsPage() unexpected error: %v", err) + } + if resp == nil { + t.Fatal("fetchTopLevelGroupsPage() returned nil response") + } + if resp.NextPage != 3 { + t.Fatalf("fetchTopLevelGroupsPage() NextPage = %d, want 3", resp.NextPage) + } + if diff := cmp.Diff([]int64{123}, []int64{got[0].ID}); diff != "" { + t.Fatalf("fetchTopLevelGroupsPage() unexpected groups (-want +got):\n%s", diff) + } +} + +func TestGetGroupPermissionStatus(t *testing.T) { + tests := map[string]struct { + memberClient *groupsfake.MockClient + wantMissing bool + wantWrong bool + wantErr error + }{ + "NotFound": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusNotFound}}, errors.New("not found") + }}, + wantMissing: true, + }, + "NilMember": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + wantWrong: true, + }, + "LowAccess": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelReporterValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + wantWrong: true, + }, + "EnoughAccess": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelMaintainerValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }}, + }, + "UnexpectedError": { + memberClient: &groupsfake.MockClient{MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + }}, + wantErr: xerrors.New("cannot check Gitlab group member: boom"), + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + e := &external{groupMemberClient: tc.memberClient} + missing, wrong, err := e.getGroupPermissionStatus(42, 99, accessLevelDeveloperValue) + if tc.wantErr == nil { + if err != nil { + t.Fatalf("getGroupPermissionStatus() unexpected error: %v", err) + } + } else if err == nil || err.Error() != tc.wantErr.Error() { + t.Fatalf("getGroupPermissionStatus() error = %v, want %v", err, tc.wantErr) + } + if missing != tc.wantMissing { + t.Fatalf("getGroupPermissionStatus() missing = %v, want %v", missing, tc.wantMissing) + } + if wrong != tc.wantWrong { + t.Fatalf("getGroupPermissionStatus() wrong = %v, want %v", wrong, tc.wantWrong) + } + }) + } +} + +func TestFetchTopLevelGroupsMissingPermissions(t *testing.T) { + var ( + pagesMu sync.Mutex + pages []int64 + ) + + client := &groupsfake.MockClient{ + MockListGroups: func(opt *gitlab.ListGroupsOptions, options ...gitlab.RequestOptionFunc) ([]*gitlab.Group, *gitlab.Response, error) { + pagesMu.Lock() + pages = append(pages, opt.Page) + pagesMu.Unlock() + + switch opt.Page { + case 1: + return []*gitlab.Group{{ID: 1}, {ID: 2}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 2}, nil + case 2: + return []*gitlab.Group{{ID: 3}}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}, NextPage: 0}, nil + default: + t.Fatalf("unexpected page %d", opt.Page) + return nil, nil, nil + } + }, + MockGetMember: func(gid interface{}, user int64, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + switch gid.(int64) { + case 1: + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusNotFound}}, errors.New("not found") + case 2: + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + case 3: + return &gitlab.GroupMember{AccessLevel: gitlab.AccessLevelValue(accessLevelReporterValue)}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + default: + t.Fatalf("unexpected group id %v", gid) + return nil, nil, nil + } + }, + } + + e := &external{groupsClient: client, groupMemberClient: client} + + notIn, wrong, err := e.fetchTopLevelGroupsMissingPermissions(accessLevelDeveloperValue, 99) + if err != nil { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() unexpected error: %v", err) + } + if diff := cmp.Diff([]int64{1}, notIn, cmpopts.SortSlices(func(a, b int64) bool { return a < b })); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() notInGroups mismatch (-want +got):\n%s", diff) + } + if diff := cmp.Diff([]int64{2, 3}, wrong, cmpopts.SortSlices(func(a, b int64) bool { return a < b })); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() wrongPermsGroups mismatch (-want +got):\n%s", diff) + } + + pagesMu.Lock() + defer pagesMu.Unlock() + if diff := cmp.Diff([]int64{1, 2}, pages); diff != "" { + t.Fatalf("fetchTopLevelGroupsMissingPermissions() page sequence mismatch (-want +got):\n%s", diff) + } +} + +func TestAddServiceAccountToGroups(t *testing.T) { + var ( + callsMu sync.Mutex + calls = map[int64]*gitlab.AddGroupMemberOptions{} + ) + + client := &groupsfake.MockClient{ + MockAddMember: func(gid interface{}, opt *gitlab.AddGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + groupID := gid.(int64) + callsMu.Lock() + calls[groupID] = opt + callsMu.Unlock() + + if groupID == 12 { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + } + + return &gitlab.GroupMember{}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusCreated}}, nil + }, + } + + e := &external{groupMemberClient: client} + err := e.addServiceAccountToGroups(context.Background(), 99, []int64{11, 12}, accessLevelMaintainerValue) + if err == nil { + t.Fatal("addServiceAccountToGroups() expected an error") + } + + callsMu.Lock() + defer callsMu.Unlock() + if len(calls) != 2 { + t.Fatalf("addServiceAccountToGroups() called AddGroupMember %d times, want 2", len(calls)) + } + for groupID, call := range calls { + if call == nil { + t.Fatalf("addServiceAccountToGroups() nil call for group %d", groupID) + } + if call.UserID == nil || *call.UserID != 99 { + t.Fatalf("addServiceAccountToGroups() UserID for group %d = %v, want 99", groupID, call.UserID) + } + if call.AccessLevel == nil || int(*call.AccessLevel) != accessLevelMaintainerValue { + t.Fatalf("addServiceAccountToGroups() AccessLevel for group %d = %v, want %d", groupID, call.AccessLevel, accessLevelMaintainerValue) + } + } +} + +func TestUpdateServiceAccountGroupPermissions(t *testing.T) { + var ( + callsMu sync.Mutex + calls = map[int64]*gitlab.EditGroupMemberOptions{} + ) + + client := &groupsfake.MockClient{ + MockEditMember: func(gid interface{}, user int64, opt *gitlab.EditGroupMemberOptions, options ...gitlab.RequestOptionFunc) (*gitlab.GroupMember, *gitlab.Response, error) { + groupID := gid.(int64) + callsMu.Lock() + calls[groupID] = opt + callsMu.Unlock() + + if groupID == 12 { + return nil, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, errors.New("boom") + } + + return &gitlab.GroupMember{}, &gitlab.Response{Response: &http.Response{StatusCode: http.StatusOK}}, nil + }, + } + + e := &external{groupMemberClient: client} + err := e.updateServiceAccountGroupPermissions(context.Background(), 99, []int64{11, 12}, accessLevelMaintainerValue) + if err == nil { + t.Fatal("updateServiceAccountGroupPermissions() expected an error") + } + + callsMu.Lock() + defer callsMu.Unlock() + if len(calls) != 2 { + t.Fatalf("updateServiceAccountGroupPermissions() called EditGroupMember %d times, want 2", len(calls)) + } + for groupID, call := range calls { + if call == nil { + t.Fatalf("updateServiceAccountGroupPermissions() nil call for group %d", groupID) + } + if call.AccessLevel == nil || int(*call.AccessLevel) != accessLevelMaintainerValue { + t.Fatalf("updateServiceAccountGroupPermissions() AccessLevel for group %d = %v, want %d", groupID, call.AccessLevel, accessLevelMaintainerValue) + } + } +} diff --git a/pkg/namespaced/controller/instance/serviceaccounts/controller_test.go b/pkg/namespaced/controller/instance/serviceaccounts/controller_test.go index ae05cb1f..9f2f5d01 100644 --- a/pkg/namespaced/controller/instance/serviceaccounts/controller_test.go +++ b/pkg/namespaced/controller/instance/serviceaccounts/controller_test.go @@ -50,8 +50,7 @@ const ( testServiceAccountEmail = "sa@example.org" ) -// MockClient is a small, purpose-built mock for instance.ServiceAccountClient. -// We keep it local to this test file to avoid adding any non-test code. +// MockClient is a small mock for instance.ServiceAccountClient used by controller tests. type MockClient struct { MockGetUser func(user int64, opt gitlab.GetUsersOptions, options ...gitlab.RequestOptionFunc) (*gitlab.User, *gitlab.Response, error) MockCreateServiceAccountUser func(opts *gitlab.CreateServiceAccountUserOptions, options ...gitlab.RequestOptionFunc) (*gitlab.User, *gitlab.Response, error) From 5ff00987ccae7586abfb2929fa12055036a1b14f Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Mon, 13 Apr 2026 01:52:21 +0200 Subject: [PATCH 2/4] docs: update example Signed-off-by: BoxBoxJason --- examples/instance/serviceaccount.yaml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/examples/instance/serviceaccount.yaml b/examples/instance/serviceaccount.yaml index 33d2ed17..ac31c791 100644 --- a/examples/instance/serviceaccount.yaml +++ b/examples/instance/serviceaccount.yaml @@ -13,5 +13,7 @@ spec: username: example-service-account-2 # WARNING: field is immutable after creation # email: example@example.com - admin: true - baselinePermissions: developer + # WARNING: dangerous field, it will grant admin permissions to the service account + # admin: true + # WARNING: dangerous field, it will grant developer permissions on all top level groups to the service account + # baselinePermissions: developer From b24b62af2881b157393748c74ff95863a78efce5 Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Mon, 13 Apr 2026 02:09:33 +0200 Subject: [PATCH 3/4] fix: unrequired observation update Signed-off-by: BoxBoxJason --- .../controller/instance/serviceaccounts/zz_controller.go | 6 +----- .../controller/instance/serviceaccounts/controller.go | 6 +----- 2 files changed, 2 insertions(+), 10 deletions(-) diff --git a/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go b/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go index dd33d36f..995df0f0 100644 --- a/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go +++ b/pkg/cluster/controller/instance/serviceaccounts/zz_controller.go @@ -249,7 +249,7 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account fields updateWaitGroup.Go(func() { - serviceAccount, _, err := e.client.ModifyUser( + _, _, err := e.client.ModifyUser( serviceAccountID, instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), gitlab.WithContext(ctx)) @@ -257,8 +257,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext errorsChannel <- errors.Wrap(err, errUpdateFailed) return } - - cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) }) minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) @@ -267,7 +265,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account group memberships updateWaitGroup.Go(func() { if cr.Status.AtProvider.MissingMemberShipGroups != nil { - errorsChannel <- e.addServiceAccountToGroups(ctx, serviceAccountID, cr.Status.AtProvider.MissingMemberShipGroups, minPermissionValue) } }) @@ -275,7 +272,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account group permissions updateWaitGroup.Go(func() { if cr.Status.AtProvider.WrongPermissionsGroups != nil { - errorsChannel <- e.updateServiceAccountGroupPermissions(ctx, serviceAccountID, cr.Status.AtProvider.WrongPermissionsGroups, minPermissionValue) } }) diff --git a/pkg/namespaced/controller/instance/serviceaccounts/controller.go b/pkg/namespaced/controller/instance/serviceaccounts/controller.go index 06ab2ccf..db4505eb 100644 --- a/pkg/namespaced/controller/instance/serviceaccounts/controller.go +++ b/pkg/namespaced/controller/instance/serviceaccounts/controller.go @@ -247,7 +247,7 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account fields updateWaitGroup.Go(func() { - serviceAccount, _, err := e.client.ModifyUser( + _, _, err := e.client.ModifyUser( serviceAccountID, instance.GenerateUpdateServiceAccountOptions(&cr.Spec.ForProvider), gitlab.WithContext(ctx)) @@ -255,8 +255,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext errorsChannel <- errors.Wrap(err, errUpdateFailed) return } - - cr.Status.AtProvider = instance.GenerateServiceAccountObservation(serviceAccount) }) minPermission := ptr.Deref(cr.Spec.ForProvider.BaselinePermissions, accessLevelNoAccess) @@ -265,7 +263,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account group memberships updateWaitGroup.Go(func() { if cr.Status.AtProvider.MissingMemberShipGroups != nil { - errorsChannel <- e.addServiceAccountToGroups(ctx, serviceAccountID, cr.Status.AtProvider.MissingMemberShipGroups, minPermissionValue) } }) @@ -273,7 +270,6 @@ func (e *external) Update(ctx context.Context, mg resource.Managed) (managed.Ext // Reconcile service account group permissions updateWaitGroup.Go(func() { if cr.Status.AtProvider.WrongPermissionsGroups != nil { - errorsChannel <- e.updateServiceAccountGroupPermissions(ctx, serviceAccountID, cr.Status.AtProvider.WrongPermissionsGroups, minPermissionValue) } }) From b32c1c241ec3235b0098d225c7d866fb89dbdb9e Mon Sep 17 00:00:00 2001 From: BoxBoxJason Date: Mon, 13 Apr 2026 02:38:33 +0200 Subject: [PATCH 4/4] docs: fix example number Signed-off-by: BoxBoxJason --- examples/instance/serviceaccount.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/instance/serviceaccount.yaml b/examples/instance/serviceaccount.yaml index ac31c791..9f523a2c 100644 --- a/examples/instance/serviceaccount.yaml +++ b/examples/instance/serviceaccount.yaml @@ -10,7 +10,7 @@ spec: kind: ProviderConfig forProvider: name: Example Service Account - username: example-service-account-2 + username: example-service-account # WARNING: field is immutable after creation # email: example@example.com # WARNING: dangerous field, it will grant admin permissions to the service account