Merge pull request #1991 from marcolan018/ocm-7729

OCM-7729 | fix: check policy attached in detach manual mode
openshift · May 2, 2024 · 26bdf17 · 26bdf17
2 parents cc4debc + e4b2263
commit 26bdf17
Show file tree

Hide file tree

Showing 6 changed files with 75 additions and 22 deletions.
diff --git a/cmd/detach/policy/cmd.go b/cmd/detach/policy/cmd.go
@@ -122,9 +122,18 @@ func DetachPolicyRunner(userOptions *RosaDetachPolicyOptions) rosa.CommandRunner
 				return err
 			}
 		case interactive.ModeManual:
-			r.Reporter.Infof("Run the following command to detach the policy:")
-			fmt.Print(policySvc.ManualDetachArbitraryPolicy(options.roleName, policyArns,
-				r.Creator.AccountID, orgID))
+			output, warn, err := policySvc.ManualDetachArbitraryPolicy(options.roleName, policyArns,
+				r.Creator.AccountID, orgID)
+			if err != nil {
+				return err
+			}
+			if len(warn) > 0 {
+				fmt.Print(warn)
+			}
+			if len(output) > 0 {
+				r.Reporter.Infof("Run the following command to detach the policy:")
+				fmt.Print(output)
+			}
 		default:
 			return fmt.Errorf("Invalid mode. Allowed values are %s", interactive.Modes)
 		}

diff --git a/pkg/aws/client.go b/pkg/aws/client.go
@@ -138,6 +138,7 @@ type Client interface {
 	ListOCMRoles() ([]Role, error)
 	ListAccountRoles(version string) ([]Role, error)
 	ListOperatorRoles(version string, clusterID string) (map[string][]OperatorRoleDetail, error)
+	ListAttachedRolePolicies(roleName string) ([]string, error)
 	ListOidcProviders(targetClusterId string, config *cmv1.OidcConfig) ([]OidcProviderOutput, error)
 	GetRoleByARN(roleARN string) (iamtypes.Role, error)
 	GetRoleByName(roleName string) (iamtypes.Role, error)

diff --git a/pkg/aws/mock_client.go b/pkg/aws/mock_client.go
diff --git a/pkg/aws/policies.go b/pkg/aws/policies.go
@@ -1941,6 +1941,20 @@ func (c *awsClient) ValidateHCPAccountRolesManagedPolicies(prefix string,
 	return nil
 }
 
+func (c *awsClient) ListAttachedRolePolicies(roleName string) ([]string, error) {
+	policies := []string{}
+	listPolicies, err := c.iamClient.ListAttachedRolePolicies(context.Background(), &iam.ListAttachedRolePoliciesInput{
+		RoleName: aws.String(roleName),
+	})
+	if err != nil {
+		return policies, err
+	}
+	for _, policy := range listPolicies.AttachedPolicies {
+		policies = append(policies, *policy.PolicyArn)
+	}
+	return policies, nil
+}
+
 func (c *awsClient) validateManagedPolicy(policies map[string]*cmv1.AWSSTSPolicy, policyKey string,
 	roleName string) error {
 	managedPolicyARN, err := GetManagedPolicyARN(policies, policyKey)

diff --git a/pkg/policy/policy_service.go b/pkg/policy/policy_service.go
@@ -40,7 +40,7 @@ type PolicyService interface {
 	ManualAttachArbitraryPolicy(roleName string, policyArns []string, accountID, orgID string) string
 	ValidateDetachOptions(roleName string, policyArns []string) error
 	AutoDetachArbitraryPolicy(roleName string, policyArns []string, accountID, orgID string) (string, error)
-	ManualDetachArbitraryPolicy(roleName string, policyArns []string, accountID, orgID string) string
+	ManualDetachArbitraryPolicy(roleName string, policyArns []string, accountID, orgID string) (string, string, error)
 }
 
 type policyService struct {
@@ -92,7 +92,7 @@ func (p *policyService) AutoAttachArbitraryPolicy(roleName string, policyArns []
 			ocm.PolicyArn:    policyArn,
 		})
 	}
-	return output, nil
+	return output[:len(output)-1], nil
 }
 
 func (p *policyService) ManualAttachArbitraryPolicy(roleName string, policyArns []string,
@@ -134,23 +134,33 @@ func (p *policyService) AutoDetachArbitraryPolicy(roleName string, policyArns []
 			})
 		}
 	}
-	return output, nil
+	return output[:len(output)-1], nil
 }
 
 func (p *policyService) ManualDetachArbitraryPolicy(roleName string, policyArns []string,
-	accountID, orgID string) string {
+	accountID, orgID string) (string, string, error) {
 	cmd := ""
+	warn := ""
+	policies, err := p.AWSClient.ListAttachedRolePolicies(roleName)
+	if err != nil {
+		return cmd, warn, err
+	}
 	for _, policyArn := range policyArns {
-		cmd = cmd + fmt.Sprintf("aws iam detach-role-policy --role-name %s --policy-arn %s\n",
-			roleName, policyArn)
-		p.OCMClient.LogEvent("ROSADetachPolicyManual", map[string]string{
-			ocm.Account:      accountID,
-			ocm.Organization: orgID,
-			ocm.RoleName:     roleName,
-			ocm.PolicyArn:    policyArn,
-		})
+		if slices.Contains(policies, policyArn) {
+			cmd = cmd + fmt.Sprintf("aws iam detach-role-policy --role-name %s --policy-arn %s\n",
+				roleName, policyArn)
+			p.OCMClient.LogEvent("ROSADetachPolicyManual", map[string]string{
+				ocm.Account:      accountID,
+				ocm.Organization: orgID,
+				ocm.RoleName:     roleName,
+				ocm.PolicyArn:    policyArn,
+			})
+		} else {
+			warn = warn + fmt.Sprintf("The policy '%s' is currently not attached to role '%s'\n",
+				policyArn, roleName)
+		}
 	}
-	return cmd
+	return cmd, warn, nil
 }
 
 func validatePolicyQuota(c aws.Client, roleName string, policyArns []string) error {

diff --git a/pkg/policy/policy_service_test.go b/pkg/policy/policy_service_test.go
@@ -122,7 +122,7 @@ var _ = Describe("Policy Service", func() {
 				"sample-account-id", "sample-org-id")
 			Expect(err).ShouldNot(HaveOccurred())
 			Expect(output).To(Equal(fmt.Sprintf("Attached policy '%s' to role '%s'\n"+
-				"Attached policy '%s' to role '%s'\n",
+				"Attached policy '%s' to role '%s'",
 				policyArn1, roleName, policyArn2, roleName)))
 		})
 		It("Test ManualAttachArbitraryPolicy", func() {
@@ -147,16 +147,20 @@ var _ = Describe("Policy Service", func() {
 				"sample-account-id", "sample-org-id")
 			Expect(err).ShouldNot(HaveOccurred())
 			Expect(output).To(Equal(fmt.Sprintf("Detached policy '%s' from role '%s'\n"+
-				"The policy '%s' is currently not attached to role '%s'\n",
+				"The policy '%s' is currently not attached to role '%s'",
 				policyArn1, roleName, policyArn2, roleName)))
 		})
 		It("Test ManualDetachArbitraryPolicy", func() {
-			output := policySvc.ManualDetachArbitraryPolicy(roleName, policyArns,
+			awsClient.EXPECT().ListAttachedRolePolicies(roleName).Return([]string{policyArn1}, nil)
+			output, warn, err := policySvc.ManualDetachArbitraryPolicy(roleName, policyArns,
 				"sample-account-id", "sample-org-id")
+			Expect(err).ShouldNot(HaveOccurred())
+			Expect(warn).To(Equal(fmt.Sprintf(
+				"The policy '%s' is currently not attached to role '%s'\n",
+				policyArn2, roleName)))
 			Expect(output).To(Equal(fmt.Sprintf(
-				"aws iam detach-role-policy --role-name %s --policy-arn %s\n"+
-					"aws iam detach-role-policy --role-name %s --policy-arn %s\n",
-				roleName, policyArn1, roleName, policyArn2)))
+				"aws iam detach-role-policy --role-name %s --policy-arn %s\n",
+				roleName, policyArn1)))
 		})
 	})
 })