Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions apis/v1alpha1/ack-generate-metadata.yaml
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
ack_generate_info:
build_date: "2022-01-24T21:17:47Z"
build_hash: cccec82a27ddd880095383360df1fdc8f530842f
go_version: go1.17.5
build_date: "2022-02-01T21:34:16Z"
build_hash: 4ebcd703a95a2fbd71bd07130f92aa6813c1398b
go_version: go1.17.1
version: v0.16.3
api_directory_checksum: 5c586ade18ff0bb36fe5fcb6d3ffa78b36a2b2c6
api_version: v1alpha1
aws_sdk_go_version: v1.40.2
generator_config_info:
file_checksum: e1e788f094e9560f25c4aa9d3aad9f9b3628bd3d
file_checksum: 2b79f78908b7e3c53c99ec7a56f25d986f107882
original_file_name: generator.yaml
last_modification:
reason: API generation
6 changes: 6 additions & 0 deletions apis/v1alpha1/generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,14 @@ resources:
input_fields:
PolicyName: Name
hooks:
sdk_read_one_post_set_output:
template_path: hooks/policy/sdk_read_one_post_set_output.go.tpl
sdk_create_post_set_output:
template_path: hooks/policy/sdk_create_post_set_output.go.tpl
sdk_update_post_set_output:
template_path: hooks/policy/sdk_update_post_set_output.go.tpl
update_operation:
custom_method_name: customUpdatePolicy
exceptions:
terminal_codes:
- InvalidInput
Expand Down
6 changes: 6 additions & 0 deletions generator.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,14 @@ resources:
input_fields:
PolicyName: Name
hooks:
sdk_read_one_post_set_output:
template_path: hooks/policy/sdk_read_one_post_set_output.go.tpl
sdk_create_post_set_output:
template_path: hooks/policy/sdk_create_post_set_output.go.tpl
sdk_update_post_set_output:
template_path: hooks/policy/sdk_update_post_set_output.go.tpl
update_operation:
custom_method_name: customUpdatePolicy
Copy link
Collaborator

@jaypipes jaypipes Feb 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed something... I see you've added a custom_method_name configuration option to the generator.yaml file for the Policy resource (unlike the Role resource, which uses the sdk_update_post_set_output generic hook point.

I prefer to use the generic hook points instead of creating a custom update method. Can you change that?

LATER...

Never mind, @RedbackThomson, I realized now that you had to use a custom update method because there isn't an actual UpdatePolicy API call...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, @RedbackThomson, I realized now that you had to use a custom update method because there isn't an actual UpdatePolicy API call...

Yeah this is a non-obvious configuration problem you have when trying to use hooks. I'll remove the hook point, though. Good call.

exceptions:
terminal_codes:
- InvalidInput
Expand Down
193 changes: 193 additions & 0 deletions pkg/resource/policy/hooks.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License"). You may
// not use this file except in compliance with the License. A copy of the
// License is located at
//
// http://aws.amazon.com/apache2.0/
//
// or in the "license" file accompanying this file. This file is distributed
// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
// express or implied. See the License for the specific language governing
// permissions and limitations under the License.

package policy

import (
"context"

ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
ackcondition "github.com/aws-controllers-k8s/runtime/pkg/condition"
ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
svcsdk "github.com/aws/aws-sdk-go/service/iam"
corev1 "k8s.io/api/core/v1"

svcapitypes "github.com/aws-controllers-k8s/iam-controller/apis/v1alpha1"
)

func (rm *resourceManager) customUpdatePolicy(
ctx context.Context,
desired *resource,
latest *resource,
delta *ackcompare.Delta,
) (*resource, error) {
ko := desired.ko.DeepCopy()

rm.setStatusDefaults(ko)

if err := rm.syncTags(ctx, &resource{ko}); err != nil {
return nil, err
}
Comment on lines +38 to +40
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this statement should go inside an if statement similar to:

if delta.DifferentAt("Spec.Tags") { ... }

In the future we should also deal with cases when a user modifies other fields like: Description, PolicyDocument etc...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically, there won't be any calls any update APIs if there aren't any changes (there will be an additional List operation, however, so we should be able to remove that call by guarding this with a delta.DifferentAt()` conditional. 👍

// There really isn't a status of a policy... it either exists or doesn't.
// If we get here, that means the update was successful and the desired
// state of the policy matches what we provided...
ackcondition.SetSynced(&resource{ko}, corev1.ConditionTrue, nil, nil)

return &resource{ko}, nil
}

// syncTags examines the Tags in the supplied Policy and calls the
// ListPolicyTags, TagPolicy and UntagPolicy APIs to ensure that the set of
// associated Tags stays in sync with the Policy.Spec.Tags
func (rm *resourceManager) syncTags(
ctx context.Context,
r *resource,
) (err error) {
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("rm.syncTags")
defer exit(err)
toAdd := []*svcapitypes.Tag{}
toDelete := []*svcapitypes.Tag{}

existingTags, err := rm.getTags(ctx, r)
if err != nil {
return err
}

for _, t := range r.ko.Spec.Tags {
if !inTags(*t.Key, *t.Value, existingTags) {
toAdd = append(toAdd, t)
}
}

for _, t := range existingTags {
if !inTags(*t.Key, *t.Value, r.ko.Spec.Tags) {
toDelete = append(toDelete, t)
}
}

if len(toAdd) > 0 {
for _, t := range toAdd {
rlog.Debug("adding tag to policy", "key", *t.Key, "value", *t.Value)
}
if err = rm.addTags(ctx, r, toAdd); err != nil {
return err
}
}

if len(toDelete) > 0 {
for _, t := range toDelete {
rlog.Debug("removing tag from policy", "key", *t.Key, "value", *t.Value)
}
if err = rm.removeTags(ctx, r, toDelete); err != nil {
return err
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RedbackThomson so I'm wondering if the fact that we are doing the deletes AFTER the adds here is the cause of the test failure. When a Tag's value only is changed, the Tag will be in both the toAdd (the updated Tag k/v) and toDelete (the original Tag k/v) collections. I'm wondering if the the RemoveTags API call only looks at the Tag key? If that is the case, the call to RemoveTag will end up removing the Tag erroneously. An easy way of checking if my hypothesis is correct would be to simply change the order above and do the removeTags call before the addTags call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, yeah. You should be able to use TagPolicy to update any tag, just by giving it an existing key with a new value. But for the sake of staying consistent with your hooks on Role, I'll just switch the order for now and make it do a delete then add.

I did test updating values in my manual testing and didn't see an issue, but I think combining an update with a delete might be the condition that is causing this problem here.


return nil
}

// inTags returns true if the supplied key and value can be found in the
// supplied list of Tag structs.
//
// TODO(jaypipes): When we finally standardize Tag handling in ACK, move this
// to the ACK common runtime/ or pkg/ repos
func inTags(
key string,
value string,
tags []*svcapitypes.Tag,
) bool {
for _, t := range tags {
if *t.Key == key && *t.Value == value {
return true
}
}
return false
}

// getTags returns the list of tags attached to the Policy
func (rm *resourceManager) getTags(
ctx context.Context,
r *resource,
) ([]*svcapitypes.Tag, error) {
var err error
var resp *svcsdk.ListPolicyTagsOutput
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("rm.getTags")
defer exit(err)

input := &svcsdk.ListPolicyTagsInput{}
input.PolicyArn = (*string)(r.ko.Status.ACKResourceMetadata.ARN)
res := []*svcapitypes.Tag{}

for {
resp, err = rm.sdkapi.ListPolicyTagsWithContext(ctx, input)
if err != nil || resp == nil {
break
}
for _, t := range resp.Tags {
res = append(res, &svcapitypes.Tag{Key: t.Key, Value: t.Value})
}
if resp.IsTruncated != nil && !*resp.IsTruncated {
break
}
}
rm.metrics.RecordAPICall("GET", "ListPolicyTags", err)
return res, err
}

// addTags adds the supplied Tags to the supplied Policy resource
func (rm *resourceManager) addTags(
ctx context.Context,
r *resource,
tags []*svcapitypes.Tag,
) (err error) {
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("rm.addTags")
defer exit(err)

input := &svcsdk.TagPolicyInput{}
input.PolicyArn = (*string)(r.ko.Status.ACKResourceMetadata.ARN)
inTags := []*svcsdk.Tag{}
for _, t := range tags {
inTags = append(inTags, &svcsdk.Tag{Key: t.Key, Value: t.Value})
}
input.Tags = inTags

_, err = rm.sdkapi.TagPolicyWithContext(ctx, input)
rm.metrics.RecordAPICall("CREATE", "TagPolicy", err)
return err
}

// removeTags removes the supplied Tags from the supplied Policy resource
func (rm *resourceManager) removeTags(
ctx context.Context,
r *resource,
tags []*svcapitypes.Tag,
) (err error) {
rlog := ackrtlog.FromContext(ctx)
exit := rlog.Trace("rm.removeTags")
defer exit(err)

input := &svcsdk.UntagPolicyInput{}
input.PolicyArn = (*string)(r.ko.Status.ACKResourceMetadata.ARN)
inTagKeys := []*string{}
for _, t := range tags {
inTagKeys = append(inTagKeys, t.Key)
}
input.TagKeys = inTagKeys

_, err = rm.sdkapi.UntagPolicyWithContext(ctx, input)
rm.metrics.RecordAPICall("DELETE", "UntagPolicy", err)
return err
}
9 changes: 7 additions & 2 deletions pkg/resource/policy/sdk.go

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

2 changes: 1 addition & 1 deletion pkg/resource/role/hooks.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ func inTags(
return false
}

// getTags returns the list of Policy ARNs currently attached to the Role
// getTags returns the list of tags to the Role
func (rm *resourceManager) getTags(
ctx context.Context,
r *resource,
Expand Down
5 changes: 5 additions & 0 deletions templates/hooks/policy/sdk_read_one_post_set_output.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
if tags, err := rm.getTags(ctx, &resource{ko}); err != nil {
return nil, err
} else {
ko.Spec.Tags = tags
}
3 changes: 3 additions & 0 deletions templates/hooks/policy/sdk_update_post_set_output.go.tpl
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
if err := rm.syncTags(ctx, &resource{ko}); err != nil {
return nil, err
}
13 changes: 13 additions & 0 deletions test/e2e/policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -100,3 +100,16 @@ def get(policy_arn):
return resp['Policy']
except c.exceptions.NoSuchEntityException:
return None

def get_tags(policy_arn):
"""Returns a list containing the tags that have been associated to the
supplied Policy.

If no such Policy exists, returns None.
"""
c = boto3.client('iam')
try:
resp = c.list_policy_tags(PolicyArn=policy_arn)
return resp['Tags']
except c.exceptions.NoSuchEntityException:
return None
3 changes: 3 additions & 0 deletions test/e2e/resources/policy_simple.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ spec:
name: $POLICY_NAME
description: $POLICY_DESCRIPTION
policyDocument: '{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Action":"s3:ListAllMyBuckets","Resource":"arn:aws:s3:::*"},{"Effect":"Allow","Action":["s3:List*"],"Resource":["*"]}]}'
tags:
- key: tag1
value: val1
45 changes: 45 additions & 0 deletions test/e2e/tests/test_policy.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

DELETE_WAIT_AFTER_SECONDS = 10
CHECK_WAIT_AFTER_SECONDS = 10
MODIFY_WAIT_AFTER_SECONDS = 10


@service_marker
Expand Down Expand Up @@ -65,6 +66,50 @@ def test_crud(self):

policy.wait_until_exists(policy_arn)

# Same update code path check for tags...
latest_tags = policy.get_tags(policy_arn)
before_update_expected_tags = [
{
"Key": "tag1",
"Value": "val1"
}
]
assert latest_tags == before_update_expected_tags
new_tags = [
{
"key": "tag2",
"value": "val2",
}
]
updates = {
"spec": {"tags": new_tags},
}
k8s.patch_custom_resource(ref, updates)
time.sleep(MODIFY_WAIT_AFTER_SECONDS)

latest_tags = policy.get_tags(policy_arn)
after_update_expected_tags = [
{
"Key": "tag2",
"Value": "val2",
}
]
assert latest_tags == after_update_expected_tags
new_tags = [
{
"key": "tag2",
"value": "val3", # Update the value
}
]
updates = {
"spec": {"tags": new_tags},
}
k8s.patch_custom_resource(ref, updates)
time.sleep(MODIFY_WAIT_AFTER_SECONDS)

latest_tags = policy.get_tags(policy_arn)
assert latest_tags == new_tags

k8s.delete_custom_resource(ref)

time.sleep(DELETE_WAIT_AFTER_SECONDS)
Expand Down