ngaranko commented on a change in pull request #14701:
URL: https://github.com/apache/airflow/pull/14701#discussion_r594529324



##########
File path: airflow/providers/http/hooks/http.py
##########
@@ -176,15 +181,20 @@ def run_and_check(
         """
         extra_options = extra_options or {}
 
+        request_options = {}
+        if "verify" in extra_options:
+            # Overwrite verify only if it is needed
+            request_options["verify"] = extra_options["verify"]
+
         try:
             response = session.send(
                 prepped_request,
                 stream=extra_options.get("stream", False),
-                verify=extra_options.get("verify", True),
                 proxies=extra_options.get("proxies", {}),
                 cert=extra_options.get("cert"),
                 timeout=extra_options.get("timeout"),
                 allow_redirects=extra_options.get("allow_redirects", True),
+                **request_options,

Review comment:
       Hey @marcosmarxm thanks for looking into this!
   
   Default `verify=True` is correct only for convenience methods of requests 
library (e.g. `requests.get`, `requests.post` etc), as they are using session 
factory and building correct `session.verify` value inside `Session.request`:
    
https://requests.readthedocs.io/en/master/_modules/requests/sessions/#Session.merge_environment_settings
   
   I've updated hook and replaced `requests.Request` with `session.request` as 
it does more and behaves more like expected from `requests.post`.
   
   Issue of `session.send` is that if we say `session.send(..., verify=True)` 
it will overwrite whatever is set by `merge_environment_settings` and will 
forcefully set `verify=True` (thanks to `kwargs.setdefault('verify', 
self.verify)` in `Session.send` code) and this breaks SSL certificate 
configuration provided by OS, same goes for `verify=False`.
   
   > you can change the value in your hook.run
   
   I can not, as we're not using `hook.run` directly, rather via other hooks, 
such as slack hook etc.
   
   Hope my comment clears out reasoning behind this change.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to