Skip to content
This repository was archived by the owner on Apr 30, 2025. It is now read-only.

Allow users to HTML template error messages - #271

Merged
jrussett merged 19 commits into
cloudfoundry:mainfrom
tlwr:main
Aug 20, 2020
Merged

Allow users to HTML template error messages#271
jrussett merged 19 commits into
cloudfoundry:mainfrom
tlwr:main

Conversation

@tlwr

@tlwr tlwr commented Aug 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Checklist

  • basic functionality and tests
  • configuration
  • contextual helpers available in HTML template
  • better PR description
  • content types

Context

Currently, when gorouter encounters an error (eg route not found) a user receives a plaintext error response using http.Error, which looks like this:

404 Not Found: Requested route ('my-route.my-domain.tld') does not exist.

This is usually not a big problem because we do not expect users to see gorouter error pages often, however they do occur often enough that users notice:

  • App timeouts
  • App crashes
  • Platform issues (eg TLS certificates)

This has been discussed before in #171 but the work was not pursued

Explanation

This PR does not change the behaviour of gorouter unless using a newly created configuration parameter html_error_template_file.

Templates are written using go html/template syntax.

Using this parameter, Gorouter can be configured to generate HTML error messages that render nicely in a user's browser, and an operator can configure the templates to provide additional contextual information that would otherwise require a user to open up their developer tools.

For example:

<html>
  <body>
  Code: {{ .Status }}
  Cause: {{ .Header.Get "X-Cf-RouterError" }}
  </body>
</html>

I have also created a GOV.UK PaaS error page:

Screenshot of GOV.UK PaaS error page
Screenshot of GOV.UK PaaS error page

The following HTML is the relevant bit:

<h1 class="govuk-heading-xl">Something went wrong</h1>
<details>
  <summary>Additional diagnostic information</summary>
  <dl class="govuk-summary-list">
    <div class="govuk-summary-list__row">
      <dt class="govuk-summary-list__key">HTTP code</dt>
      <dd class="govuk-summary-list__value">{{ .Status}} {{ .StatusText }}</dd>
    </div>

    <div class="govuk-summary-list__row">
      <dt class="govuk-summary-list__key">Error message</dt>
      <dd class="govuk-summary-list__value">{{ .Message }}</dd>
    </div>

    <div class="govuk-summary-list__row">
      <dt class="govuk-summary-list__key">Reason</dt>
      <dd class="govuk-summary-list__value">{{ .Header.Get "X-Cf-RouterError" }}</dd>
    </div>

    {{ if .Header.Get "X-Vcap-Request-Id" }}
    <div class="govuk-summary-list__row">
      <dt class="govuk-summary-list__key">Request ID</dt>
      <dd class="govuk-summary-list__value">{{ .Header.Get "X-Vcap-Request-Id" }}</dd>
    </div>
    {{ end }}
  </dl>
</details>

How to review

Create a configuration file /tmp/gorouter.yml with contents:

html_error_template_file: /tmp/example.html

Create a template /tmp/example.html with contents:

<html>
  <body>
  Code: {{ .Status }}
  Cause: {{ .Header.Get "X-Cf-RouterError" }}
  <img src="https://golang.org/lib/godoc/images/home-gopher.png" alt="A cute gopher in black and white, as penance for responding with an error"/>
  </body>
</html>

Run gorouter:

go build && ./gorouter -c /tmp/example.yaml

Navigate to http://localhost:8081 and observe there is a HTML page which includes

Code: 400 Cause: empty_host

Future behaviour

Error pages in your browser render HTML instead of plain text.

Current behaviour

Error pages cannot be templated, and render as plain text.

Other PRs

If this change is welcome I will raise a routing release PR which allows a user to provide a template via the gorouter BOSH job.

Checklist

  • I have viewed signed and have submitted the Contributor License Agreement

  • I have made this pull request to the develop branch (⚠️ I've done this to main not develop which I think is correct)

  • I have run all the unit tests using scripts/run-unit-tests-in-docker

  • (Optional) I have run Routing Acceptance Tests and Routing Smoke Tests on bosh lite

  • (Optional) I have run CF Acceptance Tests on bosh lite

@ameowlia

ameowlia commented Aug 7, 2020

Copy link
Copy Markdown
Member

Hi @tlwr,

Just wanted to say thanks for being a great community member! I look forward to reviewing this PR when you are ready!

@tlwr
tlwr force-pushed the main branch 2 times, most recently from 49ec777 to 950db67 Compare August 11, 2020 19:59
@tlwr

tlwr commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

Thank you for your kind words @ameowlia :D

I've updated the description for this PR and all the tests which do not use the localhost.routing.cf-app.com (did someone delete the DNS record?)

Am I good to raise a routing-release PR? I don't want to create unwanted noise 📢 unless it is warranted :)

@tlwr tlwr changed the title work-in-progress: allow operators to HTML template error messages Allow users to HTML template error messages Aug 11, 2020
@ameowlia

ameowlia commented Aug 13, 2020

Copy link
Copy Markdown
Member

Hi @tlwr,

This looks good to me. I'm going to let another team member review before I merge it in.

Once we merge it then you can make the bump PR to routing-release that points at the merge commit. We'll let you do it though so you get credit 👍

tlwr added 19 commits August 15, 2020 09:09
@tlwr
handlers: error writer interface and impls
4aa6992 
so we can render HTML error messages in future

Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: clientcert uses ErrorWriter
ba5e815 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: WriteError impls use fmt.Fprintln
2fa8f54 
like the http pkg

Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: lookup handler uses ErrorWriter
a3b7162 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: protocolcheck uses ErrorWriter
b857644 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: routeservice uses ErrorWriter
8e7e0ea 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: remove unused helper writeStatus
4e2c054 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
proxy/handler: request handler uses ErrorWriter
623c35d 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
proxy: uses hardcoded plaintext ErrorWriter
06c7356 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
config: add html_error_template_file option
7cf0aeb 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
handlers: errorwriter is own package
770f3de 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
errorwriter: user a file rather than a template
9780d5c 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
proxy: takes errorwriter as constructor arg
2ad334f 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
integration-tests: add helper to check if started
736df74 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
gorouter: can use static html templates
b1995b0 
and integration tests

Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
errorwriter: sets content type correctly
62d2669 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
errorwriter: html templates have context
8cc7b82 
which includes headers, status, and message

Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
errorwriter: add integration test for headers
95c6995 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr
router: uses errorwriter
e60b55b 
Signed-off-by: toby lorne <toby@toby.codes>
@tlwr

tlwr commented Aug 15, 2020

Copy link
Copy Markdown
Contributor Author

Rebased on latest main, merge conflict resolved (git didn't know what to do with this diff)

@mcwumbly

mcwumbly commented Aug 17, 2020

Copy link
Copy Markdown
Contributor

@tlwr heads up - we have our hands full this week, so it may take us a little bit to get to this, but I know we aim to review it as soon as we can.

In the meantime, would you mind opening an issue on routing-release to capture your intent here and surface it to the community? we are trying to make routing-release issues be the entry point for community discussions about changes to routing-release and underlying components. it'd be good to surface this there sooner than later.

tlwr pushed a commit to govuk-paas/paas-routing-release that referenced this pull request Aug 17, 2020
Toby Lorne
gorouter: bump to version including html templates
30ecd5d 
cloudfoundry/gorouter#271

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>
@tlwr tlwr mentioned this pull request Aug 18, 2020
Closed
5 tasks
@tlwr

tlwr commented Aug 18, 2020

Copy link
Copy Markdown
Contributor Author

Hi @mcwumbly I totally understand, no worries at all :)

I've raised a PR cloudfoundry/routing-release#179 against routing-release which should hopefully make it easier to review when you get a chance

@jrussett
jrussett merged commit 7ee1978 into cloudfoundry:main Aug 20, 2020
jrussett pushed a commit to cloudfoundry/routing-release that referenced this pull request Aug 20, 2020
Toby Lorne @jrussett
gorouter: bump to version including html templates
d63fa38 
cloudfoundry/gorouter#271

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>
jrussett pushed a commit to cloudfoundry/routing-release that referenced this pull request Aug 20, 2020
Toby Lorne
gorouter: bump to version including html templates
146e675 
cloudfoundry/gorouter#271

Signed-off-by: Toby Lorne <toby.lornewelch-richards@digital.cabinet-office.gov.uk>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tlwr @ameowlia @mcwumbly @jrussett