Skip to content

Commit

Permalink
Merge pull request #364 from akhil-rane/ccoctl_upgrade
Browse files Browse the repository at this point in the history 
ccoctl create-iam-roles should update policies for existing roles
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Jul 21, 2021
2 parents 7a86a2d + 8a94930 commit 60a783d
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 19 deletions.
9 changes: 4 additions & 5 deletions pkg/cmd/provisioning/aws/create-iam-roles.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ func createRole(awsClient aws.Client, name string, credReq *credreqv1.Credential
role = roleOutput.Role
log.Printf("Role %s created", *role.Arn)

if err := writeCredReqSecret(credReq, targetDir, *role.Arn); err != nil {
return "", errors.Wrap(err, "failed to save Secret for install manifests")
}

default:
return "", err
}
Expand All @@ -216,7 +220,6 @@ func createRole(awsClient aws.Client, name string, credReq *credreqv1.Credential
} else {
role = outRole.Role
log.Printf("Existing role %s found", *role.Arn)
return *role.Arn, nil
}

_, err = awsClient.PutRolePolicy(&iam.PutRolePolicyInput{
Expand All @@ -229,10 +232,6 @@ func createRole(awsClient aws.Client, name string, credReq *credreqv1.Credential
}
log.Printf("Updated Role policy for Role %s", *role.RoleName)

if err := writeCredReqSecret(credReq, targetDir, *role.Arn); err != nil {
return "", errors.Wrap(err, "failed to save Secret for install manifests")
}

return *role.Arn, nil
}
}
Expand Down
54 changes: 40 additions & 14 deletions pkg/cmd/provisioning/aws/create-iam-roles_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
"testing"

"github.com/golang/mock/gomock"
Expand All @@ -15,6 +16,7 @@ import (
"github.com/aws/aws-sdk-go/service/iam"

mockaws "github.com/openshift/cloud-credential-operator/pkg/aws/mock"
"github.com/openshift/cloud-credential-operator/pkg/cmd/provisioning"
)

const (
Expand All @@ -30,7 +32,7 @@ func TestIAMRoles(t *testing.T) {
name string
mockAWSClient func(mockCtrl *gomock.Controller) *mockaws.MockClient
setup func(*testing.T) string
verify func(t *testing.T, tempDirName string)
verify func(t *testing.T, targetDir, manifestsDir string)
cleanup func(*testing.T)
generateOnly bool
expectError bool
Expand All @@ -48,12 +50,14 @@ func TestIAMRoles(t *testing.T) {
require.NoError(t, err, "Failed to create temp directory")
return tempDirName
},
verify: func(t *testing.T, targetDir string) {
verify: func(t *testing.T, targetDir string, manifestsDir string) {
files, err := ioutil.ReadDir(targetDir)
require.NoError(t, err, "unexpected error listing files in targetDir")
assert.Zero(t, countNonDirectoryFiles(files), "Should be no files in targetDir when no CredReqs to process")

assert.Zero(t, len(files), "Should be no files in targetDir when no CredReqs to process")

files, err = ioutil.ReadDir(manifestsDir)
require.NoError(t, err, "unexpected error listing files in manifestsDir")
assert.Zero(t, countNonDirectoryFiles(files), "Should be no files in manifestsDir when no CredReqs to process")
},
},
{
Expand All @@ -73,12 +77,14 @@ func TestIAMRoles(t *testing.T) {

return tempDirName
},
verify: func(t *testing.T, targetDir string) {
verify: func(t *testing.T, targetDir string, manifestsDir string) {
files, err := ioutil.ReadDir(targetDir)
require.NoError(t, err, "unexpected error listing files in targetDir")
assert.Equal(t, 2, countNonDirectoryFiles(files), "Should be exactly 1 IAM Role JSON and 1 IAM Role Policy file for each CredReq")

assert.Equal(t, 2, len(files), "Should be exactly 1 IAM Role JSON and 1 IAM Role Policy file for each CredReq")

files, err = ioutil.ReadDir(manifestsDir)
require.NoError(t, err, "unexpected error listing files in manifestsDir")
assert.Equal(t, 1, countNonDirectoryFiles(files), "Should be exactly 1 secret in manifestsDir for one CredReq")
},
},
{
Expand All @@ -102,12 +108,14 @@ func TestIAMRoles(t *testing.T) {

return tempDirName
},
verify: func(t *testing.T, targetDir string) {
verify: func(t *testing.T, targetDir, manifestsDir string) {
files, err := ioutil.ReadDir(targetDir)
require.NoError(t, err, "unexpected error listing files in targetDir")
assert.Zero(t, countNonDirectoryFiles(files), "Should be no generated files when not in generate mode")

assert.Zero(t, len(files), "Should be no generated files when not in generate mode")

files, err = ioutil.ReadDir(manifestsDir)
require.NoError(t, err, "unexpected error listing files in manifestsDir")
assert.Equal(t, 1, countNonDirectoryFiles(files), "Should be exactly 1 secret in manifestsDir for one CredReq")
},
},
{
Expand All @@ -131,7 +139,7 @@ func TestIAMRoles(t *testing.T) {

return tempDirName
},
verify: func(t *testing.T, targetDir string) {},
verify: func(t *testing.T, targetDir, manifestsDir string) {},
},
{
name: "Role already exists",
Expand All @@ -141,6 +149,7 @@ func TestIAMRoles(t *testing.T) {
mockGetOpenIDConnectProvider(mockAWSClient)
roleName := fmt.Sprintf("%s-namespace1-secretName1", testNamePrefix)
mockGetRoleExists(mockAWSClient, roleName)
mockPutRolePolicy(mockAWSClient)
return mockAWSClient
},
setup: func(t *testing.T) string {
Expand All @@ -152,7 +161,7 @@ func TestIAMRoles(t *testing.T) {

return tempDirName
},
verify: func(t *testing.T, targetDir string) {},
verify: func(t *testing.T, targetDir, manifestsDir string) {},
},
}

Expand All @@ -167,14 +176,20 @@ func TestIAMRoles(t *testing.T) {
defer os.RemoveAll(credReqDir)

targetDir, err := ioutil.TempDir(os.TempDir(), "iamroletest")
require.NoError(t, err, "unexpected error creating temp dir for test")
require.NoError(t, err, "unexpected error creating target dir for test")
defer os.RemoveAll(targetDir)

manifestsDir := filepath.Join(targetDir, manifestsDirName)
err = provisioning.EnsureDir(manifestsDir)
require.NoError(t, err, "unexpected error creating manifests dir for test")
defer os.RemoveAll(manifestsDir)

err = createIAMRoles(mockAWSClient, testIdentityProviderARN, testPermissionsBoundaryARN, testNamePrefix, credReqDir, targetDir, test.generateOnly)

if test.expectError {
require.Error(t, err, "expected error returned")
} else {
test.verify(t, targetDir)
test.verify(t, targetDir, manifestsDir)
}
})
}
Expand Down Expand Up @@ -223,6 +238,17 @@ spec:
return nil
}

// countNonDirectoryFiles counts files which are not a directory
func countNonDirectoryFiles(files []os.FileInfo) int {
NonDirectoryFiles := 0
for _, f := range files {
if !f.IsDir() {
NonDirectoryFiles++
}
}
return NonDirectoryFiles
}

func mockGetOpenIDConnectProvider(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().GetOpenIDConnectProvider(gomock.Any()).Return(
&iam.GetOpenIDConnectProviderOutput{
Expand Down

0 comments on commit 60a783d

Please sign in to comment.