FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots by pablogsal · Pull Request #10011 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots - #10011

Merged
pablogsal merged 3 commits into
python:masterfrom
pablogsal:bpo35031
Oct 29, 2018
ConversationCommits (3)ChecksFiles changed
Merged

bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots#10011
pablogsal merged 3 commits into
python:masterfrom
pablogsal:bpo35031

Conversation

pablogsal commented Oct 20, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

pablogsal requested a review from 1st1 October 20, 2018 17:00
bedevere-bot added tests Tests in the Lib/test dir awaiting merge labels Oct 20, 2018

pablogsal commented Oct 20, 2018

Copy link
Copy Markdown
Member Author

Tested on the buildbot itself:

CURRENT-amd64% uname -a
FreeBSD CURRENT-amd64 12.0-ALPHA10 FreeBSD 12.0-ALPHA10 r339405 GENERIC-NODEBUG  amd64
CURRENT-amd64% ./python -m test test_asyncio -m test_start_tls_server_1 -v
== CPython 3.8.0a0 (tags/v3.7.0a4-747-g35230d08e0:35230d08e0, Oct 21 2018, 03:49:42) [Clang 6.0.1 (tags/RELEASE_601/final 335540)]
== FreeBSD-12.0-ALPHA10-amd64-64bit-ELF little-endian
== cwd: /usr/home/pablo/cpython/build/test_python_32337
== CPU count: 4
== encodings: locale=UTF-8, FS=utf-8
Run tests sequentially
0:00:00 load avg: 3.63 [1/1] test_asyncio
test_start_tls_server_1 (test.test_asyncio.test_sslproto.ProactorStartTLSTests) ... skipped 'Windows only'
test_start_tls_server_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.424s

OK (skipped=1)
1 test OK.

Total duration: 1 sec
Tests result: SUCCESS
CURRENT-amd64%

pablogsal requested a review from asvetlov October 20, 2018 17:01
pablogsal requested a review from vstinner October 25, 2018 22:08

pablogsal commented Oct 25, 2018

Copy link
Copy Markdown
Member Author

Example of the current failure:

https://buildbot.python.org/all/#/builders/173/builds/57

vstinner requested a review from tiran October 25, 2018 23:47
pablogsal mentioned this pull request Oct 26, 2018
Merged
vstinner approved these changes Oct 29, 2018
View reviewed changes

vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.

Honestly, I'm not a TLS expert, but this bug is annoying and makes the buildbot fail randomly. So I suggest to apply it as soon as possible :-)

If it's wrong, it can be fixed later, but at least the buildbot will become useful again and stop to spam buildbot-status and random PRs.

1st1 reviewed Oct 29, 2018
View reviewed changes
Comment thread Lib/test/test_asyncio/test_sslproto.py Outdated

server_context = test_utils.simple_server_sslcontext()
client_context = test_utils.simple_client_sslcontext()
client_context.options |= ssl.OP_NO_TLSv1_3

1st1 Oct 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we add a comment explaining this line?

pablogsal Oct 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done in 93e66f6

tiran Oct 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we please restrict this to FreeBSD?

@1st1 In the long run, we should have parametrized tests to test TLS 1.2 and TLS 1.3 behavior. The protocols behave differently on mulitple accounts. For client cert auth, we even need to have TLS 1.3 with and without PHA.

pablogsal Oct 29, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done in f777fa5

pablogsal self-assigned this Oct 29, 2018
tiran reviewed Oct 29, 2018
View reviewed changes

tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please limit ssl.OP_NO_TLSv1_3 to FreeBSD for now.

Comment thread Lib/test/test_asyncio/test_sslproto.py Outdated

server_context = test_utils.simple_server_sslcontext()
client_context = test_utils.simple_client_sslcontext()
client_context.options |= ssl.OP_NO_TLSv1_3

tiran Oct 29, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can we please restrict this to FreeBSD?

@1st1 In the long run, we should have parametrized tests to test TLS 1.2 and TLS 1.3 behavior. The protocols behave differently on mulitple accounts. For client cert auth, we even need to have TLS 1.3 with and without PHA.

pablogsal commented Oct 29, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@tiran I restricted the fix for FreeBSD in commit f777fa5.

pablogsal merged commit f6a47f3 into python:master Oct 29, 2018
pablogsal deleted the bpo35031 branch October 29, 2018 20:47

miss-islington commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

Thanks @pablogsal for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

Sorry, @pablogsal, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f6a47f3e316cede2a07a1f74a509f6d80ab8fef0 3.6

bedevere-bot commented Nov 13, 2018

Copy link
Copy Markdown

GH-10496 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 13, 2018
bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots (pythonGH…
7aef645 
…-10011)

Some FreeBSD buildbots fail to run this test as the eof was not being received by the server if the size is not big enough. This behaviour only appears if the client is using TLS1.3.
(cherry picked from commit f6a47f3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>

pablogsal commented Nov 13, 2018

Copy link
Copy Markdown
Member Author

Backporting after checking that it works on the buildbots.

pablogsal commented Nov 13, 2018

Copy link
Copy Markdown
Member Author

Apparently, the issue in 3.6 is different. It happens on test_ssl. Example:

https://buildbot.python.org/all/#/builders/172/builds/86/steps/4/logs/stdio

miss-islington added a commit that referenced this pull request Nov 13, 2018
bpo-35031: Fix test_start_tls_server_1 on FreeBSD buildbots (GH-10011)
3f9fe53 
Some FreeBSD buildbots fail to run this test as the eof was not being received by the server if the size is not big enough. This behaviour only appears if the client is using TLS1.3.
(cherry picked from commit f6a47f3)

Co-authored-by: Pablo Galindo <Pablogsal@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1st1 1st1 left review comments
tiran tiran left review comments
vstinner vstinner approved these changes
asvetlov Awaiting requested review from asvetlov

Assignees

pablogsal

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL