Eliminate stdlib forks living at core/

[ahmetb]

core/ currently has net/http and crypto/tls vendored (with some changes we don't entirely know). We must watch out if we can make feature requests to golang about these and eventually get rid of those (nobody will vendor our entire repo like this) 😄

[paulmey]

👍

[ahmetb]

@paulmey can you dump here what you know about those (i.e. why we have them) it'd be nice to know where exactly we modified if we can.

[ruslangabitov]

There was an issue about that, I left my comment there: #5

Copying my answer here:
Standard Go TLS library does not support TLS renegotiation. There is a patch for this issue, but it is not included into current Go release, and looks like it will not even make it into 1.4 or 1.5.

Patch:
https://gist.github.com/ruslangabitov/951ea755a3b76aa3f80f

You can find more information about this issue here:
https://code.google.com/p/go/issues/detail?id=5742

@ahmetalpbalkan @paulmey hope it helps.

[ahmetb]

@hopetobelievein sure it will, thanks! Was that the only patch for vendoring crypto/tls package here?

Any ideas about net/http?

[ruslangabitov]

Yes, that is the only thing that changed, plus references to changed packages.
I've added net/http because it has references to crypto/tls in some places:
https://github.com/MSOpenTech/azure-sdk-for-go/blob/master/core/http/response.go
https://github.com/MSOpenTech/azure-sdk-for-go/blob/master/core/http/transport.go
So final code in ./management/http.go uses changed net/http which uses patched crypto/tls.

[ahmetb]

@hopetobelievein thanks for clarifications. it took us some time in the past to dig deep and understand what's the diff'ed part.

[mvanotti]

Hi!

Is it possible to add the patch as a separate commit? That way the differences are more visible, and also it adds more context.

It would be: One commit with the changes of the original repo (to revert everything), and one commit adding the patch and some context.

[paulmey]

Well, that history is sorta gone. This project was passed around a bit before ending up here. From my own archeology, the tls package looks like go1.2: (full diff)

paulmey@paulmey-z800:~/src/tmp-golang$ git co go1.2
HEAD is now at 402d359... go1.2
paulmey@paulmey-z800:~/src/tmp-golang$ diff -rw ~/src/go/src/github.com/Azure/azure-sdk-for-go/core/tls/ src/pkg/crypto/tls/|diffstat
 common.go             |    1 -
 conn.go               |   31 +------------------------------
 handshake_messages.go |   11 -----------
 3 files changed, 1 insertion(+), 42 deletions(-)

The http package looks like go1.3:

paulmey@paulmey-z800:~/src/tmp-golang$ git co go1.3
Previous HEAD position was 402d359... go1.2
HEAD is now at 1cdd48c... go1.3
paulmey@paulmey-z800:~/src/tmp-golang$ diff -rw ~/src/go/src/github.com/Azure/azure-sdk-for-go/core/http/ src/pkg/net/http/|diffstat 
 response.go       |    2 +-
 transport.go      |    2 +-
 transport_test.go |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Note that the only changes to the http package are to reference the modified tls package.

[ahmetb]

Heads up go1.7 probably will ship with TLS renegotation support golang/go#5742 (comment) and we confirmed that fix works with Azure Service Management REST APIs. So we'll finally have a way to fix forks of crypto/tls and net/http. 🎉

[colemickens]

Are we comfortable saying azure-sdk-for-go is only compatible with the very latest Go compiler/stdlib? (Not that I'm against removing unneeded code...)

[ahmetb]

@colemickens well first of all we expect all users to use vendoring, they always can go back. second is, this is management/** packages only. so ARM users won't get broken with this.

[paulmey]

👍 very much in favor of dropping /core/

[ahmetb]

🔜 go1.7 will come out.