Implemented packages metadata to the test suite #6126

matejak · 2020-09-25T15:31:16Z

This PR centralises installation of packages that are used to test scenarios.
As some checks now require packages installed in order to be applicable, it could be that if 5 scenarios required a package that was not present in the test suite image, it had to be downloaded and installed 5 times, which is wasteful and takes a long time too.

Using the new approach, all scenarios are examined prior to the test execution, and list of all packages to install is gathered.
Those packages are installed to the base tests_upladed snapshot using an SSH command that is deduced according to CPEs of the datastream benchmark, so Fedora ends up with dnf, while RHEL7 with yum.

What this PR consists of:

Refactored send_scripts - two functions were extracted, and one of those is then reused. Similar functions already exist in the module, but I think that further refactoring in this regard is out of scope of this PR.
The "CPE to platform" function along to the "platform to install command" dictionaries were added. This is not the most elegant solution, but it seems to fit the purpose.
Test prep in the rule mode has been reworked - as all scenarios are now enumerated early on, the code has been slightly refactored, and the installation of packages has been introduced. The data.upload.log log file has been renamed to env-preparation.log.

mildas · 2020-09-25T15:31:36Z

Changes identified:
Others:
Python abstract syntax tree change found in tests/ssg_test_suite/common.py.
Python abstract syntax tree change found in tests/ssg_test_suite/rule.py.

Recommended tests to execute:
(cd build && cmake ../ && ctest -j4)

jan-cerny

Testing random rule:

python3 tests/test_suite.py rule --libvirt qemu:///system ssgts_rhel8 --datastream build/ssg-rhel8-ds.xml selinux_state
Setting console output to log level INFO
INFO - The base image option has not been specified, choosing libvirt-based test environment.
INFO - Logging into /home/jcerny/work/git/scap-security-guide/logs/rule-custom-2020-09-29-1257/test_suite.log
ERROR - Nothing has been tested!
Traceback (most recent call last):
  File "tests/test_suite.py", line 369, in <module>
    main()
  File "tests/test_suite.py", line 365, in main
    options.func(options)
  File "/home/jcerny/work/git/scap-security-guide/tests/ssg_test_suite/rule.py", line 385, in perform_rule_check
    checker.test_target(options.target)
  File "/home/jcerny/work/git/scap-security-guide/tests/ssg_test_suite/oscap.py", line 650, in test_target
    self._test_target(target)
  File "/home/jcerny/work/git/scap-security-guide/tests/ssg_test_suite/rule.py", line 253, in _test_target
    self._prepare_environment()
  File "/home/jcerny/work/git/scap-security-guide/tests/ssg_test_suite/rule.py", line 221, in _prepare_environment
    self._ensure_package_present_for_all_scenarios()
  File "/home/jcerny/work/git/scap-security-guide/tests/ssg_test_suite/rule.py", line 205, in _ensure_package_present_for_all_scenarios
    for rule, scenarios in scenarios_by_rule.items():
NameError: name 'scenarios_by_rule' is not defined

Could you take a look into this?

jan-cerny · 2020-09-29T07:54:49Z

tests/ssg_test_suite/common.py

-            msg = "Cannot extract data tarball {0}.".format(remote_archive_file)
-            logging.error(msg)
-            raise RuntimeError(msg)
+        print("Setting up test setup scripts", file=log_file)


The unit tests on RHEL 7 fails, probably Python 2.7 doesn't know file= yet.

from ssg_test_suite import common E File "/home/jenkins/workspace/scap-security-guide-pull-requests/label/rhel7/tests/ssg_test_suite/common.py", line 281 E print("Setting up test setup scripts", file=log_file) E ^ E SyntaxError: invalid syntax

It doesn't know print as a function - that's an easy fix.

jan-cerny · 2020-09-29T08:36:10Z

tests/ssg_test_suite/common.py

+    try:
+        run_with_stdout_logging("scp", SSH_ADDITIONAL_OPTS + (what, scp_dest), log_file)
+    except Exception:
+        logging.error(error_msg)


I think we can add the text of the exception (str(exc)) the same way as in execute_remote_command.

jan-cerny · 2020-09-29T09:01:05Z

tests/ssg_test_suite/common.py

+INSTALL_COMMANDS = dict(
+        fedora=("dnf", "install", "-y"),
+        rhel7=("yum", "install", "-y"),
+        rhel8=("yum", "install", "-y"),
+)


coding style

What's wrong?

I'd say that the indentation of items in dict. One level of indentation looks better.

INSTALL_COMMANDS = dict( fedora=("dnf", "install", "-y"), rhel7=("yum", "install", "-y"), rhel8=("yum", "install", "-y"), )

And I don't know if the last comma is intended.

OK, makes sense.

The comma is indented, it makes additions git-friendly.

jan-cerny · 2020-09-29T10:26:12Z

...s/guide/system/software/gnome/gnome_login_screen/gnome_gdm_disable_xdmcp/tests/empty.fail.sh

-# platform = multi_platform_all
-
-dnf install -y gdm
+# packages = gdm


jan-cerny · 2020-09-29T10:56:27Z

tests/ssg_test_suite/rule.py

-    def _test_target(self, target):
+    def _ensure_package_present_for_all_scenarios(self):
+        packages_required = set()
+        for rule, scenarios in scenarios_by_rule.items():


The scenarios_by_rule isn't defined.

openscap-ci · 2020-09-29T15:28:39Z

FAILURE

jan-cerny · 2020-09-30T12:54:41Z

@openscap-ci test this please

jan-cerny · 2020-09-30T13:28:20Z

/test e2e-aws-rhcos4-e8

jan-cerny · 2020-09-30T14:39:20Z

tests/ssg_test_suite/common.py

+        if "fedora" in cpe:
+            return "fedora"
+        if "redhat:enterprise_linux" in cpe:
+            version = re.match(r"enterprise_linux:(\d)+:", cpe).groups()[1]


This causes problems for me when running rule test scenarios against a RHEL 8 VM target.

Suggested change

version = re.match(r"enterprise_linux:(\d)+:", cpe).groups()[1]

version = re.search(r"enterprise_linux:(\d+)", cpe).groups()[0]

It turned out to be even more complicated, but thanks for the suggestion - you were right on the search part.

jan-cerny · 2020-09-30T14:46:58Z

tests/ssg_test_suite/common.py

+    log_file_name = os.path.join(LogHelper.LOG_DIR, "env-preparation.log")
+
+    with open(log_file_name, 'a') as log_file:
+        print("Installing packages", file=log_file)


There is something weird about this. When I open env-preparation.log in the run log directory, I can see the output of yum command there, and after that there is "Installing packages" and the ssh command arguments. It should be before the yum output. It might be something with locking or flushing the log file.

Yes, it was the flush.

Before each test, all scenarios are examined, and list of all packages to install is gathered. Those packages are installed to the base tests_upladed snapshot using an SSH command that is determined according to CPEs of the datastream benchmark, so Fedora ends up with dnf, while RHEL7 with yum.

It's the platform where tests are run rather than the benchmark platform which is important when determining what package manager to use.

mildas · 2020-10-01T12:59:42Z

tests/ssg_test_suite/rule.py

@@ -292,17 +324,14 @@ def _get_scenarios(self, rule_dir, scripts, scenarios_regex, benchmark_cpes):

        return scenarios

-    def _check_rule(self, rule, remote_dir, state, remediation_available):
+    def _check_rule(self, rule, scenarios, remote_dir, state, remediation_available):


CombinedChecker class in ssg_test_suite/combined.py inherits from RuleChecker and uses this _check_rule method. Please, fix the method usage there.

Good catch, this is a non-trivial finding, and I have done my best to make the RuleChecker more extension-friendly, so the CombinedChecker doesn't contain duplicated code.

jan-cerny · 2020-10-01T14:08:55Z

tests/ssg_test_suite/common.py

@@ -242,42 +245,56 @@ def create_tarball():
        return fp.name


+def execute_remote_command(machine, args, log_file, error_msg=""):


There are already similar functions to this, eg. run_cmd_remote and _run_cmd. I haven't checked, but what do you think? Can we somehow unify them?

I would love to, but preferably not in this PR.

OK, fair enough

jan-cerny · 2020-10-01T14:11:22Z

tests/ssg_test_suite/common.py

+
+
+def cpes_to_platform(cpes):
+    for cpe in cpes:


Why does it eat a list? In the only usage you artifically create a list with a single item.

It used to work with a list of CPEs that were associated with a benchmark, and to basically find the OS one among them. We don't use that right now, but I think that it may come in handy.

jan-cerny · 2020-10-01T14:13:22Z

tests/ssg_test_suite/common.py

+        log_file.flush()
+        execute_remote_command(
+                machine, INSTALL_COMMANDS[platform] + tuple(packages), log_file,
+                "Couldn't install required packages {packages}".format(packages=packages))


too big indentation

jan-cerny · 2020-10-01T14:36:01Z

Yes, it was the flush.

It now moved the "Installing pcakges" string to the right position, but the command is still printed after it.

mildas

nitpick indentation review :)

tests/ssg_test_suite/common.py

mildas

ah, my last review didn't include these comments... sorry for that

tests/ssg_test_suite/common.py

mildas · 2020-10-02T14:47:48Z

LGTM

jan-cerny · 2020-10-05T07:14:13Z

/test e2e-aws-rhcos4-e8

jan-cerny · 2020-10-05T07:14:28Z

@openscap-ci test this please

jan-cerny · 2020-10-05T07:18:32Z

tests/ssg_test_suite/combined.py

+    def _rule_should_be_tested(self, rule, rules_to_be_tested):
+        if rule.short_id not in rules_to_be_tested:
+            return False
+        return True


It's weird but let's not delay it.

jan-cerny · 2020-10-05T07:55:58Z

@matejak It's great! I'm only waiting for CI.

jan-cerny · 2020-10-05T10:26:33Z

/test e2e-aws-rhcos4-e8

matejak added the Test Suite Update in Test Suite. label Sep 25, 2020

matejak added this to the 0.1.53 milestone Sep 25, 2020

jan-cerny changed the title ~~Implemented packages metadata to the test suite.~~ Implemented packages metadata to the test suite Sep 29, 2020

jan-cerny requested changes Sep 29, 2020

View reviewed changes

matejak force-pushed the tests_packages branch from bb47c33 to 253780f Compare September 29, 2020 14:27

matejak force-pushed the tests_packages branch 2 times, most recently from 7daaa43 to 1ebb5ba Compare September 29, 2020 15:48

jan-cerny self-assigned this Sep 30, 2020

jan-cerny reviewed Sep 30, 2020

View reviewed changes

matejak added 4 commits October 1, 2020 11:25

Refactored sending of files to test systems.

d18b11e

Ported a couple of the scenarios to the new metadata.

7c78e76

Converted explicit package installs to package metadata.

735fbce

matejak force-pushed the tests_packages branch 2 times, most recently from 1aed579 to 99814cb Compare October 1, 2020 12:18

Redesigned platform detection.

55e7b0b

It's the platform where tests are run rather than the benchmark platform which is important when determining what package manager to use.

matejak force-pushed the tests_packages branch from 99814cb to 55e7b0b Compare October 1, 2020 12:25

mildas reviewed Oct 1, 2020

View reviewed changes

jan-cerny reviewed Oct 1, 2020

View reviewed changes

Propagated rule mode refactoring to the combined mode.

d3e2676

matejak force-pushed the tests_packages branch from a3f07db to d3e2676 Compare October 1, 2020 14:36

mildas reviewed Oct 2, 2020

View reviewed changes

tests/ssg_test_suite/common.py Outdated Show resolved Hide resolved

mildas reviewed Oct 2, 2020

View reviewed changes

tests/ssg_test_suite/common.py Outdated Show resolved Hide resolved

tests/ssg_test_suite/common.py Outdated Show resolved Hide resolved

tests/ssg_test_suite/common.py Outdated Show resolved Hide resolved

matusmarhefka mentioned this pull request Oct 2, 2020

SSGTS various test scenarios metadata updates (part 2) #6145

Merged

Fixed indentation.

90064fe

jan-cerny reviewed Oct 5, 2020

View reviewed changes

jan-cerny merged commit 63c4fab into ComplianceAsCode:master Oct 5, 2020

This pull request was closed.

	version = re.match(r"enterprise_linux:(\d)+:", cpe).groups()[1]
	version = re.search(r"enterprise_linux:(\d+)", cpe).groups()[0]

		@@ -242,42 +245,56 @@ def create_tarball():
		return fp.name


		def execute_remote_command(machine, args, log_file, error_msg=""):

Implemented packages metadata to the test suite #6126

Implemented packages metadata to the test suite #6126

Conversation

matejak commented Sep 25, 2020

mildas commented Sep 25, 2020

jan-cerny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openscap-ci commented Sep 29, 2020

jan-cerny commented Sep 30, 2020

jan-cerny commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-cerny commented Oct 1, 2020

mildas left a comment

Choose a reason for hiding this comment

mildas left a comment

Choose a reason for hiding this comment

mildas commented Oct 2, 2020

jan-cerny commented Oct 5, 2020

jan-cerny commented Oct 5, 2020

Choose a reason for hiding this comment

jan-cerny commented Oct 5, 2020

jan-cerny commented Oct 5, 2020