Skip to content

Commit

Permalink
recreate inline policy document for iam roles if policy changes (#268)
Browse files Browse the repository at this point in the history
  • Loading branch information
Berk Dehrioglu authored Mar 1, 2024
1 parent 452809e commit ef35d06
Show file tree
Hide file tree
Showing 5 changed files with 101 additions and 104 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- Change inline policy document attach logic to recreate it if it's already attached to the role.

## [0.16.0] - 2024-02-28

### Changed
Expand Down
19 changes: 5 additions & 14 deletions controllers/awsmachinepool_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -352,21 +352,12 @@ var _ = Describe("AWSMachinePoolReconciler", func() {
RoleName: aws.String(info.ExpectedName),
}).Return(&iam.AddRoleToInstanceProfileOutput{}, nil)

// Implementation detail: instead of storing the ARN, the controller calls `GetRole` multiple times
// from different places. Remove once we don't do this anymore (hence the `MinTimes` call so we
// would notice).
mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{
RoleName: aws.String(info.ExpectedName),
}).MinTimes(1).Return(&iam.GetRoleOutput{
Role: &iam.Role{
Arn: aws.String(info.ReturnRoleArn),
Tags: expectedIAMTags,
mockIAMClient.EXPECT().GetRolePolicy(
&iam.GetRolePolicyInput{
PolicyName: aws.String(info.ExpectedPolicyName),
RoleName: aws.String(info.ExpectedName),
},
}, nil)

mockIAMClient.EXPECT().ListRolePolicies(&iam.ListRolePoliciesInput{
RoleName: aws.String(info.ExpectedName),
}).Return(&iam.ListRolePoliciesOutput{}, nil)
).Return(&iam.GetRolePolicyOutput{}, awserr.New(iam.ErrCodeNoSuchEntityException, "unit test", nil))

mockIAMClient.EXPECT().PutRolePolicy(&iam.PutRolePolicyInput{
PolicyName: aws.String(info.ExpectedPolicyName),
Expand Down
12 changes: 6 additions & 6 deletions controllers/awsmachinetemplate_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,18 +378,18 @@ var _ = Describe("AWSMachineTemplateReconciler", func() {
// Implementation detail: instead of storing the ARN, the controller calls `GetRole` multiple times
// from different places. Remove once we don't do this anymore (hence the `MinTimes` call so we
// would notice).
mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{
/*mockIAMClient.EXPECT().GetRole(&iam.GetRoleInput{
RoleName: aws.String(info.ExpectedName),
}).MinTimes(1).Return(&iam.GetRoleOutput{
Role: &iam.Role{
Arn: aws.String(info.ReturnRoleArn),
Tags: expectedIAMTags,
},
}, nil)

mockIAMClient.EXPECT().ListRolePolicies(&iam.ListRolePoliciesInput{
RoleName: aws.String(info.ExpectedName),
}).Return(&iam.ListRolePoliciesOutput{}, nil)
}, nil)*/
mockIAMClient.EXPECT().GetRolePolicy(&iam.GetRolePolicyInput{
PolicyName: aws.String(info.ExpectedPolicyName),
RoleName: aws.String(info.ExpectedName),
}).Return(nil, awserr.New(iam.ErrCodeNoSuchEntityException, "unit test", nil))

mockIAMClient.EXPECT().PutRolePolicy(&iam.PutRolePolicyInput{
PolicyName: aws.String(info.ExpectedPolicyName),
Expand Down
158 changes: 77 additions & 81 deletions pkg/iam/iam.go
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package iam

import (
"encoding/json"
"errors"
"fmt"
"net/url"
"reflect"
"strings"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -216,20 +219,11 @@ func (s *IAMService) reconcileRole(roleName string, roleType string, params inte
}

// we only attach the inline policy to a role that is owned (and was created) by iam controller
owned, err := isOwnedByIAMController(roleName, s.iamClient)
err = s.attachInlinePolicy(roleName, roleType, params)
if err != nil {
l.Error(err, "Failed to fetch IAM Role")
return err
}
// check if the policy is created by this controller, if its not than we skip adding inline policy
if !owned {
l.Info("IAM role is not owned by IAM controller, skipping adding inline policy", "role_name", roleName)
} else {
err = s.attachInlinePolicy(roleName, roleType, params)
if err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -357,49 +351,58 @@ func (s *IAMService) applyAssumePolicyRole(roleName string, roleType string, par
// attachInlinePolicy will attach inline policy to the main IAM role
func (s *IAMService) attachInlinePolicy(roleName string, roleType string, params interface{}) error {
l := s.log.WithValues("role_name", roleName)
i := &awsiam.ListRolePoliciesInput{
RoleName: aws.String(roleName),
}
tmpl := getInlinePolicyTemplate(roleType)

alreadyExists := false
policyDocument, err := generatePolicyDocument(tmpl, params)
if err != nil {
l.Error(err, "failed to generate inline policy document from template for IAM role")
return err
}

// check if the inline policy already exists
o, err := s.iamClient.ListRolePolicies(i)
if err == nil {
for _, p := range o.PolicyNames {
if *p == policyName(s.roleType, s.clusterName) {
alreadyExists = true
break
}
}
output, err := s.iamClient.GetRolePolicy(&awsiam.GetRolePolicyInput{
RoleName: aws.String(roleName),
PolicyName: aws.String(policyName(s.roleType, s.clusterName)),
})
if err != nil && !IsNotFound(err) {
l.Error(err, "failed to fetch inline policy for IAM role")
return err
}

// add inline policy to the main IAM role if it do not exist yet
if !alreadyExists {
tmpl := getInlinePolicyTemplate(roleType)

policyDocument, err := generatePolicyDocument(tmpl, params)
if err == nil {
isEqual, err := areEqualPolicy(*output.PolicyDocument, policyDocument)
if err != nil {
l.Error(err, "failed to generate inline policy document from template for IAM role")
l.Error(err, "failed to compare inline policy documents")
return err
}

i := &awsiam.PutRolePolicyInput{
PolicyName: aws.String(policyName(s.roleType, s.clusterName)),
PolicyDocument: aws.String(policyDocument),
RoleName: aws.String(roleName),
if isEqual {
l.Info("inline policy for IAM role already exists, skipping")
return nil
}

_, err = s.iamClient.PutRolePolicy(i)
_, err = s.iamClient.DeleteRolePolicy(&awsiam.DeleteRolePolicyInput{
PolicyName: aws.String(policyName(s.roleType, s.clusterName)),
RoleName: aws.String(roleName),
})
if err != nil {
l.Error(err, "failed to add inline policy to IAM Role")
l.Error(err, "failed to delete inline policy from IAM Role")
return err
}
l.Info("successfully added inline policy to IAM role")
} else {
l.Info("inline policy for IAM role already added, skipping")
}

i := &awsiam.PutRolePolicyInput{
PolicyName: aws.String(policyName(s.roleType, s.clusterName)),
PolicyDocument: aws.String(policyDocument),
RoleName: aws.String(roleName),
}

_, err = s.iamClient.PutRolePolicy(i)
if err != nil {
l.Error(err, "failed to add inline policy to IAM Role")
return err
}
l.Info("successfully added inline policy to IAM role")

return nil
}

Expand Down Expand Up @@ -459,22 +462,8 @@ func (s *IAMService) DeleteRolesForIRSA() error {
func (s *IAMService) deleteRole(roleName string) error {
l := s.log.WithValues("role_name", roleName)

owned, err := isOwnedByIAMController(roleName, s.iamClient)
if IsNotFound(err) {
// role do not exists, nothing to delete, lets just finish
return nil
} else if err != nil {
l.Error(err, "Failed to fetch IAM Role")
return err
}
// check if the policy is created by this controller, if its not than we skip deletion
if !owned {
l.Info("IAM role is not owned by IAM controller, skipping deletion")
return nil
}

// clean any attached policies, otherwise deletion of role will not work
err = s.cleanAttachedPolicies(roleName)
err := s.cleanAttachedPolicies(roleName)
if err != nil {
return err
}
Expand Down Expand Up @@ -611,33 +600,6 @@ func (s *IAMService) GetIRSAOpenIDForEKS(clusterName string) (string, error) {
return id, nil
}

func isOwnedByIAMController(iamRoleName string, iamClient iamiface.IAMAPI) (bool, error) {
i := &awsiam.GetRoleInput{
RoleName: aws.String(iamRoleName),
}

o, err := iamClient.GetRole(i)
if err != nil {
return false, err
}

if hasIAMControllerTag(o.Role.Tags) {
return true, nil
} else {
return false, nil
}
}

func hasIAMControllerTag(tags []*awsiam.Tag) bool {
for _, tag := range tags {
if *tag.Key == IAMControllerOwnedTag {
return true
}
}

return false
}

func roleName(role string, clusterID string) string {
if role == Route53Role {
return fmt.Sprintf("%s-Route53Manager-Role", clusterID)
Expand Down Expand Up @@ -684,3 +646,37 @@ func getIRSARoles() []string {
ClusterAutoscalerRole,
}
}

func areEqualPolicy(encodedPolicy, expectedPolicy string) (bool, error) {
decodedPolicy, err := urlDecode(encodedPolicy)
if err != nil {
return false, err
}
return areEqualJSON(decodedPolicy, expectedPolicy)
}

func areEqualJSON(s1, s2 string) (bool, error) {
var o1 interface{}
var o2 interface{}

var err error
err = json.Unmarshal([]byte(s1), &o1)
if err != nil {
return false, err
}
err = json.Unmarshal([]byte(s2), &o2)
if err != nil {
return false, err
}

return reflect.DeepEqual(o1, o2), nil
}

func urlDecode(encodedValue string) (string, error) {
decodedValue, err := url.QueryUnescape(encodedValue)
if err != nil {
return "", err
}

return decodedValue, nil
}
12 changes: 9 additions & 3 deletions pkg/iam/iam_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ var _ = Describe("ReconcileRole", func() {
sess awsclientgo.ConfigProvider
)

const controlPlanePolicyTemplate = "%7B%0A%09%09%22Version%22%3A%20%222012-10-17%22%2C%0A%09%09%22Statement%22%3A%20%5B%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%22elasticloadbalancing%3A%2A%22%2C%0A%09%09%09%22Resource%22%3A%20%22%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22autoscaling%3ADescribeAutoScalingGroups%22%2C%0A%09%09%09%20%20%22autoscaling%3ADescribeAutoScalingInstances%22%2C%0A%09%09%09%20%20%22autoscaling%3ADescribeTags%22%2C%0A%09%09%09%20%20%22autoscaling%3ADescribeLaunchConfigurations%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeLaunchTemplateVersions%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%22%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Condition%22%3A%20%7B%0A%09%09%09%20%20%22StringEquals%22%3A%20%7B%0A%09%09%09%09%22autoscaling%3AResourceTag%2Fsigs.k8s.io%2Fcluster-api-provider-aws%2Fcluster%2Ftest-cluster%22%3A%20%22owned%22%0A%09%09%09%20%20%7D%0A%09%09%09%7D%2C%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22autoscaling%3ASetDesiredCapacity%22%2C%0A%09%09%09%20%20%22autoscaling%3ATerminateInstanceInAutoScalingGroup%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%22%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22ecr%3AGetAuthorizationToken%22%2C%0A%09%09%09%20%20%22ecr%3ABatchCheckLayerAvailability%22%2C%0A%09%09%09%20%20%22ecr%3AGetDownloadUrlForLayer%22%2C%0A%09%09%09%20%20%22ecr%3AGetRepositoryPolicy%22%2C%0A%09%09%09%20%20%22ecr%3ADescribeRepositories%22%2C%0A%09%09%09%20%20%22ecr%3AListImages%22%2C%0A%09%09%09%20%20%22ecr%3ABatchGetImage%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%22%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22ec2%3AAssignPrivateIpAddresses%22%2C%0A%09%09%09%20%20%22ec2%3AAttachNetworkInterface%22%2C%0A%09%09%09%20%20%22ec2%3ACreateNetworkInterface%22%2C%0A%09%09%09%20%20%22ec2%3ADeleteNetworkInterface%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeInstances%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeInstanceTypes%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeTags%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeNetworkInterfaces%22%2C%0A%09%09%09%20%20%22ec2%3ADetachNetworkInterface%22%2C%0A%09%09%09%20%20%22ec2%3AModifyNetworkInterfaceAttribute%22%2C%0A%09%09%09%20%20%22ec2%3AUnassignPrivateIpAddresses%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%22%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22autoscaling%3ADescribeAutoScalingGroups%22%2C%0A%09%09%09%20%20%22autoscaling%3ADescribeLaunchConfigurations%22%2C%0A%09%09%09%20%20%22autoscaling%3ADescribeTags%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeInstances%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeImages%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeRegions%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeRouteTables%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeSecurityGroups%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeSubnets%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeVolumes%22%2C%0A%09%09%09%20%20%22ec2%3ACreateSecurityGroup%22%2C%0A%09%09%09%20%20%22ec2%3ACreateTags%22%2C%0A%09%09%09%20%20%22ec2%3ACreateVolume%22%2C%0A%09%09%09%20%20%22ec2%3AModifyInstanceAttribute%22%2C%0A%09%09%09%20%20%22ec2%3AModifyVolume%22%2C%0A%09%09%09%20%20%22ec2%3AAttachVolume%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeVolumesModifications%22%2C%0A%09%09%09%20%20%22ec2%3AAuthorizeSecurityGroupIngress%22%2C%0A%09%09%09%20%20%22ec2%3ACreateRoute%22%2C%0A%09%09%09%20%20%22ec2%3ADeleteRoute%22%2C%0A%09%09%09%20%20%22ec2%3ADeleteSecurityGroup%22%2C%0A%09%09%09%20%20%22ec2%3ADeleteVolume%22%2C%0A%09%09%09%20%20%22ec2%3ADetachVolume%22%2C%0A%09%09%09%20%20%22ec2%3ARevokeSecurityGroupIngress%22%2C%0A%09%09%09%20%20%22ec2%3ADescribeVpcs%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AAddTags%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AAttachLoadBalancerToSubnets%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AApplySecurityGroupsToLoadBalancer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ACreateLoadBalancer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ACreateLoadBalancerPolicy%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ACreateLoadBalancerListeners%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AConfigureHealthCheck%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADeleteLoadBalancer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADeleteLoadBalancerListeners%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeLoadBalancers%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeLoadBalancerAttributes%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADetachLoadBalancerFromSubnets%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADeregisterInstancesFromLoadBalancer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AModifyLoadBalancerAttributes%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ARegisterInstancesWithLoadBalancer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ASetLoadBalancerPoliciesForBackendServer%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AAddTags%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ACreateListener%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ACreateTargetGroup%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADeleteListener%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADeleteTargetGroup%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeListeners%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeLoadBalancerPolicies%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeTargetGroups%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ADescribeTargetHealth%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AModifyListener%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3AModifyTargetGroup%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ARegisterTargets%22%2C%0A%09%09%09%20%20%22elasticloadbalancing%3ASetLoadBalancerPoliciesOfListener%22%2C%0A%09%09%09%20%20%22iam%3ACreateServiceLinkedRole%22%2C%0A%09%09%09%20%20%22kms%3ADescribeKey%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%5B%0A%09%09%09%20%20%22%2A%22%0A%09%09%09%5D%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%2C%0A%09%09%20%20%7B%0A%09%09%09%22Action%22%3A%20%5B%0A%09%09%09%20%20%22secretsmanager%3AGetSecretValue%22%2C%0A%09%09%09%20%20%22secretsmanager%3ADeleteSecret%22%0A%09%09%09%5D%2C%0A%09%09%09%22Resource%22%3A%20%22arn%3A%2A%3Asecretsmanager%3A%2A%3A%2A%3Asecret%3Aaws.cluster.x-k8s.io%2F%2A%22%2C%0A%09%09%09%22Effect%22%3A%20%22Allow%22%0A%09%09%20%20%7D%0A%09%09%5D%0A%09%20%20%7D"

BeforeEach(func() {
//create me new iam service config with mocks
sess, err = session.NewSession(&aws.Config{
Expand Down Expand Up @@ -62,7 +64,11 @@ var _ = Describe("ReconcileRole", func() {
})
When("inline policy is already attached", func() {
BeforeEach(func() {
mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{PolicyNames: aws.StringSlice([]string{"control-plane-test-cluster-policy"})}, nil)
mockIAMClient.EXPECT().GetRolePolicy(gomock.Any()).Return(&awsIAM.GetRolePolicyOutput{
PolicyDocument: aws.String(controlPlanePolicyTemplate),
PolicyName: aws.String("control-plane-test-cluster-policy"),
RoleName: aws.String("test-role"),
}, nil).AnyTimes()
})
It("should return nil", func() {
err := iamService.ReconcileRole()
Expand All @@ -71,7 +77,7 @@ var _ = Describe("ReconcileRole", func() {
})
When("could not attach InlinePolicy", func() {
JustBeforeEach(func() {
mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{}, nil).AnyTimes()
mockIAMClient.EXPECT().GetRolePolicy(gomock.Any()).Return(&awsIAM.GetRolePolicyOutput{}, awserr.New(awsIAM.ErrCodeNoSuchEntityException, "test", nil)).AnyTimes()
mockIAMClient.EXPECT().PutRolePolicy(gomock.Any()).Return(&awsIAM.PutRolePolicyOutput{}, errors.New("test error")).AnyTimes()
})
It("should return error", func() {
Expand All @@ -90,7 +96,7 @@ var _ = Describe("ReconcileRole", func() {
mockIAMClient.EXPECT().GetRole(gomock.Any()).Return(&awsIAM.GetRoleOutput{Role: &awsIAM.Role{
Tags: []*awsIAM.Tag{{Key: aws.String("capi-iam-controller/owned"), Value: aws.String("test-cluster")}},
}}, nil).AnyTimes()
mockIAMClient.EXPECT().ListRolePolicies(gomock.Any()).Return(&awsIAM.ListRolePoliciesOutput{}, nil).AnyTimes()
mockIAMClient.EXPECT().GetRolePolicy(gomock.Any()).Return(&awsIAM.GetRolePolicyOutput{}, awserr.New(awsIAM.ErrCodeNoSuchEntityException, "test", nil)).AnyTimes()
mockIAMClient.EXPECT().PutRolePolicy(gomock.Any()).Return(&awsIAM.PutRolePolicyOutput{}, nil).AnyTimes()
})
It("should create the role", func() {
Expand Down

0 comments on commit ef35d06

Please sign in to comment.