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

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

Closed
1 task
ivanbarlog opened this issue Nov 5, 2024 · 6 comments · Fixed by #32020
Closed
1 task
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@ivanbarlog
Copy link
Contributor

Describe the bug

The property from this interface is not being used anywhere.

Hence setting the property in here does nothing.

Regression Issue

  • Select this option if this issue appears to be a regression.

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

@ivanbarlog ivanbarlog added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 5, 2024
@github-actions github-actions bot added the @aws-cdk/aws-cloudfront Related to Amazon CloudFront label Nov 5, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 5, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 5, 2024
@khushail khushail added needs-reproduction This issue needs reproduction. p2 and removed bug This issue is a bug. labels Nov 5, 2024
@khushail khushail self-assigned this Nov 5, 2024
@ashishdhingra ashishdhingra removed the needs-triage This issue or PR still needs to be triaged. label Nov 5, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 6, 2024
@khushail khushail added the bug This issue is a bug. label Nov 7, 2024
@khushail
Copy link
Contributor

khushail commented Nov 7, 2024

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!

@khushail khushail added effort/small Small work item – less than a day of effort and removed needs-reproduction This issue needs reproduction. labels Nov 7, 2024
@khushail khushail removed their assignment Nov 7, 2024
@ivanbarlog
Copy link
Contributor Author

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.

ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 7, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 7, 2024
@khushail
Copy link
Contributor

khushail commented Nov 7, 2024

@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.

ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 7, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 8, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 8, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 8, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 9, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 9, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 9, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 13, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 13, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 13, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 17, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 17, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 17, 2024
@gracelu0
Copy link
Contributor

Hi @ivanbarlog , thanks for raising this issue. We just merged #31339 which introduces L2 support for setting up a lambda function URL origin with origin access control, which means setting originAccessControlId is automatically handled. I see how you would need this if you are using an L1 CfnOriginAccessControl in your setup though.

ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 21, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 22, 2024
ivanbarlog added a commit to ivanbarlog/aws-cdk that referenced this issue Nov 24, 2024
@mergify mergify bot closed this as completed in #32020 Nov 28, 2024
@mergify mergify bot closed this as completed in f9708a6 Nov 28, 2024
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

1 similar comment
Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-cloudfront Related to Amazon CloudFront bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
4 participants