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: use existing policies #195

Closed
wants to merge 4 commits into from

Conversation

onhate
Copy link
Contributor

@onhate onhate commented Jan 4, 2024

closes #194

@onhate
Copy link
Contributor Author

onhate commented Jan 4, 2024

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
Copy link
Contributor

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
Copy link
Contributor Author

onhate commented Jan 4, 2024

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
Copy link
Contributor

@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
Copy link
Contributor Author

onhate commented Jan 5, 2024

@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
Copy link
Contributor

bestickley commented Jan 5, 2024

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
Copy link
Contributor Author

onhate commented Jan 5, 2024

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

@onhate
Copy link
Contributor Author

onhate commented Jan 5, 2024

I'll start a fresh PR from scratch.

@onhate onhate closed this Jan 5, 2024
@onhate onhate deleted the feat-use-existing-policies branch January 5, 2024 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Limit exceeded for resource of type 'AWS::CloudFront::ResponseHeadersPolicy' when deploying multiple sites
2 participants