Skip to content

Commit

Permalink
Merge pull request #1986 from marcolan018/ocm-7725
Browse files Browse the repository at this point in the history 
OCM-7725 | fix: ensure arbitrary policy not removed during cluster roles deletion
  • Loading branch information
@openshift-merge-bot
openshift-merge-bot[bot] authored May 2, 2024
2 parents eeaa7c4 + 5a82d5c commit cc4debc
Show file tree
Hide file tree
Showing 6 changed files with 348 additions and 66 deletions.
4 changes: 2 additions & 2 deletions cmd/dlt/accountroles/cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func deleteAccountRoles(r *rosa.Runtime, env string, prefix string, clusters []*
continue
}
r.Reporter.Infof("Deleting account role '%s'", role)
err := r.AWSClient.DeleteAccountRole(role, managedPolicies)
err := r.AWSClient.DeleteAccountRole(role, prefix, managedPolicies)
if err != nil {
r.Reporter.Warnf("There was an error deleting the account roles or policies: %s", err)
continue
Expand All @@ -229,7 +229,7 @@ func deleteAccountRoles(r *rosa.Runtime, env string, prefix string, clusters []*
r.Reporter.Infof(fmt.Sprintf("Successfully deleted the %saccount roles", roleTypeString))
case interactive.ModeManual:
r.OCMClient.LogEvent("ROSADeleteAccountRoleModeManual", nil)
policyMap, err := r.AWSClient.GetAccountRolePolicies(finalRoleList)
policyMap, err := r.AWSClient.GetAccountRolePolicies(finalRoleList, prefix)
if err != nil {
return fmt.Errorf("There was an error getting the policy: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/dlt/operatorrole/cmd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ func run(cmd *cobra.Command, _ []string) {
}
case interactive.ModeManual:
r.OCMClient.LogEvent("ROSADeleteOperatorroleModeManual", nil)
policyMap, err := r.AWSClient.GetPolicies(foundOperatorRoles)
policyMap, err := r.AWSClient.GetOperatorRolePolicies(foundOperatorRoles)
if err != nil {
r.Reporter.Errorf("There was an error getting the policy: %v", err)
os.Exit(1)
Expand Down
6 changes: 3 additions & 3 deletions pkg/aws/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,15 @@ type Client interface {
credRequests map[string]*cmv1.STSOperator,
) ([]string, error)
GetOperatorRolesFromAccountByPrefix(prefix string, credRequest map[string]*cmv1.STSOperator) ([]string, error)
GetPolicies(roles []string) (map[string][]string, error)
GetOperatorRolePolicies(roles []string) (map[string][]string, error)
GetAccountRolesForCurrentEnv(env string, accountID string) ([]Role, error)
GetAccountRoleForCurrentEnv(env string, roleName string) (Role, error)
GetAccountRoleForCurrentEnvWithPrefix(env string, rolePrefix string,
accountRolesMap map[string]AccountRole) ([]Role, error)
DeleteAccountRole(roleName string, managedPolicies bool) error
DeleteAccountRole(roleName string, prefix string, managedPolicies bool) error
DeleteOCMRole(roleARN string, managedPolicies bool) error
DeleteUserRole(roleName string) error
GetAccountRolePolicies(roles []string) (map[string][]PolicyDetail, error)
GetAccountRolePolicies(roles []string, prefix string) (map[string][]PolicyDetail, error)
GetAttachedPolicy(role *string) ([]PolicyDetail, error)
HasPermissionsBoundary(roleName string) (bool, error)
GetOpenIDConnectProviderByClusterIdTag(clusterID string) (string, error)
Expand Down
46 changes: 23 additions & 23 deletions pkg/aws/mock_client.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

154 changes: 121 additions & 33 deletions pkg/aws/policies.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
errors "github.com/zgalor/weberr"

client "github.com/openshift/rosa/pkg/aws/api_interface"
"github.com/openshift/rosa/pkg/aws/tags"
"github.com/openshift/rosa/pkg/helper"
)
Expand Down Expand Up @@ -111,6 +112,8 @@ const (
HCPSuffixPattern = "HCP-ROSA"

IngressOperatorCloudCredentialsRoleType = "ingress_operator_cloud_credentials"

TrueString = "true"
)

const (
Expand Down Expand Up @@ -999,7 +1002,11 @@ func checkIfROSAOperatorRole(roleName *string, credRequest map[string]*cmv1.STSO

func (c *awsClient) DeleteOperatorRole(roleName string, managedPolicies bool) error {
role := aws.String(roleName)
policies, err := c.GetPolicies([]string{*role})
tagFilter, err := getOperatorRolePolicyTags(c.iamClient, roleName)
if err != nil {
return err
}
policies, err := getAttachedPolicies(c.iamClient, roleName, tagFilter)
if err != nil {
return err
}
Expand All @@ -1022,7 +1029,7 @@ func (c *awsClient) DeleteOperatorRole(roleName string, managedPolicies bool) er
return err
}
if !managedPolicies {
_, err = c.deletePolicies(policies[*role])
_, err = c.deletePolicies(policies)
}
return err
}
Expand Down Expand Up @@ -1060,13 +1067,13 @@ func (c *awsClient) GetInstanceProfilesForRole(r string) ([]string, error) {
return instanceProfiles, nil
}

func (c *awsClient) DeleteAccountRole(roleName string, managedPolicies bool) error {
func (c *awsClient) DeleteAccountRole(roleName string, prefix string, managedPolicies bool) error {
role := aws.String(roleName)
err := c.DeleteInlineRolePolicies(aws.ToString(role))
if err != nil {
return err
}
policyMap, err := c.GetPolicies([]string{*role})
policyMap, err := getAttachedPolicies(c.iamClient, roleName, getAcctRolePolicyTags(prefix))
if err != nil {
return err
}
Expand All @@ -1092,7 +1099,7 @@ func (c *awsClient) DeleteAccountRole(roleName string, managedPolicies bool) err
return err
}
if !managedPolicies {
_, err = c.deletePolicies(policyMap[*role])
_, err = c.deletePolicies(policyMap)
}
return err
}
Expand Down Expand Up @@ -1249,7 +1256,7 @@ func (c *awsClient) deletePolicyVersions(policyArn string) error {
return nil
}

func (c *awsClient) GetAttachedPolicy(role *string) ([]PolicyDetail, error) {
func (c *awsClient) GetAttachedPolicyWithTags(role *string, tagFilter map[string]string) ([]PolicyDetail, error) {
policies := []PolicyDetail{}
attachedPoliciesOutput, err := c.iamClient.ListAttachedRolePolicies(
context.Background(),
Expand All @@ -1260,12 +1267,18 @@ func (c *awsClient) GetAttachedPolicy(role *string) ([]PolicyDetail, error) {
}

for _, policy := range attachedPoliciesOutput.AttachedPolicies {
policyDetail := PolicyDetail{
PolicyName: aws.ToString(policy.PolicyName),
PolicyArn: aws.ToString(policy.PolicyArn),
PolicyType: Attached,
hasTags, err := isPolicyHasTags(c.iamClient, policy.PolicyArn, tagFilter)
if err != nil {
return policies, err
}
if hasTags {
policyDetail := PolicyDetail{
PolicyName: aws.ToString(policy.PolicyName),
PolicyArn: aws.ToString(policy.PolicyArn),
PolicyType: Attached,
}
policies = append(policies, policyDetail)
}
policies = append(policies, policyDetail)
}

rolePolicyOutput, err := c.iamClient.ListRolePolicies(context.Background(),
Expand All @@ -1284,6 +1297,10 @@ func (c *awsClient) GetAttachedPolicy(role *string) ([]PolicyDetail, error) {
return policies, nil
}

func (c *awsClient) GetAttachedPolicy(role *string) ([]PolicyDetail, error) {
return c.GetAttachedPolicyWithTags(role, map[string]string{})
}

func (c *awsClient) detachOperatorRolePolicies(role *string) error {
// get attached role policies as operator roles have managed policies
policiesOutput, err := c.iamClient.ListAttachedRolePolicies(context.Background(),
Expand Down Expand Up @@ -1357,25 +1374,6 @@ func (c *awsClient) GetOperatorRolesFromAccountByPrefix(prefix string,
return roleList, nil
}

func (c *awsClient) GetPolicies(roles []string) (map[string][]string, error) {
roleMap := make(map[string][]string)
for _, role := range roles {
policyArr := []string{}
policiesOutput, err := c.iamClient.ListAttachedRolePolicies(context.Background(),
&iam.ListAttachedRolePoliciesInput{
RoleName: aws.String(role),
})
if err != nil && !awserr.IsNoSuchEntityException(err) {
return roleMap, err
}
for _, policy := range policiesOutput.AttachedPolicies {
policyArr = append(policyArr, aws.ToString(policy.PolicyArn))
}
roleMap[role] = policyArr
}
return roleMap, nil
}

func (c *awsClient) GetAccountRolesForCurrentEnv(env string, accountID string) ([]Role, error) {
roleList := []Role{}
roles, err := c.ListRoles()
Expand Down Expand Up @@ -1532,10 +1530,10 @@ func (c *awsClient) buildRoles(roleName string, accountID string) ([]Role, error
return roles, nil
}

func (c *awsClient) GetAccountRolePolicies(roles []string) (map[string][]PolicyDetail, error) {
func (c *awsClient) GetAccountRolePolicies(roles []string, prefix string) (map[string][]PolicyDetail, error) {
roleMap := make(map[string][]PolicyDetail)
for _, role := range roles {
policies, err := c.GetAttachedPolicy(aws.String(role))
policies, err := c.GetAttachedPolicyWithTags(aws.String(role), getAcctRolePolicyTags(prefix))
if err != nil && !awserr.IsNoSuchEntityException(err) {
return roleMap, err
}
Expand All @@ -1544,6 +1542,22 @@ func (c *awsClient) GetAccountRolePolicies(roles []string) (map[string][]PolicyD
return roleMap, nil
}

func (c *awsClient) GetOperatorRolePolicies(roles []string) (map[string][]string, error) {
roleMap := map[string][]string{}
for _, role := range roles {
tagFilter, err := getOperatorRolePolicyTags(c.iamClient, role)
if err != nil {
return roleMap, err
}
policies, err := getAttachedPolicies(c.iamClient, role, tagFilter)
if err != nil {
return roleMap, err
}
roleMap[role] = policies
}
return roleMap, nil
}

func (c *awsClient) GetOpenIDConnectProviderByClusterIdTag(clusterID string) (string, error) {
providers, err := c.iamClient.ListOpenIDConnectProviders(context.Background(),
&iam.ListOpenIDConnectProvidersInput{})
Expand Down Expand Up @@ -1852,7 +1866,7 @@ func (c *awsClient) IsAdminRole(roleName string) (bool, error) {
}

for _, tag := range role.Role.Tags {
if aws.ToString(tag.Key) == tags.AdminRole && aws.ToString(tag.Value) == "true" {
if aws.ToString(tag.Key) == tags.AdminRole && aws.ToString(tag.Value) == TrueString {
return true, nil
}
}
Expand Down Expand Up @@ -1975,3 +1989,77 @@ func (c *awsClient) listRoleAttachedPolicies(roleName string) ([]iamtypes.Attach

return attachedPoliciesOutput.AttachedPolicies, nil
}

// check whether the policy contains specified tags
func isPolicyHasTags(c client.IamApiClient, poilcyArn *string, tagFilter map[string]string) (bool, error) {
if len(tagFilter) != 0 {
tags, err := c.ListPolicyTags(context.Background(),
&iam.ListPolicyTagsInput{
PolicyArn: poilcyArn,
},
)
if err != nil {
return false, err
}
fitTagSize := 0
for _, tag := range tags.Tags {
value, ok := tagFilter[aws.ToString(tag.Key)]
if ok && value != aws.ToString(tag.Value) {
return false, nil
}
fitTagSize++
}
if fitTagSize < len(tagFilter) {
return false, nil
}
}
return true, nil
}

func getAttachedPolicies(c client.IamApiClient, role string,
tagFilter map[string]string) ([]string, error) {
policyArr := []string{}
policiesOutput, err := c.ListAttachedRolePolicies(context.Background(),
&iam.ListAttachedRolePoliciesInput{
RoleName: aws.String(role),
})
if err != nil {
return policyArr, err
}
for _, policy := range policiesOutput.AttachedPolicies {
hasTags, err := isPolicyHasTags(c, policy.PolicyArn, tagFilter)
if err != nil {
return policyArr, err
}
if hasTags {
policyArr = append(policyArr, aws.ToString(policy.PolicyArn))
}
}
return policyArr, nil
}

func getAcctRolePolicyTags(prefix string) map[string]string {
tagmap := map[string]string{}
tagmap[tags.RedHatManaged] = TrueString
tagmap[tags.RolePrefix] = prefix
return tagmap
}

func getOperatorRolePolicyTags(c client.IamApiClient, roleName string) (map[string]string, error) {
tagmap := map[string]string{}
tagmap[tags.RedHatManaged] = TrueString
roleTags, err := c.ListRoleTags(context.Background(), &iam.ListRoleTagsInput{
RoleName: aws.String(roleName),
})
if err != nil {
return tagmap, err
}
for _, tag := range roleTags.Tags {
key := aws.ToString(tag.Key)
switch key {
case tags.OperatorName, tags.OperatorNamespace:
tagmap[key] = aws.ToString(tag.Value)
}
}
return tagmap, nil
}
Loading

0 comments on commit cc4debc

Please sign in to comment.