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

Improved handling of origins and lambda triggers #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gehan
Copy link

@gehan gehan commented May 1, 2020

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

janoist1 commented May 1, 2020

+1

1 similar comment
@evilrob666
Copy link

+1

@gehan gehan changed the title Improved handling of origins Improved handling of origins and lambda triggers May 1, 2020
lib/index.js Outdated Show resolved Hide resolved
Copy link
Contributor

@danielcondemarin danielcondemarin left a comment

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

gehan commented May 14, 2020

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

@krish-dev
Copy link

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

gehan commented Jun 8, 2020

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

krish-dev commented Jun 8, 2020

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

gehan commented Jun 8, 2020

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

@krish-dev
Copy link

@gehan @danielcondemarin any update on the same.

@bboure
Copy link

bboure commented Aug 11, 2020

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

gehan commented Aug 19, 2020

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