From 4f424af4982e707aeb1be2b82b47353ced989371 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Tue, 1 Feb 2022 14:02:25 -0800 Subject: [PATCH 1/5] Add hooks to sync policy tags --- apis/v1alpha1/ack-generate-metadata.yaml | 8 +- apis/v1alpha1/generator.yaml | 6 + generator.yaml | 6 + pkg/resource/policy/hooks.go | 193 ++++++++++++++++++ pkg/resource/policy/sdk.go | 9 +- pkg/resource/role/hooks.go | 2 +- .../sdk_read_one_post_set_output.go.tpl | 5 + .../policy/sdk_update_post_set_output.go.tpl | 3 + 8 files changed, 225 insertions(+), 7 deletions(-) create mode 100644 pkg/resource/policy/hooks.go create mode 100644 templates/hooks/policy/sdk_read_one_post_set_output.go.tpl create mode 100644 templates/hooks/policy/sdk_update_post_set_output.go.tpl diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 3d1f5b8..d907779 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -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 diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index f95e0b3..764ff56 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -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 diff --git a/generator.yaml b/generator.yaml index f95e0b3..764ff56 100644 --- a/generator.yaml +++ b/generator.yaml @@ -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 diff --git a/pkg/resource/policy/hooks.go b/pkg/resource/policy/hooks.go new file mode 100644 index 0000000..b1488f8 --- /dev/null +++ b/pkg/resource/policy/hooks.go @@ -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 + } + // 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 + } + } + + 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 +} diff --git a/pkg/resource/policy/sdk.go b/pkg/resource/policy/sdk.go index a6e568d..f17ba26 100644 --- a/pkg/resource/policy/sdk.go +++ b/pkg/resource/policy/sdk.go @@ -155,6 +155,12 @@ func (rm *resourceManager) sdkFind( } rm.setStatusDefaults(ko) + if tags, err := rm.getTags(ctx, &resource{ko}); err != nil { + return nil, err + } else { + ko.Spec.Tags = tags + } + return &resource{ko}, nil } @@ -339,8 +345,7 @@ func (rm *resourceManager) sdkUpdate( latest *resource, delta *ackcompare.Delta, ) (*resource, error) { - // TODO(jaypipes): Figure this out... - return nil, ackerr.NotImplemented + return rm.customUpdatePolicy(ctx, desired, latest, delta) } // sdkDelete deletes the supplied resource in the backend AWS service API diff --git a/pkg/resource/role/hooks.go b/pkg/resource/role/hooks.go index fc32deb..6555458 100644 --- a/pkg/resource/role/hooks.go +++ b/pkg/resource/role/hooks.go @@ -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, diff --git a/templates/hooks/policy/sdk_read_one_post_set_output.go.tpl b/templates/hooks/policy/sdk_read_one_post_set_output.go.tpl new file mode 100644 index 0000000..d15c8cb --- /dev/null +++ b/templates/hooks/policy/sdk_read_one_post_set_output.go.tpl @@ -0,0 +1,5 @@ + if tags, err := rm.getTags(ctx, &resource{ko}); err != nil { + return nil, err + } else { + ko.Spec.Tags = tags + } diff --git a/templates/hooks/policy/sdk_update_post_set_output.go.tpl b/templates/hooks/policy/sdk_update_post_set_output.go.tpl new file mode 100644 index 0000000..d7181f7 --- /dev/null +++ b/templates/hooks/policy/sdk_update_post_set_output.go.tpl @@ -0,0 +1,3 @@ + if err := rm.syncTags(ctx, &resource{ko}); err != nil { + return nil, err + } From 735ec0d583d20967aed4fe6fa504afa3326a72c2 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Tue, 1 Feb 2022 14:10:21 -0800 Subject: [PATCH 2/5] Add e2e tests for tag updates --- test/e2e/policy.py | 13 +++++++++++ test/e2e/resources/policy_simple.yaml | 3 +++ test/e2e/tests/test_policy.py | 31 +++++++++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/test/e2e/policy.py b/test/e2e/policy.py index ef4b191..17c29c4 100644 --- a/test/e2e/policy.py +++ b/test/e2e/policy.py @@ -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 \ No newline at end of file diff --git a/test/e2e/resources/policy_simple.yaml b/test/e2e/resources/policy_simple.yaml index ab8f042..3bfc7d0 100644 --- a/test/e2e/resources/policy_simple.yaml +++ b/test/e2e/resources/policy_simple.yaml @@ -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 \ No newline at end of file diff --git a/test/e2e/tests/test_policy.py b/test/e2e/tests/test_policy.py index 445586b..2a6e802 100644 --- a/test/e2e/tests/test_policy.py +++ b/test/e2e/tests/test_policy.py @@ -27,6 +27,7 @@ DELETE_WAIT_AFTER_SECONDS = 10 CHECK_WAIT_AFTER_SECONDS = 10 +MODIFY_WAIT_AFTER_SECONDS = 10 @service_marker @@ -65,6 +66,36 @@ 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) + + after_update_expected_tags = [ + { + "Key": "tag2", + "Value": "val2", + } + ] + latest_tags = policy.get_tags(policy_arn) + assert latest_tags == after_update_expected_tags + k8s.delete_custom_resource(ref) time.sleep(DELETE_WAIT_AFTER_SECONDS) From 895adfec5cfe3ef9260c7c77f2b74b06b11e6332 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Tue, 1 Feb 2022 16:29:23 -0800 Subject: [PATCH 3/5] Add test for modifying policy tags --- test/e2e/tests/test_policy.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/test/e2e/tests/test_policy.py b/test/e2e/tests/test_policy.py index 2a6e802..cc14c56 100644 --- a/test/e2e/tests/test_policy.py +++ b/test/e2e/tests/test_policy.py @@ -87,14 +87,28 @@ def test_crud(self): 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", } ] - latest_tags = policy.get_tags(policy_arn) 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) From 2b750c8fbd6516b6979e48151de42f29d07348fe Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Wed, 2 Feb 2022 11:52:07 -0800 Subject: [PATCH 4/5] Fix policy tag update method ordering --- apis/v1alpha1/ack-generate-metadata.yaml | 4 ++-- apis/v1alpha1/generator.yaml | 8 ++++++-- generator.yaml | 8 ++++++-- pkg/resource/policy/hooks.go | 16 ++++++++-------- .../policy/sdk_update_post_set_output.go.tpl | 3 --- 5 files changed, 22 insertions(+), 17 deletions(-) delete mode 100644 templates/hooks/policy/sdk_update_post_set_output.go.tpl diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index d907779..f45a334 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,5 +1,5 @@ ack_generate_info: - build_date: "2022-02-01T21:34:16Z" + build_date: "2022-02-02T19:49:37Z" build_hash: 4ebcd703a95a2fbd71bd07130f92aa6813c1398b go_version: go1.17.1 version: v0.16.3 @@ -7,7 +7,7 @@ api_directory_checksum: 5c586ade18ff0bb36fe5fcb6d3ffa78b36a2b2c6 api_version: v1alpha1 aws_sdk_go_version: v1.40.2 generator_config_info: - file_checksum: 2b79f78908b7e3c53c99ec7a56f25d986f107882 + file_checksum: 72469db1ef2738db804a8c42687c20305eadc2c1 original_file_name: generator.yaml last_modification: reason: API generation diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 764ff56..1dc3f86 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -32,9 +32,13 @@ resources: 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: + # There is no `UpdatePolicy` API operation. The only way to update a + # policy is to update the properties individually (only a few properties + # support this) or to delete the policy and recreate it entirely. + # + # This custom method will support updating the properties individually, + # but there is currently no support for the delete/create option. custom_method_name: customUpdatePolicy exceptions: terminal_codes: diff --git a/generator.yaml b/generator.yaml index 764ff56..1dc3f86 100644 --- a/generator.yaml +++ b/generator.yaml @@ -32,9 +32,13 @@ resources: 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: + # There is no `UpdatePolicy` API operation. The only way to update a + # policy is to update the properties individually (only a few properties + # support this) or to delete the policy and recreate it entirely. + # + # This custom method will support updating the properties individually, + # but there is currently no support for the delete/create option. custom_method_name: customUpdatePolicy exceptions: terminal_codes: diff --git a/pkg/resource/policy/hooks.go b/pkg/resource/policy/hooks.go index b1488f8..5493251 100644 --- a/pkg/resource/policy/hooks.go +++ b/pkg/resource/policy/hooks.go @@ -76,20 +76,20 @@ func (rm *resourceManager) syncTags( } } - if len(toAdd) > 0 { - for _, t := range toAdd { - rlog.Debug("adding tag to policy", "key", *t.Key, "value", *t.Value) + if len(toDelete) > 0 { + for _, t := range toDelete { + rlog.Debug("removing tag from policy", "key", *t.Key, "value", *t.Value) } - if err = rm.addTags(ctx, r, toAdd); err != nil { + if err = rm.removeTags(ctx, r, toDelete); 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 len(toAdd) > 0 { + for _, t := range toAdd { + rlog.Debug("adding tag to policy", "key", *t.Key, "value", *t.Value) } - if err = rm.removeTags(ctx, r, toDelete); err != nil { + if err = rm.addTags(ctx, r, toAdd); err != nil { return err } } diff --git a/templates/hooks/policy/sdk_update_post_set_output.go.tpl b/templates/hooks/policy/sdk_update_post_set_output.go.tpl deleted file mode 100644 index d7181f7..0000000 --- a/templates/hooks/policy/sdk_update_post_set_output.go.tpl +++ /dev/null @@ -1,3 +0,0 @@ - if err := rm.syncTags(ctx, &resource{ko}); err != nil { - return nil, err - } From b39551db5520dd3cd5ec2bfb43932c68abf03823 Mon Sep 17 00:00:00 2001 From: Nicholas Thomson Date: Wed, 2 Feb 2022 12:05:22 -0800 Subject: [PATCH 5/5] Fix casing in test assertion --- test/e2e/tests/test_policy.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/test/e2e/tests/test_policy.py b/test/e2e/tests/test_policy.py index cc14c56..e7bfda8 100644 --- a/test/e2e/tests/test_policy.py +++ b/test/e2e/tests/test_policy.py @@ -108,7 +108,13 @@ def test_crud(self): time.sleep(MODIFY_WAIT_AFTER_SECONDS) latest_tags = policy.get_tags(policy_arn) - assert latest_tags == new_tags + after_update_expected_tags = [ + { + "Key": "tag2", + "Value": "val3", + } + ] + assert latest_tags == after_update_expected_tags k8s.delete_custom_resource(ref)