Skip to content

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

Closed
roboll wants to merge 1 commit intoetcd-io:masterfrom
roboll:tls-reload
Closed

pkg/transport: don't set certificates on tls config#9542
roboll wants to merge 1 commit intoetcd-io:masterfrom
roboll:tls-reload

Conversation

@roboll
Copy link
Copy Markdown
Contributor

@roboll roboll commented Apr 6, 2018

Fixes #9541

@gyuho
Copy link
Copy Markdown
Contributor

gyuho commented Apr 6, 2018

Can you confirm this fixes #9541?

@roboll
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

roboll commented Apr 6, 2018

👍 Let me know what you need from me.

@roboll
Copy link
Copy Markdown
Contributor Author

roboll commented Apr 10, 2018

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

@gyuho
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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