feat(aws-iam): add aws iam instance profile: fixes #1223 #1224

cmaurer · 2018-11-21T04:10:28Z

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

eladb

Thanks for the contribution!

packages/@aws-cdk/aws-iam/lib/instance-profile.ts

packages/@aws-cdk/aws-iam/test/test.instance-profile.ts

eladb

A few more requests that I missed last time...

packages/@aws-cdk/aws-iam/README.md

packages/@aws-cdk/aws-iam/lib/instance-profile.ts

packages/@aws-cdk/aws-iam/lib/instance-profile-ref.ts

packages/@aws-cdk/aws-iam/lib/instance-profile.ts

eladb · 2018-12-09T13:43:30Z

packages/@aws-cdk/aws-iam/lib/role.ts

+  /**
+   * The role's Name
+   */
+  roleName: string;


I am not sure that making both ARN and name required is the best developer experience. In many cases one can be deduced from the other. See example in ECR, where both are actually optional and one is deduced from the other in certain cases.

Not 100% sure that's the ideal situation, but I wouldn't want to require both if they can be deduced.

Sounds good. I will take a look.

eladb · 2018-12-13T04:50:56Z

packages/@aws-cdk/aws-iam/lib/role.ts

+      this.roleArn = ArnUtils.fromComponents({
+        service: 'iam',
+        resource: 'role'
+      });


Where is resourceName?

eladb · 2018-12-13T04:53:03Z

packages/@aws-cdk/aws-iam/lib/role.ts

+      if (unresolved(this.roleArn)) {
+        throw new Error('roleArn is a late-bound value, and therefore roleName is required');
+      }
+      this.roleName = this.roleArn.split('/').slice(1).join('/');


Use ArnUtils.parse

@eladb
this.roleName is required (which I am pretty sure is correct), but its optional on ArnComponents, so I am getting the typescript error 'type string | undefined is not assignable...'. Thats why I used the split/slice/join. So, I am not sure what I should do here if the name is not supplied. undefined seems wrong, '' seems wrong, and making up a name seems wrong. I am probably missing something small, here.

nevermind, I think I figured it out.

eladb · 2018-12-13T05:02:05Z

packages/@aws-cdk/aws-iam/test/test.instance-profile.ts

+
+/**
+ *  Test Cases:
+ *  parameters: role


I don't think this is necessary, and will likely become stale very quickly

this was my 'punch list', I will remove.

eladb · 2018-12-13T05:02:30Z

packages/@aws-cdk/aws-dynamodb/lib/table.ts

@@ -559,6 +559,7 @@ export class Table extends Construct {
  private makeScalingRole(): iam.IRole {
    // Use a Service Linked Role.
    return iam.Role.import(this, 'ScalingRole', {
+      roleName: "DynamoDbAutoScalingRole",


eladb · 2018-12-13T05:02:39Z

packages/@aws-cdk/aws-ecs/lib/base/base-service.ts

@@ -227,6 +227,7 @@ export abstract class BaseService extends cdk.Construct
  private makeAutoScalingRole(): iam.IRole {
    // Use a Service Linked Role.
    return iam.Role.import(this, 'ScalingRole', {
+      roleName: 'ECSServiceAutoScalingRole',


eladb · 2018-12-13T05:04:22Z

packages/@aws-cdk/aws-iam/lib/instance-profile-ref.ts

+     * @link http://docs.aws.amazon.com/IAM/latest/UserGuide/Using_Identifiers.html#Identifiers_FriendlyNames
+     * @default / By default, AWS CloudFormation specifies '/' for the path.
+     */
+    path: string;


shouldn't that be optional?

eladb · 2018-12-13T05:04:56Z

packages/@aws-cdk/aws-iam/lib/instance-profile.ts

+import { InstanceProfileRef } from './instance-profile-ref';
+import { PolicyStatement, ServicePrincipal } from './policy-document';
+import { IRole, Role } from './role';
+export interface InstanceProfileProps {


eladb · 2018-12-13T05:11:06Z

packages/@aws-cdk/aws-iam/lib/instance-profile-ref.ts

+    /**
+     * Path for the Instance Profile
+     * @link http://docs.aws.amazon.com/IAM/latest/UserGuide/Using_Identifiers.html#Identifiers_FriendlyNames
+     * @default / If a path is not provided, CloudFormation defaults the path to '/'.


is it cfn that provides the default?

eladb · 2018-12-13T05:13:53Z

packages/@aws-cdk/aws-iam/test/test.role.ts

+  },
+  'import/export':  {
+
+    'import with name'(test: Test) {


This is actually "import with an exported role"

eladb · 2018-12-13T05:15:11Z

packages/@aws-cdk/aws-iam/test/test.role.ts

+
+    test.done();
+  },
+


missing some cases here:

import with ARN token (failure)

import with name string (success)

import with name token (success)

import with ARN + name

import without ARN+name (failure)

rix0rrr · 2019-01-15T15:29:13Z

This PR needs some additional work and a merge from master, unfortunately.

eladb · 2019-02-05T11:40:07Z

@cmaurer closing for now. feel free to reopen when/if you wish to continue this work.

cmaurer added 2 commits November 20, 2018 21:38

cmaurer: adding resources for iam instance profile

32827f1

feat(aws-iam): add aws iam instance profile: fixes aws#1223

0904436

eladb reviewed Nov 21, 2018

View reviewed changes

feat(aws-iam): add aws iam instance profile: fixes aws#1223

aa082b4

eladb suggested changes Nov 22, 2018

View reviewed changes

cmaurer added 3 commits November 26, 2018 11:14

feat(aws-iam): add aws iam instance profile: fixes aws#1223

b5b02de

feat(aws-iam): add aws iam instance profile: fixes aws#1223

3e0d0d1

feat(aws-iam): add aws iam instance profile: fixes aws#1223

c0a865d

rix0rrr reviewed Nov 27, 2018

View reviewed changes

cmaurer added 4 commits November 27, 2018 15:38

feat(aws-iam): add aws iam instance profile: fixes aws#1223

e85e4a0

feat(aws-iam): add aws iam instance profile: fixes aws#1223

0c6fdd2

feat(aws-iam): add aws iam instance profile: fixes aws#1223

dda44a6

Merge branch 'master' into cmaurer-1223

d3a292b

eladb suggested changes Dec 9, 2018

View reviewed changes

feat(aws-iam): add aws iam instance profile: fixes aws#1223

fbf3841

cmaurer requested a review from a team as a code owner December 13, 2018 03:39

eladb reviewed Dec 13, 2018

View reviewed changes

rix0rrr added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 15, 2019

eladb closed this Feb 5, 2019

sumodgeorge mentioned this pull request Dec 2, 2023

[Snyk] Security upgrade nyc from 13.0.1 to 15.0.0 sumodgeorge/aws-cdk#19

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(aws-iam): add aws iam instance profile: fixes #1223 #1224

feat(aws-iam): add aws iam instance profile: fixes #1223 #1224

cmaurer commented Nov 21, 2018

eladb left a comment

eladb left a comment

eladb Dec 9, 2018

cmaurer Dec 9, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

cmaurer Dec 13, 2018

cmaurer Dec 27, 2018

eladb Dec 13, 2018

cmaurer Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

eladb Dec 13, 2018

rix0rrr commented Jan 15, 2019

eladb commented Feb 5, 2019

feat(aws-iam): add aws iam instance profile: fixes #1223 #1224

feat(aws-iam): add aws iam instance profile: fixes #1223 #1224

Conversation

cmaurer commented Nov 21, 2018

eladb left a comment

Choose a reason for hiding this comment

eladb 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

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

rix0rrr commented Jan 15, 2019

eladb commented Feb 5, 2019