Skip to content

Commit

Permalink
remove duplicate invocation to aws to get policy of roles
Browse files Browse the repository at this point in the history
Signed-off-by: marcolan018 <llan@redhat.com>
  • Loading branch information
marcolan018 committed May 2, 2024
1 parent 685df81 commit bd7a589
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 13 deletions.
29 changes: 18 additions & 11 deletions cmd/upgrade/roles/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@ func run(cmd *cobra.Command, argv []string) {
reporter.Errorf("Error in rolePolicyBinding: %s", err)
os.Exit(1)
}
rolePolicyDetails := rolepolicybindings.TransformToRolePolicyDetails(rolePolicyBindings)

if !isUpgradeNeedForAccountRolePolicies {
reporter.Infof("Account roles/policies for cluster '%s' are already up-to-date.", r.ClusterKey)
Expand All @@ -299,6 +300,7 @@ func run(cmd *cobra.Command, argv []string) {
accountRolePolicies,
policyVersion,
isPolicyVersionChosen,
rolePolicyDetails,
)
if err != nil {
LogError(roles.RosaUpgradeAccRolesModeAuto, ocmClient, policyVersion, err, reporter)
Expand Down Expand Up @@ -335,6 +337,7 @@ func run(cmd *cobra.Command, argv []string) {
isUpgradeNeedForAccountRolePolicies,
awsClient,
policyVersion,
rolePolicyDetails,
)
if err != nil {
reporter.Errorf("%s", err)
Expand Down Expand Up @@ -422,6 +425,7 @@ func run(cmd *cobra.Command, argv []string) {
credRequests,
cluster,
operatorRolePolicyPrefix,
rolePolicyDetails,
)
if err != nil {
r.Reporter.Errorf("%s", err)
Expand Down Expand Up @@ -515,12 +519,8 @@ func handleAccountRolePolicyARN(
rolePath string,
partition string,
accountID string,
policiesDetails []aws.PolicyDetail,
) (string, []string, error) {
policiesDetails, err := awsClient.GetAttachedPolicy(&roleName)
if err != nil {
return "", nil, err
}

attachedPoliciesDetail := aws.FindAllAttachedPolicyDetails(policiesDetails)

generatedPolicyARN := aws.GetPolicyARN(partition, accountID, roleName, rolePath)
Expand Down Expand Up @@ -587,6 +587,7 @@ func upgradeAccountRolePoliciesFromCluster(
policies map[string]*v1.AWSSTSPolicy,
policyVersion string,
isVersionChosen bool,
rolePolicyDetails map[string][]aws.PolicyDetail,
) error {
for file, role := range aws.AccountRoles {
roleName, err := aws.GetAccountRoleName(cluster, role.Name)
Expand Down Expand Up @@ -617,7 +618,8 @@ func upgradeAccountRolePoliciesFromCluster(
}
filename := fmt.Sprintf("sts_%s_permission_policy", file)

policyARN, _, err := handleAccountRolePolicyARN(mode, awsClient, roleName, rolePath, partition, accountID)
policyARN, _, err := handleAccountRolePolicyARN(mode, awsClient, roleName, rolePath,
partition, accountID, rolePolicyDetails[roleName])
if err != nil {
return err
}
Expand Down Expand Up @@ -666,6 +668,7 @@ func buildAccountRoleCommandsFromCluster(
isUpgradeNeedForAccountRolePolicies bool,
awsClient aws.Client,
defaultPolicyVersion string,
rolePolicyDetails map[string][]aws.PolicyDetail,
) (string, error) {
commands := []string{}
if isUpgradeNeedForAccountRolePolicies {
Expand All @@ -690,6 +693,7 @@ func buildAccountRoleCommandsFromCluster(
rolePath,
partition,
accountID,
rolePolicyDetails[accRoleName],
)
if err != nil {
return "", err
Expand Down Expand Up @@ -749,6 +753,7 @@ func upgradeOperatorPolicies(
credRequests map[string]*v1.STSOperator,
cluster *v1.Cluster,
operatorRolePolicyPrefix string,
rolePolicyDetails map[string][]aws.PolicyDetail,
) error {
switch mode {
case interactive.ModeAuto:
Expand All @@ -769,6 +774,7 @@ func upgradeOperatorPolicies(
credRequests,
cluster,
operatorRolePolicyPrefix,
rolePolicyDetails,
)
if err != nil {
if strings.Contains(err.Error(), "Throttling") {
Expand Down Expand Up @@ -805,6 +811,7 @@ func upgradeOperatorPolicies(
defaultPolicyVersion,
credRequests,
cluster,
rolePolicyDetails,
)
if err != nil {
return fmt.Errorf("there was an error generating the commands: %s", err)
Expand All @@ -827,6 +834,7 @@ func upgradeOperatorRolePoliciesFromCluster(
credRequests map[string]*v1.STSOperator,
cluster *v1.Cluster,
operatorRolePolicyPrefix string,
rolePolicyDetails map[string][]aws.PolicyDetail,
) error {
operatorRoles := cluster.AWS().STS().OperatorIAMRoles()
isSharedVpc := cluster.AWS().PrivateHostedZoneRoleARN() != ""
Expand Down Expand Up @@ -863,6 +871,7 @@ func upgradeOperatorRolePoliciesFromCluster(
operator,
partition,
accountID,
rolePolicyDetails[operatorRoleName],
)
if err != nil {
return err
Expand Down Expand Up @@ -916,6 +925,7 @@ func buildOperatorRoleCommandsFromCluster(
defaultPolicyVersion string,
credRequests map[string]*v1.STSOperator,
cluster *v1.Cluster,
rolePolicyDetails map[string][]aws.PolicyDetail,
) (string, error) {
operatorRoles := cluster.AWS().STS().OperatorIAMRoles()
commands := []string{}
Expand Down Expand Up @@ -952,6 +962,7 @@ func buildOperatorRoleCommandsFromCluster(
operator,
partition,
accountID,
rolePolicyDetails[operatorRoleName],
)
if err != nil {
return "", err
Expand Down Expand Up @@ -1009,12 +1020,8 @@ func handleOperatorRolePolicyARN(
operator *v1.STSOperator,
partition string,
accountID string,
policiesDetails []aws.PolicyDetail,
) (string, []string, error) {
policiesDetails, err := awsClient.GetAttachedPolicy(&operatorRoleName)
if err != nil {
return "", nil, err
}

generatedPolicyARN := aws.GetOperatorPolicyARN(
partition,
accountID,
Expand Down
23 changes: 23 additions & 0 deletions pkg/helper/rolepolicybindings/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"slices"

cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/openshift/rosa/pkg/aws"
)

const (
Expand Down Expand Up @@ -75,3 +76,25 @@ func CheckMissingRolePolicyBindings(desired, actual *cmv1.RolePolicyBindingList)
}
return output, true
}

func TransformToRolePolicyDetails(bindingList *cmv1.RolePolicyBindingList) map[string][]aws.PolicyDetail {
rolePolicyDetails := map[string][]aws.PolicyDetail{}
for _, binding := range bindingList.Slice() {
policyDetails := []aws.PolicyDetail{}
if binding.Policies() != nil {
for _, policy := range binding.Policies() {
policyType := policy.Type()
if policyType != aws.Inline {
policyType = aws.Attached
}
policyDetails = append(policyDetails, aws.PolicyDetail{
PolicyName: policy.Name(),
PolicyArn: policy.Arn(),
PolicyType: policyType,
})
}
}
rolePolicyDetails[binding.Name()] = policyDetails
}
return rolePolicyDetails
}
26 changes: 24 additions & 2 deletions pkg/helper/rolepolicybindings/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/openshift/rosa/pkg/aws"
)

func TestIdp(t *testing.T) {
Expand All @@ -34,15 +35,17 @@ var (
roleName1 = "sample-role-1"
roleName2 = "sample-role-2"
policyArn1 = "sample-policy-arn-1"
policyName1 = "sample-policy-name-1"
policyArn2 = "sample-policy-arn-2"
policyName2 = "sample-policy-name-2"
errDesc = "sample-err-description"
failedBinding = cmv1.NewRolePolicyBinding().
Name(roleName1).
Status(cmv1.NewRolePolicyBindingStatus().
Value(RolePolicyBindingFailedStatus).
Description(errDesc))
policyBuilder1 = cmv1.NewRolePolicy().Arn(policyArn1)
policyBuilder2 = cmv1.NewRolePolicy().Arn(policyArn2)
policyBuilder1 = cmv1.NewRolePolicy().Arn(policyArn1).Name(policyName1).Type(aws.Inline)
policyBuilder2 = cmv1.NewRolePolicy().Arn(policyArn2).Name(policyName2).Type("customer")
binding1 = cmv1.NewRolePolicyBinding().
Name(roleName1).
Policies(policyBuilder1)
Expand Down Expand Up @@ -85,5 +88,24 @@ var _ = Describe("Policy Service", func() {
"rosa attach policy --role-name %s --policy-arns %s --mode auto\n",
policyArn1, roleName2, roleName2, policyArn1)))
})
It("Test TransformToRolePolicyDetails", func() {
bindingList, err := cmv1.NewRolePolicyBindingList().Items(binding1, actualBinding2).Build()
Expect(err).ShouldNot(HaveOccurred())
rolePoliyDetails := TransformToRolePolicyDetails(bindingList)
Expect(rolePoliyDetails[roleName1]).NotTo(BeNil())
Expect(rolePoliyDetails[roleName1]).To(HaveLen(1))
Expect(rolePoliyDetails[roleName1][0]).To(BeEquivalentTo(aws.PolicyDetail{
PolicyName: policyName1,
PolicyArn: policyArn1,
PolicyType: aws.Inline,
}))
Expect(rolePoliyDetails[roleName2]).NotTo(BeNil())
Expect(rolePoliyDetails[roleName2]).To(HaveLen(1))
Expect(rolePoliyDetails[roleName2][0]).To(BeEquivalentTo(aws.PolicyDetail{
PolicyName: policyName2,
PolicyArn: policyArn2,
PolicyType: aws.Attached,
}))
})
})
})

0 comments on commit bd7a589

Please sign in to comment.