Repository: fission/fission
Pull request #2673 (2022-12-15 06:49:53)

Allow service account check to run only once at start of executor
Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>

<!--  Thanks for sending a pull request! We request you provide detailed description as much as possible. -->

## Description
<!--- Describe your changes in detail. -->
<!-- Typically try to give details of what, why and how of the PR changes. -->
- Allow setting interval to 0 so that service account check only once
- Set namespace in LocalSubjectAccessReview 
- Change log level to Info for important logs

## Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
-->
Fixes #

## Testing
<!--- Please describe in detail how you tested your changes. -->

## Checklist:
<!-- Please tick following checkboxes as per your understanding. -->
- [ ] I ran tests as well as code linting locally to verify my changes. 
- [ ] I have done manual verification of my changes, changes working as expected.
- [ ] I have added new tests to cover my changes.
- [ ] My changes follow contributing guidelines of Fission.
- [ ] I have signed all of my commits.


codecov[bot]: # [Codecov](https://codecov.io/gh/fission/fission/pull/2673?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission) Report
> Merging [#2673](https://codecov.io/gh/fission/fission/pull/2673?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission) (d362300) into [main](https://codecov.io/gh/fission/fission/commit/3e25f474b0f560a2ece3bff1be8a943898d16372?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission) (3e25f47) will **increase** coverage by `1.97%`.
> The diff coverage is `76.47%`.

```diff
@@            Coverage Diff             @@
##             main    #2673      +/-   ##
==========================================
+ Coverage   19.14%   21.12%   +1.97%     
==========================================
  Files          65       65              
  Lines        7209     7211       +2     
==========================================
+ Hits         1380     1523     +143     
+ Misses       5727     5574     -153     
- Partials      102      114      +12     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `21.12% <76.47%> (+1.97%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/fission/fission/pull/2673?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission) | Coverage Δ | |
|---|---|---|
| [pkg/utils/serviceaccount.go](https://codecov.io/gh/fission/fission/pull/2673/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission#diff-cGtnL3V0aWxzL3NlcnZpY2VhY2NvdW50Lmdv) | `77.90% <76.47%> (+77.90%)` | :arrow_up: |
| [pkg/utils/namespace.go](https://codecov.io/gh/fission/fission/pull/2673/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission#diff-cGtnL3V0aWxzL25hbWVzcGFjZS5nbw==) | `63.54% <0.00%> (+2.08%)` | :arrow_up: |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=fission)


Patch:
diff --git a/charts/fission-all/values.yaml b/charts/fission-all/values.yaml
index f68ef75c..a55368b9 100644
--- a/charts/fission-all/values.yaml
+++ b/charts/fission-all/values.yaml
@@ -192,8 +192,9 @@ executor:
     enabled: true
     ## indicates the time interval in minutes, after that fission will create service account, roles and rolebinding for builder and fetcher.
     ## interval will be applicable only if enable value is set to true.
-    ## default timing will be 30 minutes.
-    interval: 30  
+    ## default timing will be 0 minutes. That means check will run only once.
+    ## if you want to run check every 30 minutes then set interval to 30.
+    interval: 0
 ## router is responsible for routing function calls to the appropriate function.
 ##
 router:
diff --git a/pkg/utils/serviceaccount.go b/pkg/utils/serviceaccount.go
index 98314994..19216309 100644
--- a/pkg/utils/serviceaccount.go
+++ b/pkg/utils/serviceaccount.go
@@ -97,7 +97,12 @@ func CreateMissingPermissionForSA(ctx context.Context, kubernetesClient kubernet
 		interval := getSAInterval()
 		logger.Debug("interval value", zap.Any("interval", interval))
 		sa := getSAObj(ctx, kubernetesClient, logger)
-		go sa.doSACheck(ctx, interval)
+		logger.Info("Starting service account check", zap.Any("interval", interval))
+		if interval > 0 {
+			go wait.UntilWithContext(ctx, sa.runSACheck, interval)
+		} else {
+			sa.runSACheck(ctx)
+		}
 	}
 }
 
@@ -112,10 +117,6 @@ func getSAObj(ctx context.Context, kubernetesClient kubernetes.Interface, logger
 	return saObj
 }
 
-func (sa *ServiceAccount) doSACheck(ctx context.Context, interval time.Duration) {
-	wait.UntilWithContext(ctx, sa.runSACheck, interval)
-}
-
 func (sa *ServiceAccount) runSACheck(ctx context.Context) {
 	for _, ns := range sa.nsResolver.FissionResourceNS {
 		for _, permission := range sa.permissions {
@@ -133,7 +134,7 @@ func setupSAAndRoleBindings(ctx context.Context, client kubernetes.Interface, lo
 	SAObj, err := createGetSA(ctx, client, ps.saName, namespace)
 	if err != nil {
 		logger.Error("error while creating or getting service account",
-			zap.String("SA_name", ps.saName),
+			zap.String("sa_name", ps.saName),
 			zap.String("namespace", namespace),
 			zap.Error(err))
 		return
@@ -179,7 +180,7 @@ func setupSAAndRoleBindings(ctx context.Context, client kubernetes.Interface, lo
 func setupRoles(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, sa *v1.ServiceAccount, rules []rbac.PolicyRule, suffix string) (*rbac.Role, error) {
 	logger.Debug("creating role",
 		zap.String("role_name", fmt.Sprintf("%s-role-%s", sa.Name, suffix)),
-		zap.String("SA_Name", sa.Name),
+		zap.String("sa_name", sa.Name),
 		zap.String("namespace", sa.Namespace))
 
 	roleObj := &rbac.Role{
@@ -193,17 +194,17 @@ func setupRoles(ctx context.Context, client kubernetes.Interface, logger *zap.Lo
 	if err != nil {
 		return nil, fmt.Errorf("error while creating role for sa %s in namespace %s error: %s", sa.Name, sa.Namespace, err.Error())
 	}
-	logger.Debug("role created successfully",
+	logger.Info("role created successfully",
 		zap.String("role_name", role.Name),
 		zap.String("namespace", role.Namespace),
-		zap.String("SA_Name", sa.Name))
+		zap.String("sa_name", sa.Name))
 	return role, nil
 }
 
 func setupRoleBinding(ctx context.Context, client kubernetes.Interface, logger *zap.Logger, sa *v1.ServiceAccount, role *rbac.Role, suffix string) (*rbac.RoleBinding, error) {
 	logger.Debug("creating role binding",
 		zap.String("rolebinding_name", fmt.Sprintf("%s-rolebinding-%s", sa.Name, suffix)),
-		zap.String("SA_Name", sa.Name),
+		zap.String("sa_name", sa.Name),
 		zap.String("namespace", sa.Namespace))
 
 	roleBindingObj := &rbac.RoleBinding{
@@ -227,16 +228,19 @@ func setupRoleBinding(ctx context.Context, client kubernetes.Interface, logger *
 	if err != nil {
 		return nil, fmt.Errorf("error while creating rolebinding for sa %s in namespace %s error: %s", sa.Name, sa.Namespace, err.Error())
 	}
-	logger.Debug("role binding created successfully",
+	logger.Info("role binding created successfully",
 		zap.String("rolebinding_name", roleBinding.Name),
 		zap.String("namespace", roleBinding.Namespace),
-		zap.String("SA_Name", sa.Name))
+		zap.String("sa_name", sa.Name))
 	return roleBinding, nil
 }
 
 func checkPermission(ctx context.Context, client kubernetes.Interface, sa *v1.ServiceAccount, gvr *schema.GroupVersionResource, verb string) (bool, error) {
 	user := fmt.Sprintf("system:serviceaccount:%s:%s", sa.Namespace, sa.Name)
 	sar := authorizationv1.LocalSubjectAccessReview{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: sa.Namespace,
+		},
 		Spec: authorizationv1.SubjectAccessReviewSpec{
 			ResourceAttributes: &authorizationv1.ResourceAttributes{
 				Namespace: sa.Namespace,
@@ -298,9 +302,6 @@ func createServiceAccount() bool {
 }
 
 func getSAInterval() time.Duration {
-	SAInterval, err := GetUIntValueFromEnv(ENV_SA_INTERVAL)
-	if err != nil {
-		return time.Duration(30) * time.Minute
-	}
+	SAInterval, _ := GetUIntValueFromEnv(ENV_SA_INTERVAL)
 	return time.Duration(SAInterval) * time.Minute
 }


Test patch:
diff --git a/pkg/utils/serviceaccount_test.go b/pkg/utils/serviceaccount_test.go
new file mode 100644
index 00000000..60f5282b
--- /dev/null
+++ b/pkg/utils/serviceaccount_test.go
@@ -0,0 +1,37 @@
+package utils
+
+import (
+	"context"
+	"os"
+	"regexp"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/fake"
+
+	"github.com/fission/fission/pkg/utils/loggerfactory"
+)
+
+func TestServiceAccountCheck(t *testing.T) {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	kubernetesClient := fake.NewSimpleClientset()
+	logger := loggerfactory.GetLogger()
+	os.Setenv(ENV_CREATE_SA, "true")
+	CreateMissingPermissionForSA(ctx, kubernetesClient, logger)
+
+	// Get rolebinding for a service account
+	rolebindings, err := kubernetesClient.RbacV1().RoleBindings("default").List(ctx, metav1.ListOptions{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(rolebindings.Items) != 2 {
+		t.Fatal("Rolebinding not created", len(rolebindings.Items))
+	}
+	regexp := regexp.MustCompile(`fission\-(fetcher|builder)\-rolebinding\-[a-z0-9]{6}`)
+	for _, rolebinding := range rolebindings.Items {
+		if !regexp.Match([]byte(rolebinding.Name)) {
+			t.Fatal("Rolebinding not created for fission-builder or fission-fetcher", rolebinding.Name)
+		}
+	}
+}