Merge pull request #101 from shaoxt/master

To support multiple service accounts on one IamRole
keikoproj · Aug 2, 2022 · d29a6e5 · d29a6e5
2 parents 359c8cf + 4523fb6
commit d29a6e5
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 30 deletions.
diff --git a/controllers/iamrole_controller.go b/controllers/iamrole_controller.go
@@ -190,11 +190,16 @@ func (r *IamroleReconciler) HandleReconcile(ctx context.Context, req ctrl.Reques
 
 		// If IRSA is enabled, make sure the service account has the needed annotations
 		saConsistent := false
-		if saExists, saName := utils.ParseIRSAAnnotation(ctx, iamRole); saExists {
-			// Get the service account in kubernetes
-			// If it exists, check the annotations are correct
-			if saSpec := k8s.NewK8sManagerClient(r.Client).GetServiceAccount(ctx, iamRole.Namespace, saName); saSpec != nil {
-				saConsistent = validation.CompareRoleIRSA(ctx, saSpec, *config.Props)
+		if saExists, saNames := utils.ParseIRSAAnnotation(ctx, iamRole); saExists {
+			for i := 0; i < len(saNames); i++ {
+				// Get the service account in kubernetes
+				// If it exists, check the annotations are correct
+				if saSpec := k8s.NewK8sManagerClient(r.Client).GetServiceAccount(ctx, iamRole.Namespace, saNames[i]); saSpec != nil {
+					saConsistent = validation.CompareRoleIRSA(ctx, saSpec, *config.Props)
+					if !saConsistent {
+						break
+					}
+				}
 			}
 		}
 		if validation.CompareRole(ctx, *input, targetRole, *targetPolicy) && saConsistent {
@@ -258,11 +263,13 @@ func (r *IamroleReconciler) HandleReconcile(ctx context.Context, req ctrl.Reques
 
 		//OK. Successful!!
 		// Is this IRSA role? If yes, Create/update Service Account with required annotation
-		if saFlag, saName := utils.ParseIRSAAnnotation(ctx, iamRole); saFlag {
-			if err := k8s.NewK8sManagerClient(r.Client).CreateOrUpdateServiceAccount(ctx, saName, iamRole.Namespace, resp.RoleARN, config.Props.IsIRSARegionalEndpointDisabled()); err != nil {
-				log.Error(err, "error in updating service account for IRSA role")
-				r.Recorder.Event(iamRole, v1.EventTypeWarning, string(iammanagerv1alpha1.Error), "Unable to create/update service account for IRSA role due to error "+err.Error())
-				return r.UpdateStatus(ctx, iamRole, iammanagerv1alpha1.IamroleStatus{RetryCount: iamRole.Status.RetryCount + 1, RoleName: roleName, ErrorDescription: err.Error(), State: iammanagerv1alpha1.Error, LastUpdatedTimestamp: metav1.Now()}, requeueTime)
+		if saFlag, saNames := utils.ParseIRSAAnnotation(ctx, iamRole); saFlag {
+			for i := 0; i < len(saNames); i++ {
+				if err := k8s.NewK8sManagerClient(r.Client).CreateOrUpdateServiceAccount(ctx, saNames[i], iamRole.Namespace, resp.RoleARN, config.Props.IsIRSARegionalEndpointDisabled()); err != nil {
+					log.Error(err, "error in updating service account for IRSA role")
+					r.Recorder.Event(iamRole, v1.EventTypeWarning, string(iammanagerv1alpha1.Error), "Unable to create/update service account for IRSA role due to error "+err.Error())
+					return r.UpdateStatus(ctx, iamRole, iammanagerv1alpha1.IamroleStatus{RetryCount: iamRole.Status.RetryCount + 1, RoleName: roleName, ErrorDescription: err.Error(), State: iammanagerv1alpha1.Error, LastUpdatedTimestamp: metav1.Now()}, requeueTime)
+				}
 			}
 		}
 

diff --git a/controllers/iamrole_controller_test.go b/controllers/iamrole_controller_test.go
@@ -5,7 +5,7 @@ import (
 	. "github.com/onsi/gomega"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
-		iammanagerv1alpha1 "github.com/keikoproj/iam-manager/api/v1alpha1"
+	iammanagerv1alpha1 "github.com/keikoproj/iam-manager/api/v1alpha1"
 	. "github.com/keikoproj/iam-manager/controllers"
 )
 

diff --git a/internal/utils/oidc.go b/internal/utils/oidc.go
@@ -8,6 +8,7 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
+	"strings"
 
 	"github.com/keikoproj/iam-manager/api/v1alpha1"
 	"github.com/keikoproj/iam-manager/internal/config"
@@ -78,6 +79,14 @@ func parseURL(ctx context.Context, idpUrl string) (string, error) {
 }
 
 //ParseIRSAAnnotation parses IAM role to see if the role to be used in IRSA method
-func ParseIRSAAnnotation(ctx context.Context, iamRole *v1alpha1.Iamrole) (bool, string) {
-	return parseAnnotations(ctx, config.IRSAAnnotation, iamRole.Annotations)
+func ParseIRSAAnnotation(ctx context.Context, iamRole *v1alpha1.Iamrole) (bool, []string) {
+	var exists, annoStr = parseAnnotations(ctx, config.IRSAAnnotation, iamRole.Annotations)
+	if exists {
+		array := strings.Split(annoStr, ",")
+		for i := 0; i < len(array); i++ {
+			array[i] = strings.Trim(array[i], " ")
+		}
+		return exists, array
+	}
+	return false, []string{}
 }
diff --git a/internal/utils/oidc_test.go b/internal/utils/oidc_test.go
@@ -54,9 +54,26 @@ func (s *OIDCTestSuite) TestParseIRSAAnnotationValid(c *check.C) {
 			},
 		},
 	}
-	flag, saName := utils.ParseIRSAAnnotation(s.ctx, input)
+	flag, saNames := utils.ParseIRSAAnnotation(s.ctx, input)
 	c.Assert(flag, check.Equals, true)
-	c.Assert(saName, check.Equals, "default")
+	c.Assert(saNames[0], check.Equals, "default")
+}
+
+func (s *OIDCTestSuite) TestParseIRSAAnnotationValidArray(c *check.C) {
+	input := &v1alpha1.Iamrole{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "iam-role",
+			Namespace: "k8s-namespace-dev",
+			Annotations: map[string]string{
+				config.IRSAAnnotation: "default, my-sa",
+			},
+		},
+	}
+	flag, saNames := utils.ParseIRSAAnnotation(s.ctx, input)
+	c.Assert(flag, check.Equals, true)
+	c.Assert(saNames[0], check.Equals, "default")
+	c.Assert(saNames[1], check.Equals, "my-sa")
+
 }
 
 func (s *OIDCTestSuite) TestParseIRSAAnnotationOtherAnnotations(c *check.C) {
@@ -68,9 +85,9 @@ func (s *OIDCTestSuite) TestParseIRSAAnnotationOtherAnnotations(c *check.C) {
 			},
 		},
 	}
-	flag, saName := utils.ParseIRSAAnnotation(s.ctx, input)
+	flag, saNames := utils.ParseIRSAAnnotation(s.ctx, input)
 	c.Assert(flag, check.Equals, false)
-	c.Assert(saName, check.HasLen, 0)
+	c.Assert(len(saNames), check.Equals, 0)
 }
 
 func (s *OIDCTestSuite) TestGetIdpServerCertThumbprintSuccess(c *check.C) {

diff --git a/internal/utils/utils.go b/internal/utils/utils.go
@@ -25,21 +25,24 @@ func GetTrustPolicy(ctx context.Context, role *iammanagerv1alpha1.Iamrole) (stri
 
 	// Is it IRSA use case
 	// Construct AssumeRoleWithWebIdentity
-	if flag, saName := ParseIRSAAnnotation(ctx, role); flag {
-		hostPath := fmt.Sprintf("%s", strings.TrimPrefix(config.Props.OIDCIssuerUrl(), "https://"))
-		statement := iammanagerv1alpha1.TrustPolicyStatement{
-			Effect: "Allow",
-			Action: "sts:AssumeRoleWithWebIdentity",
-			Principal: iammanagerv1alpha1.Principal{
-				Federated: fmt.Sprintf("arn:aws:iam::%s:oidc-provider/%s", config.Props.AWSAccountID(), hostPath),
-			},
-			Condition: &iammanagerv1alpha1.Condition{
-				StringEquals: map[string]string{
-					fmt.Sprintf("%s:sub", hostPath): fmt.Sprintf("system:serviceaccount:%s:%s", role.ObjectMeta.Namespace, saName),
+	if flag, saNames := ParseIRSAAnnotation(ctx, role); flag {
+		for i := 0; i < len(saNames); i++ {
+			saName := saNames[i]
+			hostPath := fmt.Sprintf("%s", strings.TrimPrefix(config.Props.OIDCIssuerUrl(), "https://"))
+			statement := iammanagerv1alpha1.TrustPolicyStatement{
+				Effect: "Allow",
+				Action: "sts:AssumeRoleWithWebIdentity",
+				Principal: iammanagerv1alpha1.Principal{
+					Federated: fmt.Sprintf("arn:aws:iam::%s:oidc-provider/%s", config.Props.AWSAccountID(), hostPath),
 				},
-			},
+				Condition: &iammanagerv1alpha1.Condition{
+					StringEquals: map[string]string{
+						fmt.Sprintf("%s:sub", hostPath): fmt.Sprintf("system:serviceaccount:%s:%s", role.ObjectMeta.Namespace, saName),
+					},
+				},
+			}
+			statements = append(statements, statement)
 		}
-		statements = append(statements, statement)
 
 	} else {
 		// NON - IRSA which should cover AssumeRole usecase