aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.66k stars 3.92k forks source link

aws-cloudfront: setting `originAccessControlId` in `aws-cloudfront-origins` does nothing #32018

Open ivanbarlog opened 2 days ago

ivanbarlog commented 2 days ago

Describe the bug

The property from this interface is not being used anywhere.

Hence setting the property in here does nothing.

Regression Issue

Last Known Working CDK Version

No response

Expected Behavior

When setting originAccessControlId attribute on FunctionUrlOrigin the CloudFront template should contain OriginAccessControlId.

Current Behavior

When setting originAccessControlId attribute on FunctionUrlOrigin the CloudFront template does not contain OriginAccessControlId.

Reproduction Steps

  1. Deploy following template via CDK
import { App, Duration, Stack } from "aws-cdk-lib";
import {
  CfnOriginAccessControl,
  Distribution,
  PriceClass,
} from "aws-cdk-lib/aws-cloudfront";
import { FunctionUrlOrigin } from "aws-cdk-lib/aws-cloudfront-origins";
import { ServicePrincipal } from "aws-cdk-lib/aws-iam";
import { Code, FunctionUrlAuthType } from "aws-cdk-lib/aws-lambda";
import { NodejsFunction } from "aws-cdk-lib/aws-lambda-nodejs";

const app = new App();

const stack = new Stack(app, "BugReport");

const handler = new NodejsFunction(stack, "Handler", {
  code: Code.fromInline(`export const handler = async (event, context) => {
console.log("EVENT: \n" + JSON.stringify(event, null, 2));
return context.logStreamName;
};`),
});

const handlerUrl = handler.addFunctionUrl({
  authType: FunctionUrlAuthType.AWS_IAM,
});

const oac = new CfnOriginAccessControl(stack, "HandlerOriginAccessControl", {
  originAccessControlConfig: {
    name: "sample",
    originAccessControlOriginType: "lambda",
    signingBehavior: "always",
    signingProtocol: "sigv4",
  },
});

const distribution = new Distribution(stack, "Distribution", {
  defaultBehavior: {
    origin: new FunctionUrlOrigin(handlerUrl, {
      keepaliveTimeout: Duration.seconds(60),
      originAccessControlId: oac.attrId, // this line does not propagate to CloudFormation template
    }),
  },
  priceClass: PriceClass.PRICE_CLASS_100,
});

handler.addPermission("AllowCloudFrontInvoke", {
  principal: ServicePrincipal.fromStaticServicePrincipleName(
    "cloudfront.amazonaws.com"
  ),
  action: "lambda:InvokeFunctionUrl",
  sourceArn: `arn:aws:cloudfront::${Stack.of(stack).account}:distribution/${
    distribution.distributionId
  }`,
  functionUrlAuthType: FunctionUrlAuthType.AWS_IAM,
});
  1. the CloudFront instance won't contain any link to the OriginAccessControl.

Possible Solution

Add following line below this https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts#L151

  private readonly originAccessControlId?: string;

Add following line below this https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts#L165

    this.originAccessControlId = props.originAccessControlId;

Add following line below this https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-cloudfront/lib/origin.ts#L189

        originAccessControlId: this.originAccessControlId,

I have already tested this by adding the lines to the code in my node_modules and it works as expected.

Additional Information/Context

No response

CDK CLI Version

2.165.0 (build 00f70f1)

Framework Version

No response

Node.js Version

v20.18.0

OS

macOS

Language

TypeScript

Language Version

5.6.3

Other information

No response

khushail commented 1 day ago

Hi @ivanbarlog , thanks for reporting this.

The mentioned issue is reproducible with this sample code -

    const distribution = new Distribution(this, "Distribution", {
      defaultBehavior: {
        origin: new FunctionUrlOrigin(fn.addFunctionUrl(), {
          keepaliveTimeout: Duration.seconds(60),
          originAccessControlId: oac.attrId, // this line does not propagate to CloudFormation template
        }),
      },
      priceClass: PriceClass.PRICE_CLASS_100,
    });

Synthesized template snippet -

"Distribution830FAC52": {
   "Type": "AWS::CloudFront::Distribution",
   "Properties": {
    "DistributionConfig": {
     "DefaultCacheBehavior": {
      "CachePolicyId": "658327ea-f89d-4fab-a63d-7e88639e58f6",
      "Compress": true,
      "TargetOriginId": "CloudfrontIssuev2StackDistributionOrigin1427B910B",
      "ViewerProtocolPolicy": "allow-all"
     },
     "Enabled": true,
     "HttpVersion": "http2",
     "IPV6Enabled": true,
     "Origins": [
      {
       "CustomOriginConfig": {
        "OriginKeepaliveTimeout": 60,
        "OriginProtocolPolicy": "https-only",
        "OriginSSLProtocols": [
         "TLSv1.2"
        ]
       },
       "DomainName": {
        "Fn::Select": [
         2,
         {
          "Fn::Split": [
           "/",
           {
            "Fn::GetAtt": [
             "MyFunctionFunctionUrlFF6DE78C",
             "FunctionUrl"
            ]
           }
          ]
         }
        ]
       },
       "Id": "CloudfrontIssuev2StackDistributionOrigin1427B910B"
      }
     ],
     "PriceClass": "PriceClass_100"
    }
   },

As it can be seen, the mentioned property originAccessControlId is missing from the template.

@ivanbarlog , thanks for your contribution. For further help, you could do these-

  1. go through these available youtube tutorial which will guide to how to write integ-test.
  2. go through our ReadMe on integration tests.
  3. Reach out to Cdk.dev community on slack for help/guidance.

Hope that would be helpful!

ivanbarlog commented 9 hours ago

Hi @khushail!

Is this reply to my pull request?

I just cannot see the point of integration test for this one. Eg. this should be already covered somewhere, or not? I am not sure how this works, but I am keen to check the tutorials.

For me this is just a simple 3 line code fix and I would appreciate the help of someone who already have experience with this.

As I believe this will result in adding another 3 lines of code in some integration test, another sweating of my computer running that test suite but me studying the whole process for hours which I don't really have.

Only the video you've posted here is more than hour long.

Nevertheless I will give it a shot... I guess.

khushail commented 8 hours ago

@ivanbarlog , I totally understand your point but adding the property requires one to submit integration test and snapshots. You could refer to this PR , which adds a pproperty to EC2 and snapshots and integ test are also included. Might be helpful.