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

Release notes for 2.2.0 #146

Merged
merged 2 commits into from May 31, 2017
Merged

Release notes for 2.2.0 #146

merged 2 commits into from May 31, 2017

Conversation

thedrow
Copy link
Member

@thedrow thedrow commented May 22, 2017

These are the release notes for 2.2.0.
This release is going to be used for the upcoming Celery release.
The motivation behind the minor version bump is due to the authentication refactor which adds new capabilities but doesn't break backwards compatibility.

Please review and let me know if I got anything wrong.

@codecov-io
Copy link

codecov-io commented May 22, 2017

Codecov Report

Merging #146 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #146   +/-   ##
=======================================
  Coverage   91.96%   91.96%           
=======================================
  Files          14       14           
  Lines        1656     1656           
  Branches      227      227           
=======================================
  Hits         1523     1523           
  Misses        104      104           
  Partials       29       29

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dbed5a4...9b751dd. Read the comment docs.

Copy link
Member

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

Looks ok on an general perspective.

Copy link

@georgepsarakis georgepsarakis left a comment

Choose a reason for hiding this comment

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

Noted a few things, I don't see other significant changes. Nice work 👍 !

Changelog Outdated
:release-date: TBD
:release-by: Ask Solem

- Fix random delays in task execusion.

Choose a reason for hiding this comment

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

Small type here: execution

Changelog Outdated

- Added support for setting the SNI hostname header.

The SSL protocol version is now set to SSLv23

Choose a reason for hiding this comment

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

I am wondering if this change is a secure and sane default. The documentation states that this constant will be deprecated in 2.7.13 and is actually an alias for ssl.PROTOCOL_TLS.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm wondering the same as well. Should we revert this and recreate a pull request?

@michael-k
Copy link
Contributor

michael-k commented May 22, 2017

I've noticed the same typo and same typo and some whitespace issues. See PR #147
Feel free to squash it into your commit.

@thedrow
Copy link
Member Author

thedrow commented May 23, 2017

@auvipy @georgepsarakis Should we revert and recreate the pull request for SNI support? Do we understand the implications of setting our SSL implementation to SSLv23?
I think it's the safest bet.

@auvipy
Copy link
Member

auvipy commented May 23, 2017

@thedrow go for it as it seems to be safest bet

@georgepsarakis
Copy link

@thedrow I agree with recreating the SNI support PR.

@dhananjaysathe
Copy link
Contributor

Hey so I thought about the compatiblity. Infact you can pass a more specific version as well as a dict in the new code . Sslv23 is the most compatible version of found on a number of devices and architectectures.
https://docs.python.org/2/library/ssl.html#protocol-versions
Here is a more detailed matrix here
https://docs.python.org/2/library/ssl.html#socket-creation

If you look at python 3 protocoltls and sslv23 are alias. It defaults to protocoltls
https://docs.python.org/3/library/ssl.html#socket-creation
https://docs.python.org/3/library/ssl.html#protocol-versions

Here is python 2.6 (I actually found protocoltls to break on an arm board with these old versions)
https://docs.python.org/2.6/library/ssl.html#functions-constants-and-exceptions

If you look at the source across all versions both these alias to integer value 2

That said give me a day or so I'll add more exception handling and checks in place so it has the same behavior across every combination.

@thedrow
Copy link
Member Author

thedrow commented May 26, 2017

Why do we have to set it? It wasn't set before and everything worked as expected.

@dhananjaysathe
Copy link
Contributor

Hello @thedrow and other kind folks
The earlier code called the built-in ssl.wrap_socket method (https://github.com/celery/py-amqp/pull/139/files#diff-e3b09014ce12269dff5eece1f9f7c082L296) src here: this doesn't allow one to pass the needed headers https://github.com/python/cpython/blob/master/Lib/ssl.py#L1131

The workaround to this was manually creating a context dummy options (dict) for the context (https://github.com/dhananjaysathe/py-amqp/blob/4fec0db649679c6e04d3698949e28dd1dbb0527b/amqp/transport.py#L298)and passing it ssl_opts as well so the context.wrap_socket would be called that supported setting the headers. This is tad messy. On the other hand, we could allow passing a ssl.SSLContext object directly. If you want a PR for that I could provide one. Lemme know what you folks think.

Also here is the version compatibility line from CPython src:
https://github.com/python/cpython/blob/master/Lib/ssl.py#L153

also sent a PR with some more checks
#150

@thedrow thedrow merged commit dcd2b56 into master May 31, 2017
@thedrow thedrow deleted the release-2.2 branch May 31, 2017 11:19
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.

None yet

6 participants