Skip to content

Commit

Permalink
Fix bug in updating spec.inlinePolicies (#81)
Browse files Browse the repository at this point in the history
The bug occurs when updating an existing item in `spec.inlinePolicies`.
The code incorrectly includes both the old(to remove) and new(to
add)values when using `lo.Difference`. This causes the code to first
attempt to replace the xisting policies with a call to
`iam::PutRolePolicy` and then mistakenly "delete" them right after.

This patch fixes this bug by calling `lo.Find` to double check whether
we need to keep an inline policy (if it only needs to be updated)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
a-hilaly authored Jul 7, 2023
1 parent 0e0658d commit 3f75c59
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 3 deletions.
7 changes: 6 additions & 1 deletion pkg/resource/role/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,13 @@ func (rm *resourceManager) syncInlinePolicies(
return err
}
}

for _, pair := range toDelete {
// do not remove elements we just updated with `addInlinePolicy`
if _, ok := lo.Find(toAdd, func(entry lo.Entry[string, string]) bool { return entry.Key == pair.Key }); ok {
continue
}

polName := pair.Key
rlog.Debug(
"removing inline policy from role",
Expand All @@ -236,7 +242,6 @@ func (rm *resourceManager) syncInlinePolicies(
return err
}
}

return nil
}

Expand Down
50 changes: 48 additions & 2 deletions test/e2e/tests/test_role.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,20 @@ def test_crud(self, simple_role):
"Action": ["ec2:Get*"],
"Resource": ["*"]
}]
}'''
inline_doc_2 = '''{
"Version": "2012-10-17",
"Statement": [{
"Effect": "Allow",
"Action": ["s3:Get*"],
"Resource": ["*"]
}]
}'''
updates = {
"spec": {
"inlinePolicies": {
"ec2get": inline_doc,
"s3get": inline_doc_2,
},
},
}
Expand All @@ -189,21 +198,58 @@ def test_crud(self, simple_role):

expect_inline_policies = {
'ec2get': inline_doc,
's3get': inline_doc_2,
}
cr = k8s.get_resource(ref)
assert cr is not None
assert 'spec' in cr
assert 'inlinePolicies' in cr['spec']
assert len(cr['spec']['inlinePolicies']) == 1
assert len(cr['spec']['inlinePolicies']) == 2
assert expect_inline_policies == cr['spec']['inlinePolicies']

latest_inline_policies = role.get_inline_policies(role_name)
assert len(latest_inline_policies) == 1
assert len(latest_inline_policies) == 2
assert 'ec2get' in latest_inline_policies

got_pol_doc = latest_inline_policies['ec2get']
nospace_got_doc = "".join(c for c in got_pol_doc if not c.isspace())
nospace_exp_doc = "".join(c for c in inline_doc if not c.isspace())
assert nospace_exp_doc == nospace_got_doc

got_pol_doc = latest_inline_policies['s3get']
nospace_got_doc = "".join(c for c in got_pol_doc if not c.isspace())
nospace_exp_doc = "".join(c for c in inline_doc_2 if not c.isspace())
assert nospace_exp_doc == nospace_got_doc

inline_doc_s3_get_object = '''{
"Version": "2012-10-17",
"Statement": [{
"Effect": "Allow",
"Action": ["s3:GetObject"],
"Resource": ["*"]
}]
}'''
# update s3get policy document
updates = {
"spec": {
"inlinePolicies": {
"ec2get": inline_doc,
"s3get": inline_doc_s3_get_object,
},
},
}
k8s.patch_custom_resource(ref, updates)
time.sleep(MODIFY_WAIT_AFTER_SECONDS)

latest_inline_policies = role.get_inline_policies(role_name)
assert len(latest_inline_policies) == 2
assert 's3get' in latest_inline_policies
assert 'ec2get' in latest_inline_policies

# expect s3get policy document to change into inlinde_doc_s3_get_object
got_pol_doc = latest_inline_policies['s3get']
nospace_got_doc = "".join(c for c in got_pol_doc if not c.isspace())
nospace_exp_doc = "".join(c for c in inline_doc_s3_get_object if not c.isspace())
assert nospace_exp_doc == nospace_got_doc

# Remove the inline policy we just added and check the updates are
Expand Down

0 comments on commit 3f75c59

Please sign in to comment.