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

Fix SPA error page behavior #165

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

adriencaccia
Copy link
Contributor

@adriencaccia adriencaccia commented Feb 6, 2022

Switch the origin bucket configuration to static website hosting,
leaving the configuration on the bucket side, without having to rely
on a custom origin access identity.

Fix #163, #164

An other possible solution would be to simply re-attach a custom origin access policy with the s3::ListBucket policy, as explained in my comment in #163. The reason I did not implement this solution is to match lift goal on leveraging as much CDK default behaviors as possible.
If this solution is preferred, I would be happy to update the PR 🙂

Switch the origin bucket configuration to static website hosting,
leaving the configuration on the bucket side, without having to rely
on a custom origin access identity.

Fix getlift#163, getlift#164
@t-richard
Copy link
Contributor

This seems the right way forward to me 👍

And bonus, we get better support for static website hosting by supporting static site generation tools like Jekyll, Hugo, Gatsby, Nuxt/Next static target build, etc

@piolet
Copy link

piolet commented Feb 7, 2022

no need to handle 403 errors?
I ask, more by anticipation than to raise a problem

@t-richard
Copy link
Contributor

@piolet I think 403 could not happen anymore because the bucket now has public read access

See https://github.com/getlift/lift/pull/165/files#diff-5c9d8bfb4efe3d543af639b9a5524b66d81496d2e940def74fafca2294332e06R97

@piolet
Copy link

piolet commented Feb 7, 2022

Yes, with public access maybe.

When I tested, my bucket had remained private, that's probably why I had to manage the 403 error

@piolet
Copy link

piolet commented Feb 7, 2022

And bonus, we get better support for static website hosting by supporting static site generation tools like Jekyll, Hugo, Gatsby, Nuxt/Next static target build, etc

ideally, this bonus could be applied to the server side website. I know I'm repeating myself, but it happens that sites generated with server and nuxt cohabit (a subdirectory of the main site for example #94 )
I know it's not the subject of this PR, but if it can trigger an action, you never know ;)

@adriencaccia
Copy link
Contributor Author

@piolet I think 403 could not happen anymore because the bucket now has public read access

See #165 (files)

Yes that's exact !

@mnapoli mnapoli added the bug Something isn't working label Feb 7, 2022
if (errorPath.startsWith("./") || errorPath.startsWith("../")) {
throw new ServerlessError(
`The 'errorPage' option of the '${this.id}' static website cannot start with './' or '../'. ` +
`(it cannot be a relative path).`,
"LIFT_INVALID_CONSTRUCT_CONFIGURATION"
);
}
if (!errorPath.startsWith("/")) {
errorPath = `/${errorPath}`;
}
Copy link
Member

Choose a reason for hiding this comment

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

This bit was removed (and the corresponding test changed): this isn't necessary anymore with S3 doing the logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly, the correct path is resolved with or without a leading /

@mnapoli
Copy link
Member

mnapoli commented Feb 7, 2022

Just tested, worked well, thanks! That approach makes sense, I guess there's no way to disable the public S3 URL? (to avoid public duplicated content)

And bonus, we get better support for static website hosting by supporting static site generation tools like Jekyll, Hugo, Gatsby, Nuxt/Next static target build, etc

That's a very good point.

Also I keep reading the code change but I don't understand why the OAI isn't deployed anymore. If you have an answer to that I'm interested. In any case, it's not important since the bucket would be public now (so the OAI isn't needed).

@piolet
Copy link

piolet commented Feb 7, 2022

I guess there's no way to disable the public S3 URL? (to avoid public duplicated content)

an answer exists. Strangely enough, this is what was done before :) https://stackoverflow.com/questions/27636977/how-to-disable-amazon-s3-raw-endpoint-access

@adriencaccia
Copy link
Contributor Author

Just tested, worked well, thanks! That approach makes sense, I guess there's no way to disable the public S3 URL? (to avoid public duplicated content)

If we want to let the bucket handle its website behavior, and not CF, I'm afraid not.

Also I keep reading the code change but I don't understand why the OAI isn't deployed anymore. If you have an answer to that I'm interested. In any case, it's not important since the bucket would be public now (so the OAI isn't needed).

That's it yes, the OAI will no longer be needed since the bucket handles its static website access.

@mnapoli
Copy link
Member

mnapoli commented Feb 7, 2022

👍 thanks for all the answers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Static website deploy does not work for SPA projects anymore
4 participants