Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) #10

mirelap-amazon · 2020-05-19T11:31:28Z

Issue #, if available:

Description of changes:
This includes updates for the schema, to the CreateHandler resource and its related unit test.

Testing was done as suggested in the README.md file. See the screenshots for more details.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java

gimki

Please make sure it is handling the edge case when PG has been created successfully but putPermission failed.

...filinggroup/src/main/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandler.java

aws-codeguruprofiler-profilinggroup/sample-template.json

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java

ivoanjo

Tiniest of tiniest of notes: We usually never end commit titles with "." :)

ivoanjo · 2020-05-19T15:33:25Z

aws-codeguruprofiler-profilinggroup/aws-codeguruprofiler-profilinggroup.json

@@ -16,6 +20,33 @@
      "maxLength": 255,
      "pattern": "^[\\w-]+$"
    },
+    "Permissions": {
+      "description": "The permissions attached for this profiling group.",


Suggested change

"description": "The permissions attached for this profiling group.",

"description": "The permissions attached to this profiling group.",

Thanks, I will update.

ivoanjo · 2020-05-19T15:45:24Z

aws-codeguruprofiler-profilinggroup/aws-codeguruprofiler-profilinggroup.json

+      "type": "object",
+      "additionalProperties": false,
+      "required": [
+        "AgentPermissions"
+      ],
+      "properties": {
+        "AgentPermissions": {
+          "type": "object",
+          "description": "The permissions for the agent.",
+          "additionalProperties": false,
+          "required": [
+            "Principals"
+          ],
+          "properties": {
+            "Principals": {
+              "description": "The principals for the agent permissions.",
+              "type": "array",
+              "items": {
+                "$ref": "#/definitions/ArnIam"
+              }
+            }
+          }
+        }


This structure seems quite deep:

{ "Type": "AWS::CodeGuruProfiler::ProfilingGroup", "Properties": { "ProfilingGroupName": "some-profiling-group", "Permissions": { "AgentPermissions": { "Principals": ["...list of principals... "] } } } }

What do you think of making it a little more flatter by going with

{ "Type": "AWS::CodeGuruProfiler::ProfilingGroup", "Properties": { "ProfilingGroupName": "some-profiling-group", "Permissions": [ { "ActionGroup": "agentPermissions", "Principals": ["...list of principals..."] } ] } }

or

{ "Type": "AWS::CodeGuruProfiler::ProfilingGroup", "Properties": { "ProfilingGroupName": "some-profiling-group", "AgentPermissions": { "Principals": ["...list of principals... "] } } }

?

Cloudformation is already quite verbose, so the more lightweight we can make it, the better :)

Good point, I will update the code with having AgentPermissions as first-level property, thanks.

ivoanjo · 2020-05-19T15:46:07Z

aws-codeguruprofiler-profilinggroup/aws-codeguruprofiler-profilinggroup.json

@@ -26,7 +57,8 @@
  },
  "additionalProperties": false,
  "required": [
-    "ProfilingGroupName"
+    "ProfilingGroupName",
+    "Permissions"


This seems potentially problematic -- what happens to existing customers that have cloudformation written without specifying permissions?

It makes sense, I will update then the code for Permissions to not be required, thanks.

ivoanjo · 2020-05-19T15:46:40Z

aws-codeguruprofiler-profilinggroup/pom.xml

+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>software.amazon.awssdk</groupId>
+                <artifactId>bom</artifactId>
+                <version>2.13.16</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>


Would be nice to see this change in a separate commit and with an explanation of what it does, so that it would be clear and we could refer to it in the future.

This was later extracted by @gimki as part of PR #11.

…restructuring of the unit tests.

ivoanjo

Looking good! I've left a bunch of comments regarding code style and small improvements, and I think we're almost good to go.

...filinggroup/src/main/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandler.java

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java

ivoanjo · 2020-05-21T13:13:34Z

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java

        proxy = mock(AmazonWebServicesClientProxy.class);
        logger = mock(Logger.class);
-
-        request = makeValidRequest();
+        subject = new CreateHandler();


Minor: All of these can be inlined with the filed declaration itself.

Good point, thanks, I will update.

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java

...filinggroup/src/main/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandler.java

…safeInvokeApi

ivoanjo

Thanks, LGTM 👍

PatMyron · 2020-06-10T00:14:31Z

aws-codeguruprofiler-profilinggroup/aws-codeguruprofiler-profilinggroup.json

+    },
+    "ArnIam": {
+      "type": "string",
+      "pattern": "^arn:aws(-(cn|gov))?:iam::([0-9]{12}):[^.]+$"


The GovCloud partition is incorrect and the isolated partitions are missing in the current Arn definitions

Looking into it in #19

@PatMyron

As pointed out by @PatMyron [in this comment](#10 (comment)) our regular expression patterns for validating arns were missing quite a few partitions. I've gone ahead and fixed that, adding tests as well. I also did the following extra changes: * Made the pattern used for ProfilingGroupArns more specific * Tweaked the pattern used for ChannelUri to allow Arns without region

Add support for AgentPermissions in CloudFormation.

1f35702

gimki reviewed May 19, 2020

View reviewed changes

...nggroup/src/test/java/software/amazon/codeguruprofiler/profilinggroup/CreateHandlerTest.java Outdated Show resolved Hide resolved

Address comment about the static imports.

47d8e94

gimki suggested changes May 19, 2020

View reviewed changes

ivoanjo suggested changes May 19, 2020

View reviewed changes

gimki mentioned this pull request May 20, 2020

Add support for AgentPermissions in CloudFormation (UpdateHandler) #12

Merged

mirelap-amazon added 3 commits May 20, 2020 19:34

Addressed most comments from Colman and Ivo regarding, except of the …

df79639

…restructuring of the unit tests.

Update unit tests to use nested tests

c8e6f63

Merge remote-tracking branch 'origin/master'

ab7580d

mirelap-amazon requested review from ivoanjo and gimki May 20, 2020 18:10

mirelap-amazon changed the title ~~Add support for AgentPermissions in CloudFormation.~~ Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) May 20, 2020

ivoanjo reviewed May 21, 2020

View reviewed changes

gimki reviewed May 21, 2020

View reviewed changes

Addressed most of the comments - exceptions, unit tests, formatting, …

1713e9d

…safeInvokeApi

mirelap-amazon requested review from ivoanjo and gimki May 23, 2020 10:09

gimki self-assigned this May 26, 2020

gimki approved these changes May 26, 2020

View reviewed changes

ivoanjo approved these changes May 26, 2020

View reviewed changes

gimki changed the base branch from master to resource-auth May 26, 2020 13:10

gimki merged commit ee0cbc5 into aws-cloudformation:resource-auth May 26, 2020

gimki mentioned this pull request May 27, 2020

Support specifying resource policy in ProfilingGroup cloudformation #14

Merged

PatMyron reviewed Jun 10, 2020

View reviewed changes

ivoanjo mentioned this pull request Jul 16, 2020

Fix and improve patterns used for ARN validation #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) #10

Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) #10

mirelap-amazon commented May 19, 2020 •

edited

Loading

gimki left a comment

ivoanjo left a comment

ivoanjo May 19, 2020

mirelap-amazon May 20, 2020

ivoanjo May 19, 2020

mirelap-amazon May 20, 2020

ivoanjo May 19, 2020

mirelap-amazon May 20, 2020

ivoanjo May 19, 2020

mirelap-amazon May 20, 2020

ivoanjo left a comment

ivoanjo May 21, 2020

mirelap-amazon May 22, 2020

ivoanjo left a comment

PatMyron Jun 10, 2020

ivoanjo Jul 16, 2020

	"description": "The permissions attached for this profiling group.",
	"description": "The permissions attached to this profiling group.",

Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) #10

Add support for AgentPermissions in CloudFormation (Schema + CreateHandler) #10

Conversation

mirelap-amazon commented May 19, 2020 • edited Loading

gimki left a comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirelap-amazon commented May 19, 2020 •

edited

Loading