From 55c0d3652440df339b3caa1b9e87a6379a497571 Mon Sep 17 00:00:00 2001 From: Alex Schultz Date: Tue, 23 Jul 2019 12:21:52 -0600 Subject: [PATCH] Properly close web request Currently this is throwing warnings about unclosed connections. We need to properly close the response when we try to connect to the container registries. Change-Id: Ib93a0edb2e789855aa9e5908130a03ffcd9439c2 Closes-Bug: #1837393 Co-Authored-By: Sorin Sbarnea --- tripleo_common/image/image_uploader.py | 45 +++++----- .../tests/image/test_image_uploader.py | 53 ++++++------ .../tests/image/test_kolla_builder.py | 85 +++++++++---------- 3 files changed, 90 insertions(+), 93 deletions(-) diff --git a/tripleo_common/image/image_uploader.py b/tripleo_common/image/image_uploader.py index d286cd178..d3f030dc7 100644 --- a/tripleo_common/image/image_uploader.py +++ b/tripleo_common/image/image_uploader.py @@ -690,30 +690,31 @@ class BaseImageUploader(object): if (registry_host in self.insecure_registries or registry_host in self.no_verify_registries): return True - try: - requests.get('https://%s/v2' % registry_host, timeout=30) - except requests.exceptions.SSLError: - # Might be just a TLS certificate validation issue - # Just retry without the verification + with requests.Session() as s: try: - requests.get('https://%s/v2' % registry_host, timeout=30, - verify=False) - self.no_verify_registries.add(registry_host) - # Techinically these type of registries are insecure when - # the container engine tries to do a pull. The python uploader - # ignores the certificate problem, but they are still inscure - # so we return True here while we'll still use https when we - # access the registry. LP#1833751 - return True + s.get('https://%s/v2' % registry_host, timeout=30) except requests.exceptions.SSLError: - # So nope, it's really not a certificate verification issue - self.insecure_registries.add(registry_host) - return True - except Exception: - # for any other error assume it is a secure registry, because: - # - it is secure registry - # - the host is not accessible - pass + # Might be just a TLS certificate validation issue + # Just retry without the verification + try: + s.get('https://%s/v2' % registry_host, timeout=30, + verify=False) + self.no_verify_registries.add(registry_host) + # Techinically these type of registries are insecure when + # the container engine tries to do a pull. The python + # uploader ignores the certificate problem, but they are + # still inscure so we return True here while we'll still + # use https when we access the registry. LP#1833751 + return True + except requests.exceptions.SSLError: + # So nope, it's really not a certificate verification issue + self.insecure_registries.add(registry_host) + return True + except Exception: + # for any other error assume it is a secure registry, because: + # - it is secure registry + # - the host is not accessible + pass self.secure_registries.add(registry_host) return False diff --git a/tripleo_common/tests/image/test_image_uploader.py b/tripleo_common/tests/image/test_image_uploader.py index 896431a65..d83d749f9 100644 --- a/tripleo_common/tests/image/test_image_uploader.py +++ b/tripleo_common/tests/image/test_image_uploader.py @@ -241,57 +241,56 @@ class TestBaseImageUploader(base.TestCase): self.uploader._inspect.retry.sleep = mock.Mock() self.requests = self.useFixture(rm_fixture.Fixture()) - def test_is_insecure_registry_known(self): + @mock.patch.object(requests.Session, 'get', return_value=True) + def test_is_insecure_registry_known(self, mock_session): self.assertFalse( self.uploader.is_insecure_registry('docker.io')) - def test_is_insecure_registry_secure(self): + @mock.patch.object(requests.Session, 'get', return_value=True) + def test_is_insecure_registry_secure(self, mock_session): self.assertFalse( self.uploader.is_insecure_registry('192.0.2.0:8787')) self.assertFalse( self.uploader.is_insecure_registry('192.0.2.0:8787')) - self.assertEqual( - 'https://192.0.2.0:8787/v2', - self.requests.request_history[0].url - ) + calls = [mock.call('https://192.0.2.0:8787/v2', timeout=30)] + mock_session.assert_has_calls(calls) + self.assertEqual(mock_session.call_count, 1) - @mock.patch('requests.get') - def test_is_insecure_registry_bad_cert(self, mock_get): - mock_get.side_effect = [requests.exceptions.SSLError('ouch'), True] + @mock.patch.object(requests.Session, 'get', + side_effect=[requests.exceptions.SSLError('err'), True]) + def test_is_insecure_registry_bad_cert(self, mock_session): self.assertTrue( self.uploader.is_insecure_registry('bcert:8787')) self.assertTrue( self.uploader.is_insecure_registry('bcert:8787')) calls = [mock.call('https://bcert:8787/v2', timeout=30), mock.call('https://bcert:8787/v2', timeout=30, verify=False)] - mock_get.assert_has_calls(calls) - self.assertEqual(mock_get.call_count, 2) + mock_session.assert_has_calls(calls) + self.assertEqual(mock_session.call_count, 2) - def test_is_insecure_registry_timeout(self): - self.requests.get( - 'https://192.0.2.0:8787/', - exc=requests.exceptions.ReadTimeout('ouch')) + @mock.patch.object(requests.Session, 'get', + side_effect=requests.exceptions.ReadTimeout('ouch')) + def test_is_insecure_registry_timeout(self, mock_session): self.assertFalse( self.uploader.is_insecure_registry('192.0.2.0:8787')) self.assertFalse( self.uploader.is_insecure_registry('192.0.2.0:8787')) - self.assertEqual( - 'https://192.0.2.0:8787/v2', - self.requests.request_history[0].url - ) + calls = [mock.call('https://192.0.2.0:8787/v2', timeout=30)] + mock_session.assert_has_calls(calls) + self.assertEqual(mock_session.call_count, 1) - def test_is_insecure_registry_insecure(self): - self.requests.get( - 'https://192.0.2.0:8787/v2', - exc=requests.exceptions.SSLError('ouch')) + @mock.patch.object(requests.Session, 'get', + side_effect=requests.exceptions.SSLError('ouch')) + def test_is_insecure_registry_insecure(self, mock_session): self.assertTrue( self.uploader.is_insecure_registry('192.0.2.0:8787')) self.assertTrue( self.uploader.is_insecure_registry('192.0.2.0:8787')) - self.assertEqual( - 'https://192.0.2.0:8787/v2', - self.requests.request_history[0].url - ) + calls = [mock.call('https://192.0.2.0:8787/v2', timeout=30), + mock.call('https://192.0.2.0:8787/v2', timeout=30, + verify=False)] + mock_session.assert_has_calls(calls) + self.assertEqual(mock_session.call_count, 2) @mock.patch('tripleo_common.image.image_uploader.' 'BaseImageUploader.authenticate') diff --git a/tripleo_common/tests/image/test_kolla_builder.py b/tripleo_common/tests/image/test_kolla_builder.py index 4bed21b6e..0db7ec2d8 100644 --- a/tripleo_common/tests/image/test_kolla_builder.py +++ b/tripleo_common/tests/image/test_kolla_builder.py @@ -16,7 +16,6 @@ import mock import os -import requests import six import sys import tempfile @@ -606,37 +605,25 @@ class TestPrepare(base.TestCase): with open(imagefile.name, 'w') as f: f.write(template_filedata) - @mock.patch('requests.get') - def test_detect_insecure_registry(self, mock_get): + @mock.patch.object(image_uploader.ImageUploadManager, 'uploader') + def test_detect_insecure_registry(self, mock_uploader): + mock_f = mock.MagicMock() + mock_f.is_insecure_registry.side_effect = [False, True] + mock_uploader.return_value = mock_f self.assertEqual( {}, kb.detect_insecure_registries( {'foo': 'docker.io/tripleo'})) self.assertEqual( - {}, + {'DockerInsecureRegistryAddress': ['tripleo']}, kb.detect_insecure_registries( {'foo': 'tripleo'})) - @mock.patch('requests.get') - def test_detect_insecure_registry_readtimeout(self, mock_get): - - mock_get.side_effect = requests.exceptions.ReadTimeout('ouch') - self.assertEqual( - {}, - kb.detect_insecure_registries( - {'foo': '192.0.2.0:8787/tripleo'})) - - @mock.patch('requests.get') - def test_detect_insecure_registry_sslerror(self, mock_get): - mock_get.side_effect = requests.exceptions.SSLError('ouch') - self.assertEqual( - {'DockerInsecureRegistryAddress': ['192.0.2.0:8787']}, - kb.detect_insecure_registries( - {'foo': '192.0.2.0:8787/tripleo'})) - - @mock.patch('requests.get') - def test_detect_insecure_registry_multi(self, mock_get): - mock_get.side_effect = requests.exceptions.SSLError('ouch') + @mock.patch.object(image_uploader.ImageUploadManager, 'uploader') + def test_detect_insecure_registry_multi(self, mock_uploader): + mock_f = mock.MagicMock() + mock_f.is_insecure_registry.return_value = True + mock_uploader.return_value = mock_f self.assertEqual( {'DockerInsecureRegistryAddress': [ '192.0.2.0:8787', @@ -647,15 +634,17 @@ class TestPrepare(base.TestCase): 'baz': '192.0.2.1:8787/tripleo/baz', })) - @mock.patch('requests.get') - def test_prepare_noargs(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_noargs(self, mock_insecure): self.assertEqual( {}, kb.container_images_prepare(template_file=TEMPLATE_PATH) ) - @mock.patch('requests.get') - def test_prepare_simple(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_simple(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [ {'image_source': 'kolla', @@ -682,8 +671,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_includes(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_includes(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [ {'image_source': 'kolla', @@ -707,8 +697,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_includes_excludes(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_includes_excludes(self, mock_insecure): # assert same result as includes only. includes trumps excludes self.assertEqual({ 'container_images.yaml': [ @@ -734,8 +725,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_push_dest(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_push_dest(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [{ 'image_source': 'kolla', @@ -767,9 +759,10 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) @mock.patch('tripleo_common.image.image_uploader.get_undercloud_registry') - def test_prepare_push_dest_discover(self, mock_gur, mock_get): + def test_prepare_push_dest_discover(self, mock_gur, mock_insecure): mock_gur.return_value = '192.0.2.0:8787' self.assertEqual({ 'container_images.yaml': [{ @@ -802,8 +795,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_ceph(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_ceph(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [{ 'image_source': 'ceph', @@ -825,8 +819,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_neutron_driver_default(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_neutron_driver_default(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [ {'image_source': 'kolla', @@ -853,8 +848,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_neutron_driver_ovn(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_neutron_driver_ovn(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [ {'image_source': 'kolla', @@ -886,8 +882,9 @@ class TestPrepare(base.TestCase): ) ) - @mock.patch('requests.get') - def test_prepare_neutron_driver_odl(self, mock_get): + @mock.patch('tripleo_common.image.kolla_builder.' + 'detect_insecure_registries', return_value={}) + def test_prepare_neutron_driver_odl(self, mock_insecure): self.assertEqual({ 'container_images.yaml': [ {'image_source': 'kolla',