Skip to content

Improved handling of origins and lambda triggers#19

Open
gehan wants to merge 4 commits into
serverless-components:masterfrom
HeroTeamLdn:origin-improvements
Open

Improved handling of origins and lambda triggers#19
gehan wants to merge 4 commits into
serverless-components:masterfrom
HeroTeamLdn:origin-improvements

Conversation

@gehan

@gehan gehan commented May 1, 2020

Copy link
Copy Markdown

Related to #18

  • Handles s3 website urls differently - S3 buckets used to serve websites should use CustomOrigin as per https://docs.aws.amazon.com/cloudfront/latest/APIReference/API_S3OriginConfig.html
  • Allow multiple origins from same s3 bucket - Sets origin to ID to include path
  • Allow setting of origin protocol policy option
  • Handles IncludeBody for lambda triggers correctly - ie allows option and requires set to false for response triggers

* Handle s3 website urls properly
* Allow multiple origins from same s3 bucket
* Allow settings of original protocol polic
@gehan gehan changed the title Improve handling of s3 origins Improve handling of origins May 1, 2020
@gehan gehan changed the title Improve handling of origins Improved handling of origins May 1, 2020
@janoist1

janoist1 commented May 1, 2020

Copy link
Copy Markdown

+1

1 similar comment
@evilrob666

Copy link
Copy Markdown

+1

@gehan gehan force-pushed the origin-improvements branch from 4f2eb14 to 1e0086c Compare May 1, 2020 19:02
@gehan gehan changed the title Improved handling of origins Improved handling of origins and lambda triggers May 1, 2020
Comment thread lib/addLambdaAtEdgeToCacheBehavior.js Outdated
Comment thread lib/index.js Outdated

@danielcondemarin danielcondemarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from just a few things as per my comments 🚀

It also doesn't seem like the change would be breaking, could you confirm?

@gehan

gehan commented May 14, 2020

Copy link
Copy Markdown
Author

Will update. Changes should not break anything but will confirm.

@krish-dev

Copy link
Copy Markdown

Wonderful PR, I also encountered with exactly same issue like

InvalidArgument: The parameter origin ID must be unique.

@gehan Can you please 🙏 confirm that changes should not break anything so that @danielcondemarin can take this further 🚀

@gehan

gehan commented Jun 8, 2020

Copy link
Copy Markdown
Author

Wonderful PR, I also encountered with exactly same issue like

InvalidArgument: The parameter origin ID must be unique.

@gehan Can you please 🙏 confirm that changes should not break anything so that @danielcondemarin can take this further 🚀

What config are you using to get this error? I'll update accordingly!

@krish-dev

krish-dev commented Jun 8, 2020

Copy link
Copy Markdown

What config are you using to get this error? I'll update accordingly!

Hi, @gehan I think you already fix the issue in this PR.

Actually, I have 2 origins with the same domain but with different paths like this configuration.

{
    "defaults": {...},
    "origins": [
        {
            "url": "http://xxx-pwa-prod.s3.amazonaws.com",
            "private": true,
            "pathPatterns": {
                "_next/*": {
                    "ttl": 86400
                },
                "static/*": {
                    "ttl": 86400
                }
            }
        },
        {
            "url": "http://xxx-pwa-prod.s3.amazonaws.com/_next/static",
            "private": true,
            "pathPatterns": {
                "service-worker.js": {
                    "ttl": 0
                }
            }
        }
    ]
}

and its end-up with same Id like
I just added a console.log(JSON.stringify(Origins)); in this line to get this result.

{
  "Quantity": 2,
  "Items": [
    {
      "Id": "xxx-pwa-prod",
      "DomainName": "xxx-pwa-prod.s3.amazonaws.com",
      "CustomHeaders": {
        "Quantity": 0,
        "Items": []
      },
      "OriginPath": "",
      "S3OriginConfig": {
        "OriginAccessIdentity": "origin-access-identity/cloudfront/<identity>"
      }
    },
    {
      "Id": "xxx-pwa-prod",
      "DomainName": "xxx-pwa-prod.s3.amazonaws.com",
      "CustomHeaders": {
        "Quantity": 0,
        "Items": []
      },
      "OriginPath": "/_next/static",
      "S3OriginConfig": {
        "OriginAccessIdentity": "origin-access-identity/cloudfront/<identity>"
      }
    }
  ]
}

@gehan

gehan commented Jun 8, 2020

Copy link
Copy Markdown
Author

@krish-dev oh yes sorry I see! misunderstood. i'll check my PR for breaking changes 👍

@krish-dev

Copy link
Copy Markdown

@gehan @danielcondemarin any update on the same.

@bboure

bboure commented Aug 11, 2020

Copy link
Copy Markdown

Any update on this?
I am getting errors due to IncludeBody being true for on-supported events.
This PR would fix the issue.
Thanks

@gehan

gehan commented Aug 19, 2020

Copy link
Copy Markdown
Author

LGTM aside from just a few things as per my comments 🚀

It also doesn't seem like the change would be breaking, could you confirm?

Long delay here! Updated as per comments and is non-breaking

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.

6 participants