From 4bb370eac3ab366e974fe7eeeb35e8dc47374910 Mon Sep 17 00:00:00 2001 From: hunterkepley Date: Mon, 2 Dec 2024 08:31:15 -0500 Subject: [PATCH] OCM-12806 | feat: Delete operator roles auto mode (and changes to account roles) --- cmd/dlt/accountroles/cmd.go | 7 +++- cmd/dlt/operatorrole/cmd.go | 20 +++++++++- pkg/aws/client.go | 5 ++- pkg/aws/client_mock.go | 36 +++++++++++++----- pkg/aws/policies.go | 76 +++++++++++++++++++++++++++++++------ pkg/aws/policies_test.go | 13 ++++++- pkg/roles/utils.go | 26 +++++++++++++ 7 files changed, 156 insertions(+), 27 deletions(-) diff --git a/cmd/dlt/accountroles/cmd.go b/cmd/dlt/accountroles/cmd.go index 09f66873d1..0266ad4e37 100644 --- a/cmd/dlt/accountroles/cmd.go +++ b/cmd/dlt/accountroles/cmd.go @@ -30,6 +30,7 @@ import ( "github.com/openshift/rosa/pkg/interactive" "github.com/openshift/rosa/pkg/interactive/confirm" "github.com/openshift/rosa/pkg/ocm" + "github.com/openshift/rosa/pkg/roles" "github.com/openshift/rosa/pkg/rosa" ) @@ -204,12 +205,16 @@ func deleteAccountRoles(r *rosa.Runtime, env string, prefix string, clusters []* r.Reporter.Infof(fmt.Sprintf("Deleting %saccount roles", roleTypeString)) r.OCMClient.LogEvent("ROSADeleteAccountRoleModeAuto", nil) + deleteHcpSharedVpcPolicies := false + if roles.CheckIfRolesAreHcpSharedVpc(r, finalRoleList) { + deleteHcpSharedVpcPolicies = confirm.Prompt(true, "Attempt to delete Hosted CP shared VPC policies?") + } for _, role := range finalRoleList { if !confirm.Prompt(true, "Delete the account role '%s'?", role) { continue } r.Reporter.Infof("Deleting account role '%s'", role) - err := r.AWSClient.DeleteAccountRole(role, prefix, managedPolicies) + err := r.AWSClient.DeleteAccountRole(role, prefix, managedPolicies, deleteHcpSharedVpcPolicies) if err != nil { r.Reporter.Warnf("There was an error deleting the account roles or policies: %s", err) continue diff --git a/cmd/dlt/operatorrole/cmd.go b/cmd/dlt/operatorrole/cmd.go index b2e7c2a741..004c1a824d 100644 --- a/cmd/dlt/operatorrole/cmd.go +++ b/cmd/dlt/operatorrole/cmd.go @@ -31,6 +31,7 @@ import ( "github.com/openshift/rosa/pkg/interactive" "github.com/openshift/rosa/pkg/interactive/confirm" "github.com/openshift/rosa/pkg/ocm" + "github.com/openshift/rosa/pkg/roles" "github.com/openshift/rosa/pkg/rosa" ) @@ -220,6 +221,13 @@ func run(cmd *cobra.Command, _ []string) { switch mode { case interactive.ModeAuto: r.OCMClient.LogEvent("ROSADeleteOperatorroleModeAuto", nil) + + // Only ask user if they want to delete policies if they are deleting HcpSharedVpc roles + deleteHcpSharedVpcPolicies := false + if roles.CheckIfRolesAreHcpSharedVpc(r, foundOperatorRoles) { + deleteHcpSharedVpcPolicies = confirm.Prompt(true, "Attempt to delete Hosted CP shared VPC policies?") + } + allSharedVpcPoliciesNotDeleted := make(map[string]bool) for _, role := range foundOperatorRoles { if !confirm.Prompt(true, "Delete the operator role '%s'?", role) { continue @@ -228,7 +236,11 @@ func run(cmd *cobra.Command, _ []string) { if spin != nil { spin.Start() } - err := r.AWSClient.DeleteOperatorRole(role, managedPolicies) + sharedVpcPoliciesNotDeleted, err := r.AWSClient.DeleteOperatorRole(role, managedPolicies, + deleteHcpSharedVpcPolicies) + for key, value := range sharedVpcPoliciesNotDeleted { + allSharedVpcPoliciesNotDeleted[key] = value + } if err != nil { if spin != nil { @@ -242,6 +254,12 @@ func run(cmd *cobra.Command, _ []string) { spin.Stop() } } + for policyOutput, notDeleted := range allSharedVpcPoliciesNotDeleted { + if notDeleted { + r.Logger.Warnf("Unable to delete policy %s: Policy still attached to other resources", + policyOutput) + } + } if !errOccured { r.Reporter.Infof("Successfully deleted the operator roles") } diff --git a/pkg/aws/client.go b/pkg/aws/client.go index 7c6e77473d..8e9bf378ff 100644 --- a/pkg/aws/client.go +++ b/pkg/aws/client.go @@ -145,7 +145,7 @@ type Client interface { ListOidcProviders(targetClusterId string, config *cmv1.OidcConfig) ([]OidcProviderOutput, error) GetRoleByARN(roleARN string) (iamtypes.Role, error) GetRoleByName(roleName string) (iamtypes.Role, error) - DeleteOperatorRole(roles string, managedPolicies bool) error + DeleteOperatorRole(roles string, managedPolicies bool, deleteHcpSharedVpcPolicies bool) (map[string]bool, error) GetOperatorRolesFromAccountByClusterID( clusterID string, credRequests map[string]*cmv1.STSOperator, @@ -156,11 +156,12 @@ type Client interface { GetAccountRoleForCurrentEnv(env string, roleName string) (Role, error) GetAccountRoleForCurrentEnvWithPrefix(env string, rolePrefix string, accountRolesMap map[string]AccountRole) ([]Role, error) - DeleteAccountRole(roleName string, prefix string, managedPolicies bool) error + DeleteAccountRole(roleName string, prefix string, managedPolicies bool, deleteHcpSharedVpcPolicies bool) error DeleteOCMRole(roleARN string, managedPolicies bool) error DeleteUserRole(roleName string) error GetAccountRolePolicies(roles []string, prefix string) (map[string][]PolicyDetail, map[string][]PolicyDetail, error) GetAttachedPolicy(role *string) ([]PolicyDetail, error) + GetPolicyDetailsFromRole(role *string) ([]*iam.GetPolicyOutput, error) HasPermissionsBoundary(roleName string) (bool, error) GetOpenIDConnectProviderByClusterIdTag(clusterID string) (string, error) GetOpenIDConnectProviderByOidcEndpointUrl(oidcEndpointUrl string) (string, error) diff --git a/pkg/aws/client_mock.go b/pkg/aws/client_mock.go index 01b5c88b96..9e84292079 100644 --- a/pkg/aws/client_mock.go +++ b/pkg/aws/client_mock.go @@ -178,17 +178,17 @@ func (mr *MockClientMockRecorder) CreateSecretInSecretsManager(name, secret any) } // DeleteAccountRole mocks base method. -func (m *MockClient) DeleteAccountRole(roleName, prefix string, managedPolicies bool) error { +func (m *MockClient) DeleteAccountRole(roleName, prefix string, managedPolicies, deleteHcpSharedVpcPolicies bool) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAccountRole", roleName, prefix, managedPolicies) + ret := m.ctrl.Call(m, "DeleteAccountRole", roleName, prefix, managedPolicies, deleteHcpSharedVpcPolicies) ret0, _ := ret[0].(error) return ret0 } // DeleteAccountRole indicates an expected call of DeleteAccountRole. -func (mr *MockClientMockRecorder) DeleteAccountRole(roleName, prefix, managedPolicies any) *gomock.Call { +func (mr *MockClientMockRecorder) DeleteAccountRole(roleName, prefix, managedPolicies, deleteHcpSharedVpcPolicies any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAccountRole", reflect.TypeOf((*MockClient)(nil).DeleteAccountRole), roleName, prefix, managedPolicies) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAccountRole", reflect.TypeOf((*MockClient)(nil).DeleteAccountRole), roleName, prefix, managedPolicies, deleteHcpSharedVpcPolicies) } // DeleteInlineRolePolicies mocks base method. @@ -234,17 +234,18 @@ func (mr *MockClientMockRecorder) DeleteOpenIDConnectProvider(providerURL any) * } // DeleteOperatorRole mocks base method. -func (m *MockClient) DeleteOperatorRole(roles string, managedPolicies bool) error { +func (m *MockClient) DeleteOperatorRole(roles string, managedPolicies, deleteHcpSharedVpcPolicies bool) (map[string]bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteOperatorRole", roles, managedPolicies) - ret0, _ := ret[0].(error) - return ret0 + ret := m.ctrl.Call(m, "DeleteOperatorRole", roles, managedPolicies, deleteHcpSharedVpcPolicies) + ret0, _ := ret[0].(map[string]bool) + ret1, _ := ret[1].(error) + return ret0, ret1 } // DeleteOperatorRole indicates an expected call of DeleteOperatorRole. -func (mr *MockClientMockRecorder) DeleteOperatorRole(roles, managedPolicies any) *gomock.Call { +func (mr *MockClientMockRecorder) DeleteOperatorRole(roles, managedPolicies, deleteHcpSharedVpcPolicies any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOperatorRole", reflect.TypeOf((*MockClient)(nil).DeleteOperatorRole), roles, managedPolicies) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOperatorRole", reflect.TypeOf((*MockClient)(nil).DeleteOperatorRole), roles, managedPolicies, deleteHcpSharedVpcPolicies) } // DeleteOsdCcsAdminUser mocks base method. @@ -858,6 +859,21 @@ func (mr *MockClientMockRecorder) GetOperatorRolesFromAccountByPrefix(prefix, cr return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetOperatorRolesFromAccountByPrefix", reflect.TypeOf((*MockClient)(nil).GetOperatorRolesFromAccountByPrefix), prefix, credRequest) } +// GetPolicyDetailsFromRole mocks base method. +func (m *MockClient) GetPolicyDetailsFromRole(role *string) ([]*iam.GetPolicyOutput, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPolicyDetailsFromRole", role) + ret0, _ := ret[0].([]*iam.GetPolicyOutput) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetPolicyDetailsFromRole indicates an expected call of GetPolicyDetailsFromRole. +func (mr *MockClientMockRecorder) GetPolicyDetailsFromRole(role any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPolicyDetailsFromRole", reflect.TypeOf((*MockClient)(nil).GetPolicyDetailsFromRole), role) +} + // GetRegion mocks base method. func (m *MockClient) GetRegion() string { m.ctrl.T.Helper() diff --git a/pkg/aws/policies.go b/pkg/aws/policies.go index 64510d7bff..e4b9432531 100644 --- a/pkg/aws/policies.go +++ b/pkg/aws/policies.go @@ -1065,15 +1065,37 @@ func checkIfROSAOperatorRole(role iamtypes.Role, credRequest map[string]*cmv1.ST return false } -func (c *awsClient) DeleteOperatorRole(roleName string, managedPolicies bool) error { +func (c *awsClient) GetPolicyDetailsFromRole(role *string) ([]*iam.GetPolicyOutput, error) { + policies, err := c.iamClient.ListAttachedRolePolicies(context.Background(), &iam.ListAttachedRolePoliciesInput{ + RoleName: role, + }) + if err != nil { + return nil, err + } + finalOutput := make([]*iam.GetPolicyOutput, 0) + for _, attachedPolicy := range policies.AttachedPolicies { + policy, err := c.iamClient.GetPolicy(context.Background(), &iam.GetPolicyInput{ + PolicyArn: attachedPolicy.PolicyArn, + }) + if err != nil { + return nil, err + } + finalOutput = append(finalOutput, policy) + } + return finalOutput, nil +} + +func (c *awsClient) DeleteOperatorRole(roleName string, managedPolicies bool, + deleteHcpSharedVpcPolicies bool) (map[string]bool, error) { role := aws.String(roleName) + sharedVpcPoliciesNotDeleted := make(map[string]bool) tagFilter, err := getOperatorRolePolicyTags(c.iamClient, roleName) if err != nil { - return err + return sharedVpcPoliciesNotDeleted, err } policies, excludedPolicies, err := getAttachedPolicies(c.iamClient, roleName, tagFilter) if err != nil { - return err + return sharedVpcPoliciesNotDeleted, err } err = c.detachOperatorRolePolicies(role) if err != nil { @@ -1086,25 +1108,55 @@ func (c *awsClient) DeleteOperatorRole(roleName string, managedPolicies bool) er err = nil } if err != nil { - return err + return sharedVpcPoliciesNotDeleted, err } } err = c.DeleteRole(*role) if err != nil { - return err + return sharedVpcPoliciesNotDeleted, err } if !managedPolicies { _, err = c.deletePolicies(policies) - } else { + } else if deleteHcpSharedVpcPolicies { var sharedVpcHcpPolicies []string for _, policy := range excludedPolicies { - if strings.Contains(policy, SharedVpcAssumeRolePrefix) { - sharedVpcHcpPolicies = append(sharedVpcHcpPolicies, policy) + policyOutput, err := c.iamClient.GetPolicy(context.Background(), &iam.GetPolicyInput{ + PolicyArn: aws.String(policy), + }) + if err != nil || policyOutput == nil { + return sharedVpcPoliciesNotDeleted, err + } + + containsManagedTag := false + containsHcpSharedVpcTag := false + for _, policyTag := range policyOutput.Policy.Tags { + switch strings.Trim(*policyTag.Key, " ") { + case tags.RedHatManaged: + containsManagedTag = true + case tags.HcpSharedVpc: + containsHcpSharedVpcTag = true + } + } + // Delete if no attachments and correct tags showing it's a hcp sharedvpc policy + if containsManagedTag && containsHcpSharedVpcTag { + if *policyOutput.Policy.AttachmentCount == 0 { + // If the policy is deleted, remove from the warning outputs + sharedVpcPoliciesNotDeleted[*policyOutput.Policy.Arn] = false + + // Add to list of sharedVpc policies to be actually deleted + c.logger.Infof("Deleting policy '%s'", policy) + sharedVpcHcpPolicies = append(sharedVpcHcpPolicies, policy) + } else { + // Print warning message after all roles are checked (will result in duplicated warnings without + // adding to this map and displaying at the end of role deletion due to multiple roles having + // one of the policies) + sharedVpcPoliciesNotDeleted[*policyOutput.Policy.Arn] = true + } } } _, err = c.deletePolicies(sharedVpcHcpPolicies) } - return err + return sharedVpcPoliciesNotDeleted, err } func (c *awsClient) DeleteRole(role string) error { @@ -1140,7 +1192,8 @@ func (c *awsClient) GetInstanceProfilesForRole(r string) ([]string, error) { return instanceProfiles, nil } -func (c *awsClient) DeleteAccountRole(roleName string, prefix string, managedPolicies bool) error { +func (c *awsClient) DeleteAccountRole(roleName string, prefix string, managedPolicies bool, + deleteHcpSharedVpcPolicies bool) error { role := aws.String(roleName) err := c.DeleteInlineRolePolicies(aws.ToString(role)) if err != nil { @@ -1173,7 +1226,7 @@ func (c *awsClient) DeleteAccountRole(roleName string, prefix string, managedPol } if !managedPolicies { _, err = c.deletePolicies(policyMap) - } else { + } else if deleteHcpSharedVpcPolicies { var sharedVpcHcpPolicies []string for _, policy := range excludedPolicyMap { policyOutput, err := c.iamClient.GetPolicy(context.Background(), &iam.GetPolicyInput{ @@ -1195,6 +1248,7 @@ func (c *awsClient) DeleteAccountRole(roleName string, prefix string, managedPol } if containsManagedTag && containsHcpSharedVpcTag { if *policyOutput.Policy.AttachmentCount == 0 { + c.logger.Infof("Deleting policy '%s'", policy) sharedVpcHcpPolicies = append(sharedVpcHcpPolicies, policy) } else { c.logger.Warnf("Unable to delete policy %s: Policy still attached to %v other resource(s)", diff --git a/pkg/aws/policies_test.go b/pkg/aws/policies_test.go index 7d132f4a50..a9c0919f77 100644 --- a/pkg/aws/policies_test.go +++ b/pkg/aws/policies_test.go @@ -531,6 +531,15 @@ var _ = Describe("Cluster Roles/Policies", func() { Expect(policies).To(HaveLen(1)) Expect(policies[0]).To(Equal(operatorRolePolicyArn)) }) + It("Test GetPolicyDetailsFromRole", func() { + mockIamAPI.EXPECT().ListAttachedRolePolicies(gomock.Any(), &iam.ListAttachedRolePoliciesInput{ + RoleName: aws.String(accountRole), + }).Return(accountRoleAttachedPolicies, nil).Times(1) + mockIamAPI.EXPECT().GetPolicy(gomock.Any(), gomock.Any()).Times(2) + output, err := client.GetPolicyDetailsFromRole(&accountRole) + Expect(err).NotTo(HaveOccurred()) + Expect(output).To(HaveLen(2)) + }) It("Test DeleteAccountRole", func() { mockIamAPI.EXPECT().ListRolePolicies(gomock.Any(), gomock.Any()).Return(&iam.ListRolePoliciesOutput{}, nil) mockIamAPI.EXPECT().ListAttachedRolePolicies(gomock.Any(), &iam.ListAttachedRolePoliciesInput{ @@ -567,7 +576,7 @@ var _ = Describe("Cluster Roles/Policies", func() { mockIamAPI.EXPECT().DeletePolicy(gomock.Any(), &iam.DeletePolicyInput{ PolicyArn: aws.String(accountRolePolicyArn), }).Return(&iam.DeletePolicyOutput{}, nil) - err := client.DeleteAccountRole(accountRole, rolePrefix, false) + err := client.DeleteAccountRole(accountRole, rolePrefix, false, false) Expect(err).NotTo(HaveOccurred()) }) It("Test DeleteOperatorRole", func() { @@ -602,7 +611,7 @@ var _ = Describe("Cluster Roles/Policies", func() { AttachmentCount: &attachCount, }, }, nil) - err := client.DeleteOperatorRole(operatorRole, false) + _, err := client.DeleteOperatorRole(operatorRole, false, false) Expect(err).NotTo(HaveOccurred()) }) }) diff --git a/pkg/roles/utils.go b/pkg/roles/utils.go index 097803099f..b6f932141c 100644 --- a/pkg/roles/utils.go +++ b/pkg/roles/utils.go @@ -58,3 +58,29 @@ func GetHcpSharedVpcPolicyDetails(r *rosa.Runtime, roleArn string) (bool, string return existsQuery != nil, createPolicy, policyName, nil } + +func CheckIfRolesAreHcpSharedVpc(r *rosa.Runtime, roles []string) bool { + isHcpSharedVpc := false + for _, roleName := range roles { + ptrRoleName := roleName + attachedPolicies, err := r.AWSClient.GetPolicyDetailsFromRole(&ptrRoleName) + if err != nil { + r.Reporter.Errorf("Failed to get policy details for role '%s': %v", roleName, err) + } + for _, attachedPolicy := range attachedPolicies { + rhManaged := false + hcpSharedVpc := false + for _, tag := range attachedPolicy.Policy.Tags { + if *tag.Key == tags.RedHatManaged { + rhManaged = true + } else if *tag.Key == tags.HcpSharedVpc { + hcpSharedVpc = true + } + } + if rhManaged && hcpSharedVpc { + isHcpSharedVpc = true + } + } + } + return isHcpSharedVpc +}