Skip to content
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

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

Closed
wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
de3c499
Copy the SA token of sub-spaces only for specific tiers
Kartikey-star Oct 20, 2023
47aca72
Copy the SA token of sub-spaces only for specific tiers
Kartikey-star Oct 25, 2023
6117ff1
Merge branch 'master' into sa_token_copy
Kartikey-star Oct 25, 2023
973f674
Merge branch 'master' into sa_token_copy
Kartikey-star Oct 26, 2023
34e49a8
Merge branch 'master' into sa_token_copy
mfrancisc Oct 30, 2023
2e57912
Merge branch 'master' into sa_token_copy
mfrancisc Nov 6, 2023
6651095
Merge branch 'master' into sa_token_copy
mfrancisc Nov 9, 2023
c0aa111
Merge branch 'master' into sa_token_copy
mfrancisc Nov 10, 2023
0cd9550
Merge branch 'master' into sa_token_copy
mfrancisc Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 27 additions & 25 deletions controllers/spacerequest/spacerequest_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,18 +104,19 @@
return reconcile.Result{}, err
}

subSpace, createdOrUpdated, err := r.ensureSpace(ctx, memberClusterWithSpaceRequest, spaceRequest)
subSpace, tier, createdOrUpdated, err := r.ensureSpace(ctx, memberClusterWithSpaceRequest, spaceRequest)
// if there was an error or if subSpace was just created or updated,
// let's just return.
if err != nil || createdOrUpdated {
return ctrl.Result{}, err
}

// ensure there is a secret that provides admin access to each provisioned namespaces of the subSpace
if err := r.ensureSecretForProvisionedNamespaces(ctx, memberClusterWithSpaceRequest, spaceRequest, subSpace); err != nil {
return reconcile.Result{}, r.setStatusFailedToCreateSubSpace(ctx, memberClusterWithSpaceRequest, spaceRequest, err)
if tier.Spec.SpaceRequestConfig != nil && tier.Spec.SpaceRequestConfig.ServiceAccountName != "" {
// ensure there is a secret that provides admin access to each provisioned namespaces of the subSpace
if err := r.ensureSecretForProvisionedNamespaces(ctx, memberClusterWithSpaceRequest, spaceRequest, subSpace, tier.Spec.SpaceRequestConfig.ServiceAccountName); err != nil {
return reconcile.Result{}, r.setStatusFailedToCreateSubSpace(ctx, memberClusterWithSpaceRequest, spaceRequest, err)
}
mfrancisc marked this conversation as resolved.
Show resolved Hide resolved
}

// update spaceRequest conditions and target cluster url
err = r.updateSpaceRequest(ctx, memberClusterWithSpaceRequest, spaceRequest, subSpace)

Expand Down Expand Up @@ -160,25 +161,26 @@
return nil
}

func (r *Reconciler) ensureSpace(ctx context.Context, memberCluster cluster.Cluster, spaceRequest *toolchainv1alpha1.SpaceRequest) (*toolchainv1alpha1.Space, bool, error) {
func (r *Reconciler) ensureSpace(ctx context.Context, memberCluster cluster.Cluster, spaceRequest *toolchainv1alpha1.SpaceRequest) (*toolchainv1alpha1.Space, *toolchainv1alpha1.NSTemplateTier, bool, error) {
logger := log.FromContext(ctx)
logger.Info("ensuring subSpace")

// find parent space from namespace labels
parentSpace, err := r.getParentSpace(ctx, memberCluster, spaceRequest)
if err != nil {
return nil, false, err
return nil, nil, false, err
}
// parentSpace is being deleted
if util.IsBeingDeleted(parentSpace) {
return nil, false, errs.New("parentSpace is being deleted")
return nil, nil, false, errs.New("parentSpace is being deleted")
}

var tier *toolchainv1alpha1.NSTemplateTier

// validate tierName
if err := r.validateNSTemplateTier(ctx, spaceRequest.Spec.TierName); err != nil {
return nil, false, err
if tier, err = r.validateNSTemplateTier(ctx, spaceRequest.Spec.TierName); err != nil {
return nil, nil, false, err
}

// create if not found on the expected target cluster
subSpace := &toolchainv1alpha1.Space{}
if err := r.Client.Get(ctx, types.NamespacedName{
Expand All @@ -189,21 +191,21 @@
// no spaces found, let's create it
logger.Info("creating subSpace")
if err := r.setStatusProvisioning(ctx, memberCluster, spaceRequest); err != nil {
return nil, false, errs.Wrap(err, "error updating status")
return nil, nil, false, errs.Wrap(err, "error updating status")

Check warning on line 194 in controllers/spacerequest/spacerequest_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/spacerequest/spacerequest_controller.go#L194 

Added line #L194 was not covered by tests
}
subSpace, err = r.createNewSubSpace(ctx, spaceRequest, parentSpace)
if err != nil {
// failed to create subSpace
return nil, false, r.setStatusFailedToCreateSubSpace(ctx, memberCluster, spaceRequest, err)
return nil, nil, false, r.setStatusFailedToCreateSubSpace(ctx, memberCluster, spaceRequest, err)
}
return subSpace, true, nil // a subSpace was created
return subSpace, tier, true, nil // a subSpace was created
}
// failed to create subSpace
return nil, false, r.setStatusFailedToCreateSubSpace(ctx, memberCluster, spaceRequest, err)
return nil, nil, false, r.setStatusFailedToCreateSubSpace(ctx, memberCluster, spaceRequest, err)

Check warning on line 204 in controllers/spacerequest/spacerequest_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/spacerequest/spacerequest_controller.go#L204 

Added line #L204 was not covered by tests
}
logger.Info("subSpace already exists")
updated, err := r.updateExistingSubSpace(ctx, spaceRequest, subSpace)
return subSpace, updated, err
return subSpace, tier, updated, err
}

func (r *Reconciler) createNewSubSpace(ctx context.Context, spaceRequest *toolchainv1alpha1.SpaceRequest, parentSpace *toolchainv1alpha1.Space) (*toolchainv1alpha1.Space, error) {
Expand Down Expand Up @@ -237,9 +239,9 @@
}

// 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) {
if tierName == "" {
return fmt.Errorf("tierName cannot be blank")
return &toolchainv1alpha1.NSTemplateTier{}, fmt.Errorf("tierName cannot be blank")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

}
// check if requested tier exists
tier := &toolchainv1alpha1.NSTemplateTier{}
Expand All @@ -248,12 +250,12 @@
Name: tierName,
}, tier); err != nil {
if errors.IsNotFound(err) {
return err
return tier, err
}
// Error reading the object - requeue the request.
return errs.Wrap(err, "unable to get the current NSTemplateTier")
return tier, errs.Wrap(err, "unable to get the current NSTemplateTier")
}
return nil
return tier, nil
}

// updateSubSpace updates the tierName and targetClusterRoles from the spaceRequest to the subSpace object
Expand Down Expand Up @@ -397,7 +399,7 @@
return true, nil
}

func (r *Reconciler) ensureSecretForProvisionedNamespaces(ctx context.Context, memberClusterWithSpaceRequest cluster.Cluster, spaceRequest *toolchainv1alpha1.SpaceRequest, subSpace *toolchainv1alpha1.Space) error {
func (r *Reconciler) ensureSecretForProvisionedNamespaces(ctx context.Context, memberClusterWithSpaceRequest cluster.Cluster, spaceRequest *toolchainv1alpha1.SpaceRequest, subSpace *toolchainv1alpha1.Space, serviceAccountName string) error {
logger := log.FromContext(ctx)

if len(subSpace.Status.ProvisionedNamespaces) == 0 {
Expand Down Expand Up @@ -427,7 +429,7 @@
switch {
case len(secretList.Items) == 0:
// create the secret for this namespace
clientConfig, err := r.generateKubeConfig(subSpaceTargetCluster, namespace.Name)
clientConfig, err := r.generateKubeConfig(subSpaceTargetCluster, namespace.Name, serviceAccountName)
if err != nil {
return err
}
Expand Down Expand Up @@ -483,11 +485,11 @@
return nil
}

func (r *Reconciler) generateKubeConfig(subSpaceTargetCluster cluster.Cluster, namespace string) (*api.Config, error) {
func (r *Reconciler) generateKubeConfig(subSpaceTargetCluster cluster.Cluster, namespace, serviceAccountName string) (*api.Config, error) {
// create a token request for the admin service account
token, err := restclient.CreateTokenRequest(subSpaceTargetCluster.RESTClient, types.NamespacedName{
Namespace: namespace,
Name: toolchainv1alpha1.AdminServiceAccountName,
Name: serviceAccountName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}, TokenRequestExpirationSeconds)
if err != nil {
return nil, err
Expand Down
36 changes: 36 additions & 0 deletions controllers/spacerequest/spacerequest_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,42 @@ func TestCreateSpaceRequest(t *testing.T) {
})
})

appstudioTier.Spec.SpaceRequestConfig.ServiceAccountName = "manager"
t.Run("failure service account not present", func(t *testing.T) {
Comment on lines +395 to +396
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sr := spacerequesttest.NewSpaceRequest("jane", srNamespace.GetName(),
spacerequesttest.WithTierName("appstudio"),
spacerequesttest.WithTargetClusterRoles(srClusterRoles))
t.Run("subSpace is provisioned", func(t *testing.T) {
// given
member1 := NewMemberClusterWithClient(test.NewFakeClient(t, sr, srNamespace), "member-1", corev1.ConditionTrue)
commontest.SetupGockForServiceAccounts(t, member1.APIEndpoint, types.NamespacedName{
Name: toolchainv1alpha1.AdminServiceAccountName,
Namespace: "jane-env",
})
subSpace := spacetest.NewSpace(test.HostOperatorNs, spaceutil.SubSpaceName(parentSpace, sr),
spacetest.WithLabel(toolchainv1alpha1.SpaceRequestLabelKey, sr.GetName()), // subSpace was created from spaceRequest
spacetest.WithLabel(toolchainv1alpha1.SpaceRequestNamespaceLabelKey, sr.GetNamespace()), // subSpace was created from spaceRequest
spacetest.WithLabel(toolchainv1alpha1.ParentSpaceLabelKey, "jane"),
spacetest.WithCondition(spacetest.Ready()),
spacetest.WithSpecParentSpace("jane"),
spacetest.WithSpecTargetClusterRoles(srClusterRoles),
spacetest.WithStatusTargetCluster(member1.Name),
spacetest.WithStatusProvisionedNamespaces([]toolchainv1alpha1.SpaceNamespace{{
Name: "jane-env",
Type: "default",
}}),
spacetest.WithTierName(sr.Spec.TierName))
hostClient := test.NewFakeClient(t, appstudioTier, parentSpace, subSpace)
ctrl := newReconciler(t, hostClient, member1)
// when
_, err = ctrl.Reconcile(context.TODO(), requestFor(sr))
// then
require.Error(t, err)
require.Contains(t, err.Error(), "Post \"https://api.member-1:6433/api/v1/namespaces/jane-env/serviceaccounts/manager/token\": gock: cannot match any request")
})
})

appstudioTier.Spec.SpaceRequestConfig.ServiceAccountName = "namespace-manager"
t.Run("failure", func(t *testing.T) {
// given
sr := spacerequesttest.NewSpaceRequest("jane",
Expand Down
2 changes: 2 additions & 0 deletions deploy/templates/nstemplatetiers/appstudio-env/tier.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ objects:
templateRef: ${MAINTAINER_TEMPL_REF}
contributor:
templateRef: ${CONTRIBUTOR_TEMPL_REF}
spaceRequestConfig:
serviceAccountName: namespace-manager
parameters:
- name: NAMESPACE
- name: CLUSTER_TEMPL_REF
Expand Down
3 changes: 3 additions & 0 deletions test/nstemplatetier/nstemplatetier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,9 @@ var AppStudioTemplates = toolchainv1alpha1.NSTemplateTierSpec{
TemplateRef: "appstudio-viewer-123456new",
},
},
SpaceRequestConfig: &toolchainv1alpha1.SpaceRequestConfig{
ServiceAccountName: "namespace-manager",
},
}

// Base1nsTier returns a "base1ns" NSTemplateTier with template refs in the given spec
Expand Down
Loading