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

pkg/transport: don't set certificates on tls config #9542

Closed
wants to merge 1 commit into from
Closed

pkg/transport: don't set certificates on tls config #9542

wants to merge 1 commit into from

Conversation

roboll
Copy link
Contributor

@roboll roboll commented Apr 6, 2018

Fixes #9541

@gyuho
Copy link
Contributor

gyuho commented Apr 6, 2018

Can you confirm this fixes #9541?

@roboll
Copy link
Contributor Author

roboll commented Apr 6, 2018

Yeah in my tests here it definitely fixes the issue. I haven't looked at the CI failures yet.

@gyuho
Copy link
Contributor

gyuho commented Apr 6, 2018

Our TLS reload was introduced since 3.2 and the go runtime logic works that way with Go 1.8 as well. So, we will backport this to 3.2 and 3.3.

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. thanks!

will merge after CI greens.

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this somehow breaks TLS tests for certs with SAN field. Will take another look next week.

@roboll
Copy link
Contributor Author

roboll commented Apr 6, 2018

👍 Let me know what you need from me.

@roboll
Copy link
Contributor Author

roboll commented Apr 10, 2018

hey @gyuho anything I can do to help out here?

@gyuho
Copy link
Contributor

gyuho commented Apr 10, 2018

@roboll Sorry, I looked into it and found this breaks other TLS reload tests. But, still think this is the right approach. Just want to take some time to understand how Go TLS works with this change, and fix the test failures. I had to work on something else, but I should be able to get back to this by this week and plan is release this patch by next week. I will give you more updates as I investigate further.

@roboll
Copy link
Contributor Author

roboll commented Apr 10, 2018

@gyuho sounds good, ping me if you need a hand otherwise I'll check back in a few days 👍.

@gyuho
Copy link
Contributor

gyuho commented Apr 13, 2018

Test failures happen in our integration testing, where Go httptest defaults to its internal test certs if initial Certificates is empty as with this patch. Will see if we can work around it.

@gyuho
Copy link
Contributor

gyuho commented Apr 13, 2018

@roboll Can you try #9570? Just cherry-picked your commit with test fixes. Thanks!

@gyuho gyuho closed this Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants