add support for customer SA namespace in snap meta lister tool #81

Rakshith-R · 2024-12-02T12:29:59Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Add support for customer SA namespace in snap meta lister tool.
Currently SA namespace is picked up from snapshot namespace which is very limiting.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2024-12-02T12:30:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Rakshith-R
Once this PR has been reviewed and has the lgtm label, please assign prasadg193 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

k8s-ci-robot · 2024-12-02T12:30:11Z

Hi @Rakshith-R. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

carlbraganza

Good functional addition.

Some minor issues...

carlbraganza · 2024-12-02T18:51:29Z

pkg/iterator/iter.go

+func (iter *iterator) createSecurityToken(ctx context.Context,
+	serviceAccount,
+	serviceAccountNamespace,
+	audience string) (string, error) {


Please don't break the arguments over multiple lines.

Since you've introduced serviceAccountNamespace please rename serviceAccount to serviceAccountName. It would be acceptable to shorten these to saName and saNamespace.

If possible, put the namespace argument before the name argument.

carlbraganza · 2024-12-02T18:52:54Z

pkg/iterator/iter.go

+	securityToken, err := iter.h.createSecurityToken(ctx,
+		serviceAccount,
+		serviceAccountNamespace,
+		smsCR.Spec.Audience)


Please stick with the style: I generally avoid multiple lines in a function call except if an argument data structure is assembled as part of the call. Exceptions are allowed of course, when the argument list gets complex, but this is not such a case.

carlbraganza · 2024-12-02T19:04:30Z

pkg/iterator/iter.go

 	tokenRequest := authv1.TokenRequest{
 		Spec: authv1.TokenRequestSpec{
 			Audiences:         []string{audience},
 			ExpirationSeconds: &iter.TokenExpirySecs,
 		},
 	}

-	tokenResp, err := iter.KubeClient.CoreV1().ServiceAccounts(iter.Namespace).CreateToken(ctx, serviceAccount, &tokenRequest, apimetav1.CreateOptions{})
+	tokenResp, err := iter.KubeClient.CoreV1().ServiceAccounts(serviceAccountNamespace).
+		CreateToken(ctx, serviceAccount, &tokenRequest, apimetav1.CreateOptions{})
 	if err != nil {
 		return "", fmt.Errorf("ServiceAccounts.CreateToken(%s): %v", serviceAccount, err)


You should change the error message to:

fmt.Errorf("ServiceAccounts.CreateToken(%s/%s): %v", serviceAccountNamespace, serviceAccount, err)

carlbraganza · 2024-12-02T19:08:00Z

pkg/iterator/iter.go

+	// ServiceAccountNamespace is the namespace of the ServiceAccount.
+	ServiceAccountNamespace string


This is optional and should be stated so. I suggest combining the comment for the two as follows:

// Specify the ServiceAccount object used to construct a security token // with the audience string from the SnapshotMetadataService CR. // If either of the following fields are unspecified, the default for the given client will be used. ServiceAccountNamespace string ServiceAccount string

It would be good if ServiceAccount was renamed to ServiceAccountName.

The args.Validate function should be enhanced to ensure that either both fields are empty or both are set - impacts validation UTs too.

carlbraganza · 2024-12-02T19:13:13Z

pkg/iterator/iter.go

@@ -191,9 +194,10 @@ func newIterator(args Args) *iterator {
 func (iter *iterator) run(ctx context.Context) error {
 	var err error

-	serviceAccount := iter.ServiceAccount // optional field
+	serviceAccount := iter.ServiceAccount                   // optional field
+	serviceAccountNamespace := iter.ServiceAccountNamespace // optional field
 	if serviceAccount == "" {


The check for an empty name (only) is fine as long as args.Validate ensures both or none semantics.

Signed-off-by: Rakshith R <[email protected]>

carlbraganza

Looking good. Minor nits ...

carlbraganza · 2024-12-03T19:04:04Z

pkg/iterator/iter.go

+	case a.SANamespace == "" && a.SAName != "":
+		return fmt.Errorf("%w: ServiceAccountName provided but ServiceAccountNamespace missing", ErrInvalidArgs)
+	case a.SANamespace != "" && a.SAName == "":
+		return fmt.Errorf("%w: ServiceAccountNamespace provided but ServiceAccountName missing", ErrInvalidArgs)


nit: The error message should refer to the fields - you're now calling them SAName and SANamespace and not ServiceAccountName and ServiceAccountNamespace

carlbraganza · 2024-12-03T19:08:06Z

pkg/iterator/iter.go

@@ -242,20 +248,19 @@ func (iter *iterator) run(ctx context.Context) error {
 	return err
 }

-func (iter *iterator) getDefaultServiceAccount(ctx context.Context) (string, error) {
+func (iter *iterator) getDefaultServiceAccount(ctx context.Context) (string, string, error) {


nit: Could you please make it clear in the function signature which return argument is the name and which is the namespace? e.g.

func (iter *iterator) getDefaultServiceAccount(ctx context.Context) (namespace string, name string, err error)

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Dec 2, 2024

k8s-ci-robot requested review from carlbraganza and PrasadG193 December 2, 2024 12:30

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 2, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 2, 2024

carlbraganza suggested changes Dec 2, 2024

View reviewed changes

Rakshith-R force-pushed the add-default-sa-ns branch from b0c095e to 9c56b79 Compare December 3, 2024 15:46

add support for customer SA namespace in snap meta lister tool

6623cdb

Signed-off-by: Rakshith R <[email protected]>

Rakshith-R force-pushed the add-default-sa-ns branch from 9c56b79 to 6623cdb Compare December 3, 2024 15:52

Rakshith-R requested a review from carlbraganza December 3, 2024 15:53

carlbraganza suggested changes Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for customer SA namespace in snap meta lister tool #81

add support for customer SA namespace in snap meta lister tool #81

Rakshith-R commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

carlbraganza left a comment

carlbraganza Dec 2, 2024

carlbraganza Dec 2, 2024

carlbraganza Dec 2, 2024

carlbraganza Dec 2, 2024

carlbraganza Dec 2, 2024

carlbraganza left a comment

carlbraganza Dec 3, 2024

carlbraganza Dec 3, 2024

		// ServiceAccountNamespace is the namespace of the ServiceAccount.
		ServiceAccountNamespace string

add support for customer SA namespace in snap meta lister tool #81

Are you sure you want to change the base?

add support for customer SA namespace in snap meta lister tool #81

Conversation

Rakshith-R commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

k8s-ci-robot commented Dec 2, 2024

carlbraganza left a comment

Choose a reason for hiding this comment

carlbraganza Dec 2, 2024

Choose a reason for hiding this comment

carlbraganza Dec 2, 2024

Choose a reason for hiding this comment

carlbraganza Dec 2, 2024

Choose a reason for hiding this comment

carlbraganza Dec 2, 2024

Choose a reason for hiding this comment

carlbraganza Dec 2, 2024

Choose a reason for hiding this comment

carlbraganza left a comment

Choose a reason for hiding this comment

carlbraganza Dec 3, 2024

Choose a reason for hiding this comment

carlbraganza Dec 3, 2024

Choose a reason for hiding this comment