Small tweaks to remove.conf (#1625)

* un-escape strings after parsing. allow removal of files post-wildcard parse

* fix goof, add unit test

* unit test for string-escape/unicode-escape

* omit files/commands by symbolic name

* reduce if statement, teardown temp file

insights/client/collection_rules.py

@@ -219,8 +219,12 @@ def get_rm_conf(self):
         parsedconfig = ConfigParser.RawConfigParser()
         parsedconfig.read(self.remove_file)
         rm_conf = {}
+
         for item, value in parsedconfig.items('remove'):
-            rm_conf[item] = value.strip().split(',')
+            if six.PY3:
+                rm_conf[item] = value.strip().encode('utf-8').decode('unicode-escape').split(',')
+            else:
+                rm_conf[item] = value.strip().decode('string-escape').split(',')
 
         return rm_conf
 

insights/client/data_collector.py

@@ -164,22 +164,27 @@ def run_collection(self, conf, rm_conf, branch_info):
             if c.get('symbolic_name') == 'hostname':
                 self.hostname_path = os.path.join(
                     'insights_commands', mangle.mangle_command(c['command']))
-
-            if c['command'] in rm_conf.get('commands', []):
+            rm_commands = rm_conf.get('commands', [])
+            if c['command'] in rm_commands or c.get('symbolic_name') in rm_commands:
                 logger.warn("WARNING: Skipping command %s", c['command'])
             elif self.mountpoint == "/" or c.get("image"):
                 cmd_specs = self._parse_command_spec(c, conf['pre_commands'])
                 for s in cmd_specs:
                     cmd_spec = InsightsCommand(self.config, s, exclude, self.mountpoint)
                     self.archive.add_to_archive(cmd_spec)
         for f in conf['files']:
-            if f['file'] in rm_conf.get('files', []):
+            rm_files = rm_conf.get('files', [])
+            if f['file'] in rm_files or f.get('symbolic_name') in rm_files:
                 logger.warn("WARNING: Skipping file %s", f['file'])
             else:
                 file_specs = self._parse_file_spec(f)
                 for s in file_specs:
-                    file_spec = InsightsFile(s, exclude, self.mountpoint)
-                    self.archive.add_to_archive(file_spec)
+                    # filter files post-wildcard parsing
+                    if s['file'] in rm_conf.get('files', []):
+                        logger.warn("WARNING: Skipping file %s", s['file'])
+                    else:
+                        file_spec = InsightsFile(s, exclude, self.mountpoint)
+                        self.archive.add_to_archive(file_spec)
         if 'globs' in conf:
             for g in conf['globs']:
                 glob_specs = self._parse_glob_spec(g)

insights/tests/client/collection_rules/test_get_rm_conf.py

@@ -1,13 +1,23 @@
 # -*- coding: UTF-8 -*-
 
+import os
+import json
 from .helpers import insights_upload_conf
 from mock.mock import patch
+from insights.client.collection_rules import InsightsUploadConf
+from insights.client.config import InsightsConfig
 
 
 conf_remove_file = '/tmp/remove.conf'
 removed_files = ["/etc/some_file", "/tmp/another_file"]
 
 
+def teardown_function(func):
+    if func is test_raw_config_parser:
+        if os.path.isfile(conf_remove_file):
+            os.remove(conf_remove_file)
+
+
 def patch_isfile(isfile):
     """
     Makes isfile return the passed result.
@@ -54,3 +64,16 @@ def test_return(isfile, raw_config_parser):
     raw_config_parser.return_value.items.assert_called_with('remove')
 
     assert result == {"files": removed_files}
+
+
+def test_raw_config_parser():
+    '''
+        Ensure that get_rm_conf and json.loads (used to load uploader.json) return the same filename
+    '''
+    raw_filename = '/etc/yum/pluginconf.d/()*\\\\w+\\\\.conf'
+    uploader_snip = json.loads('{"pattern": [], "symbolic_name": "pluginconf_d", "file": "' + raw_filename + '"}')
+    with open(conf_remove_file, 'w') as rm_conf:
+        rm_conf.write('[remove]\nfiles=' + raw_filename)
+    coll = InsightsUploadConf(InsightsConfig(remove_file=conf_remove_file))
+    items = coll.get_rm_conf()
+    assert items['files'][0] == uploader_snip['file']

insights/tests/client/test_skip_commands_files.py

@@ -0,0 +1,77 @@
+from insights.client.data_collector import DataCollector
+from insights.client.config import InsightsConfig
+from mock.mock import patch, call
+
+
+@patch("insights.client.data_collector.DataCollector._parse_file_spec")
+@patch("insights.client.data_collector.InsightsFile")
+def test_omit_before_expanded_paths(InsightsFile, parse_file_spec):
+    """
+    Files are omitted based on representation of exact string matching in uploader.json
+    """
+    c = InsightsConfig()
+    data_collector = DataCollector(c)
+
+    collection_rules = {'files': [{"file": "/etc/pam.d/vsftpd", "pattern": [], "symbolic_name": "vsftpd"}], 'commands': {}}
+    rm_conf = {'files': ["/etc/pam.d/vsftpd"]}
+    data_collector.run_collection(collection_rules, rm_conf, {})
+    parse_file_spec.assert_not_called()
+    InsightsFile.assert_not_called()
+
+
+@patch("insights.client.data_collector.DataCollector._parse_file_spec")
+@patch("insights.client.data_collector.InsightsFile")
+def test_omit_after_expanded_paths(InsightsFile, parse_file_spec):
+    """
+    Files are omitted based on the expanded paths of the uploader.json path
+    """
+    c = InsightsConfig()
+    data_collector = DataCollector(c)
+
+    collection_rules = {'files': [{"file": "/etc/yum.repos.d/()*.*\\.repo", "pattern": [], "symbolic_name": "yum_repos_d"}], 'commands': {}}
+    rm_conf = {'files': ["/etc/yum/repos.d/test.repo"]}
+    data_collector.run_collection(collection_rules, rm_conf, {})
+    parse_file_spec.assert_called_once()
+    InsightsFile.assert_not_called()
+
+
+@patch("insights.client.data_collector.DataCollector._parse_file_spec")
+@patch("insights.client.data_collector.InsightsFile")
+@patch("insights.client.data_collector.InsightsCommand")
+def test_omit_symbolic_name(InsightsCommand, InsightsFile, parse_file_spec):
+    """
+    Files/commands are omitted based on their symbolic name in uploader.json
+    """
+    c = InsightsConfig()
+    data_collector = DataCollector(c)
+
+    collection_rules = {'files': [{"file": "/etc/pam.d/vsftpd", "pattern": [], "symbolic_name": "vsftpd"}],
+                        'commands': [{"command": "/sbin/chkconfig --list", "pattern": [], "symbolic_name": "chkconfig"}],
+                        'pre_commands': []}
+    rm_conf = {'files': ["vsftpd"], "commands": ["chkconfig"]}
+    data_collector.run_collection(collection_rules, rm_conf, {})
+    parse_file_spec.assert_not_called()
+    InsightsFile.assert_not_called()
+    InsightsCommand.assert_not_called()
+
+
+@patch("insights.client.data_collector.InsightsCommand")
+@patch("insights.client.data_collector.InsightsFile")
+@patch("insights.client.data_collector.archive.InsightsArchive")
+def test_symbolic_name_bc(InsightsArchive, InsightsFile, InsightsCommand):
+    """
+    WICKED EDGE CASE: in case uploader.json is old and doesn't have symbolic names, don't crash
+    """
+    c = InsightsConfig()
+    data_collector = DataCollector(c)
+
+    collection_rules = {'files': [{"file": "/etc/pam.d/vsftpd", "pattern": []}],
+                        'commands': [{"command": "/sbin/chkconfig --list", "pattern": []}],
+                        'pre_commands': []}
+    rm_conf = {'files': ["vsftpd"], "commands": ["chkconfig"]}
+    data_collector.run_collection(collection_rules, rm_conf, {})
+    InsightsFile.assert_called_once()
+    InsightsCommand.assert_called_once()
+    InsightsArchive.return_value.add_to_archive.assert_has_calls(
+        [call(InsightsFile.return_value), call(InsightsCommand.return_value)],
+        any_order=True)