Allow passing ALPN next protocols down to connect services. Fixes #4466.

[apollo13]

@banks This should cover the technical aspects of the PR. I am not to happy with the pointer to the nextProto array, but I am not sure how we could pass the default nicely otherwise…

[banks]

Thanks for the super quick work!

I realise you're still working on this as the proxy.go change isn't here yet but some minor early feedback before I'm out for the day.

I agree the pointer to string slice is a little odd. It seems like that can just be changed though right? Slices in go are already "pointer" types that can be nil without explicitly pointing to them. Was there something I missed that made it tricky to just use a slice?

[apollo13]

I realise you're still working on this as the proxy.go change isn't here yet but some minor early feedback before I'm out for the day.

Well not exactly, proxy.go calls into proxy/config.go which is where I changed the call to use the config.

Was there something I missed that made it tricky to just use a slice?

I do not "know" go, I will read up on slices and try to get it to work.

Thanks for the early feedback, I posted the PR early on because I didn't want to go into the wrong direction…

[apollo13]

It would be great if one could copy the CI failure out of circle-ci for me. I am not going to login to circle ci when it wants access to my private repos…

Locally make test yields:

make test
--> Checking for other consul instances
==> Building Consul - OSes: linux, Architectures: amd64
Building sequentially with go install
--->   linux/amd64
--> Running go golangci-lint
testutil/server_methods.go:235:7: SA5011: possible nil pointer dereference (staticcheck)
	if s.Config == nil {
	     ^
testutil/server_methods.go:238:7: SA5011: possible nil pointer dereference (staticcheck)
	if s.Config.Ports == nil {
	     ^
testutil/server_methods.go:241:7: SA5011: possible nil pointer dereference (staticcheck)
	if s.Config.Ports.HTTP == 0 {
	     ^
make: *** [GNUmakefile:301: lint] Error 1

but that is unrelated to my changes.

[jsosulska]

Hey @apollo13 - Here's the errors I collected. The first error doesn't provide any feedback. The second is a bit more clear.

# TestACLEndpoint_Login_with_TokenLocality

=== CONT  TestACLEndpoint_Login_with_TokenLocality

At the start of the second test, we have:

=== RUN   TestCoordinate_Update_ACLDeny
=== PAUSE TestCoordinate_Update_ACLDeny
=== CONT  TestCoordinate_Update_ACLDeny
[WARN] freeport: 7 out of 14 pending ports are still in use; something probably didn't wait around for the port to be closed!
[WARN] freeport: 7 out of 7 pending ports are still in use; something probably didn't wait around for the port to be closed!```

And then a bit later on:

## TestCoordinate_Update_ACLDeny

=== CONT  TestCoordinate_Update_ACLDeny
    testagent.go:102: Error while waiting for test agent to start: unavailable. last error: Catalog.ListNodes failed: ACL not found
    retry.go:178: testagent.go:103: TestAgent: unavailable. last error: Catalog.ListNodes failed: ACL not found
        testagent.go:103: TestAgent already started

Hope this helps!

[apollo13]

@jsosulska Thanks, can't say it helps much though. I cannot imagine that those are triggered by my changes and cannot reproduce it locally either :/ (I removed the linting check so I can actually run make test)

[banks]

@apollo13 yes those CI failures appear to be unrelated. I'll double check again before we merge of course but we do sadly still have flaky tests some times.

[banks]

@apollo13 thanks so much for getting this change in so quickly!

I think this is pretty close to mergeable. The one thing missing as the coverage checker pointed out is that we there are no tests that validate the new behaviour. It's a small plumbing change and a little awkward to test but it would be ideal to have tests to ensure we don't regress this again.

I think the simplest approach would be to just test end-to-end in the proxy package - perhaps with another test along side TestPublicListener in listener.go. That test didn't fail before I think because it's only running a TCP test server.

If we had a second test (TestPublicListenerHTTP?) that used httptest.Server as the "application", ran the proxy as in the existing test but then used svc.HTTPClient to connect to it, I think that should reproduce the failure outlined in the original ticket without this fix. That would then be a good test that the built in proxy is no longer advertising h2 and would still be relatively simpler/cheap to run.

Do you think that's something you'd be able to look into to get this landed?

Thanks again for your effort here!

[apollo13]

@banks I did adjust the existing proxy e2e test to verify that ALPN was not used. I also double checked this by "reverting" parts of my fix so that the proxy would accept h2 via ALPN. Since the changes are rather minimal I just adjusted the test, a new one would have been a 90% duplication.

I did not manage to do the listener test. This is mainly because those use connect.TestService which has it's own TestTLSConfig and as such circumvents most of the things I would like to test. If it is okay for you I'd leave those out.

[banks]

Amazing job @apollo13, this looks great to me.

I had a minor suggestion on the test comment wording if you agree and want to use that suggestion or modify it yourself somehow I'll hold of merging it until you do, but it's a nitpick so I'll approve now anyway!

[apollo13]

Jupp, the wording change looks good.

[banks]

Thanks so much for getting this fix in @apollo13!

[banks]

This will be part of Consul 1.10 which will be released in roughly a month or two.