Removing "accept" header from cache/originRequest policy when AutoWebP is disabled.

[fvsnippets]

There's no need to include the accept http header in the cloudfront's cache key when AutoWebP is disabled.
These changes will improve the cloudfront's cache hit ratio when AutoWebP is not used. Also stops sending unnecesary header (in such case) to origin (resizing backend).

Issue #, if available:

#304

Description of changes:

Infrastructure generation code was modified. Added a cloudformation's boolean condition (CfnCondition) based on AutoWebPParameter value, and evaluated it (using a cloudformation's Fn::If instrinsic function) inside cache/originRequest policy code.
Had to switch to "Cfn", aka L1, versions of cache/originRequest policy. Please read this issue.

Tests were updated to reflect changes.

Checklist

• 👋 I have run the unit tests, and all unit tests have passed.
• ⚠️ This pull request might incur a breaking change.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[fvsnippets]

Notice: There's another PR that tries to address the same task. Current PR (mine) aims to replace it.

[fvsnippets]

Also related: #32 and #59
But this PR (current) addresses (successfully I think) the removal of accept header from whitelist (cache key & sending to image handler lambda) when AutoWebP is disabled.

[fvsnippets]

Hi!
Added a commit to preserve logical IDs of cachePolicy and originRequestPolicy resources (now all pre-existing logical IDs will be preserved).

--

What I did was extracting the code that creates that resources into classes imitating L2 classes CachePolicy and OriginRequestPolicy behavior (the new CustomBackEndCachePolicy and CustomBackEndOriginRequestPolicy classes) but adding the selection of proper headers.

Notice for code reviewers: github's diff is not smart enough when showing only the second commit, and is displaying as removed / added a lot of lines that hasn't been modified. I would recomend using the "all commits" view which shows the differences properly.

[fisenkodv]

@fvsnippets thank you for your contribution. We will look into it and add it to our backlog.

[github-actions]

This pr has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[fvsnippets]

This pr has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

Keeping it open.

[github-actions]

This pr has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[fvsnippets]

Updated with current main branch (using rebase). Conflicts were resolved. Also fixes eslint issues on my PR.

[simonkrol]

Hi @fvsnippets,

Overall I like this change, my only concern is that this will remove the ability to enable AutoWebP through the Lambda environment variables after the fact. Some changes to the documentation may overcome this, though I'm concerned it may still confuse users when modifying the environment variable doesn't do anything.

Simon

[fvsnippets]

Hi @simonkrol
Thanks for taking a look into this PR.

I do not think that (manual) modification of this particular variable after deploy (in the lambda) is the best choice for consumers of this solution, but yes, you are right.

I haven't worked with AWS in a while, but if I recall correctly you can also (manually) modify CloudFront to re-include the (now missing) header "accept", right?
So, as an alternative, if you are going to (manually) enable that variable after deploy, you can also (manually) add the header "accept" in CloudFront. Documenting this procedure is an alternative.

[simonkrol]

Hi @fvsnippets,
Yep, ideally manual modification of the environment variables doesn't happen too often, redeploying an updated stack with those new values would be better, but those modifications are something documented in the Implementation Guide that some users may rely on.

We can definitely improve the documentation there, my concern is mostly around having exceptions like these, where the IG indicates "you can change any of the env variables in the same way, except AutoWebP, which requires that you read this other section and perform an additional step". It may be wise that we rework that section of the document, aiming instead to have users update their stacks with the updated parameters, which would handle all of these issues for us. I'll chat with the team some more, and hopefully we can come to a decent solution.

Thanks for your contributions here :)

[simonkrol]

Hi @fvsnippets,
Just an update here, we've decided to implement this functionality and will be updating the IG to reflect the change as far as updating Environment variables goes. While our internal processes have us make the change manually, once this is released, you'll see this PR and your Github profile mentioned in the External Contributors section towards the bottom of the readme and this PR will be closed.

Thanks again for your contributions to SIH,
Simon