Fix use of HTTPS proxies #1070

[jakob-keller]

Description of Change

This PR fixes support for HTTPS proxies.

Assumptions

Connection to the HTTPS proxy and to the HTTPS endpoint may use a single ssl.SSLContext.

Checklist for All Submissions

• I have added change info to CHANGES.rst
• If this is resolving an issue (needed so future developers can determine if change is still necessary and under what conditions) (can be provided via link to issue with these details): closes Proxy not working due to signature mismatch #1070
  • Detailed description of issue
  • Alternative methods considered (if any)
  • How issue is being resolved: Use separate aiohttp sessions per proxy URL and merge any relevant configuration options into a single ssl.SSLContext to be used both for connections to the HTTPS proxy as well as the HTTPS endpoint
  • How issue can be reproduced
• If this is providing a new feature (can be provided via link to issue with these details):
  • Detailed description of new feature
  • Why needed
  • Alternatives methods considered (if any)

Checklist when updating botocore and/or aiohttp versions

• I have read and followed CONTRIBUTING.rst
• I have updated test_patches.py where/if appropriate (also check if no changes necessary)
• I have ensured that the awscli/boto3 versions match the updated botocore version

[codecov]

Codecov Report

Attention: Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.29%. Comparing base (c74c796) to head (704508c).
Report is 76 commits behind head on master.

Files	Patch %	Lines
aiobotocore/httpsession.py	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1077      +/-   ##
==========================================
+ Coverage   86.23%   86.29%   +0.06%     
==========================================
  Files          62       62              
  Lines        5885     5889       +4     
==========================================
+ Hits         5075     5082       +7     
+ Misses        810      807       -3     

Flag	Coverage Δ
unittests	86.29% <96.96%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thehesiod]

sorry for delay! Kids got hand foot mouth sequentially and I've been working on getting our condo back on the market due to tenant destruction (pain) with no money (they didn't pay). Will get to this ASAP.

[jakob-keller]

sorry for delay! Kids got hand foot mouth sequentially and I've been working on getting our condo back on the market due to tenant destruction (pain) with no money (they didn't pay). Will get to this ASAP.

Ouch, I feel ya! Take your time!

[thehesiod]

gonna get this done today! (pep talking myself lol)

[thehesiod]

is it possible to use their proxy manager concept to make the logic more similar? would help to maintain going forwards.

[jakob-keller]

is it possible to use their proxy manager concept to make the logic more similar? would help to maintain going forwards.

I don't see how. We don't use (proxy) managers at all.

We could create a ._get_session() method that extracts relevant logic from .send(), if that is what you propose. Should I give it a try?

[jakob-keller]

I just rebased the PR onto main and added a commit that pulls out session creation into ._get_session(). It makes the code a bit more readable, but introduces another round trip through the event loop, which could have a minor performance impact. Let me know, what you think. I can drop the commit easily.

[thehesiod]

ok looks great, we really need to add a proxy test later though. One suggestion, replace:

            chunked = None
            if headers_.get('Transfer-Encoding', '').lower() == 'chunked':
                # aiohttp wants chunking as a param, and not a header
                headers_.pop('Transfer-Encoding', '')
                chunked = True

with something like this function after _get_session

    def _chunked(self, headers):
        transfer_encoding = headers.get('Transfer-Encoding', '')
        if chunked := transfer_encoding.lower() == 'chunked':
            # aiohttp wants chunking as a param, and not a header
            headers.pop('Transfer-Encoding', '')
        return chunked

so it matches botocore some more.

[thehesiod]

oh and then chunked=self._chunked(headers_),

[thehesiod]

thank you and sorry for the delays! finally finished the painting, now for all the other 1000 tiny things to fix lol

[jakob-keller]

I'm not sure we need to deal with chunked at all. To me it seems that aiohttp is able to handle that internally at least since aio-libs/aiohttp@2c85834.

[thehesiod]

hmm, seems like there's test coverage so it seems like you could pull it out to see what happens (assuming it's an integration test to ensure it's testing w/ aiohttp and moto)

[thehesiod]

although I'd open an issue with comments for better testing in this area as well to do later unless you feel very motivated :)

[jakob-keller]

Let's see what happens: #1096

[jakob-keller]

although I'd open an issue with comments for better testing in this area as well to do later unless you feel very motivated :)

Unit tests in what's still an unfamiliar codebase are not my strong suit :-/

[jakob-keller]

Let's see what happens: #1096

CI doesn't complain, but I'm not sure that's enough evidence. Maybe aiohttp folks can weigh in?

[thehesiod]

we can just add a test later, i think werkzerg via moto should be able to handle it

[jakob-keller]

I have no idea, why CI is failing. This should be unrelated to this PR.

[thehesiod]

hmm:

>               raise ParamValidationError(report=report.generate_report())
E               botocore.exceptions.ParamValidationError: Parameter validation failed:
E               Invalid type for parameter TopicArn, value: None, type: <class 'NoneType'>, valid types: <class 'str'>

https://github.com/aio-libs/aiobotocore/actions/runs/8129847790/job/22217412080?pr=1077

[thehesiod]

hmm, also seeing an xml parse error, I wonder if it's moto that got upgraded

[jakob-keller]

I have no idea, why CI is failing. This should be unrelated to this PR.

For some reason chunked=None is not equivalent to chunked=False. I just reverted to the old logic and now CI is passing.

[thehesiod]

thank you so much, sorry for delay!

[jakob-keller]

Thanks for the thorough review! Much appreciated!