Skip to content

Commit

Permalink
chore: Disable legacy upload
Browse files Browse the repository at this point in the history 
* Card ID: CCT-252
* Card ID: RHINENG-7569

This patch disables the legacy APIs. The code and configuration option
will be removed later; this is the first step to get rid of the
duplication and differences.

Only the absolutely necessary changes were made to tests; without these
removals the suite wouldn't pass. The rest will be cleaned up later.

Signed-off-by: mhorky <[email protected]>
  • Loading branch information
@m-horky
m-horky committed Sep 5, 2024
1 parent c7ff320 commit d30e6d7
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 252 deletions.
11 changes: 10 additions & 1 deletion insights/client/config.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand Down
14 changes: 0 additions & 14 deletions insights/tests/client/test_ansiblehost.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
196 changes: 4 additions & 192 deletions insights/tests/client/test_client.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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'])
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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")]
Expand All @@ -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')
Expand All @@ -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')
Expand All @@ -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())
Expand Down Expand Up @@ -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):
'''
Expand All @@ -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')
Expand All @@ -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_):
'''
Expand Down
35 changes: 0 additions & 35 deletions insights/tests/client/test_platform.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
10 changes: 0 additions & 10 deletions insights/tests/client/test_upload_archive_cfacts.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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='')
Expand Down

0 comments on commit d30e6d7

Please sign in to comment.