Eliminate stdlib forks living at core/ about azure-sdk-for-go HOT 13 CLOSED

👍

from azure-sdk-for-go.

@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.

from azure-sdk-for-go.

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.

from azure-sdk-for-go.

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

Any ideas about net/http?

from azure-sdk-for-go.

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.

from azure-sdk-for-go.

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

from azure-sdk-for-go.

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.

from azure-sdk-for-go.

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.

from azure-sdk-for-go.

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. 🎉

from azure-sdk-for-go.

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...)

from azure-sdk-for-go.

@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.

from azure-sdk-for-go.

👍 very much in favor of dropping /core/

from azure-sdk-for-go.

🔜 go1.7 will come out.

from azure-sdk-for-go.