Add SSL related params to ClientSession.request #2184

cecton · 2017-08-09T15:48:51Z

What do these changes do?

...

Are there changes in behavior for the user?

...

Related issue number

#1128

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the changes folder
- name it <issue_id>.<type> for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

cecton · 2017-08-09T16:03:19Z

Having issues with too much mocking.... if someone has any advice?

asvetlov · 2017-08-09T18:22:03Z

I suggest the following:

Make a self signed certificate and push it into tests (or just reuse tests/sample.crt if it's good for you).
Write functional tests using real app started with self signed certs.

Feel free to ask if my recommendation is not clean.

P.S.
Very thank you for picking up the issue and working on implementation.

asvetlov · 2017-08-09T18:29:48Z

P.P.S.
Side note: as committer you are free to drop github template for new issues and PRs.
These templates are created for helping newbies to file PR properly.
You know our rules already and might use free form for issue/PR description.
For example PR checklist could be skipped if you are keeping the list in your memory.
The only requirement: if PR is related to existing issue please mention it in description text -- it simplifies navigation between issue and PR.

asvetlov · 2017-08-09T18:32:30Z

Obviously it's up to you: use free form or file all template fields if template is more comfortable to you.
Personally I prefer free form for both.

asvetlov · 2017-08-09T18:34:53Z

Regarding to proxy tests: take a look on tests/proxy_functional.py design.

cecton · 2017-08-10T08:43:48Z

@asvetlov PR template is very fine for me :) it helped me a lot to not forget anything (documentation and change logs especially) but thx

I will have to change a few (all?) tests that mock the entire ClientRequest class. This thing is settings properties in the ClientRequest that I don't want.

asvetlov · 2017-08-10T08:58:06Z

Are you talking about proxy tests which should be affected by the PR, not fingerprint related tests, right?

In this case please don't hesitate to update all mocks if method signatures changed. I see no way to do it better.

cecton · 2017-08-10T09:55:13Z

Yes at this point the code I did should have kept the original behavior intact and the tests should pass unchanged. But 3 of the 4 failures are due to the fact that the mocking is actually giving a fingerprint to the ClientRequest objects.

asvetlov · 2017-08-10T12:06:52Z

Please update failed mocks, it's totally correct.

asvetlov · 2017-09-06T16:09:28Z

@cecton would you continue work on the PR?
I have a feeling that we need a new aiohttp release, will dedicate a full time week to lift up pending issues.

cecton · 2017-09-06T16:22:28Z

@asvetlov it shouldn't have take that much time but I got stuck with the mocking thing and then I had a lot of changes recently (change of project basically).

I was going to ask for help with the tests actually.

codecov-io · 2017-09-10T09:26:14Z

Codecov Report

Merging #2184 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2184      +/-   ##
==========================================
+ Coverage   97.31%   97.33%   +0.01%     
==========================================
  Files          39       39              
  Lines        7945     7993      +48     
  Branches     1378     1387       +9     
==========================================
+ Hits         7732     7780      +48     
  Misses         90       90              
  Partials      123      123

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.35% <100%> (+0.14%)`	⬆️
aiohttp/connector.py	`97.76% <100%> (+0.1%)`	⬆️
tests/autobahn/client.py	`96.85% <0%> (ø)`	⬆️

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 a5ee9f2...fb470f6. Read the comment docs.

cecton · 2017-09-10T11:30:16Z

@asvetlov okay ready for review

asvetlov · 2017-09-12T16:46:57Z

Looks good. Feel free to merge, while I very appreciate full test coverage.

cecton · 2017-09-12T16:53:04Z

@asvetlov Thanks I will improve that! I thought I had all the cases but I hadn't check the diff on codecov.

asvetlov · 2017-09-12T17:09:19Z

Just a hint: install codecov browser extension: https://docs.codecov.io/v4.3.6/docs/browser-extension
It allows to see coverage just in github 'Files Changes' tab.
If you'll update contributors instruction to mention the plugin -- it would be awesome!

jettify · 2017-09-13T09:21:25Z

that is nice trick!

kxepal · 2017-09-13T13:08:50Z

aiohttp/client_reqrep.py

+            if not hashfunc:
+                raise ValueError('fingerprint has invalid length')
+            elif hashfunc is md5 or hashfunc is sha1:
+                warnings.simplefilter('always')


Wouldn't this cause suddenly global side effect?

I have no idea. I copy-pasted that from there a610cf1

I think you're right. After this the "warning filter specifications" is altered. https://docs.python.org/3.5/library/warnings.html#warnings.simplefilter @asvetlov can you confirm you want to keep this behavior?

No.
@kxepal good catch.
Please don't alter warnings filter but duplicate the message to aiohttp.log.client_logger.
From my experience people are much more sensible to log messages than deprecation warnings.
Log level should be `warning' sure.

I suppose I change the one in connector too? ^_^

asvetlov · 2017-09-13T17:10:58Z

@jettify

that is nice trick!

What are you talking about?

asvetlov · 2017-09-13T17:18:08Z

aiohttp/client_reqrep.py

+            if not hashfunc:
+                raise ValueError('fingerprint has invalid length')
+            elif hashfunc is md5 or hashfunc is sha1:
+                warnings.simplefilter('always')


No.
@kxepal good catch.
Please don't alter warnings filter but duplicate the message to aiohttp.log.client_logger.
From my experience people are much more sensible to log messages than deprecation warnings.
Log level should be `warning' sure.

jettify · 2017-09-13T17:36:38Z

@asvetlov codecov browser extension, very usefull

asvetlov · 2017-09-13T17:39:48Z

@jettify thanks

cecton · 2017-09-14T08:12:19Z

@asvetlov I just saw that the original ticket mention a "move" of those parameters. She I implement a deprecation for the using the parameter in the connector?

asvetlov · 2017-09-14T08:34:57Z

I not sure.
Obviously there is a need for per-request ssl configuration but I don't know is deprecation makes sense.
Maybe yes.
Guys what do you think?

cecton · 2017-09-14T09:30:37Z

@asvetlov the fingerprint is something you will get per server basis so per request makes sense. On the other hand the connector has the argument fingerprint like it would connect always to the same server. To me it doesn't make any sense. Unless maybe for a UNIX socket?

verify_ssl and ssl_context could make sense because you may want to disable the entire SSL checking for a complete ClientSession.

asvetlov · 2017-09-15T14:18:01Z

Make sense

asvetlov · 2017-09-15T14:18:21Z

Thank you!

lock · 2019-10-28T18:03:56Z

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

cecton mentioned this pull request Aug 9, 2017

Feature request: accept a set of fingerprints in TCPConnector #1965

Closed

cecton force-pushed the ssl-parms-in-request branch from 06f5bbc to fd1547b Compare August 11, 2017 11:47

cecton force-pushed the ssl-parms-in-request branch 2 times, most recently from 0a50695 to 3ba69a5 Compare September 2, 2017 15:04

cecton force-pushed the ssl-parms-in-request branch 2 times, most recently from 90b624b to fc19160 Compare September 10, 2017 09:05

cecton force-pushed the ssl-parms-in-request branch 3 times, most recently from 08433b4 to 7f0948c Compare September 10, 2017 11:28

asvetlov changed the title ~~WIP Add SSL related params to ClientSession.request~~ Add SSL related params to ClientSession.request Sep 10, 2017

asvetlov previously approved these changes Sep 12, 2017

View reviewed changes

cecton dismissed asvetlov’s stale review via b844b26 September 12, 2017 18:30

cecton force-pushed the ssl-parms-in-request branch from 0b08900 to b844b26 Compare September 12, 2017 18:30

cecton force-pushed the ssl-parms-in-request branch 2 times, most recently from 953118e to 69bf0a5 Compare September 13, 2017 12:43

kxepal reviewed Sep 13, 2017

View reviewed changes

cecton force-pushed the ssl-parms-in-request branch from 69bf0a5 to 05890fa Compare September 13, 2017 13:30

asvetlov requested changes Sep 13, 2017

View reviewed changes

cecton force-pushed the ssl-parms-in-request branch 3 times, most recently from b07beb3 to 1143a08 Compare September 14, 2017 08:10

Fix do not modify user's warnings filter

6f978e5

cecton force-pushed the ssl-parms-in-request branch from 6f0c650 to 5fe5d46 Compare September 14, 2017 10:28

Add SSL related params to ClientSession.request

fb470f6

cecton force-pushed the ssl-parms-in-request branch from 5fe5d46 to fb470f6 Compare September 14, 2017 10:29

asvetlov approved these changes Sep 15, 2017

View reviewed changes

asvetlov merged commit 6ccf1b9 into aio-libs:master Sep 15, 2017

cecton deleted the ssl-parms-in-request branch September 15, 2017 18:12

asvetlov mentioned this pull request Sep 15, 2017

Move ssl-related params from TCPConnector to request args #1128

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SSL related params to ClientSession.request #2184

Add SSL related params to ClientSession.request #2184

cecton commented Aug 9, 2017 •

edited

Loading

cecton commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

cecton commented Aug 10, 2017

asvetlov commented Aug 10, 2017

cecton commented Aug 10, 2017

asvetlov commented Aug 10, 2017

asvetlov commented Sep 6, 2017

cecton commented Sep 6, 2017

codecov-io commented Sep 10, 2017 •

edited

Loading

cecton commented Sep 10, 2017

asvetlov commented Sep 12, 2017

cecton commented Sep 12, 2017

asvetlov commented Sep 12, 2017

jettify commented Sep 13, 2017

kxepal Sep 13, 2017

cecton Sep 13, 2017

cecton Sep 13, 2017

asvetlov Sep 13, 2017

cecton Sep 13, 2017

asvetlov commented Sep 13, 2017

asvetlov Sep 13, 2017

jettify commented Sep 13, 2017

asvetlov commented Sep 13, 2017

cecton commented Sep 14, 2017

asvetlov commented Sep 14, 2017

cecton commented Sep 14, 2017

asvetlov commented Sep 15, 2017

asvetlov commented Sep 15, 2017

lock bot commented Oct 28, 2019

Add SSL related params to ClientSession.request #2184

Add SSL related params to ClientSession.request #2184

Conversation

cecton commented Aug 9, 2017 • edited Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

cecton commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

asvetlov commented Aug 9, 2017

cecton commented Aug 10, 2017

asvetlov commented Aug 10, 2017

cecton commented Aug 10, 2017

asvetlov commented Aug 10, 2017

asvetlov commented Sep 6, 2017

cecton commented Sep 6, 2017

codecov-io commented Sep 10, 2017 • edited Loading

Codecov Report

cecton commented Sep 10, 2017

asvetlov commented Sep 12, 2017

cecton commented Sep 12, 2017

asvetlov commented Sep 12, 2017

jettify commented Sep 13, 2017

kxepal Sep 13, 2017

Choose a reason for hiding this comment

cecton Sep 13, 2017

Choose a reason for hiding this comment

cecton Sep 13, 2017

Choose a reason for hiding this comment

asvetlov Sep 13, 2017

Choose a reason for hiding this comment

cecton Sep 13, 2017

Choose a reason for hiding this comment

asvetlov commented Sep 13, 2017

asvetlov Sep 13, 2017

Choose a reason for hiding this comment

jettify commented Sep 13, 2017

asvetlov commented Sep 13, 2017

cecton commented Sep 14, 2017

asvetlov commented Sep 14, 2017

cecton commented Sep 14, 2017

asvetlov commented Sep 15, 2017

asvetlov commented Sep 15, 2017

lock bot commented Oct 28, 2019

cecton commented Aug 9, 2017 •

edited

Loading

codecov-io commented Sep 10, 2017 •

edited

Loading