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

adding customized error pages with a specific api for error handling #107

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

marwanehcine
Copy link
Contributor

@marwanehcine marwanehcine commented Feb 26, 2024

adding customized error pages with a specific api for error handling (404, 403, 500, 501, 503)
I have added a new CustomAccessDeniedHandler class to throw an exception when 403 is returned.
Exception should be throw to be handled later by CustomErrorAttributes.java. (set error status and be redirected to correct html error page)

Comment on lines 130 to 134
@GetMapping(path = "/error/{code}")
public String handleError(@PathVariable String code) {
return "error/" + code;
}

Copy link
Member

Choose a reason for hiding this comment

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

is this necessary ? Relying on the doc provided by @edevosc2c (https://github.com/georchestra/georchestra-gateway/blob/main/docs/custom-error-pages.adoc), as long as we have a template for each http error code under templates/error/xyz.html, it should work.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is related to "traefik.http.middlewares.static-errors-middleware.errors.query=/error/{status}"
But @pmauduit , it did work for me after just adding files under emplates/error (only 404 page worked!).
You can just check locally if you want.

Copy link
Contributor Author

@marwanehcine marwanehcine Feb 26, 2024

Choose a reason for hiding this comment

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

Just as update, we need to set traefik conf as follows to make it works :

  - "traefik.http.middlewares.static-errors-middleware.errors.status=400-599"
  - "traefik.http.middlewares.static-errors-middleware.errors.service=gateway"
  - "traefik.http.middlewares.static-errors-middleware.errors.query=/error/{status}"

Here, we are saying that error will be handled by gateway and we redirect it to path /error/{status}.
after that we add a specific api for this path as show on the top.

Also traefik.http.routers.gateway.rule have to change to allow gateway to handle all request without restriction.
Check
https://github.com/georchestra/docker/pull/275/files#diff-2286cdeca85adb587a9640aa34352ba7c2fb6e49c0129dab05892ff67d0d4c58R78

Copy link
Member

Choose a reason for hiding this comment

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

Then, as far as I understand it, it is specific to the fact that the docker composition is making use of traefik ? Then we should consider dropping it, and keep the gateway agnostic to whatever is used as a proxy before it.

Copy link
Member

Choose a reason for hiding this comment

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

Good idea for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but how to handle errors for specific projects in such a case ?

Copy link
Member

Choose a reason for hiding this comment

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

For me static-errors-middleware in traefik is not needed anymore with the gateway because it's the gateway itself that will send the error pages.

@marwanehcine marwanehcine force-pushed the customizing_error_pages branch 2 times, most recently from b0517dd to 80ae913 Compare March 5, 2024 08:23
@f-necas f-necas self-requested a review March 5, 2024 08:38
Copy link
Collaborator

@f-necas f-necas left a comment

Choose a reason for hiding this comment

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

LGTM on a fuctionnal way. Thanks @marwanehcine !

@f-necas f-necas self-requested a review March 6, 2024 11:41
gateway/src/main/resources/templates/error/500.html Outdated Show resolved Hide resolved
gateway/src/main/resources/templates/error/501.html Outdated Show resolved Hide resolved
gateway/src/main/resources/templates/error/503.html Outdated Show resolved Hide resolved
Copy link
Member

@edevosc2c edevosc2c left a comment

Choose a reason for hiding this comment

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

If we now include the error pages directly into the gateway code, can we still override them through the datadir like in MEL: https://github.com/camptocamp/georchestra-mel-configuration/tree/k8s-master-dev/gateway/templates/error?

@marwanehcine
Copy link
Contributor Author

If we now include the error pages directly into the gateway code, can we still override them through the datadir like in MEL: https://github.com/camptocamp/georchestra-mel-configuration/tree/k8s-master-dev/gateway/templates/error?

Yes, just we need to set correct path in prop file.
image

Thanks

@marwanehcine
Copy link
Contributor Author

@marwanehcine don't forget about my message above 😃: #107 (review)

tested then replied, Thanks

Copy link
Member

@pmauduit pmauduit left a comment

Choose a reason for hiding this comment

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

looks globally good to me even if I miss some context, just one remark though.

@marwanehcine marwanehcine merged commit 638e3c8 into georchestra:main Mar 7, 2024
3 checks passed
@marwanehcine marwanehcine deleted the customizing_error_pages branch March 7, 2024 19:01
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.

None yet

4 participants