URLs not calculated correctly behind a proxy

Upon further investigation, I see that this problem applies only to the deprecated Waterfall display; WebStatus? doesn't use the root URL for a waterfall display. May I suggest an auto-redirect for Waterfall, instead of displaying the same resource as /waterfall?

E.g., if you're using the Waterfall class instead of WebStatus?, it should send a redirect to the web browser to the correct waterfall URI instead of using the broken behavior it has currently.

Yeah, I was uncertain about doing that "/" -> "/waterfall" redirect, because my reading of RFC2616 (the HTTP specification) says that redirects are supposed to be absolute URIs, and of course any absolute URI from behind a reverse proxy is going to be wrong. So I punted for 0.7.6 and made "/" be a copy of "/waterfall", but then some of the URLs are broken.

I suppose we need to keep Waterfall around for another release or so, it's deprecated in 0.7.7 but it's been around for so long, it's only fair to give people a chance to switch over. Do you think an out-of-spec relative redirect URI is likely to cause fewer problems than the current issues? Or is it enough to point folks with this problem towards the modern WebStatus? class?

so, it didn't occur to me earlier that we could use c['buildbotURL'] to get an absolute URL of the /waterfall page, and just require that anyone who's using a proxy and still using the deprecated Waterfall make sure that they get the buildbotURL right. This would use an absolute redirect, in keeping with the HTTP spec.

I'll try to implement that one tomorrow and see how it looks.

There are a lot of other redirects in the web code. For example, after posting to the force build form, a redirect to "../.." is generated. This is clearly not an absolute URL either.

A trivial grep reveals a number of things which probably need to be fixed:

./buildbot/status/web/build.py:        #return Redirect("../%d" % self.build.getNumber())
./buildbot/status/web/build.py:        r = Redirect("../../..") # TODO: no longer correct
./buildbot/status/web/build.py:        #r = Redirect("../../../..") # this takes us back to the welcome page
./buildbot/status/web/build.py:        #r = Redirect("../../../../waterfall") # or the Waterfall
./buildbot/status/web/build.py:        #r = Redirect("../../../../waterfall?show=%s" % builder_name)
./buildbot/status/web/build.py:        r = Redirect("../..") # the Builder's page
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:            return Redirect("..")
./buildbot/status/web/builder.py:        return Redirect("../..")
./buildbot/status/web/builder.py:        return Redirect("../..")

This breaks real HTTP clients which don't expect to have to figure out relative paths.

Ah, a random comment.

Using c['buildbotURL'] seemed like a good idea to me at first, but then I looked at my buildbot configuration and remembered that I have more than one web status reporter. The single global configuration only provides one URL, but there are three commonly used URLs which expose status information. Clearly each needs to generate URLs which point to it, not to one of the other statuses.

If buildbot isn't doing anything excessively clever with internal redirects (which it probably isn't, since it uses twisted.web which probably can't even do such a thing currently), then it may be possible to use Request.prepath to automatically determine the correct URL. Alternatively, if only Resources which are being rendered need to generate redirects (as opposed to generating a redirect partway through URL path segment traversal, eg by returning something funny from a getChild), then you can look at Request.uri and Request.getHeader('host'). Another option would be to pass the root into WebStatus as a parameter, which is nice and explicit and puts the burden of correctness on whoever is writing the configuration. ;) Of these, the first is the coolest but also probably the most likely to have subtle annoying bugs. The second is probably the best, if it applies.

The second approach sounds doable.. are reverse proxies prohibited from modifying the Host: header? And, well, I guess most browsers are speaking HTTP/1.1 these days and will therefore have a Host header. (most of my buildmasters are on vhosts, so they aren't even reachable without a Host header).

hrm, I can't figure out a good way to fix this that will be done in time for 0.7.7 . Gotta push it out a release.