jetbridge / cdk-nextjs

Deploy a NextJS application using AWS CDK
https://constructs.dev/packages/cdk-nextjs-standalone
Apache License 2.0
260 stars 43 forks source link

feat: use existing policies #195

Closed onhate closed 6 months ago

onhate commented 6 months ago

closes #194

onhate commented 6 months ago

passing

nextjsDistribution: {
          imageCachePolicy: getCachePolicy(
            scope, //
            `${prefix}-ImageCachePolicy`,
            props.policies.imageCachePolicyId,
          ),
          imageResponseHeadersPolicy: getResponseHeadersPolicy(
            scope, //
            `${prefix}-ImageResponseHeadersPolicy`,
            props.policies.imageResponseHeadersPolicyId,
          ),
          serverCachePolicy: getCachePolicy(
            scope, //
            `${prefix}-ServerCachePolicy`,
            props.policies.serverCachePolicyId,
          ),
          serverResponseHeadersPolicy: getResponseHeadersPolicy(
            scope, //
            `${prefix}-ServerResponseHeadersPolicy`,
            props.policies.serverResponseHeadersPolicyId,
          ),
          staticResponseHeadersPolicy: getResponseHeadersPolicy(
            scope, //
            `${prefix}-StaticResponseHeadersPolicy`,
            props.policies.staticResponseHeadersPolicyId,
          ),
        },

it deploys using the given policies

2024-01-04T13-04-07

bestickley commented 6 months ago

Thanks for this feature, @onhate! I'm wondering if passing in these cache and response headers policies should be done on NextjsOverrides or NextjsProps. My reasoning for thinking we should put on NextjsProps is that attributes of NexjsOverrides are all props based, no constructs (class instances) which is what you're currently proposing. However, I could argue that this customization is deeper level customization and shouldn't be a top level prop on NextjsProps so we should use NextjsOverrides instead. Do you have any opinions on that?

onhate commented 6 months ago

well, now that you said, considering that the distribution is passed on the NextjsDistributionProps I think this should also be on the Props instead of the Overrides.

I'll adjust.

bestickley commented 6 months ago

@onhate, it seems like the code changes in NextjsDistribution that make it so cache/response header policies aren't always created have been removed. Was that on purpose?

Thinking through this more, I think a simpler way to achieve the goal you have is to use the already existing NextjsDistributionOverrides.{image,server,static}BehaviorOptions.{cachePolicy,responseHeadersPolicy} arguments and change code in create{Image,Server,Static}BehaviorOptions to conditionally create the policies based on whether or not those values are defined. This way, those policies aren't created every time and you wouldn't hit the CloudFront quota of 20. Agree?

onhate commented 6 months ago

@onhate, it seems like the code changes in NextjsDistribution that make it so cache/response header policies aren't always created have been removed. Was that on purpose?

no, it's there. am I missing something?

to conditionally create the policies based on whether or not those values are defined.

but aren't those policies required for it to work? and what if I want custom policies but to reusing existing?

bestickley commented 6 months ago

Whoops, sorry, I see those changes now.

but aren't those policies required for it to work? and what if I want custom policies but to reusing existing?

What I am suggesting I think would result in the same behavior this PR creates. If you were to pass in an override, then Nextjs wouldn't create the corresponding cache or response headers policy. You could create a custom policy and pass it in to be used instead of it being created inside construct.

You'd use it something like:

const cachePolicy = new CachePolicy(...);
new Nextjs(this, "MyNextjs", {
  overrides: {
    nextjsDistribution: {
      serverBehaviorOptions: {
        cachePolicy,
      }
    }
  }
});

and then in construct:

private createServerBehaviorOptions(): cloudfront.BehaviorOptions {
    const fnUrl = this.props.serverFunction.addFunctionUrl({ authType: this.fnUrlAuthType });
    const origin = new origins.HttpOrigin(Fn.parseDomainName(fnUrl.url), this.props.overrides?.serverHttpOriginProps);
    let cachePolicy: cloudfront.CachePolicy | undefined;
    if (!this.props.overrides?.serverBehaviorOptions?.cachePolicy) {
      cachePolicy = new cloudfront.CachePolicy(this, 'ServerCachePolicy', {
        queryStringBehavior: cloudfront.CacheQueryStringBehavior.all(),
        headerBehavior: cloudfront.CacheHeaderBehavior.allowList(
          'accept',
          'rsc',
          'next-router-prefetch',
          'next-router-state-tree',
          'next-url',
          'x-prerender-revalidate'
        ),
        cookieBehavior: cloudfront.CacheCookieBehavior.all(),
        defaultTtl: Duration.seconds(0),
        maxTtl: Duration.days(365),
        minTtl: Duration.seconds(0),
        enableAcceptEncodingBrotli: true,
        enableAcceptEncodingGzip: true,
        comment: 'Nextjs Server Cache Policy',
        ...this.props.overrides?.serverCachePolicyProps,
      });
    }
    let responseHeadersPolicy: ResponseHeadersPolicy | undefined;
    if (!this.props.overrides?.serverBehaviorOptions?.responseHeadersPolicy) {
      responseHeadersPolicy = new ResponseHeadersPolicy(this, 'ServerResponseHeadersPolicy', {
        customHeadersBehavior: {
          customHeaders: [
            {
              header: 'cache-control',
              override: false,
              // MDN Cache-Control Use Case: Up-to-date contents always
              // @see: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#up-to-date_contents_always
              value: `no-cache`,
            },
          ],
        },
        securityHeadersBehavior: this.commonSecurityHeadersBehavior,
        comment: 'Nextjs Server Response Headers Policy',
        ...this.props.overrides?.serverResponseHeadersPolicyProps,
      });
    }
    return {
      ...this.commonBehaviorOptions,
      origin,
      allowedMethods: cloudfront.AllowedMethods.ALLOW_ALL,
      originRequestPolicy: cloudfront.OriginRequestPolicy.ALL_VIEWER_EXCEPT_HOST_HEADER,
      cachePolicy,
      edgeLambdas: this.edgeLambdas.length ? this.edgeLambdas : undefined,
      functionAssociations: this.createCloudFrontFnAssociations(),
      responseHeadersPolicy,
      ...this.props.overrides?.serverBehaviorOptions,
    };
  }
onhate commented 6 months ago

ooow, now I see, and in this case we wouldn't even need to add properties. Ok, I like it better.

onhate commented 6 months ago

I'll start a fresh PR from scratch.