Go: x/net/trace: Proposal: Remove import side effects

[SamWhited]

Cross posting this issue from the mailing list (https://groups.google.com/forum/#!topic/golang-dev/IfEhzW_mmV4) in accordance with the contributing doc (I'm still unsure what the usual procedure is here; should I really be posting things in two places?):

Right now if you import the x/net/trace package you wind up with the
side effect that net/http's DefaultServeMux has handlers for
/debug/requests and /debug/events registered on it. The only mention
of this side effect in the docs is a somewhat unclear statement right
at the start:

Package trace implements tracing of requests and long-lived objects. It exports HTTP interfaces on /debug/requests and /debug/events.

To me, this was very confusing. I did not take that statement to mean
that the package implicitly registers the handlers, but that this was
behavior you could achieve with the package (eg. maybe there's a
RegisterHandlers function that registers those two handlers).

I've put in a CL to make the documentation clearer
(https://go-review.googlesource.com/#/c/22208/1) but I'd also like to
propose that this "feature" be removed.

Implicitly setting up the handlers on package import violates Go's
principal of least suprise; in my case, that was just a panic and a
few minutes of me starting blankly at a screen wondering how the
handler I was registering could already be registered, but in someone
elses case it could involve a route to their debugging info being
exposed to the public (because they only thought to put authorization
or authentication in front of the /trace route which is where they
explicitly exposed the trace handler after importing this package).

In the elegant words of one of my coworkers when we discovered this
behavior: "that is a pretty gross side effect".

[dsymonds]

There's a number of packages (including some in the standard library)
that register HTTP interfaces by default, and that happens via
net/http.DefaultServeMux. x/net/trace is following the same pattern.

Auth shouldn't be a problem if people are unaware. The default
configuration of that package will only serve those interfaces to
requests coming from the same machine; you have to go out of your way
to expose it to the public.

[SamWhited]

There's a number of packages (including some in the standard library) that register HTTP interfaces by default

A quick greap for http.Handle( and http.HandleFunc( only showed a few things, all under the http/ tree. This feels a bit less suprising to me (though still not ideal) since they are in the same package.

The default configuration of that package will only serve those interfaces to
requests coming from the same machine

Ah yes, I missed that; that's much _much_ better, but it still feels like this would be much better off being something that I as an API consumer have to enable explicitly. Anything that modifies my servers behavior, I probably want to know about. We don't know how users machines are set up, or how they're running their infrastructure, for all we know access to those methods from the same machine is still a problem (maybe there's sensative info in those logs that not all devs get access too and only ops can see? It's a contrived and vague example, but I think the point still stands).

[dsymonds]

expvar exports on /debug/vars by default too.

If you're in a truly paranoid situation, you can always set up your own http.ServeMux. Remember that you won't get any of this doing anything unless you call something like http.ListenAndServe.

The issue with having to enable it explicitly is that it's the kind of debugging endpoint you want to have present by default. It's annoying to have to load up your func main() with a whole lot of setup things, and if you're getting x/net/trace in your program it's because something is trying to expose information through it (e.g. grpc-go) presumably because that information is useful to be able to access.

I think this is a massive convenience, and a pretty minor risk in practice.

[SamWhited]

expvar exports on /debug/vars by default too.

Ah, yup, didn't see that. I'm not a huge fan of it there either, but that's in the standard library so there's no changing it. At least it's very clearly documented in that case. I'll grant you that this does establish prescident, but just because something else was done this way before it doesn't make it a good idea or mean we should keep doing it this way in the future.

If you're in a truly paranoid situation, you can always set up your own http.ServeMux

You could do that, but the point is that you have to _know_ to do that and this violates the principal of least suprise. Most users won't expect a package to register http handlers just by being imported [citation needed].

A small bit of setup in main (or wherever you're doing your HTTP setup) does not feel inconvenient to me, it feels explicit and readable; it makes it immediately obvious what's happening (whereas otherwise there is no indication from reading a snippet of code that these handlers are being registered).

[peterbourgon]

This feels similarly annoying to when packages register flags in the global flagset as an import side effect. Explicit is better than implicit.

[kr]

If you want explicitness, don't use DefaultServeMux. Once you use it, you're already in implicit-land. (And the whole point of that variable is convenience. Better to go all the way, rather than "sort of a little bit more convenient, but not really".)

[nightlyone]

Multiple suggestions here:

• library writers: simply export the handlers instead of registering them
• application writers: insert http.DefaultServeMux at any convenient point in your router chain.

[SamWhited]

So it's been a few days and comments have trailed off. Who generally arbitrates / decides final behavior in disputes about packages in the x/ tree?

[ianlancetaylor]

This package was originally written by @dsymonds and he seems OK with the current behavior. Assuming it is documented correctly I'm not sure there is any change to make here.

[SamWhited]

Oh, I didn't realize that; yah, fair enough. It still feels poor to me, and I don't fully understand ownership in the x/ tree, but if it's original-author's-call I suppose I don't really have anything else to say that I haven't already said to try and convince him.

If this were ever merged into the standard library I'd certainly want to bring this up again though; it doesn't feel like appropriate behavior to me (and still needs to be better documented).

Thanks for the discussion all.

[SamWhited]

I know I was overruled on this, but I'd like to bring it up again. I'm not having an issue where I'm trying to build an application that imports a vendored version of trace, and also imports a (not-vendored because it's in the same repo using the "repo root is a library with a cmd subdirectory that imports that library" pattern) library that imports trace itself. This causes a panic because both versions of trace call init() and try to register the handler. This seems like a very solid reason not to have this sort of implicit behavior; it leads to unexpected things happening that will just confuse users. Since applications should vendor, and libraries should not, this doesn't seem like that difficult of a situation to get into.

ML discussion in case I'm missing an obvious solution is here: https://groups.google.com/forum/#!topic/golang-nuts/J9BZwJfLnXI

/cc @dsymonds @bradfitz

[bradfitz]

I have not expressed an opinion in this thread and don't wish to do so, but I was summoned so I'll reply to this part:

Since applications should vendor, and libraries should not, this doesn't seem like that difficult of a situation to get into.

How do you get to that conclusion? If an application vendors two version of net/trace and it blows up, the application can fix their vendoring config and move on.

[SamWhited]

have not expressed an opinion in this thread and don't wish to do so

Oops, sorry about that; not sure who i was trying to CC there, maybe I just saw that you closed the issue.

If an application vendors two version of net/trace and it blows up, the application can fix their vendoring config and move on.

In this case the application is _not_ vendoring two versions of net/trace; the application is vendering 1 version, and the library that provides the applications core functionality is not vendoring (and not vendored by the application since it's part of the contract that the application will always be up to date with the library that provides its core functionality).

I suppose one could argue that this is really a problem with vendoring, and not net/trace (if you don't vendor _everything_ you'll run into problems), but it still seems foolish to put things that could panic like this in the init() method of a library; it violates the principle of least suprise (although thankfully in this case it does so loudly, so it's not likely to sneak into prod).

[bradfitz]

Yeah, I suppose you should vendor everything or nothing.