feat: Implement public API that will open SpaceBindings to end users through proxy part 2 #350

mfrancisc · 2023-10-02T15:38:53Z

Populate workspace.Status.Bindings with the list of usernames, roles and available actions on those usernames.

This set of PRs will cover all the remaining work for jira: https://issues.redhat.com/browse/ASC-410

Some followup refactor work will remain only.

Related PRs:
toolchain-common: codeready-toolchain/toolchain-common#329

Paired with:
toolchain-e2e: codeready-toolchain/toolchain-e2e#805

Co-authored-by: Alexey Kazakov <alkazako@redhat.com>

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

…ebining for get, add more tests

# Conflicts: # go.mod # go.sum # pkg/proxy/handlers/spacelister.go

# Conflicts: # go.sum # pkg/proxy/handlers/spacelister.go

mfrancisc · 2023-10-02T15:49:55Z

/retest

alexeykazakov · 2023-10-03T00:37:58Z

pkg/proxy/handlers/spacelister.go

+		//  let's only log the issue and consider this as not found
+		ctx.Logger().Error(fmt.Sprintf("expected only 1 spacebinding, got 0 for user %s and workspace %s", userSignup.CompliantUsername, workspaceName))
+		return nil, nil
+	} else if len(userBindings) > 1 {


Couldn't a user have multiple SpaceBindings in a hierarchy of the spaces? Even if we don't really support it now we could support it in the future, right? Do we really need to make sure there is no than one here?

BTW, aren't we already checking that user has access to the workspace? Do we need to do this additional SpaceBinding check?

Couldn't a user have multiple SpaceBindings in a hierarchy of the spaces? Even if we don't really support it now we could support it in the future, right? Do we really need to make sure there is no than one here?

We could avoid the check, but right now:

allSpaceBindings, err := spaceBindingLister.ListForSpace(space, []toolchainv1alpha1.SpaceBinding{}) at line 117

should return only 1 spacebinding for the given Space and MUR (since it merges them based on the MUR name). So that's why I'm checking that there is no more than one returned from there.

BTW, aren't we already checking that user has access to the workspace? Do we need to do this additional SpaceBinding check?

before those changes there was this function here checking for the binding: https://github.com/codeready-toolchain/registration-service/pull/350/files#diff-cffbabd2ac8378a758970ae247dbee7b644f1d94e702181d836d5d5d19a048c5L88

I've replaced that with filterUserSpaceBindings which searches for the user binding from the list of all bindings that has already fetched above using the new spaceBindingLister.ListForSpace .

I don't see any other place where we check if user has access to the workspace 🤷‍♂️ , but maybe I'm missing something.

before those changes there was this function here checking for the binding:

Ah.. OK. I missed that. So you are replacing the current check with the new one.

mfrancisc · 2023-10-03T14:28:55Z

/retest

alexeykazakov

Looks good. Thanks.

mfrancisc · 2023-10-04T13:17:41Z

/retest

codecov · 2023-10-04T15:16:07Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
pkg/proxy/handlers/spacelister.go	`91.07% <95.83%> (+6.77%)`	⬆️

📢 Thoughts on this report? Let us know!.

pkg/proxy/handlers/spacelister.go

MatousJobanek · 2023-10-06T14:05:24Z

pkg/proxy/handlers/spacelister.go

+func filterUserSpaceBindings(username string, allSpaceBindings []toolchainv1alpha1.SpaceBinding) (outputBindings []toolchainv1alpha1.SpaceBinding) {
+	for _, binding := range allSpaceBindings {
+		if binding.Spec.MasterUserRecord == username {
+			outputBindings = append(outputBindings, binding)
+		}
+	}
+	return outputBindings
+}


We could change it so we wouldn't care about if the user has more than one SpaceBinding. What we care is if the user has access to the Space at all, not about the actual content

Suggested change

func filterUserSpaceBindings(username string, allSpaceBindings []toolchainv1alpha1.SpaceBinding) (outputBindings []toolchainv1alpha1.SpaceBinding) {

for _, binding := range allSpaceBindings {

if binding.Spec.MasterUserRecord == username {

outputBindings = append(outputBindings, binding)

}

}

return outputBindings

}

func filterUserSpaceBindings(username string, allSpaceBindings []toolchainv1alpha1.SpaceBinding) *toolchainv1alpha1.SpaceBinding {

for _, binding := range allSpaceBindings {

if binding.Spec.MasterUserRecord == username {

return &binding

}

}

return nil

}

I think that it would simplify the code a bit and it wouldn't have any negative impact on the functional aspects.

yes this makes it easier!

I think we would just not be able to report in case there are issues with the system , generating more spacebindings for the same MUR (let's say for whatever reason, a bug in Space controller or SBR controller or other parts of the system like the sandbox-cli ... ). If this is acceptable then yes, the code can be simplified.

as decided earlier today, let's not return this error to the user. I'm not checking only for the user binding as you suggested. Plz check e8eee83

MatousJobanek · 2023-10-09T08:35:20Z

pkg/proxy/handlers/spacelister.go

+			Role:             spaceBinding.Spec.SpaceRole,
+			AvailableActions: []string{},
+		}
+		if name, exists := spaceBinding.GetLabels()[toolchainv1alpha1.SpaceBindingSpaceLabelKey]; exists && name == space.GetName() {


We should check the SBR labels instead - if the SpaceBinding was created by the UserSignup controller (as the default SpaceBinding) then the users cannot do anything with it. They can only update/delete those that have corresponding SBR CR in the namespace.

Good point, I think the logic was incorrect here 🤦‍♂️

I'm now checking for SBR label on the SB resource. Plz take a look mainly at: 64ba7ba

Thanks!

There were actually a few cases missing:

the case in which there should be no available action, since the spacebinding was generated by the system or system admin

spacebinding was generated from SBR but on the parentSpace, so in this case it can only be overridden , and not updated/deleted as SBRs generated on the current space.

Please check also 485a52e

MatousJobanek · 2023-10-09T08:35:26Z

pkg/proxy/handlers/spacelister.go

+			// this is a binding that was created on the current space, it can be updated or deleted
+			binding.AvailableActions = []string{UpdateBindingAction, DeleteBindingAction}


how does the client (UI) know which SBR it should either update or delete?
I think that the API is missing the bindingRequest field, right?
https://docs.google.com/document/d/1hWaFvCfsEXrbqs8ndDR0JPjKROXYJkvj1ZCnio4P1WU/edit#bookmark=id.tttpu2pbe4a6

Ohh jeez 🤦‍♂️ , I missed that field in the original API pr and totally forgot about it! Great catch!

I've raised codeready-toolchain/api#378 for this. And I'm updating this PR and e2e tests to use it!

Thanks!

I'm now populating the bindingRequest field as well. PTAL at 64ba7ba

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

MatousJobanek

Looks great, thanks a lot for addressing my comments 👍 🥇 🚀

pkg/proxy/handlers/spacelister.go

openshift-ci · 2023-10-11T12:52:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, MatousJobanek, mfrancisc

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:

~~OWNERS~~ [MatousJobanek,alexeykazakov]

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

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

sonarcloud · 2023-10-12T07:04:07Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
28.4% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

mfrancisc · 2023-10-12T08:43:12Z

/retest

mfrancisc and others added 19 commits September 20, 2023 18:17

add available roles to workspace api

49aa7e7

replace toolchain-common

9e08ece

update common

b6a4d8f

fix linter

44eee9d

add nstemplatetier to informer start

c3639a3

create HandleSpaceGetRequest handler in proxy

f333b61

add notes for modifying registration-service.yaml

3eeb8fd

Merge branch 'master' into sbapi

9278a66

Update pkg/proxy/handlers/spacelister.go

cc77790

Co-authored-by: Alexey Kazakov <alkazako@redhat.com>

update name

e83efa4

use NSTemplateTierFunc from informer

1305bcf

Update pkg/proxy/handlers/spacelister.go

db915bf

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

move usersignup retrieval , retrun error in case of more the one spac…

0175925

…ebining for get, add more tests

populate binding list in workspace type

a1507b3

Merge branch 'master' into sbapi

01124e8

# Conflicts: # go.mod # go.sum # pkg/proxy/handlers/spacelister.go

fix spacelist unit test

4704269

Merge branch 'sbapi' into sbapi2

0b72c66

# Conflicts: # go.sum # pkg/proxy/handlers/spacelister.go

go sum

5e393f3

update common

7130609

mfrancisc requested review from sbryzak, xcoulon, alexeykazakov, MatousJobanek, rajivnathan and ranakan19 October 2, 2023 15:38

alexeykazakov reviewed Oct 3, 2023

View reviewed changes

alexeykazakov approved these changes Oct 3, 2023

View reviewed changes

openshift-ci bot added the approved label Oct 3, 2023

Merge branch 'master' into sbapi2

e3d2fec

MatousJobanek reviewed Oct 9, 2023

View reviewed changes

mfrancisc and others added 8 commits October 9, 2023 12:40

Update pkg/proxy/handlers/spacelister.go

9160b2e

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

wip add sbr to bindings request

418ef6a

check for SBR label and populate bindingRequest field

64ba7ba

small typo

31c54dd

handle sbr created on parent space case

485a52e

avoid checking for multiple spacebindings for user

e8eee83

update api

075917a

Merge branch 'master' into sbapi2

3e1a9d0

MatousJobanek approved these changes Oct 11, 2023

View reviewed changes

pkg/proxy/handlers/spacelister.go Outdated Show resolved Hide resolved

pkg/proxy/handlers/spacelister.go Outdated Show resolved Hide resolved

mfrancisc and others added 5 commits October 11, 2023 15:14

Update pkg/proxy/handlers/spacelister.go

819cd5f

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

Update pkg/proxy/handlers/spacelister.go

6ae41cc

Co-authored-by: Matous Jobanek <mjobanek@redhat.com>

update api and common

e433e7f

Merge branch 'master' into sbapi2

7da2560

update common

105c90a

mfrancisc merged commit 9dd5dd6 into codeready-toolchain:master Oct 12, 2023
11 of 13 checks passed

mfrancisc deleted the sbapi2 branch October 12, 2023 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement public API that will open SpaceBindings to end users through proxy part 2 #350

feat: Implement public API that will open SpaceBindings to end users through proxy part 2 #350

mfrancisc commented Oct 2, 2023 •

edited

Loading

mfrancisc commented Oct 2, 2023

alexeykazakov Oct 3, 2023

alexeykazakov Oct 3, 2023

mfrancisc Oct 3, 2023

mfrancisc Oct 3, 2023

alexeykazakov Oct 3, 2023

mfrancisc commented Oct 3, 2023

alexeykazakov left a comment

mfrancisc commented Oct 4, 2023

codecov bot commented Oct 4, 2023 •

edited

Loading

MatousJobanek Oct 6, 2023

mfrancisc Oct 9, 2023

mfrancisc Oct 10, 2023

MatousJobanek Oct 9, 2023

mfrancisc Oct 9, 2023

mfrancisc Oct 9, 2023

MatousJobanek Oct 9, 2023

mfrancisc Oct 9, 2023

mfrancisc Oct 9, 2023

MatousJobanek left a comment

openshift-ci bot commented Oct 11, 2023

sonarcloud bot commented Oct 12, 2023

mfrancisc commented Oct 12, 2023

		// this is a binding that was created on the current space, it can be updated or deleted
		binding.AvailableActions = []string{UpdateBindingAction, DeleteBindingAction}

feat: Implement public API that will open SpaceBindings to end users through proxy part 2 #350

feat: Implement public API that will open SpaceBindings to end users through proxy part 2 #350

Conversation

mfrancisc commented Oct 2, 2023 • edited Loading

mfrancisc commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfrancisc commented Oct 3, 2023

alexeykazakov left a comment

Choose a reason for hiding this comment

mfrancisc commented Oct 4, 2023

codecov bot commented Oct 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatousJobanek left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 11, 2023

sonarcloud bot commented Oct 12, 2023

mfrancisc commented Oct 12, 2023

mfrancisc commented Oct 2, 2023 •

edited

Loading

codecov bot commented Oct 4, 2023 •

edited

Loading