api: add SNIHostName to Info #6407

rogpeppe · Oct 7, 2016

This means that we can have an arbitrary API addresses
in api.Info, including resolved IP addresses, but still connect
using the officially signed certificate to check. If there's
a private Juju CA cert available, we use that by preference
to make it possible to connect even if the server cannot
obtain an officially signed certificate.

kat-co · Oct 7, 2016

kat-co suggested changes Oct 7, 2016

View changes

We need to instead take this in as a dependency to the call-chain.

This is something that already exists. I would prefer not to have to fix everything just because it's been touched.

Also, as expressed in our chat online, this is very much an implementation detail - the tests that use this
are checking that some particular logic has been wired up correctly. It wouldn't make sense for external
code to swap out this particular implementation detail.

If we were to make this an external dependency, I'd be more inclined to make the dependency
a higher level thing (something that returns an rpc.Codec for example), but that's something that
would better suit a separate PR, I think.

I don't think that's really necessary for this change. If this were all new code, it would be worth talking about, but this is really just a small change to existing code. Reworking how it does all this stuff is not a good use of our time, IMO.

To address your comment, @natefinch: "broken windows principle". We must start addressing our technical debt, or risk being crushed by it. I think this is also a decision we've made: moving forward, we fix things as we touch them.

Also, exposing one new function which takes in a dialer would not be a very invasive change and would allow us to remove the patched global variables.

To address your comment, @rogpeppe: I agree, usually exposing these kinds of things reveals a higher-level abstraction which has been missing all along. Probably not worth adressing that in this PR, but it doesn't have to be all or nothing.

I got most of the way to doing a higher level abstraction, but it ended up quite intrusive because
we don't just dial websockets for a JSON codec - we also use them for streaming,
so the interface required grew until I couldn't really justify it in terms of cost/benefit.

If we want to change the websocket implementation, we'll be much better just doing it
internally - it really feels to me like an internal detail of the api package (which is
why I don't like exposing that implementation package as part of the public
API as I am now doing).

I think this is also a decision we've made: moving forward, we fix things as we touch them.

For me "touching" something doesn't just involve a mechanical change to that thing, but actually
involves changing something substantial about the code, otherwise you could say, for example,
that changing an import path "touched" every file that was changed and therefore a PR that
changes an import path needs to fix every file that uses that import.

I have been doing my best to fix any tech debt I've been encountering in code that I change - I
believe strongly in leaving code cleaner than I find it, but I also think that it's really important
to maintain focus in every PR as far as possible (and, yes, I do break that rule
sometimes, usually when rewinding the change to make it independent is going to take lots
of time).

Is ensuring the proxy can connect a precondition for the real test? If so, I don't think we need this check.

If not, we need to split this test into two.

This is an existing test that I've just changed because I renamed some things. If we want to fix it, it would be better to do in another PR, if that's OK.

Also going to claim "broken windows principle" here. I'm still not sure if it's a precondition or not, but either way, it would be a small change to remove that line or split the test into two.

No, really, let's please not make multiple independent changes in one PR just because the code happens to be visible in the PR. It's important that tests remain unchanged when possible so that we know that the change hasn't broken them (of course, it's not always possible to leave them unchanged, but the principle remains).

Fair enough. Please create another PR with this comment addressed and ping me for review.

Having just gone through fixing a bunch of tests because the error message is now prepended with something different, please modify this to only check the relevant portion of the error message.

Again, this is existing code. I'd prefer to avoid mixing too many concerns in this PR.

And although I feel your pain (and I've had the same issue before), I have also had problems when error messages weren't checked properly. I think that, on balance, although it's a pain to change the error messages, it's also good to see in the tests what the actual message will look like (to some degree of accuracy), as that's often the only time you'll get to check that the user will see an appropriate message in an often-rare case.

That is, error messages are important and it's good to see what they actually look like. I've often made changes to error messages because I've asserted them in the tests and they look too awkward.

If we feel it's important to check the entirety of the error message, that should be done in its own test so we don't repeat the check in every test and make our suite brittle.

That's probably a good idea, but I'm not in the business of fixing every other test with this issue in this PR. If I was fixing this, I'd also fix the other places that have a similar error message, and that may well make the PR considerably bigger and certainly less focused.

For the record, I'm not requesting that you fix every test, just modify the error message in the test you're fixing. You mentioned wanting to check the entire error message, so I suggested a new test to check just that.

Pursuant to your previous comment about not wanting test-fixes in this PR, please open another PR with the test modified and ping me for review.

I'm not sure that a mechanical rename really counts as "fixing" a test. I did a global search and replace - I didn't cast my eye on this test for a moment. There's a class of problem you've identified: some error messages in tests are more specific than would be ideal. A decent way to fix that is not, in my view, to change an error message because it's visible in a particular code review, but to make a PR specifically to address the issue - that way the tests remain self-consistent. Otherwise someone coming in later might be confused as to which precedent to follow to remain consistent with the tests in the rest of the package. I think consistency is a worthy aim.

What is the expected behavior of this test? Should be in the name or at least a comment.

done

Sorry, @rogpeppe. It's probably because I don't know how to use GitHub reviews yet, but did this name change? The name I see is TestOpenWithSNIHostNameEmptyCert which clearly enumerates what's being tested, but gives me no clue what behavior I should expect.

Same comment for the other tests.

You suggested "in the name or at least a comment". I added a comment (L170). I guess I could name the test TestOpenWithSNIHostNameEmptyCertAlwaysUsesSNIName, but I thought the comment should be ok.

Oh, OK. Sorry, GitHub reviews make it difficult to see what's changed when commits are squashed.

Do you think the comment should by on the method and not within the test?

What is the expected behavior of this test? Should be in the name or at least a comment.

done

As mentioned earlier, we need to get rid of the patching.

Done

I think we can make this test a little easier to read through by inverting what you're doing here:

Create a timeout channel that your fake NewWebsocketDialer can close over (or somehow use the one we're ignoring on L212 -- actually, why isn't that bubbling up to the API we're interacting with?).
In your anonymous goroutine, do sleep for jtesting.LongWait and then close the timeout channel (or maybe just fail the test outright?). This is semantically different in that we have one timeout for all sends, but 10*time.Second should be long enough for all sends.
Invert this for loop to range over dialed

This way I think you can get rid of both select blocks and we're looking at a simple for loop.

Won't that cause the test to take 10 seconds to run?
Also, there are (at least) two goroutines in play - they can't both close the timeout channel.

Perhaps I've misunderstood though - could you explain with some (pseudo)code?

Sure, sorry it wasn't more clear:

func (s *apiclientSuite) testSNIHostName(c *gc.C, info *api.Info, expectDials []apiDialInfo) {
    timeout := time.After(jtesting.LongWait)
    dialed := make(chan *websocket.Config)

    fakeDialer := func(cfg *websocket.Config, opts api.DialOpts) func(<-chan struct{}) (io.Closer, error) {
        return func(<-chan struct{}) (io.Closer, error) {
            select {
            case <-timeout:
                // Consider panicing... error doesn't make it all the
                // way up to returning from api.Open.
                return nil, errors.New("timeout")
            case dialed <- cfg:
            }
            return nil, errors.New("nope")
        }
    }
    s.PatchValue(api.NewWebsocketDialer, fakeDialer)
    go func() {
        defer close(dialed)
        conn, err := api.Open(info, api.DialOpts{})
        c.Check(conn, jc.ErrorIsNil)
        c.Check(err, gc.ErrorMatches, `nope`)
    }()

    i := 0
    for cfg := range dialed {
        c.Check(cfg.Location.String(), gc.Equals, expectDials[i].location)
        c.Assert(cfg.TlsConfig, gc.NotNil)
        c.Check(cfg.TlsConfig.RootCAs != nil, gc.Equals, expectDials[i].hasRootCAs)
        c.Check(cfg.TlsConfig.ServerName, gc.Equals, expectDials[i].serverName)
        i++
    }

    c.Check(i, gc.Equals, len(expectDials))
}

Ah I see now, clever, thanks! We know we can close the channel after api.Open returns because each attempt returns an error so we can be sure that they're all tried before api.Open completes.
I don't think the timeout channel is necessary at all - if things go wrong enough that the function can't send on dialed, we'll get a panic not a timeout because the channel will have been closed.

Then we end up with something like this, which is definitely much nicer, thanks again.

func (s *apiclientSuite) testSNIHostName(c *gc.C, info *api.Info, expectDials []apiDialInfo) {
    dialed := make(chan *websocket.Config)
    fakeDialer := func(cfg *websocket.Config, opts api.DialOpts) func(<-chan struct{}) (io.Closer, error) {
        return func(<-chan struct{}) (io.Closer, error) {
            dialed <- cfg
            return nil, errors.New("nope")
        }
    }
    s.PatchValue(api.NewWebsocketDialer, fakeDialer)
    go func() {
        conn, err := api.Open(info, api.DialOpts{})
        c.Check(conn, jc.ErrorIsNil)
        c.Check(err, gc.ErrorMatches, `nope`)
        // Because all the dials return an error, we're sure that
        // all the addresses will have been tried (and the values
        // sent on dialed) before api.Open returns, so it's safe
        // to close the channel now.
        close(dialed)
    }()

    i := 0
    for cfg := range dialed {
        expect := expectDials[i]
        c.Check(cfg.Location.String(), gc.Equals, expect.location)
        c.Assert(cfg.TlsConfig, gc.NotNil)
        c.Check(cfg.TlsConfig.RootCAs != nil, gc.Equals, expect.hasRootCAs)
        c.Check(cfg.TlsConfig.ServerName, gc.Equals, expect.serverName)
        i++
    }

    c.Check(i, gc.Equals, len(expectDials))
}

After changing the test and making it work, I'm no longer so sure it's better.
I think the new version is more brittle - it presupposes more about the internals
of the code being tested and will fail in a not-very-nice way (panic or hang forwever) if that
doesn't behave as expected.

I think the new version imposes somewhat more cognitive load, as the reader needs
to figure out why it's OK to close the channel.

The old version is longer, it's true, but has a more obvious failure mode - every
receive is guarded by a select so it's immediately clear that the test can't hang
forever, and there's no implicit ordering constraint between the call to the dial
function and the return from api.Open.

I think I'd prefer to stick with the original if that's OK.

I think with my version it won't hang forever as the timeout is for the entire test, not each send, and panics in tests don't bother me one bit :)

Which way you go is definitely up to you, but FWIW I think the clean-reading test is worth it! I just find the multiple select statements to obfuscate the intent of the test.

I think we can change these to c.Check.

L228 still needs to be an Assert but otherwise, done.

rogpeppe · Oct 7, 2016

rogpeppe reviewed Oct 7, 2016

View changes

Thanks for the review, @kat-co. I have a few comments in response.