Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement code generated handler framework #28251

Merged
merged 77 commits into from
Dec 16, 2023

Conversation

colifran
Copy link
Contributor

@colifran colifran commented Dec 4, 2023

This PR introduces an internal handler framework used to code generate constructs that extend a lambda Function, lambda SingletonFunction, or core CustomResourceProvider construct and prohibit the user from directly configuring the handler, runtime, code, and codeDirectory properties. In doing this, we are able to establish best practices, runtime enforcement, and consistency across all handlers we build and vend within the aws-cdk.

As expected, no integ tests were changed as a result of this PR. To verify that the code generated custom resource providers are working correctly I force ran three integ tests all targeted at an individual custom resource provider:

  1. integ.global.ts to test replica provider and the code generated construct extending Function
  2. integ.bucket-auto-delete-objects.ts to test auto delete objects provider and the code generated construct extending CustomResourceProvider
  3. integ.aws-api.ts to test aws api provider and the code generated construct SingletonFunction

All of these integ tests passed successfully.

Closes #27303


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

Signed-off-by: Francis <colifran@amazon.com>
@aws-cdk-automation aws-cdk-automation requested a review from a team December 4, 2023 22:42
@github-actions github-actions bot added the p2 label Dec 4, 2023
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Dec 4, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

colifran and others added 25 commits December 4, 2023 15:48
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
…minify and bundle script for framework generation

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
… for singleton props interface

Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Signed-off-by: Francis <colifran@amazon.com>
Copy link
Contributor

@MrArnoldPalmer MrArnoldPalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Thanks @colifran

As discussed with @mrgrain and @rix0rrr offline, there are a couple things we want to continue to pursue here.

  1. On a case by case basis, move everything possible to generate CustomResourceProvider and deprecate supporting for Function and SingletonFunction. I believe we should still support Function as a general case in the future, but the only handler that currently is NOT a custom resource is the aws-events AwsApi handler.
  2. Once everything is using a common base, reevaluate the abstraction of CustomResourceProvider for our use case more generally. Since we are code generating, there is little cost to us outputting constructs that extend CfnResource which would remove all of the concerns around circular dependencies that exist with some of these handlers.
  3. Separately pursue moving every handler to use NODEJS_LATEST, and make the runtime region availability aware. This would mean making sure all of our handlers are runtime version portable, bundling all of their assets. This PR gives us the ability to change the default runtime in one place and have every handler affected, as well as know where handlers are not using the default requiring separate upgrading and testing. However more consolidation here would mean even faster and more reliable runtime upgrades.

@MrArnoldPalmer MrArnoldPalmer dismissed rix0rrr’s stale review December 15, 2023 23:38

Discussed offline, gotta merge

Copy link
Contributor

mergify bot commented Dec 15, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2dffd0a
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 1a9c30e into main Dec 16, 2023
12 checks passed
@mergify mergify bot deleted the colifran/codegen-handler-framework branch December 16, 2023 00:05
Copy link
Contributor

mergify bot commented Dec 16, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@portswigger-tim
Copy link

portswigger-tim commented Dec 22, 2023

Hmmm.... looks like this change breaks utilities like CfnJson?

jsii.errors.JavaScriptError: 
  Error: Cannot find module '../dist/core/cfn-utils-provider.generated'
  Require stack:
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/cfn-json.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/index.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/index.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/function-base.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/lambda-augmentations.generated.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/index.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/index.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/@aws-cdk/lambda-layer-kubectl-v28/lib/kubectl-layer.js
  - /tmp/jsii-kernel-JhJ26s/node_modules/@aws-cdk/lambda-layer-kubectl-v28/lib/index.js
  - /tmp/jsii-kernel-JhJ26s
      at Module._resolveFilename (node:internal/modules/cjs/loader:1134:15)
      at Module._load (node:internal/modules/cjs/loader:975:27)
      at Module.require (node:internal/modules/cjs/loader:12[25](https://github.com/portswigger-cloud/scanner/actions/runs/7298299094/job/19889011039#step:10:26):19)
      at require (node:internal/modules/helpers:177:18)
      at cfn_utils_provider_generated_1 (/tmp/jsii-kernel-JhJ[26](https://github.com/portswigger-cloud/scanner/actions/runs/7298299094/job/19889011039#step:10:27)s/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js:1:436)
      at CfnUtilsProvider.getOrCreate (/tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js:1:629)
      at new CfnJson (/tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/cfn-json.js:1:1371)
      at Kernel._Kernel_create (/tmp/tmp8oq0hyj0/lib/program.js:10108:25)
      at Kernel.create (/tmp/tmp8oq0hyj0/lib/program.js:9779:93)
      at KernelHost.processRequest (/tmp/tmp8oq0hyj0/lib/program.js:11699:36)

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/runner/work/scanner/scanner/infra/app.py", line 69, in <module>
    eks_cluster_stack = EksClusterStack(
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/ps_cdk/stacks/eks_cluster_stack.py", line 67, in __init__
    self.karpenter_role = self.create_karpenter_role(cluster_name)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/ps_cdk/stacks/eks_cluster_stack.py", line 565, in create_karpenter_role
    role = self.create_irsa_role(
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/ps_cdk/stacks/eks_cluster_stack.py", line 413, in create_irsa_role
    return ps_iam.IrsaRole(
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/ps_cdk/constructs/iam/irsa_role.py", line 21, in __init__
    "StringEquals": CfnJson(
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_runtime.py", line 118, in __call__
    inst = super(JSIIMeta, cast(JSIIMeta, cls)).__call__(*args, **kwargs)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/aws_cdk/__init__.py", line [27](https://github.com/portswigger-cloud/scanner/actions/runs/7298299094/job/19889011039#step:10:28)574, in __init__
    jsii.create(self.__class__, self, [scope, id, props])
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_kernel/__init__.py", line 334, in create
    response = self.provider.create(
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_kernel/providers/process.py", line 365, in create
    return self._process.send(request, CreateResponse)
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/jsii/_kernel/providers/process.py", line 342, in send
    raise RuntimeError(resp.error) from JavaScriptError(resp.stack)
RuntimeError: Cannot find module '../dist/core/cfn-utils-provider.generated'
Require stack:
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/cfn-json.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/lib/index.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/core/index.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/function-base.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/lambda-augmentations.generated.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/lib/index.js
- /tmp/jsii-kernel-JhJ26s/node_modules/aws-cdk-lib/aws-lambda/index.js
- /tmp/jsii-kernel-JhJ26s/node_modules/@aws-cdk/lambda-layer-kubectl-v[28](https://github.com/portswigger-cloud/scanner/actions/runs/7298299094/job/19889011039#step:10:29)/lib/kubectl-layer.js
- /tmp/jsii-kernel-JhJ26s/node_modules/@aws-cdk/lambda-layer-kubectl-v28/lib/index.js
- /tmp/jsii-kernel-JhJ26s
Subprocess exited with error 1

Pinning to version 2.115.0 gets everything working again.

@mrgrain
Copy link
Contributor

mrgrain commented Dec 22, 2023

@portswigger-tim Thanks for the report. Any chance you could post a code snippet how you use CfnJson for me to reprdoduce?

@portswigger-tim
Copy link

Sure @mrgrain :

import aws_cdk.aws_iam as iam
from aws_cdk import CfnJson
from constructs import Construct


class IrsaRole(iam.Role):
    def __init__(
        self,
        scope: Construct,
        id: str,
        eks_cluster_oidc_provider_arn: str,
        eks_cluster_oidc_provider_issuer: str,
        namespace: str,
        service_account_name: str,
        **kwargs,
    ) -> None:
        assumed_by = iam.FederatedPrincipal(
            eks_cluster_oidc_provider_arn,
            assume_role_action="sts:AssumeRoleWithWebIdentity",
            conditions={
                "StringEquals": CfnJson(
                    scope,
                    f"{id}Conditions",
                    value={
                        f"{eks_cluster_oidc_provider_issuer}:aud": "sts.amazonaws.com",
                        f"{eks_cluster_oidc_provider_issuer}:sub": f"system:serviceaccount:{namespace}:{service_account_name}",
                    },
                )
            },
        )

        super().__init__(
            scope,
            id,
            assumed_by=assumed_by,
            description=f"IAM role for ServiceAccount: {service_account_name}",
            **kwargs,
        )

I think that this was necessary due to the tokenization of outputs in CDK when used like:

   # pre-canned IRSA roles for various common applications
   def create_irsa_role(
       self, id: str, namespace: str, service_account_name: str, **kwargs
   ):
       return ps_iam.IrsaRole(
           self,
           id,
           eks_cluster_oidc_provider_arn=self.cluster.open_id_connect_provider.open_id_connect_provider_arn,
           eks_cluster_oidc_provider_issuer=self.cluster.open_id_connect_provider.open_id_connect_provider_issuer,
           namespace=namespace,
           service_account_name=service_account_name,
           **kwargs,
       )

   def create_teleport_kube_agent_role(self) -> iam.Role:
       return self.create_irsa_role(
           "TeleportKubeAgentRole",
           namespace="teleport",
           service_account_name="teleport-kube-agent",
           inline_policies={
               "TeleportAgentPolicy": iam.PolicyDocument(
                   statements=[
                       iam.PolicyStatement(
                           actions=[
                               "sts:AssumeRole",
                           ],
                           resources=[
                               "arn:aws:iam::*:role/pipeline-roles/teleport-*-role"
                           ],
                       ),
                   ]
               )
           },
       )

   def create_aws_load_balancer_controller_role(self) -> iam.Role:
       with open(
           f"{os.path.dirname(__file__)}/files/aws-load-balancer-controller-policy.json"
       ) as policy_data:
           return self.create_irsa_role(
               "AwsLoadBalancerControllerRole",
               namespace="kube-system",
               service_account_name="aws-load-balancer-controller",
               inline_policies={
                   "AwsLoadBalancerControllerPolicy": iam.PolicyDocument.from_json(
                       json.load(policy_data)
                   )
               },
           )

@mrgrain
Copy link
Contributor

mrgrain commented Dec 22, 2023

Thanks @portswigger-tim I think I've found the issue. Fix underway.

@Feder1co5oave
Copy link

@mrgrain I'm experiencing the same issue with aws-cdk-lib@2.116.0

$ node_modules/.bin/cdk diff

Error: Cannot find module '../dist/core/cfn-utils-provider.generated'
Require stack:
- /home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js
- /home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/cfn-json.js
- /home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/index.js
- /home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/index.js
- /home/federico/MyCdkProject/node_modules/aws-cdk-lib/index.js
- /home/federico/MyCdkProject/bin/cdk.ts
    at Function.Module._resolveFilename (node:internal/modules/cjs/loader:1077:15)
    at Function.Module._resolveFilename.sharedData.moduleResolveFilenameHook.installedValue [as _resolveFilename] (/home/federico/MyCdkProject/node_modules/@cspotcode/source-map-support/source-map-support.js:811:30)
    at Function.Module._load (node:internal/modules/cjs/loader:922:27)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:119:18)
    at cfn_utils_provider_generated_1 (/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js:1:436)
    at Function.getOrCreate (/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js:1:629)
    at new CfnJson (/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/cfn-json.js:1:1371)
    at new TagmeStack (/home/federico/MyCdkProject/lib/tagme-stack.ts:22:33)
    at Object.<anonymous> (/home/federico/MyCdkProject/bin/cdk.ts:15:15) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/private/cfn-utils-provider.js',
    '/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/cfn-json.js',
    '/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/lib/index.js',
    '/home/federico/MyCdkProject/node_modules/aws-cdk-lib/core/index.js',
    '/home/federico/MyCdkProject/node_modules/aws-cdk-lib/index.js',
    '/home/federico/MyCdkProject/bin/cdk.ts'
  ]
}

I too am using CfnJson to create a IAM trust policy for an OIDC pricipal:

const trustConditions = new CfnJson(this, 'iam-role-condition', {
    value: {
        [`${oidcProviderArn}:aud`]: oidcAudience,
        [`${oidcProviderArn}:sub`]: `system:serviceaccount:${serviceAccountNamespace}:${serviceAccountName}`,
    }
});
const oidcPrincipal = new iam.OpenIdConnectPrincipal(oidcProvider).withConditions({
    StringEquals: trustConditions
});
const k8sRole = new iam.Role(this, 'iam-role', { assumedBy: oidcPrincipal });

@mrgrain
Copy link
Contributor

mrgrain commented Dec 22, 2023

Thanks @Feder1co5oave Fix is on the way: #28467

mergify bot pushed a commit that referenced this pull request Dec 22, 2023
…ist/core/<file>.generated'` (#28467)

#28251 added new files to `aws-cdk-lib/core/lib/dist/core` but this path was excluded from the npm package, causing the above error.

This fix includes the generated file into the package.

Closes #28465

Manually tested with a locally build package that includes the fix.
<img width="1449" alt="image" src="https://github.com/aws/aws-cdk/assets/379814/11714c41-edea-403e-9b64-454ba9768c08">

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mrgrain added a commit that referenced this pull request Dec 22, 2023
…ist/core/<file>.generated'` (#28467)

#28251 added new files to `aws-cdk-lib/core/lib/dist/core` but this path was excluded from the npm package, causing the above error.

This fix includes the generated file into the package.

Closes #28465

Manually tested with a locally build package that includes the fix.
<img width="1449" alt="image" src="https://github.com/aws/aws-cdk/assets/379814/11714c41-edea-403e-9b64-454ba9768c08">

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
…ist/core/<file>.generated'` (aws#28467)

aws#28251 added new files to `aws-cdk-lib/core/lib/dist/core` but this path was excluded from the npm package, causing the above error.

This fix includes the generated file into the package.

Closes aws#28465

Manually tested with a locally build package that includes the fix.
<img width="1449" alt="image" src="https://github.com/aws/aws-cdk/assets/379814/11714c41-edea-403e-9b64-454ba9768c08">

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-cdk-lib): Internal Lambda Handler Framework
7 participants