Repository: udondan/cdk-ec2-key-pair (language: python)

Issue:
names function but doesn't constrain name length
```
❯ npx cdk list

/Users/ahammond/Documents/ClickUp/cold-storage-cdk/node_modules/aws-cdk-lib/aws-lambda/lib/function.ts:662
        throw new Error(`Function name can not be longer than 64 characters but has ${props.functionName.length} characters.`);
              ^
Error: Function name can not be longer than 64 characters but has 74 characters.
    at new Function (/Users/ahammond/Documents/ClickUp/cold-storage-cdk/node_modules/aws-cdk-lib/aws-lambda/lib/function.ts:662:15)
    at KeyPair.ensureLambda (/Users/ahammond/Documents/ClickUp/cold-storage-cdk/node_modules/cdk-ec2-key-pair/lib/index.ts:369:16)
    at new KeyPair (/Users/ahammond/Documents/ClickUp/cold-storage-cdk/node_modules/cdk-ec2-key-pair/lib/index.ts:214:24)
```

feat: remove the function name
Named functions are more trouble than they're worth.
The description provides the same value, without the annoyance.

Fixes: #53



Discussion / hints:
This popped up when I tried to deploy using a CdkPipeline. I'm guessing the pipelines do stupid tricks with the names.
Ah, I see in the code that it's testing to confirm the function name isn't too long, and throwing. So, the workaround is to use the `resourcePrefix`. But... why give the function an explicit name at all? That's just asking for trouble.
Oh man. So when I just set the prefix to "x", I get collisions when I try to deploy multiples of these in an environment. Having a name on this function is not a feature. It's just a source of problems.
> Ah, I see in the code that it's testing to confirm the function name isn't too long, and throwing. So, the workaround is to use the `resourcePrefix`. But... why give the function an explicit name at all? That's just asking for trouble.

Hello @ahammond,
when you wrote you used the `resourcePrefix` as a workaround, did you maybe mean `secretPrefix`?
I can't seem to find any `resourcePrefix` in the documentation.

Sorry guys. My amount of time for OS currently is negative.

You could just use the CDK escape hatches to get around this. The Lambda is exposed as a resource property.

```ts
const cfnFunction = keyPair.lambda.node.defaultChild as cdk.aws_lambda.CfnFunction;

const cfnRole = keyPair.lambda.role!.node.defaultChild as cdk.aws_iam.CfnRole;

cfnFunction.addPropertyDeletionOverride('FunctionName');
cfnRole.addPropertyDeletionOverride('RoleName');
```

Unfortunately the policy is not exposed. But you could visit it via aspects. Something like this:

```ts
cdk.Aspects.of(this).add(new RemovePolicyName(this));
```

```ts
export class RemovePolicyName implements cdk.IAspect {
  constructor(stack: cdk.Stack) {}

  public visit(construct: IConstruct): void {
    if (construct instanceof cdk.aws_iam.Policy) {
      const cfnPolicy = construct.node.defaultChild as cdk.aws_iam.CfnPolicy;
      cfnPolicy.addPropertyDeletionOverride('PolicyName');
    }
  }
}
```
Hi @udondan ,

thanks for the hint.
Unfortunately, at least for the python implementations, this will not work.
It seems that when using CDK escape hatches, you are expected to operate on an already existing object.
But the object does not exist - cdk throws the reported exception when trying to create an instance of it (i.e., of the KeyPair object).

Could you give us an estimation of more or less when could we expect the solution of #54 to be merged?
(Not trying to push nobody in any way)

I'm having the same issue here. I tried to apply the workaround mentioned for @udondan, but doesn't work. I'm not quite sure how to apply it. I have this:

```typescript
        const key = new KeyPair(this, `${id}-ec2-keypair`, {
            name: 'keypair',
            storePublicKey: true,
        });

        // workaround for KeyPair implementation
        const cfnFunction = key.lambda.node.defaultChild as aws_lambda.CfnFunction;
        const cfnRole = key.lambda.role!.node.defaultChild as aws_iam.CfnRole;
        cfnFunction.addPropertyDeletionOverride('FunctionName');
        cfnRole.addPropertyDeletionOverride('RoleName');
```
        
What am I doing wrong?
Think I'm going to deprecate this package. CFN now supports creation of KeyPairs and this feature now is available since CDK 2.29.0. 

https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2.CfnKeyPair.html

Havent tested it yet. The only difference seems to be it stores the key in ssm paramater instead of secret. If that realy all works there is no need for this package any longer.
NOT going to deprecate this. 😸 The CfnKeyPair is generally working and certainly does the trick for most use cases. But it does unfortunately not cover everyting this construct does. Directly accessing the public and private keys from CDK and forwarding it to other resources was the blocker in my use cases.
For visibility, I'll quote my [comment](https://github.com/udondan/cdk-ec2-key-pair/pull/54#issuecomment-1722846139) here:

> Personally I do not have a problem with this. Though AWS does. You cannot change the Lambda function name for **existing** custom resources. CFN will fail with:
> 
> > Modifying service token is not allowed.
> 
> By removing the function name we would force all users to delete and re-create their KeyPairs. 
> 
> I see only one option how we can change this: A new contruct property, e.g. `lambdaName`. 
> 
> - If `undefined`: We use the name as currently implemented. So this is backwards compatible and not messing with existing KeyPairs
> - If `false`: No names for the function, role and policy will be set
> - If `typeof String`: User can set a custom name
The alternative is to use something like https://www.npmjs.com/package/multi-convention-namer with the `maxLength` option.
@udondan This is a good first step, but I think this ressource should follow the CDK standard of autogenerating all names and leaving it up to L2 ressource where possible
So please merge :)

I'm running into a function length problem as well and came here to make this exact PR. Glad someone else beat me to it. Removing the function name would be fantastic and unblock my work.
Looks like the role name and managed policy name have similar "explicitly named resources" problems.  The error checking on length that reports "Cloudformation limits names to 63 characters." could also be removed.

My use case is using a domain name as part of the key name, which can get quite long. 
@cameroncf you can override it using the `resourcePrefix` property, but it's still problematic.
@cameroncf I also removed the name from the role, which as you point out has the same problem. And standardized the imports.
bump
cc/ @udondan 
Please see the related ticket #53. CFN now has native support for creating Key pairs. Most probably the only thing I'm going to do with this project is to archive it 😸 
Personally I do not have a problem with this. Though AWS does. You cannot change the Lambda function name for **existing** custom resources. CFN will fail with:

> Modifying service token is not allowed.

By removing the function name we would force all users to delete and re-create their KeyPairs. 

I see only one option how we can change this: A new contruct property, e.g. `lambdaName`. 

- If `undefined`: We use the name as currently implemented. So this is backwards compatible and not messing with existing KeyPairs
- If `false`: No names for the function, role and policy will be set
- If `typeof String`: User can set a custom name

Fix patch:
diff --git a/Makefile b/Makefile
index 52f4b614..854cd903 100644
--- a/Makefile
+++ b/Makefile
@@ -19,11 +19,3 @@ install:
 	@echo -e "$(TARGET_COLOR)Running install$(NO_COLOR)"
 	@npm clean-install --prefer-offline --cache .npm
 	@npm list
-
-test: build
-	@echo -e "$(TARGET_COLOR)Running test$(NO_COLOR)"
-	@npm run test
-
-test-update:
-	@echo -e "$(TARGET_COLOR)Running test-update$(NO_COLOR)"
-	@npm run test -- -u
diff --git a/lib/index.ts b/lib/index.ts
index fd95d536..b9cd9688 100644
--- a/lib/index.ts
+++ b/lib/index.ts
@@ -13,7 +13,7 @@ import {
   TagType,
 } from 'aws-cdk-lib';
 import { Construct } from 'constructs';
-import path = require('path');
+import * as path from 'path';
 
 const resourceType = 'Custom::EC2-Key-Pair';
 const ID = `CFN::Resource::${resourceType}`;
@@ -130,6 +130,17 @@ export interface KeyPairProps extends ResourceProps {
    * @default Name of the stack
    */
   readonly resourcePrefix?: string;
+
+  /**
+   * Whether to use the legacy name for the Lambda function, which backs the custom resource.
+   *
+   * Starting with v4 of this package, the Lambda function by default has no longer a fixed name.
+   *
+   * If you migrate from v3 to v4, you need to set this to `true` as CloudFormation does not allow to change the name of the Lambda function used by custom resource.
+   *
+   * @default false
+   */
+  readonly legacyLambdaName?: boolean;
 }
 
 /**
@@ -204,14 +215,18 @@ export class KeyPair extends Construct implements ITaggable {
     }
 
     const stack = Stack.of(this).stackName;
-    this.prefix = props.resourcePrefix ?? stack;
-    if (this.prefix.length + cleanID.length > 62)
-      // Cloudformation limits names to 63 characters.
-      Annotations.of(this).addError(
-        `Cloudformation limits names to 63 characters.
-         Prefix ${this.prefix} is too long to be used as a prefix for your roleName. Define parameter resourcePrefix?:`
-      );
-    this.lambda = this.ensureLambda();
+
+    if (props.legacyLambdaName) {
+      this.prefix = props.resourcePrefix ?? stack;
+      if (this.prefix.length + cleanID.length > 62) {
+        // Cloudformation limits names to 63 characters.
+        Annotations.of(this).addError(
+          `Cloudformation limits names to 63 characters.
+           Prefix ${this.prefix} is too long to be used as a prefix for your roleName. Define parameter resourcePrefix?:`
+        );
+      }
+    }
+    this.lambda = this.ensureLambda(props.legacyLambdaName || false);
 
     this.tags = new TagManager(TagType.MAP, 'Custom::EC2-Key-Pair');
     this.tags.setTag(createdByTag, ID);
@@ -265,9 +280,11 @@ export class KeyPair extends Construct implements ITaggable {
     this.keyPairID = key.getAttString('KeyPairID');
   }
 
-  private ensureLambda(): aws_lambda.Function {
+  private ensureLambda(legacyLambdaName: boolean): aws_lambda.Function {
     const stack = Stack.of(this);
-    const constructName = 'EC2-Key-Name-Manager-Lambda';
+    const constructName = legacyLambdaName
+      ? 'EC2-Key-Name-Manager-Lambda' // this name was not intentional but we keep it for legacy resources
+      : 'EC2-Key-Pair-Manager-Lambda';
     const existing = stack.node.tryFindChild(constructName);
     if (existing) {
       return existing as aws_lambda.Function;
@@ -275,98 +292,72 @@ export class KeyPair extends Construct implements ITaggable {
 
     const resources = [`arn:${stack.partition}:ec2:*:*:key-pair/*`];
 
-    const policy = new aws_iam.ManagedPolicy(
-      stack,
-      'EC2-Key-Pair-Manager-Policy',
-      {
-        description: `Used by Lambda ${cleanID}, which is a custom CFN resource, managing EC2 Key Pairs`,
-        statements: [
-          new aws_iam.PolicyStatement({
-            actions: ['ec2:DescribeKeyPairs'],
-            resources: ['*'],
-          }),
-          new aws_iam.PolicyStatement({
-            actions: [
-              'ec2:CreateKeyPair',
-              'ec2:CreateTags',
-              'ec2:ImportKeyPair',
-            ],
-            conditions: {
-              StringLike: {
-                'aws:RequestTag/CreatedByCfnCustomResource': ID,
-              },
-            },
-            resources,
-          }),
-          new aws_iam.PolicyStatement({
-            // allow delete/update, only if createdByTag is set
-            actions: ['ec2:CreateTags', 'ec2:DeleteKeyPair', 'ec2:DeleteTags'],
-            conditions: {
-              StringLike: {
-                'ec2:ResourceTag/CreatedByCfnCustomResource': ID,
-              },
-            },
-            resources,
-          }),
-
-          new aws_iam.PolicyStatement({
-            // we need this to check if a secret exists before attempting to delete it
-            actions: ['secretsmanager:ListSecrets'],
-            resources: ['*'],
-          }),
-          new aws_iam.PolicyStatement({
-            actions: [
-              'secretsmanager:CreateSecret',
-              'secretsmanager:TagResource',
-            ],
-            conditions: {
-              StringLike: {
-                'aws:RequestTag/CreatedByCfnCustomResource': ID,
-              },
-            },
-            resources: ['*'],
-          }),
-          new aws_iam.PolicyStatement({
-            // allow delete/update, only if createdByTag is set
-            actions: [
-              'secretsmanager:DeleteResourcePolicy',
-              'secretsmanager:DeleteSecret',
-              'secretsmanager:DescribeSecret',
-              'secretsmanager:GetResourcePolicy',
-              'secretsmanager:GetSecretValue',
-              'secretsmanager:ListSecretVersionIds',
-              'secretsmanager:PutResourcePolicy',
-              'secretsmanager:PutSecretValue',
-              'secretsmanager:RestoreSecret',
-              'secretsmanager:UntagResource',
-              'secretsmanager:UpdateSecret',
-              'secretsmanager:UpdateSecretVersionStage',
-            ],
-            conditions: {
-              StringLike: {
-                'secretsmanager:ResourceTag/CreatedByCfnCustomResource': ID,
-              },
-            },
-            resources: ['*'],
-          }),
+    const statements = [
+      new aws_iam.PolicyStatement({
+        actions: ['ec2:DescribeKeyPairs'],
+        resources: ['*'],
+      }),
+      new aws_iam.PolicyStatement({
+        actions: ['ec2:CreateKeyPair', 'ec2:CreateTags', 'ec2:ImportKeyPair'],
+        conditions: {
+          StringLike: {
+            'aws:RequestTag/CreatedByCfnCustomResource': ID,
+          },
+        },
+        resources,
+      }),
+      new aws_iam.PolicyStatement({
+        // allow delete/update, only if createdByTag is set
+        actions: ['ec2:CreateTags', 'ec2:DeleteKeyPair', 'ec2:DeleteTags'],
+        conditions: {
+          StringLike: {
+            'ec2:ResourceTag/CreatedByCfnCustomResource': ID,
+          },
+        },
+        resources,
+      }),
+
+      new aws_iam.PolicyStatement({
+        // we need this to check if a secret exists before attempting to delete it
+        actions: ['secretsmanager:ListSecrets'],
+        resources: ['*'],
+      }),
+      new aws_iam.PolicyStatement({
+        actions: ['secretsmanager:CreateSecret', 'secretsmanager:TagResource'],
+        conditions: {
+          StringLike: {
+            'aws:RequestTag/CreatedByCfnCustomResource': ID,
+          },
+        },
+        resources: ['*'],
+      }),
+      new aws_iam.PolicyStatement({
+        // allow delete/update, only if createdByTag is set
+        actions: [
+          'secretsmanager:DeleteResourcePolicy',
+          'secretsmanager:DeleteSecret',
+          'secretsmanager:DescribeSecret',
+          'secretsmanager:GetResourcePolicy',
+          'secretsmanager:GetSecretValue',
+          'secretsmanager:ListSecretVersionIds',
+          'secretsmanager:PutResourcePolicy',
+          'secretsmanager:PutSecretValue',
+          'secretsmanager:RestoreSecret',
+          'secretsmanager:UntagResource',
+          'secretsmanager:UpdateSecret',
+          'secretsmanager:UpdateSecretVersionStage',
         ],
-      }
-    );
-
-    const role = new aws_iam.Role(stack, 'EC2-Key-Pair-Manager-Role', {
-      description: `Used by Lambda ${cleanID}, which is a custom CFN resource, managing EC2 Key Pairs`,
-      assumedBy: new aws_iam.ServicePrincipal('lambda.amazonaws.com'),
-      managedPolicies: [
-        policy,
-        aws_iam.ManagedPolicy.fromAwsManagedPolicyName(
-          'service-role/AWSLambdaBasicExecutionRole'
-        ),
-      ],
-    });
+        conditions: {
+          StringLike: {
+            'secretsmanager:ResourceTag/CreatedByCfnCustomResource': ID,
+          },
+        },
+        resources: ['*'],
+      }),
+    ];
 
     const fn = new aws_lambda.Function(stack, constructName, {
-      functionName: `${this.prefix}-${cleanID}`,
-      role: role,
+      functionName: legacyLambdaName ? `${this.prefix}-${cleanID}` : undefined,
       description: 'Custom CFN resource: Manage EC2 Key Pairs',
       runtime: aws_lambda.Runtime.NODEJS_18_X,
       handler: 'index.handler',
@@ -375,6 +366,9 @@ export class KeyPair extends Construct implements ITaggable {
       ),
       timeout: Duration.minutes(lambdaTimeout),
     });
+    statements.forEach((statement) => {
+      fn.role?.addToPrincipalPolicy(statement);
+    });
 
     return fn;
   }


Test patch:
diff --git a/test/bin/test.ts b/test/bin/test.ts
index 006b0d2b..4bab8ede 100644
--- a/test/bin/test.ts
+++ b/test/bin/test.ts
@@ -1,11 +1,32 @@
 #!/usr/bin/env node
-import { GetCallerIdentityCommand, STSClient } from '@aws-sdk/client-sts';
+import {
+  GetCallerIdentityCommand,
+  STSClient,
+  STSClientConfig,
+} from '@aws-sdk/client-sts';
 import cdk = require('aws-cdk-lib');
 
 import { TestStack } from '../lib/test-stack';
 
+const region = 'us-east-1';
+
+const clientConfig: STSClientConfig = {
+  region,
+};
+if (
+  process.env.AWS_ACCESS_KEY_ID &&
+  process.env.AWS_SECRET_ACCESS_KEY &&
+  process.env.AWS_SESSION_TOKEN
+) {
+  clientConfig.credentials = {
+    accessKeyId: process.env.AWS_ACCESS_KEY_ID,
+    secretAccessKey: process.env.AWS_SECRET_ACCESS_KEY,
+    sessionToken: process.env.AWS_SESSION_TOKEN,
+  };
+}
+
 async function getIdentity() {
-  const stsClient = new STSClient({});
+  const stsClient = new STSClient(clientConfig);
   const callerIdentity = await stsClient.send(new GetCallerIdentityCommand({}));
   return callerIdentity.Arn?.split('/')[1] as string;
 }
diff --git a/test/lib/test-stack.ts b/test/lib/test-stack.ts
index 397467c4..672a611c 100644
--- a/test/lib/test-stack.ts
+++ b/test/lib/test-stack.ts
@@ -53,6 +53,7 @@ export class TestStack extends cdk.Stack {
       exposePublicKey: true,
       storePublicKey: true,
       publicKeyFormat: PublicKeyFormat.PEM,
+      legacyLambdaName: true,
     });
 
     const currentUser = cdk.aws_iam.User.fromUserName(