Add support for custom headers from origins #261
Conversation
|
Hi @mattstrom @matthewstrom , |
|
Just as a heads up might have gotten the request behaviour incorrect here. Might need to refactor if so, |
src/function-set.ts
Outdated
| private readonly log: (message: string) => void, | ||
| public readonly origin: Origin = new Origin() | ||
| ) { | ||
| this.distribution = distribution |
There was a problem hiding this comment.
This line is unnecessary because TypeScript will automatically assign any public constructor argument as an instance value.
| this.distribution = distribution |
src/behavior-router.ts
Outdated
| @@ -49,6 +50,7 @@ export class BehaviorRouter { | |||
| this.builder = buildConfig(serverless); | |||
| this.context = buildContext(); | |||
|
|
|||
| this.cfResources = serverless.service?.resources?.Resources || {} | |||
There was a problem hiding this comment.
Sorry to be nitpicky; could you run npm run lint on your code?
examples/serverless.yml
Outdated
| onOriginResponse: | ||
| handler: src/handlers.onViewerResponse | ||
| distribution: 'WebsiteDistribution' | ||
| eventType: 'origin-request' | ||
| eventType: 'origin-response' |
There was a problem hiding this comment.
Why did this need to change? There is a onOriginResponse just below it already.
There was a problem hiding this comment.
Not sure why I thought this needed changing. Might have mixed things up myself. Will double check why this was changed and revert if not.
There was a problem hiding this comment.
Ahh I remember now! The handler is wrong (should be onOriginRequest not onViewerRequest)
src/behavior-router.ts
Outdated
| getCustomHeaders(this.cfResources[handler.distribution]): | ||
| {} | ||
|
|
||
| const cfEvent = convertToCloudFrontEvent(req, this.builder('viewer-request'), customHeaders); |
There was a problem hiding this comment.
I'm not familiar with the behavior of custom headers yet? Do the custom headers need to apply from the initial event through the entire lifecycle? Or do they only apply once the request transits to the origin (i.e. somewhere around here)?
There was a problem hiding this comment.
They get applied to a different part of the request object. I need to recheck to be honest. The current implementation here however is definatley wrong. Misread the CF event. So will need to refactor this in either case. Given I do not think the headers ever get merged. But if they do it is certainly not here 🤔
|
This might take a little while to refactor. Should hopefully get the chance to do it later this week... time permitting 🙏 |
|
Note about commit messages: this project uses No need for you to make any changes now, though. I can update the commit message before this PR is "Squashed and merged". I neglected to change the commit message for your hot reloading PR, so it didn't get released. But it should get picked up when this PR is released. |
|
Hi @matthewstrom , |
|
Hi @matthewstrom , |
|
Hi @matthewstrom @mattstrom , |
# [1.2.0](1.1.2...1.2.0) (2021-10-26) ### Features * add support for custom headers from origins ([#261](#261)) ([3fbe995](3fbe995))
|
Thanks mattstrom! 🎉 |
What:
This aims to solve #157
and #7Why
To better emulate the behaviour surrounding serverless how edge lambdas are handled
How:
For #157 the defined resources are read for custom headers and they are extracted and collapsed on a per distribution basis. This is a compromise as there is no way to really know which distribution origin needs to be used on a distribution.
For #7 the FunctionSet class is now distribution aware. It will refuse to register behaviours that have already been registered under a given pattern for a different distribution.
As part of this logging has been updated to reflect which distribution a given action is coming from.
Other details:
Some of the documentation has been updated to better reflect the current serverless landscape / corrected where errors given.
This also fixes the minor issue given in dherault/serverless-offline#1179 (comment) by @l1b3r about the uri containing query strings.