New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow non admin users to manage BSL #6589

Merged

kubermatic-bot merged 5 commits into kubermatic:main from ahmadhamzh:6563-remove-admin-checking-for-cluster-backup

Mar 14, 2024

Contributor

ahmadhamzh commented Mar 7, 2024

What this PR does / why we need it:
Allow non admin users to manage backup storage location.

Which issue(s) this PR fixes:
Fixes #6563

What type of PR is this?
/kind bug

NONE

NONE


          allow other than admins to manage cbsl

a6f9859

kubermatic-bot added kind/bug docs/none release-note-none dco-signoff: yes sig/api do-not-merge/code-freeze size/L labels

ahmadhamzh changed the title ~~Allow non admin users to manage BSL~~ WIP Allow non admin users to manage BSL

kubermatic-bot added the do-not-merge/work-in-progress label

SimonTheLeg requested changes

View reviewed changes

modules/api/pkg/ee/clusterbackup/storage-location/handler.go Outdated

Comment on lines 104 to 110

+              	_, err := userInfoGetter(ctx, req.ProjectID)
+              	if err != nil {
+              		return nil, err
+              	}

Member

SimonTheLeg Mar 8, 2024

I think this essentially only queries the user roles and so on, but since you are not using them to check for anything (like in CreateCSBL), I believe it can be removed

Member

SimonTheLeg Mar 8, 2024

same for the GetCSBL func

modules/api/pkg/ee/clusterbackup/storage-location/handler.go Outdated

Comment on lines 208 to 210

+              	if user.Roles.Has("viewers") {
+              		return fmt.Errorf("user with a viewer role is not permitted to delete storage locations.")
+              	}

Member

SimonTheLeg Mar 8, 2024

my understanding from other code is that it is possible for a user to have both "viewer" as well as another role. As a result we also need to check if viewers is the only role the user has.

For example like this is done here

dashboard/modules/api/pkg/provider/kubernetes/cluster.go

Line 428 in 140d3a4

if userInfo.Roles.Has("viewers") && userInfo.Roles.Len() == 1 {

Member

SimonTheLeg Mar 8, 2024

Same for Create and Patch as well

kubermatic-bot assigned SimonTheLeg


          add impersonated client

b7ead75

ahmadhamzh force-pushed the 6563-remove-admin-checking-for-cluster-backup branch from 6d591f8 to b7ead75 Compare

March 12, 2024 11:21

ahmadhamzh added this to the KKP 2.25 milestone

Contributor Author

ahmadhamzh commented Mar 12, 2024

/retest


          refactor code

0cc2373

ahmadhamzh force-pushed the 6563-remove-admin-checking-for-cluster-backup branch from dc913a9 to 0cc2373 Compare

March 13, 2024 15:17

Contributor Author

ahmadhamzh commented Mar 13, 2024

I changed the approach to using an Impersonated Client, so there is no need to check the role in the API.
@SimonTheLeg

ahmadhamzh requested a review from SimonTheLeg

March 13, 2024 15:22

ahmadhamzh changed the title ~~WIP Allow non admin users to manage BSL~~ Allow non admin users to manage BSL

kubermatic-bot removed the do-not-merge/work-in-progress label

Contributor Author

ahmadhamzh commented Mar 13, 2024

@SimonTheLeg sorry but this PR is blocked by kubermatic/kubermatic#13174

ahmadhamzh changed the title ~~Allow non admin users to manage BSL~~ WIP Allow non admin users to manage BSL

kubermatic-bot added the do-not-merge/work-in-progress label

ahmadhamzh force-pushed the 6563-remove-admin-checking-for-cluster-backup branch from 55fed9e to c340ace Compare

March 14, 2024 12:03


          improve errors

543798f

ahmadhamzh force-pushed the 6563-remove-admin-checking-for-cluster-backup branch from c340ace to 543798f Compare

March 14, 2024 12:26

ahmadhamzh changed the title ~~WIP Allow non admin users to manage BSL~~ Allow non admin users to manage BSL

kubermatic-bot removed the do-not-merge/work-in-progress label

SimonTheLeg requested changes

View reviewed changes

Member

SimonTheLeg left a comment

Overall I think this should all work and is in spec what we are doing in other parts of the API. I have some questions regarding the usage of the privileged client in some places. ptal.
But overall I think we are on the right track.

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

Comment on lines 80 to 82

		client := p.privilegedClient

		if err := client.List(ctx, cbslList, listOpts); err != nil {

Member

SimonTheLeg Mar 14, 2024

nit: just to keep this uniform with other code. Let's go back to the previous p.privilegedClient.List notation from before

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

Comment on lines 93 to 99

+              	cbslList, err := p.List(ctx, userInfo, labelSet)
               	if err != nil {
-              		return nil, fmt.Errorf("failed to list ClusterBackupStorageLocations: %w", err)
+              		return nil, err
               	}
               	for _, cbsl := range cbslList.Items {
               		if cbsl.Name == name {
               			return &cbsl, nil

Member

SimonTheLeg Mar 14, 2024

I know this is not part of this PR, but I would appreciate if we could create a ticket to investigate this. Judging from just the code I think it should be fine to create the impersonated client and run a client.Get.

See

dashboard/modules/api/pkg/provider/kubernetes/project.go

Lines 128 to 135 in 03648c6

    
           masterImpersonatedClient, err := createImpersonationClientWrapperFromUserInfo(userInfo, p.createMasterImpersonatedClient) 
        
           if err != nil { 
        
           	return nil, err 
        
           } 
        
           existingProject := &kubermaticv1.Project{} 
        
           if err := masterImpersonatedClient.Get(ctx, ctrlruntimeclient.ObjectKey{Name: projectInternalName}, existingProject); err != nil { 
        
           	return nil, err 
        
           }

for an example

Contributor

moelsayed Mar 14, 2024

The reason we didn't do a direct Get() here is not because of the client. The problem here is that the CBSL is a project-scoped resources. We assign it to the project using a label. We need the API to only list CBSL's that belong to the correct project. So, we use List() with a label selector and loop over the results. Unfortunately the Get() API doesn't support label selectors.

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

Comment on lines 261 to 263

+              	if userInfo == nil {
+              		return nil, errors.New("a user is missing but required")
+              	}

Member

SimonTheLeg Mar 14, 2024

I think since you are checking for userInfo to not be nil always before calling this func, this can be removed.

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

		@@ -110,57 +141,78 @@ func (p *BackupStorageProvider) CreateUnsecured(ctx context.Context, cbslName, p
		},
		Key: "cloud-credentials",

Member

SimonTheLeg Mar 14, 2024

I would recommend to move this into a global const, since we are using this in multiple places in this provider

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

               	}
               }
-              func (p *BackupStorageProvider) ListUnsecured(ctx context.Context, labelSet map[string]string) (*kubermaticv1.ClusterBackupStorageLocationList, error) {
+              func (p *BackupStorageProvider) List(ctx context.Context, userInfo *provider.UserInfo, labelSet map[string]string) (*kubermaticv1.ClusterBackupStorageLocationList, error) {

Member

SimonTheLeg Mar 14, 2024

In my opinion this should still be called ListUnsecured, because it uses the priviledged client to access the info

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

-              func (p *BackupStorageProvider) GetUnsecured(ctx context.Context, name string, labelSet map[string]string) (*kubermaticv1.ClusterBackupStorageLocation, error) {
-              	// we use List() to filter CBSLs with labels easily, to keep scoped into the owner project.
-              	cbslList, err := p.ListUnsecured(ctx, labelSet)
+              func (p *BackupStorageProvider) Get(ctx context.Context, userInfo *provider.UserInfo, name string, labelSet map[string]string) (*kubermaticv1.ClusterBackupStorageLocation, error) {

Member

SimonTheLeg Mar 14, 2024

In my opinion this should still be called GetUnsecured, because it calls the List which in turn uses the priviledged client.

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

Comment on lines 169 to 175

               	cbsl := &kubermaticv1.ClusterBackupStorageLocation{}
-              	if err := p.privilegedClient.Get(ctx, types.NamespacedName{Name: name, Namespace: resources.KubermaticNamespace}, cbsl); err != nil {
+              	if err := client.Get(ctx, types.NamespacedName{Name: name, Namespace: resources.KubermaticNamespace}, cbsl); err != nil {
               		if apierrors.IsNotFound(err) {
               			return nil
               		}
               		return err
               	}

Member

SimonTheLeg Mar 14, 2024

Is there a reason why the priviledged client is needed for getting the ClusterBackupStorageLocation? Couldn't the impersonated client be used for this?

Contributor

moelsayed Mar 14, 2024 •

edited

refactored to use the same client for both operations based on the user role.

modules/api/pkg/ee/clusterbackup/storage-location/provider.go Outdated

Comment on lines 198 to 201

               	existing := &kubermaticv1.ClusterBackupStorageLocation{}
-              	if err := p.privilegedClient.Get(ctx, types.NamespacedName{Name: cbslName, Namespace: resources.KubermaticNamespace}, existing); err != nil {
+              	if err := client.Get(ctx, types.NamespacedName{Name: cbslName, Namespace: resources.KubermaticNamespace}, existing); err != nil {
               		return nil, err
               	}

Member

SimonTheLeg Mar 14, 2024

same here. Is the priviledged client required, or could we just use the impersonated client for getting it?

Contributor

moelsayed Mar 14, 2024

done


          review comments

b2fc8fd

Member

SimonTheLeg commented Mar 14, 2024

/approve
/lgtm

kubermatic-bot added the lgtm label

Contributor

kubermatic-bot commented Mar 14, 2024

LGTM label has been added.

Git tree hash: 30e151f61a5867a76a0f97eaf83a81a7a6a78952

Contributor

kubermatic-bot commented Mar 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: SimonTheLeg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~modules/api/OWNERS~~ [SimonTheLeg]
~~modules/api/pkg/OWNERS~~ [SimonTheLeg]
~~modules/api/pkg/handler/OWNERS~~ [SimonTheLeg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kubermatic-bot added the approved label

embik added the code-freeze-approved label

kubermatic-bot removed the do-not-merge/code-freeze label

kubermatic-bot merged commit f3bb210 into kubermatic:main

8 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved code-freeze-approved dco-signoff: yes docs/none kind/bug lgtm release-note-none sig/api size/L