diff --git a/insights/client/config.py b/insights/client/config.py index 2fca41817..3e8310413 100644 --- a/insights/client/config.py +++ b/insights/client/config.py @@ -445,7 +445,7 @@ def _core_collect_default(): 'legacy_upload': { # True: upload to insights classic API # False: upload to insights platform API - 'default': True + 'default': False }, 'payload': { 'default': None, @@ -663,6 +663,11 @@ def _load_config_file(self, fname=None): 'ERROR: {0}.\nCould not read configuration file, ' 'using defaults\n'.format(e)) return + + if d.get("legacy_upload", False): + logger.debug("'legacy_upload' is set in the configuration file, but is no longer supported. Ignoring.") + del d["legacy_upload"] + self._update_dict(d) def load_all(self): @@ -763,6 +768,10 @@ def _validate_options(self): raise ValueError("Unable to find app: %s\nList of available apps: %s" % (self.app, ', '.join(sorted(manifests.keys())))) + if self.legacy_upload: + logger.debug("Legacy upload is no longer supported. Using current APIs.") + self.legacy_upload = False + def _imply_options(self): ''' Some options enable others automatically diff --git a/insights/tests/client/test_ansiblehost.py b/insights/tests/client/test_ansiblehost.py index 1046b27bd..55524d7dc 100644 --- a/insights/tests/client/test_ansiblehost.py +++ b/insights/tests/client/test_ansiblehost.py @@ -63,17 +63,3 @@ def test_ansible_host_no_reg_forces_legacy_false(): conf._cli_opts = ["ansible_host"] conf._imply_options() assert not conf.legacy_upload - - -def test_ansible_host_reg_legacy_no_change(): - ''' - When specifying --register, using --ansible-host on the CLI does not affect legacy_upload - ''' - conf = InsightsConfig(register=True, ansible_host="test", legacy_upload=True) - conf._cli_opts = ["ansible_host"] - conf._imply_options() - assert conf.legacy_upload - conf = InsightsConfig(register=True, ansible_host="test", legacy_upload=False) - conf._cli_opts = ["ansible_host"] - conf._imply_options() - assert not conf.legacy_upload diff --git a/insights/tests/client/test_client.py b/insights/tests/client/test_client.py index ecaa5ffa8..873622eb7 100644 --- a/insights/tests/client/test_client.py +++ b/insights/tests/client/test_client.py @@ -146,33 +146,6 @@ def test_get_log_handler_by_client_version(get_version_info, mock_path_dirname): pass -@patch('insights.client.client.generate_machine_id') -@patch('insights.client.utilities.delete_unregistered_file') -@patch('insights.client.utilities.write_to_disk') -@patch('insights.client.utilities.constants.registered_files', - [TEMP_TEST_REG_DIR + '/.registered', - TEMP_TEST_REG_DIR2 + '/.registered']) -@patch('insights.client.utilities.constants.unregistered_files', - [TEMP_TEST_REG_DIR + '/.unregistered', - TEMP_TEST_REG_DIR2 + '/.unregistered']) -@patch('insights.client.utilities.constants.machine_id_file', - TEMP_TEST_REG_DIR + '/machine-id') -def test_register_legacy(utilities_write, delete_unregistered_file, generate_machine_id): - config = InsightsConfig(register=True, legacy_upload=True) - client = InsightsClient(config) - client.connection = _mock_InsightsConnection(registered=None) - client.connection.config = config - client.session = True - with _mock_no_register_files(): - client.register() is True - delete_unregistered_file.assert_called_once() - generate_machine_id.assert_called_once_with() - utilities_write.assert_has_calls(( - call(constants.registered_files[0]), - call(constants.registered_files[1]) - )) - - @patch('insights.client.client.generate_machine_id') @patch('insights.client.utilities.delete_unregistered_file') @patch('insights.client.utilities.write_to_disk') @@ -304,32 +277,8 @@ def test_reg_check_unregistered(): # test function and integration in .register() with _mock_no_register_files(): - assert client.get_registration_status()['status'] is False - for r in constants.registered_files: - assert os.path.isfile(r) is False - for u in constants.unregistered_files: - assert os.path.isfile(u) is True - - -@patch('insights.client.utilities.constants.registered_files', - [TEMP_TEST_REG_DIR + '/.registered', - TEMP_TEST_REG_DIR2 + '/.registered']) -@patch('insights.client.utilities.constants.unregistered_files', - [TEMP_TEST_REG_DIR + '/.unregistered', - TEMP_TEST_REG_DIR2 + '/.unregistered']) -@patch('insights.client.utilities.constants.machine_id_file', - TEMP_TEST_REG_DIR + '/machine-id') -def test_reg_check_registered_res_unreg_legacy(): - # system is registered but receives the unregistration status - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.connection = _mock_InsightsConnection(registered=None) - client.connection.config = config - client.session = True - - # test function and integration in .register() - with _mock_registered_files(): - assert client.get_registration_status()['status'] is False + status = client.get_registration_status() + assert status is False for r in constants.registered_files: assert os.path.isfile(r) is False for u in constants.unregistered_files: @@ -361,29 +310,6 @@ def test_reg_check_registered_res_unreg(): assert os.path.isfile(u) is True -@patch('insights.client.utilities.constants.registered_files', - [TEMP_TEST_REG_DIR + '/.registered', - TEMP_TEST_REG_DIR2 + '/.registered']) -@patch('insights.client.utilities.constants.unregistered_files', - [TEMP_TEST_REG_DIR + '/.unregistered', - TEMP_TEST_REG_DIR2 + '/.unregistered']) -@patch('insights.client.utilities.constants.machine_id_file', - TEMP_TEST_REG_DIR + '/machine-id') -def test_reg_check_registered_unreachable_legacy(): - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.connection = _mock_InsightsConnection(registered=False) - client.connection.config = config - client.session = True - - with _mock_registered_files(): - assert client.get_registration_status()['unreachable'] is True - for r in constants.registered_files: - assert os.path.isfile(r) is True - for u in constants.unregistered_files: - assert os.path.isfile(u) is False - - @patch('insights.client.utilities.constants.registered_files', [TEMP_TEST_REG_DIR + '/.registered', TEMP_TEST_REG_DIR2 + '/.registered']) @@ -431,29 +357,6 @@ def test_reg_check_unregistered_unreachable(): assert os.path.isfile(u) is True -@patch('insights.client.utilities.constants.registered_files', - [TEMP_TEST_REG_DIR + '/.registered', - TEMP_TEST_REG_DIR2 + '/.registered']) -@patch('insights.client.utilities.constants.unregistered_files', - [TEMP_TEST_REG_DIR + '/.unregistered', - TEMP_TEST_REG_DIR2 + '/.unregistered']) -@patch('insights.client.utilities.constants.machine_id_file', - TEMP_TEST_REG_DIR + '/machine-id') -def test_reg_check_unregistered_unreachable_legacy(): - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.connection = _mock_InsightsConnection(registered=False) - client.connection.config = config - client.session = True - - with _mock_no_register_files(): - assert client.get_registration_status()['unreachable'] is True - for r in constants.registered_files: - assert os.path.isfile(r) is False - for u in constants.unregistered_files: - assert os.path.isfile(u) is True - - @patch('insights.client.client.constants.sleep_time', 0) @patch('insights.client.client.InsightsConnection._init_session', return_value=None) @patch('insights.client.client.InsightsConnection.upload_archive', return_value=Mock(status_code=500)) @@ -486,7 +389,7 @@ def test_upload_500_retry(logger, _, upload_archive, __): @patch('insights.client.client.InsightsConnection.upload_archive') @patch("insights.client.client.logger") def test_upload_exception_retry(logger, upload_archive, _): - from requests.exceptions import ConnectionError, ProxyError, Timeout, HTTPError, SSLError + from requests.exceptions import ConnectionError, ProxyError, Timeout upload_archive.side_effect = [ConnectionError("Connection Error"), ProxyError("Proxy Error"), Timeout("Timeout Error")] @@ -504,23 +407,6 @@ def test_upload_exception_retry(logger, upload_archive, _): logger.error.assert_any_call("Upload attempt %d of %d failed! Reason: %s", 3, config.retries, "Timeout Error") logger.error.assert_called_with("All attempts to upload have failed!") - # Test legacy uploads - logger.reset_mock() - upload_archive.reset_mock() - upload_archive.side_effect = [HTTPError("HTTP Error"), - SSLError("SSL Error")] - retries = 2 - config = InsightsConfig(legacy_upload=True, logging_file='/tmp/insights.log', retries=retries) - client = InsightsClient(config) - with patch('insights.client.os.path.exists', return_value=True): - with pytest.raises(RuntimeError): - client.upload('/tmp/insights.tar.gz') - assert upload_archive.call_count == retries - logger.debug.assert_any_call("Legacy upload attempt %d of %d ...", 1, config.retries) - logger.error.assert_any_call("Upload attempt %d of %d failed! Reason: %s", 1, config.retries, "HTTP Error") - logger.error.assert_any_call("Upload attempt %d of %d failed! Reason: %s", 2, config.retries, "SSL Error") - logger.error.assert_called_with("All attempts to upload have failed!") - @patch('insights.client.client.InsightsConnection._init_session', return_value=None) @patch('insights.client.client.InsightsConnection.handle_fail_rcs') @@ -533,7 +419,7 @@ def test_upload_412_no_retry(_, upload_archive, handle_fail_rcs, __): sys.argv = [] try: - config = InsightsConfig(logging_file='/tmp/insights.log', retries=3) + config = InsightsConfig(logging_file='/tmp/insights.log') client = InsightsClient(config) with pytest.raises(RuntimeError): client.upload('/tmp/insights.tar.gz') @@ -543,30 +429,6 @@ def test_upload_412_no_retry(_, upload_archive, handle_fail_rcs, __): sys.argv = tmp -@patch('insights.client.client.InsightsConnection._init_session', return_value=None) -@patch('insights.client.connection.write_unregistered_file') -@patch('insights.client.client.InsightsConnection.upload_archive', - return_value=Mock(**{"status_code": 412, - "json.return_value": {"unregistered_at": "now", "message": "msg"}})) -@patch('insights.client.os.path.exists', return_value=True) -def test_upload_412_write_unregistered_file(_, upload_archive, write_unregistered_file, __): - - # Hack to prevent client from parsing args to py.test - tmp = sys.argv - sys.argv = [] - - try: - config = InsightsConfig(logging_file='/tmp/insights.log', retries=3) - client = InsightsClient(config) - with pytest.raises(RuntimeError): - client.upload('/tmp/insights.tar.gz') - - unregistered_at = upload_archive.return_value.json()["unregistered_at"] - write_unregistered_file.assert_called_once_with(unregistered_at) - finally: - sys.argv = tmp - - @patch('insights.client.archive.InsightsArchive.storing_archive') @patch('insights.client.archive.tempfile.mkdtemp', Mock()) @patch('insights.client.archive.os.makedirs', Mock()) @@ -607,28 +469,6 @@ def test_cleanup_tmp_obfuscation(storing_archive): storing_archive.assert_called_once() -@patch('insights.client.client._legacy_handle_registration') -def test_legacy_register(_legacy_handle_registration): - ''' - _legacy_handle_registration called when legacy upload - ''' - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.register() - _legacy_handle_registration.assert_called_once() - - -@patch('insights.client.client._legacy_handle_unregistration') -def test_legacy_unregister(_legacy_handle_unregistration): - ''' - _legacy_handle_unregistration called when legacy upload - ''' - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.unregister() - _legacy_handle_unregistration.assert_called_once() - - @patch('insights.client.client.handle_registration') def test_register_upload(handle_registration): ''' @@ -651,20 +491,6 @@ def test_unregister_upload(handle_unregistration): handle_unregistration.assert_called_once() -@patch('insights.client.os.path.exists', return_value=True) -@patch('insights.client.connection.InsightsConnection.upload_archive', Mock(return_value=Mock(status_code=200))) -@patch('insights.client.client._legacy_upload') -@patch('insights.client.client.write_to_disk', Mock()) -def test_legacy_upload(_legacy_upload, path_exists): - ''' - _legacy_upload called when legacy upload - ''' - config = InsightsConfig(legacy_upload=True) - client = InsightsClient(config) - client.upload('test.gar.gz', 'test.content.type') - _legacy_upload.assert_called_once() - - @patch('insights.client.os.path.exists', return_value=True) @patch('insights.client.connection.InsightsConnection.upload_archive', Mock(return_value=Mock(status_code=200))) @patch('insights.client.client._legacy_upload') @@ -679,20 +505,6 @@ def test_platform_upload(_legacy_upload, path_exists): _legacy_upload.assert_not_called() -@patch('insights.client.os.path.exists', return_value=True) -@patch('insights.client.connection.InsightsConnection.upload_archive', return_value=Mock(status_code=200)) -@patch('insights.client.client._legacy_upload') -def test_platform_upload_with_no_log_path(_legacy_upload, _, path_exists): - ''' - testing logger when no path is given - ''' - config = InsightsConfig(legacy_upload=True, logging_file='tmp.log') - client = InsightsClient(config) - response = client.upload('test.gar.gz', 'test.content.type') - _legacy_upload.assert_called_once() - assert response is not None - - @patch('insights.client.shutil') def test_copy_to_output_dir(shutil_): ''' diff --git a/insights/tests/client/test_platform.py b/insights/tests/client/test_platform.py index 06e158ef7..d6b760cdd 100644 --- a/insights/tests/client/test_platform.py +++ b/insights/tests/client/test_platform.py @@ -69,11 +69,6 @@ def test_upload_urls(): ''' Ensure upload urls are defined correctly ''' - # legacy default - conf = InsightsConfig() - c = InsightsConnection(conf) - assert c.upload_url == c.base_url + '/uploads' - # plaform implied conf = InsightsConfig(legacy_upload=False) c = InsightsConnection(conf) @@ -108,33 +103,3 @@ def test_payload_upload(op, post, c, _legacy_upload_archive, rm_conf): })}, headers={}) _legacy_upload_archive.assert_not_called() - - -@patch('insights.contrib.magic.open', MockMagic) -@patch('insights.client.connection.generate_machine_id', mock_machine_id) -@patch("insights.client.connection.get_canonical_facts", return_value={'test': 'facts'}) -@patch('insights.client.connection.InsightsConnection.post') -@patch("insights.client.connection.open", new_callable=mock_open) -def test_legacy_upload(op, post, c): - ''' - Ensure an Insights collected tar upload to legacy occurs with the right URL and params - ''' - conf = InsightsConfig() - c = InsightsConnection(conf) - c.upload_archive('testp', 'testct', None) - post.assert_called_with( - c.base_url + '/uploads/XXXXXXXX', - files={ - 'file': ('testp', ANY, 'application/gzip')}, # ANY = return call from mocked open(), acts as filepointer here - headers={'x-rh-collection-time': 'None'}) - - -@patch("insights.client.connection.InsightsConnection._legacy_upload_archive") -def test_legacy_upload_func(_legacy_upload_archive): - ''' - Ensure the _legacy_upload_archive path is used - ''' - conf = InsightsConfig() - c = InsightsConnection(conf) - c.upload_archive('testp', 'testct', None) - _legacy_upload_archive.assert_called_with('testp', None) diff --git a/insights/tests/client/test_upload_archive_cfacts.py b/insights/tests/client/test_upload_archive_cfacts.py index bc86b4794..813b0a57e 100644 --- a/insights/tests/client/test_upload_archive_cfacts.py +++ b/insights/tests/client/test_upload_archive_cfacts.py @@ -25,16 +25,6 @@ def mock_init_session(obj): return MockSession() -@patch("insights.client.connection.InsightsConnection._legacy_upload_archive", return_value=None) -@patch("insights.client.connection.logger") -def test_cfacts_no_cleaning_1(logger, legacy_upload): - # 1. No need to cleaning as legacy_upload=True - config = InsightsConfig(legacy_upload=True, obfuscate=True, obfuscate_hostname=True) - conn = InsightsConnection(config) - conn.upload_archive('data_collected', 'content_type') - logger.debug.assert_not_called() - - @patch('insights.client.connection.InsightsConnection._init_session', mock_init_session) @patch('insights.client.connection.InsightsConnection.post', return_value=MockSession()) @patch(builtin_open, new_callable=mock_open, read_data='')