Implement custom noroute html response #398

tino · 2017-12-03T17:39:30Z

PR for #56

Sign CLA
Implement functionality for consul backend
Implement functionality for file backend
Write tests
~~Add default no route HTML? (Like all other servers do, but then styled a bit nicer :)~~

My first contribution to a Go project, so let me know what can be done better!

If tests are need, for what should I write them?

Closes #56

tino · 2017-12-03T17:44:12Z

BTW, I added https://github.com/fabiolb/fabio/wiki/Contributing#developing, as it took me quite a while to find out how this works with Go. Might need some rewriting?

magiconair · 2017-12-04T07:39:01Z

Hi Tino, thanks for the contribution. I will have a look right away. Also, thanks for adding the Developing section to the Wiki. Your description is spot on!

magiconair

In general, this looks OK. I think NoRouteHTMLPath is a bit too clumsy for a name but can't come up with something better right now.

The final version would need to have an integration test in the proxy/http_integration_test.go file.

However, I do think that we could generalize the approach a bit further. If the code would watch a path in consul then we could have multiple custom files, e.g.

/fabio/page/noroute.html
/fabio/page/404.html
/fabio/page/503.html
...

store would then contain a map[string]string where the key is the code and the value is the page.

If there is a page for that code or for noroute then fabio should return that.

However, this becomes more difficult in the case of 404 and 503 from the upstream service since the response has already been copied to the client. Fabio would have to sniff the status code from the response stream to inject its own response. The httputil.ReverseProxy doesn't provide proper hooks for that AFAIR but I would need to look. If not, then this would certainly be a larger change.

What we could do nevertheless is to prepare the configuration for that so that when the feature gets implemented users don't have to change things.

magiconair · 2017-12-04T07:41:26Z

route/no_route.go

+    "sync/atomic"
+)
+
+// HTML Wrapper struct so we can store the html string in an atomic.Value


You don't need this since you can store a string in an atomic.Value directly.

magiconair · 2017-12-04T07:42:34Z

route/no_route.go

+    value string
+}
+
+// html stores the no route html string


I usually add a comment after the value to indicate the type since it cannot be changed once it is set, .e.g.

var store atomic.Value // string

magiconair · 2017-12-04T07:43:14Z

route/no_route.go

+// GetHTML returns the HTML for not found routes. The function is safe to be
+// called from multiple goroutines.
+func GetHTML() string {
+    return store.Load().(HTML).value


This then becomes

return store.Load().(string)

magiconair · 2017-12-04T07:43:51Z

route/no_route.go

+    return store.Load().(HTML).value
+}
+
+// hmu guards the atomic writes in SetHTML.


An atomic.Value is already synchronized. Therefore, you don't need a mutex here.

magiconair · 2017-12-04T07:50:31Z

registry/static/backend.go

+// WatchNoRouteHTML implementation that reads the noroute html from a
+// noroute.html file if it exists
+func (b *be) WatchNoRouteHTML() chan string {
+	data, err := ioutil.ReadFile("noroute.html")


noroute.html is a magic value and would need to be configurable.

magiconair · 2017-12-04T07:51:40Z

route/no_route.go

@@ -0,0 +1,44 @@
+package route


I would probably create a separate package for this since neither route nor proxy is a good place to keep this.

tino · 2017-12-06T20:55:07Z

On the mutex part, I copied this from the route/table.go. Why is it needed there and not here?

I held off on multiple custom error pages. In my view if you set an error code in the upstream service, you should also provide the content you want to serve, and fabio should not overwrite it.

This page is just for when the complete service could not be found, and is in sync with the Proxy.NoRouteStatus value, hence the – bit clumsy, but clearer then other things I could come up with – name.

magiconair · 2017-12-08T07:57:45Z

Re mutex: I guess you're referring to this:

https://github.com/fabiolb/fabio/blob/master/route/table.go#L47-L56

The reason a mutex is required here is that it changes two things: the table and the metrics registry.

I'll have a look at the rest today.

tino · 2017-12-11T10:54:24Z

Ah okay. I missed that.

tino · 2017-12-14T15:56:53Z

I decided not to add a default HTML message. Although I would like not to serve a browsers default 503/404 by default to humans, it took me too much effort to cleanly implement with templates how I had it in mind.

Is this mergeable as-is, or do you need some more changes? If so, we can start to remove our nginx instances that are in front of fabio atm ☺️

magiconair · 2017-12-15T08:05:14Z

I’ll have a quick look today

magiconair · 2017-12-15T08:25:11Z

This looks fine except that the name is still bugging me because it is too specific. How about we call it TemplatePath and keep a generic list of templates? Then for the no route case we use the noroute.html template.

I can do this or you can pick this up. I'd like to get this merged as well.

magiconair · 2017-12-16T14:01:51Z

What I suggested is a larger refactor. I'll pick that up myself later. I'm going to merge this and amend the code with some small cleanup.

Thanks for this!

traceypooh · 2022-01-13T02:51:33Z

i'm... at an utter loss here...

i finally got consul k/v setup with html, but no matter what i do, it comes out as plaintext, w/

  Content-Type: text/plain; charset=utf-8

type headers regardless of what I try.

is there some magic "front matter"-like top part of the k/v string i'm supposed to use?
(I haven't been able to get any kind of configuration w/ files, like anything swirling around flailage like this to work on fabio.properties either. so... so far, consul k/v seems my only hope!)

registry.file.noroutehtmlpath = /etc/fabio/404.html
registry.static.noroutehtmlpath = "<h1>not found</h1>"
registry.static.noroutehtml = "<h1>not found</h1>"
registry.static.noroutehtmlpath = /etc/fabio/404.html

any help vastly appreciated as we're trying to make a bigger move to nomad and fabio...

tristanmorgan · 2022-01-13T03:25:40Z

For me I have an HTML snippet in the consul KV and Fabio returns appropriate headers.

$ curl -vv https://consul-esm.service.consul
.....
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 404 
< content-type: text/html; charset=utf-8
< content-length: 189
< date: Thu, 13 Jan 2022 03:19:01 GMT
< 
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html><body>
<a href="https://fabio.service.consul/">See Fabio</a>.
* Connection #0 to host consul-esm.service.consul left intact
</body></html>

$ consul kv get fabio/noroute.html
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd">
<html><body>
<a href="https://fabio.service.consul/">See Fabio</a>.
</body></html>

traceypooh · 2022-01-13T20:17:48Z

thanks @tristanmorgan !

ooh, that totally worked for me.
(i wonder if there's some kind of autodetection for starting kv string with <!DOCTYPE.. -- but that works while yesterday lots of attempts w/o that lead line was not working for me).

🙏

Implement custom noroute html response

a7c2412

PR for fabiolb#56

magiconair reviewed Dec 4, 2017

View reviewed changes

tino added 6 commits December 6, 2017 21:12

Add integration test

23546e1

Remove mutex

1fae4c4

Remove superfluous constructs of store

3e08824

Make file and static backend work with config too

883be35

Move noroute store to it's own package

81ea194

Rename and add test

2a09ecb

Fix error message fmt in test

24728a1

magiconair merged commit b84bb2a into fabiolb:master Dec 16, 2017

magiconair added this to the 1.5.5 milestone Dec 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement custom noroute html response #398

Implement custom noroute html response #398

tino commented Dec 3, 2017 •

edited by magiconair

Loading

tino commented Dec 3, 2017

magiconair commented Dec 4, 2017

magiconair left a comment

magiconair Dec 4, 2017

magiconair Dec 4, 2017

magiconair Dec 4, 2017

magiconair Dec 4, 2017

magiconair Dec 4, 2017

magiconair Dec 4, 2017

tino commented Dec 6, 2017

magiconair commented Dec 8, 2017

tino commented Dec 11, 2017

tino commented Dec 14, 2017

magiconair commented Dec 15, 2017

magiconair commented Dec 15, 2017 •

edited

Loading

magiconair commented Dec 16, 2017

traceypooh commented Jan 13, 2022

tristanmorgan commented Jan 13, 2022

traceypooh commented Jan 13, 2022

Implement custom noroute html response #398

Implement custom noroute html response #398

Conversation

tino commented Dec 3, 2017 • edited by magiconair Loading

tino commented Dec 3, 2017

magiconair commented Dec 4, 2017

magiconair left a comment

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

magiconair Dec 4, 2017

Choose a reason for hiding this comment

tino commented Dec 6, 2017

magiconair commented Dec 8, 2017

tino commented Dec 11, 2017

tino commented Dec 14, 2017

magiconair commented Dec 15, 2017

magiconair commented Dec 15, 2017 • edited Loading

magiconair commented Dec 16, 2017

traceypooh commented Jan 13, 2022

tristanmorgan commented Jan 13, 2022

traceypooh commented Jan 13, 2022

tino commented Dec 3, 2017 •

edited by magiconair

Loading

magiconair commented Dec 15, 2017 •

edited

Loading