Add support for Policy Tag updates #15

RedbackThomson · 2022-02-01T22:12:03Z

Fixes aws-controllers-k8s/community#1124

Description of changes:
Adds custom hook code to support the TagPolicy and UntagPolicy SDK methods for adding, updating and deleting tags on the Policy custom resource.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

vijtrip2

Looks great! 🚀

nit: For the modify tags test, can you also add another new key and also delete an existing key? : )

RedbackThomson · 2022-02-02T00:26:06Z

nit: For the modify tags test, can you also add another new key and also delete an existing key? : )

The first patch deletes the tag1 key and adds the tag2 key. I can add a test to further update the existing tag2 key.

jaypipes

appreciate you picking up my slack, @RedbackThomson. thank you sir!

pkg/resource/role/hooks.go

jaypipes · 2022-02-02T13:07:11Z

/test iam-kind-e2e

jaypipes · 2022-02-02T13:30:17Z

@RedbackThomson looks like the test failure is legit:

         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
E       AssertionError: assert [] == [{'key': 'tag...lue': 'val3'}]
E         Right contains one more item: {'key': 'tag2', 'value': 'val3'}
E         Full diff:
E         - [{'key': 'tag2', 'value': 'val3'}]
E         + []

tests/test_policy.py:111: AssertionError

jaypipes · 2022-02-02T14:28:29Z

generator.yaml

+      sdk_update_post_set_output:
+        template_path: hooks/policy/sdk_update_post_set_output.go.tpl
+    update_operation:
+      custom_method_name: customUpdatePolicy


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...

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.

generator.yaml

jaypipes · 2022-02-02T14:43:28Z

pkg/resource/policy/hooks.go

+	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
+		}
+	}


@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.

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.

RedbackThomson · 2022-02-02T20:22:14Z

Tests failing due to:

botocore.exceptions.ClientError: An error occurred (Throttling) when calling the ListPolicyTags operation (reached max retries: 4): Rate exceeded

I guess I'll wait a little while to retry? This throttling seems to take a while to lift.

vijtrip2 · 2022-02-02T21:13:25Z

/lgtm

ack-bot · 2022-02-02T21:13:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RedbackThomson, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [RedbackThomson,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

a-hilaly · 2022-02-14T01:56:34Z

pkg/resource/policy/hooks.go

+	if err := rm.syncTags(ctx, &resource{ko}); err != nil {
+		return nil, err
+	}


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...

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. 👍

Nicholas Thomson added 2 commits February 1, 2022 14:02

Add hooks to sync policy tags

4f424af

Add e2e tests for tag updates

735ec0d

ack-bot requested review from a-hilaly and vijtrip2 February 1, 2022 22:12

ack-bot added the approved label Feb 1, 2022

vijtrip2 approved these changes Feb 1, 2022

View reviewed changes

Add test for modifying policy tags

895adfe

jaypipes approved these changes Feb 2, 2022

View reviewed changes

pkg/resource/role/hooks.go Show resolved Hide resolved

jaypipes requested changes Feb 2, 2022

View reviewed changes

jaypipes reviewed Feb 2, 2022

View reviewed changes

Nicholas Thomson added 2 commits February 2, 2022 11:52

Fix policy tag update method ordering

2b750c8

Fix casing in test assertion

b39551d

ack-bot assigned vijtrip2 Feb 2, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2022

ack-bot merged commit 871897e into aws-controllers-k8s:main Feb 2, 2022

RedbackThomson deleted the policy-tag branch February 2, 2022 21:19

a-hilaly reviewed Feb 14, 2022

View reviewed changes

Add support for Policy Tag updates #15

Add support for Policy Tag updates #15

Uh oh!

Conversation

RedbackThomson commented Feb 1, 2022

Uh oh!

vijtrip2 left a comment

Choose a reason for hiding this comment

Uh oh!

RedbackThomson commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaypipes commented Feb 2, 2022

Uh oh!

jaypipes commented Feb 2, 2022

Uh oh!

jaypipes Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jaypipes Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

RedbackThomson Feb 2, 2022

Choose a reason for hiding this comment

Uh oh!

RedbackThomson commented Feb 2, 2022

Uh oh!

vijtrip2 commented Feb 2, 2022

Uh oh!

ack-bot commented Feb 2, 2022

Uh oh!

a-hilaly Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

jaypipes Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RedbackThomson commented Feb 2, 2022 •

edited

Loading

jaypipes Feb 2, 2022 •

edited

Loading