OCM-10511 | fix: missing operator roles when filtering due to extende…

…d naming cutting the postfix
openshift · Aug 20, 2024 · f181e00 · f181e00
1 parent 6816d0d
commit f181e00
Show file tree

Hide file tree

Showing 2 changed files with 125 additions and 9 deletions.
diff --git a/pkg/aws/policies.go b/pkg/aws/policies.go
@@ -630,7 +630,7 @@ func isAccountRoleVersionCompatible(tagsList []iamtypes.Tag, roleType string,
 }
 
 func (c *awsClient) ListRoles() ([]iamtypes.Role, error) {
-	roles := []iamtypes.Role{}
+	var roles []iamtypes.Role
 	paginator := iam.NewListRolesPaginator(c.iamClient, &iam.ListRolesInput{})
 	for paginator.HasMorePages() {
 		output, err := paginator.NextPage(context.TODO())
@@ -994,6 +994,18 @@ func (c *awsClient) ListOperatorRoles(targetVersion string,
 	return operatorMap, nil
 }
 
+func (c *awsClient) ValidateIfRosaOperatorRole(role iamtypes.Role,
+	credRequest map[string]*cmv1.STSOperator) (bool, error) {
+	listRoleTags, err := c.iamClient.ListRoleTags(context.Background(), &iam.ListRoleTagsInput{
+		RoleName: role.RoleName,
+	})
+	if err != nil {
+		return false, err
+	}
+	role.Tags = listRoleTags.Tags
+	return checkIfROSAOperatorRole(role, credRequest), nil
+}
+
 // Check if it is one of the ROSA account roles
 func checkIfAccountRole(roleName *string) bool {
 	for _, prefix := range AccountRoles {
@@ -1005,9 +1017,16 @@ func checkIfAccountRole(roleName *string) bool {
 }
 
 // Check if it is one of the ROSA account roles
-func checkIfROSAOperatorRole(roleName *string, credRequest map[string]*cmv1.STSOperator) bool {
+func checkIfROSAOperatorRole(role iamtypes.Role, credRequest map[string]*cmv1.STSOperator) bool {
 	for _, operatorRole := range credRequest {
-		if strings.Contains(aws.ToString(roleName), operatorRole.Namespace()) {
+		for _, tag := range role.Tags {
+			if aws.ToString(tag.Key) == "operator_namespace" {
+				if strings.Contains(aws.ToString(tag.Value), operatorRole.Namespace()) {
+					return true
+				}
+			}
+		}
+		if strings.Contains(aws.ToString(role.RoleName), operatorRole.Namespace()) {
 			return true
 		}
 	}
@@ -1341,15 +1360,20 @@ func (c *awsClient) detachOperatorRolePolicies(role *string) error {
 
 func (c *awsClient) GetOperatorRolesFromAccountByClusterID(clusterID string,
 	credRequest map[string]*cmv1.STSOperator) ([]string, error) {
-	roleList := []string{}
+	var roleList []string
 	roles, err := c.ListRoles()
 	if err != nil {
 		return roleList, err
 	}
 	for _, role := range roles {
-		if !checkIfROSAOperatorRole(role.RoleName, credRequest) {
+		isValidOperatorRole, err := c.ValidateIfRosaOperatorRole(role, credRequest)
+		if err != nil {
+			return roleList, err
+		}
+		if !isValidOperatorRole {
 			continue
 		}
+
 		listRoleTagsOutput, err := c.iamClient.ListRoleTags(context.Background(),
 			&iam.ListRoleTagsInput{
 				RoleName: role.RoleName,
@@ -1376,19 +1400,24 @@ func (c *awsClient) GetOperatorRolesFromAccountByClusterID(clusterID string,
 
 func (c *awsClient) GetOperatorRolesFromAccountByPrefix(prefix string,
 	credRequest map[string]*cmv1.STSOperator) ([]string, error) {
-	roleList := []string{}
+	var roleList []string
 	roles, err := c.ListRoles()
 	if err != nil {
 		return roleList, err
 	}
 	prefixOperatorRoleRE := regexp.MustCompile(("(?i)" + fmt.Sprintf("(%s)-(openshift|kube-system)", prefix)))
 	for _, role := range roles {
-		if !checkIfROSAOperatorRole(role.RoleName, credRequest) {
+		if !prefixOperatorRoleRE.MatchString(*role.RoleName) {
 			continue
 		}
-		if prefixOperatorRoleRE.MatchString(*role.RoleName) {
-			roleList = append(roleList, aws.ToString(role.RoleName))
+		isValidOperatorRole, err := c.ValidateIfRosaOperatorRole(role, credRequest)
+		if err != nil {
+			return roleList, err
+		}
+		if !isValidOperatorRole {
+			continue
 		}
+		roleList = append(roleList, aws.ToString(role.RoleName))
 	}
 	return roleList, nil
 }

diff --git a/pkg/aws/policies_test.go b/pkg/aws/policies_test.go
@@ -13,6 +13,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	common "github.com/openshift-online/ocm-common/pkg/aws/validations"
+	cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
 	"github.com/sirupsen/logrus"
 
 	"github.com/openshift/rosa/pkg/aws/mocks"
@@ -604,3 +605,89 @@ var _ = Describe("Cluster Roles/Policies", func() {
 		Expect(err).NotTo(HaveOccurred())
 	})
 })
+
+var _ = Describe("Validates isAwsManagedPolicy function", func() {
+	var (
+		awsManagedPolicyArn = "arn:aws:iam::aws:policy/service-role/ROSAInstallerPolicy"
+		customPolicyArn     = "arn:aws:iam::765374464689:policy/test-policy"
+	)
+	It("check aws managed policy", func() {
+		result := isAwsManagedPolicy(awsManagedPolicyArn)
+		Expect(result).To(Equal(true))
+	})
+	It("check custom policy", func() {
+		result := isAwsManagedPolicy(customPolicyArn)
+		Expect(result).To(Equal(false))
+	})
+})
+
+var _ = Describe("CheckIfROSAOperatorRole", func() {
+
+	var (
+		credRequest map[string]*cmv1.STSOperator
+		role        iamtypes.Role
+		result      bool
+	)
+
+	BeforeEach(func() {
+		stsOperator1, err := cmv1.NewSTSOperator().Namespace("namespace-1").Build()
+		Expect(err).NotTo(HaveOccurred())
+		stsOperator2, err := cmv1.NewSTSOperator().Namespace("namespace-2").Build()
+		Expect(err).NotTo(HaveOccurred())
+		credRequest = map[string]*cmv1.STSOperator{
+			"operator-1": stsOperator1,
+			"operator-2": stsOperator2,
+		}
+	})
+
+	When("the role has matching tags", func() {
+		It("should return true", func() {
+			role = iamtypes.Role{
+				RoleName: aws.String("test-role-name"),
+				Tags: []iamtypes.Tag{
+					{
+						Key:   aws.String("operator_namespace"),
+						Value: aws.String("namespace-1"),
+					},
+				},
+			}
+			result = checkIfROSAOperatorRole(role, credRequest)
+			Expect(result).To(BeTrue())
+		})
+	})
+
+	Context("the role name contains the namespace", func() {
+		BeforeEach(func() {
+			role = iamtypes.Role{
+				RoleName: aws.String("test-role-namespace-2"),
+				Tags:     []iamtypes.Tag{},
+			}
+			result = checkIfROSAOperatorRole(role, credRequest)
+		})
+
+		It("should return true", func() {
+			role = iamtypes.Role{
+				RoleName: aws.String("test-role-namespace-2"),
+				Tags:     []iamtypes.Tag{},
+			}
+			result = checkIfROSAOperatorRole(role, credRequest)
+			Expect(result).To(BeTrue())
+		})
+	})
+
+	When("the role has no matching tags or name", func() {
+		It("should return false", func() {
+			role = iamtypes.Role{
+				RoleName: aws.String("test-role-name"),
+				Tags: []iamtypes.Tag{
+					{
+						Key:   aws.String("operator_namespace"),
+						Value: aws.String("non-matching-namespace"),
+					},
+				},
+			}
+			result = checkIfROSAOperatorRole(role, credRequest)
+			Expect(result).To(BeFalse())
+		})
+	})
+})