Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go1.7: use TLS renegotiation, rm stdlib forks #375

Merged
merged 2 commits into from
Aug 16, 2016

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Aug 9, 2016

The time for removing the weird ./core/ packages has come at last. Go 1.7 introduces TLS renegotiation: golang/go@af125a5

The actual change is at management/http.go (hidden on GitHub UI due to removing so many files, you have to click on it manually, it's all the way at the end.)

Removing our net/http and crypto/tls forks and just using Renegotation field
in the crypto/tls.Config field of the net/http.Client of the ./management/**
packages. (Fixes #90)

⚠️ ⚠️ ⚠️ Should be merged once go1.7 stable is out.

cc: @paulmey

@paulmey
Copy link
Member

paulmey commented Aug 11, 2016

LGTM, are you going to update the travis file in the same PR?

@ahmetb
Copy link
Contributor Author

ahmetb commented Aug 11, 2016

@paulmey probably should update that separately when it's out (planned Aug 15). then rebase/merge this.

@paulmey
Copy link
Member

paulmey commented Aug 11, 2016

@ahmetalpbalkan makes sense, I hope travis has the new go version available as soon as its released...

ahmetb added 2 commits August 15, 2016 20:49
Go 1.7 introduces TLS renegotiation: golang/go@af125a5

Removing our net/http and crypto/tls forks and just using Renegotation field
in the crypto/tls.Config field of the net/http.Client of the ./management/**
packages. (Fixes Azure#90)

Should be merged once go1.7 stable is out and preferably like 1 month later.
If the customers of the ./management/ package are not vendoring the SDK and using
go versions prior to 1.7, they will get a compilation error.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb ahmetb force-pushed the tls-renegotiation branch from 8cb192f to 8a163b6 Compare August 16, 2016 03:50
@ahmetb
Copy link
Contributor Author

ahmetb commented Aug 16, 2016

@paulmey tests pass now.

@paulmey
Copy link
Member

paulmey commented Aug 16, 2016

awesome sauce. ship it!

@paulmey paulmey merged commit ff26f00 into Azure:master Aug 16, 2016
@ahmetb
Copy link
Contributor Author

ahmetb commented Aug 16, 2016

@masterzen
Copy link

FYI, I understand that it is the right way going forward, but this change is breaking my WinRM library (see masterzen/winrm#60), and prevents users to use Go < 1.7.
As winrm is itself a 'library', it's not really possible to vendor its dependencies.

@colemickens
Copy link
Contributor

You may not be able to (or want to) vendor but you can still lock to a version, allowing you and your users to get a correct set of dependencies.

We do this in azure-sdk-for-go, for example. If you have a project that uses glide and you consume azure-sdk-for-go, you're ensured to get a compatible version of go-autorest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants