From e4b2263f624ffe130013410cb88fc4b48d934847 Mon Sep 17 00:00:00 2001 From: Marco Date: Wed, 1 May 2024 16:50:01 -0500 Subject: [PATCH] OCM-7729 | fix: check policy attached in detach manual mode Signed-off-by: marcolan018 --- cmd/detach/policy/cmd.go | 15 ++++++++++--- pkg/aws/client.go | 1 + pkg/aws/mock_client.go | 15 +++++++++++++ pkg/aws/policies.go | 14 ++++++++++++ pkg/policy/policy_service.go | 36 ++++++++++++++++++++----------- pkg/policy/policy_service_test.go | 16 ++++++++------ 6 files changed, 75 insertions(+), 22 deletions(-) diff --git a/cmd/detach/policy/cmd.go b/cmd/detach/policy/cmd.go index 3d36ce442c..e06f88a7d8 100644 --- 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 index b38c29b4ee..2a653902b1 100644 --- 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 index 95afb3b404..06ba340b84 100644 --- a/pkg/aws/mock_client.go +++ b/pkg/aws/mock_client.go @@ -1170,6 +1170,21 @@ func (mr *MockClientMockRecorder) ListAccountRoles(version any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAccountRoles", reflect.TypeOf((*MockClient)(nil).ListAccountRoles), version) } +// ListAttachedRolePolicies mocks base method. +func (m *MockClient) ListAttachedRolePolicies(roleName string) ([]string, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListAttachedRolePolicies", roleName) + ret0, _ := ret[0].([]string) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListAttachedRolePolicies indicates an expected call of ListAttachedRolePolicies. +func (mr *MockClientMockRecorder) ListAttachedRolePolicies(roleName any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListAttachedRolePolicies", reflect.TypeOf((*MockClient)(nil).ListAttachedRolePolicies), roleName) +} + // ListOCMRoles mocks base method. func (m *MockClient) ListOCMRoles() ([]Role, error) { m.ctrl.T.Helper() diff --git a/pkg/aws/policies.go b/pkg/aws/policies.go index e06da49fc3..4087c32fd6 100644 --- a/pkg/aws/policies.go +++ b/pkg/aws/policies.go @@ -1927,6 +1927,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 index 030ec77237..122cf5aa3d 100644 --- 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 index f39938c2ea..47f8500e70 100644 --- 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))) }) }) })