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.

Sign up for GitHub

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

Jump to bottom

Support rewriting status codes in error page middleware #11520

Merged
merged 8 commits into from
Mar 3, 2025
Merged

Support rewriting status codes in error page middleware #11520

merged 8 commits into from
Mar 3, 2025

Conversation

sevensolutions
Copy link
Contributor

@sevensolutions sevensolutions commented Feb 8, 2025 •
edited
Loading

What does this PR do?

This PR adds support for rewriting status codes when using the Errors Middleware as requested in #2039.

Motivation

I always configure a low-priority fallback router for unmatched requests like this, to show a custom 404 error page:

http:
  middlewares:
    fallback-error:
      errors:
        status: ["418"]
        service: myErrorService
        query: "/{status}.html"

  routers:
    fallback:
      rule: "PathPrefix(`/`)"
      priority: 1
      service: noop@internal
      middlewares: ["fallback-error"]

But because the noop-service always returns 418, i want to rewrite it to 404 again.

This PR allows the following configuration:

http:
  middlewares:
    fallback-error:
      errors:
        status: ["418"]
        service: myErrorService
        query: "/{status}.html"
        statusRewrites:
          "418": 404

  routers:
    fallback:
      rule: "PathPrefix(`/`)"
      priority: 1
      service: noop@internal
      middlewares: ["fallback-error"]

statusRewrites defines a mapping of status codes, so also supports multiple.
Every key can also be a range like:

statusRewrites:
  "418": 404
  "500-504": 500

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

@sevensolutions sevensolutions marked this pull request as ready for review February 8, 2025 19:33
@sevensolutions
Copy link
Contributor Author

I think i should also add the originalStatus as a parameter to the query option.

@rtribotte
Copy link
Member

Hi @sevensolutions,

Thanks for this contribution!

At glance, reference documentation and Kubernetes CRDs update are missing.
You can generate them by executing go generate and make generate-crd.

@sevensolutions
Copy link
Contributor Author

@rtribotte thx for reviewing this and sorry, didn't know that.
I've pushed the missing files.

@kevinpollet kevinpollet changed the title feat: Add support for rewriting status codes in error page middleware Support rewriting status codes in error page middleware Feb 17, 2025
sdelicata
sdelicata previously requested changes Feb 21, 2025
View reviewed changes
Copy link
Contributor

@sdelicata sdelicata left a comment

Choose a reason for hiding this comment

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

Hello @sevensolutions and thanks for your contribution!
I made a few suggestions concerning option naming and documentation.

@sevensolutions
Copy link
Contributor Author

Thank you for reviewing my PR @sdelicata.
I like the suggested name statusRewrites and updated everything.

@kevinpollet kevinpollet added this to the next milestone Feb 27, 2025
mmatur
mmatur approved these changes Feb 28, 2025
View reviewed changes
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks for your contribution

youkoulayley
youkoulayley approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@youkoulayley youkoulayley left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinpollet kevinpollet dismissed sdelicata’s stale review March 3, 2025 09:42

We want to move forward with this pull request

kevinpollet
kevinpollet approved these changes Mar 3, 2025
View reviewed changes
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@traefiker traefiker merged commit fa76ed5 into traefik:master Mar 3, 2025
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinpollet kevinpollet kevinpollet approved these changes

@mmatur mmatur mmatur approved these changes

@youkoulayley youkoulayley youkoulayley approved these changes

@sdelicata sdelicata sdelicata left review comments

Assignees
No one assigned
Labels
area/middleware kind/enhancement a new or improved feature. size/M
Projects
None yet
Milestone
3.4
Development

Successfully merging this pull request may close these issues.
8 participants
@sevensolutions @rtribotte @kevinpollet @mmatur @youkoulayley @sdelicata @nmengin @traefiker