ASC-402: Copy the SA token of sub-spaces only for specific tiers #902

Kartikey-star · 2023-10-23T06:09:01Z

Acceptance Criteria
The SpaceRequest_controller copies the secret only for specific tiers like appstudio-env one
It is possible to create a sub-space using any other tier like appstudio or base1ns
If the SA token copy part is not supported for the provisioned tier, then the SpaceRequest.Status has corresponding information about it

openshift-ci · 2023-10-23T06:09:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2023-10-23T06:09:09Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Kartikey-star
Once this PR has been reviewed and has the lgtm label, please assign matousjobanek for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

Signed-off-by: Kartikey Mamgain <kmamgain@redhat.com>

mfrancisc

Looks good 👍

I've added few minor comments. Also could you please add some tests.

mfrancisc · 2023-10-23T07:28:24Z

controllers/spacerequest/spacerequest_controller.go

@@ -237,9 +239,9 @@ func (r *Reconciler) updateExistingSubSpace(ctx context.Context, spaceRequest *t
 }

 // validateNSTemplateTier checks if the provided tierName in the spaceRequest exists and is valid
-func (r *Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) error {
+func (r *Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) (toolchainv1alpha1.NSTemplateTier, error) {


how about returning a pointer? This way you could return tier or nil.

Suggested change

func (r *Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) (toolchainv1alpha1.NSTemplateTier, error) {

func (r *Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) (*toolchainv1alpha1.NSTemplateTier, error) {

mfrancisc · 2023-10-23T07:31:06Z

deploy/templates/nstemplatetiers/appstudio-env/ns_env.yaml

@@ -25,7 +25,7 @@ objects:
  kind: ServiceAccount
  metadata:
    namespace: ${SPACE_NAME}-env
-    name: namespace-manager
+    name: ${SERVICE_ACCOUNT_NAME}


i think it is fine to hard code the name here and in the tier.yaml. Also because unless I'm missing something, you are not populating this variable SERVICE_ACCOUNT_NAME right now.

mfrancisc · 2023-10-23T07:31:44Z

deploy/templates/nstemplatetiers/appstudio-env/tier.yaml

@@ -20,10 +20,13 @@ objects:
        templateRef: ${MAINTAINER_TEMPL_REF}
      contributor:
        templateRef: ${CONTRIBUTOR_TEMPL_REF}
+    spaceRequestConfig:
+      serviceAccountName: ${SERVICE_ACCOUNT_NAME}


see comment above, we could hard code the name here I guess.

controllers/spacerequest/spacerequest_controller.go

Signed-off-by: Kartikey Mamgain <kmamgain@redhat.com>

mfrancisc

Looks good!

I have few minor comments.

mfrancisc · 2023-10-27T08:13:23Z

controllers/spacerequest/spacerequest_controller.go

 	// create a token request for the admin service account
 	token, err := restclient.CreateTokenRequest(subSpaceTargetCluster.RESTClient, types.NamespacedName{
 		Namespace: namespace,
-		Name:      toolchainv1alpha1.AdminServiceAccountName,
+		Name:      serviceAccountName,


I see there are few other occurrences to be replaced, line 508 and 511. Could you please replace those as well?

mfrancisc · 2023-10-27T08:13:53Z

controllers/spacerequest/spacerequest_controller.go

 	if tierName == "" {
-		return fmt.Errorf("tierName cannot be blank")
+		return &toolchainv1alpha1.NSTemplateTier{}, fmt.Errorf("tierName cannot be blank")


minor, but we could just return nil here

Suggested change

return &toolchainv1alpha1.NSTemplateTier{}, fmt.Errorf("tierName cannot be blank")

return nil, fmt.Errorf("tierName cannot be blank")

mfrancisc · 2023-10-27T08:43:03Z

controllers/spacerequest/spacerequest_controller_test.go

+	appstudioTier.Spec.SpaceRequestConfig.ServiceAccountName = "manager"
+	t.Run("failure service account not present", func(t *testing.T) {


this is testing the case in which the service account doesn't exist. For this I would rather create a new tier that has a service account which doesn't exist or just configure gock at line 403 to return a different SA name, something like:

commontest.SetupGockForServiceAccounts(t, member1.APIEndpoint, types.NamespacedName{ Name: "another-sa-name", Namespace: "jane-env", })

Also, unless I'm missing something, I don't see a test that:

uses a tier which has .Spec.SpaceRequestConfig unset

verifies that spacerequest is provisioned with namespace access and no secretRef

the subspace is ready with the given tiername from the spacerequest

sonarqubecloud · 2023-11-13T12:40:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
8.9% Duplication

openshift-merge-robot · 2023-11-14T17:33:34Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-11-14T17:36:02Z

Codecov Report

Merging #902 (0cd9550) into master (41c56bb) will increase coverage by 0.11%.
Report is 3 commits behind head on master.
The diff coverage is 92.59%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #902      +/-   ##
==========================================
+ Coverage   83.70%   83.81%   +0.11%     
==========================================
  Files          53       53              
  Lines        6178     6183       +5     
==========================================
+ Hits         5171     5182      +11     
+ Misses        822      818       -4     
+ Partials      185      183       -2

Files	Coverage Δ
...ontrollers/spacerequest/spacerequest_controller.go	`83.75% <92.59%> (+1.34%)`	⬆️

... and 1 file with indirect coverage changes

mfrancisc · 2024-02-19T22:34:01Z

@Kartikey-star since you were assigned to work on new effort, I took the liberty to fetch the changes from your PR here and open a new PR #972 which has:

rebase with master
fix the conflicts
add the remaining changes

I'll work on doing the same for the toolchain-e2e tests PR
I hope you don't mind. Thanks a lot for your help here and great work!

openshift-ci bot added the do-not-merge/work-in-progress label Oct 23, 2023

Copy the SA token of sub-spaces only for specific tiers

de3c499

Signed-off-by: Kartikey Mamgain <kmamgain@redhat.com>

Kartikey-star force-pushed the sa_token_copy branch from b83f02c to de3c499 Compare October 23, 2023 06:21

mfrancisc reviewed Oct 23, 2023

View reviewed changes

Copy the SA token of sub-spaces only for specific tiers

47aca72

Signed-off-by: Kartikey Mamgain <kmamgain@redhat.com>

openshift-merge-robot added the needs-rebase label Oct 25, 2023

Merge branch 'master' into sa_token_copy

6117ff1

openshift-merge-robot removed the needs-rebase label Oct 25, 2023

Kartikey-star requested a review from mfrancisc October 25, 2023 05:59

Kartikey-star marked this pull request as ready for review October 25, 2023 05:59

Kartikey-star requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19 and sbryzak as code owners October 25, 2023 05:59

openshift-ci bot removed the do-not-merge/work-in-progress label Oct 25, 2023

Merge branch 'master' into sa_token_copy

973f674

mfrancisc reviewed Oct 27, 2023

View reviewed changes

mfrancisc added 5 commits October 30, 2023 09:24

Merge branch 'master' into sa_token_copy

34e49a8

Merge branch 'master' into sa_token_copy

2e57912

Merge branch 'master' into sa_token_copy

6651095

Merge branch 'master' into sa_token_copy

c0aa111

Merge branch 'master' into sa_token_copy

0cd9550

openshift-merge-robot added the needs-rebase label Nov 14, 2023

MatousJobanek mentioned this pull request Nov 15, 2023

feat: Introduce SpaceBindingLister function in handleRequestAndRedirect codeready-toolchain/registration-service#371

Merged

mfrancisc mentioned this pull request Feb 19, 2024

Copy the SA token of sub-spaces only for specific tiers #972

Merged

mfrancisc closed this Feb 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASC-402: Copy the SA token of sub-spaces only for specific tiers #902

ASC-402: Copy the SA token of sub-spaces only for specific tiers #902

Kartikey-star commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

mfrancisc left a comment

mfrancisc Oct 23, 2023

mfrancisc Oct 23, 2023

mfrancisc Oct 23, 2023

mfrancisc left a comment

mfrancisc Oct 27, 2023

mfrancisc Oct 27, 2023

mfrancisc Oct 27, 2023

sonarqubecloud bot commented Nov 13, 2023

openshift-merge-robot commented Nov 14, 2023

codecov bot commented Nov 14, 2023

mfrancisc commented Feb 19, 2024

	func (r *Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) (toolchainv1alpha1.NSTemplateTier, error) {
	func (r Reconciler) validateNSTemplateTier(ctx context.Context, tierName string) (toolchainv1alpha1.NSTemplateTier, error) {

	return &toolchainv1alpha1.NSTemplateTier{}, fmt.Errorf("tierName cannot be blank")
	return nil, fmt.Errorf("tierName cannot be blank")

		appstudioTier.Spec.SpaceRequestConfig.ServiceAccountName = "manager"
		t.Run("failure service account not present", func(t *testing.T) {

ASC-402: Copy the SA token of sub-spaces only for specific tiers #902

ASC-402: Copy the SA token of sub-spaces only for specific tiers #902

Conversation

Kartikey-star commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

openshift-ci bot commented Oct 23, 2023

mfrancisc left a comment

Choose a reason for hiding this comment

mfrancisc Oct 23, 2023

Choose a reason for hiding this comment

mfrancisc Oct 23, 2023

Choose a reason for hiding this comment

mfrancisc Oct 23, 2023

Choose a reason for hiding this comment

mfrancisc left a comment

Choose a reason for hiding this comment

mfrancisc Oct 27, 2023

Choose a reason for hiding this comment

mfrancisc Oct 27, 2023

Choose a reason for hiding this comment

mfrancisc Oct 27, 2023

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 13, 2023

openshift-merge-robot commented Nov 14, 2023

codecov bot commented Nov 14, 2023

Codecov Report

mfrancisc commented Feb 19, 2024