Fix openSCAP results #2880

Bischoff · 2020-11-20T19:10:02Z

What does this PR change?

The result of the openSCAP audit may list many not applicable rules. They make the test for passing rules difficult as they may start on page 2. This PR uses the filter button to focus on the passing rules.

Now we will install all needed packages from the test suite, instead of a mixture of sumaform and the test suite (see uyuni-project/sumaform#801). This PR adapts.

The list of packages to install for openSCAP tests was wrong, For example, the CentOS traditional client is both a CentOS client and a traditional client. Constant package lists were not fit for this, this PR computes the list dynamically.

This PR also cleans up the generated reports, the installed packages and the enabled repositories at the end to make these tests idempotent.

Links

Ports:

4.1: SUSE/spacewalk#13187 SUSE/spacewalk#13189 SUSE/spacewalk#13195 SUSE/spacewalk#13197
4.0: SUSE/spacewalk#13188 SUSE/spacewalk#13190 SUSE/spacewalk#13194 SUSE/spacewalk#13196

Changelogs

If you don't need a changelog check, please mark this checkbox:

No changelog needed

srbarrios · 2020-11-23T08:14:28Z

testsuite/features/support/constants.rb

-TRADITIONAL_STACK_RPMS = 'spacewalk-client-tools spacewalk-check spacewalk-client-setup '\
-                         'mgr-daemon mgr-osad mgr-cfg-actions'.freeze
-
-OPEN_SCAP_CENTOS_DEPS = 'spacewalk-oscap scap-security-guide'.freeze
-
-OPEN_SCAP_TRAD_DEPS = 'spacewalk-oscap'.freeze
-
-OPEN_SCAP_SALT_DEPS = 'openscap-utils openscap-content scap-security-guide'.freeze


I don't like the idea to remove these constants.
I prefer to have outside from a method this information, as it's easier to tweak and handle it.

I knew you would say that. I still don't understand how having things at different places helps. Having them where they are needed avoid me losing overview.

Said overwise, a constant (or a method) that is used only once does not really make sense to me.

That's a lost battle, you will never agree with me in relation to encapsulation and isolation, so... go ahead.

srbarrios · 2020-11-23T08:16:45Z

testsuite/features/step_definitions/common_steps.rb

+When(/^I enable universe repositories on "([^"]*)"$/) do |host|
+  node = get_target(host)
+  node.run("sed -i '/^#\\s*deb http:\\/\\/archive.ubuntu.com\\/ubuntu .* universe/ s/^#\\s*deb /deb /' /etc/apt/sources.list")
+  node.run("apt-get update")
+end
+
+When(/^I disable universe repositories on "([^"]*)"$/) do |host|
+  node = get_target(host)
+  node.run("sed -i '/^deb http:\\/\\/archive.ubuntu.com\\/ubuntu .* universe/ s/^deb /# deb /' /etc/apt/sources.list")
+  node.run("apt-get update")
+end
+


I'd prefer if we join these two steps definitions that are doing the same, but with the opposite result

I don't think that helps readability and I prefer it like this. But if you wish to "fix" this someday I won't oppose either.

Using add-apt-repository on Ubuntu20, will be safer and will simplify it in a step.
If we use Ubuntu16 or Ubuntu18, we just need to first install software-properties-common

testsuite/features/step_definitions/common_steps.rb

srbarrios · 2020-11-23T08:24:10Z

testsuite/features/step_definitions/command_steps.rb

+# On CentOS 7, OpenSCAP files are for RedHat and need a small adaptation for CentOS
+When(/^I fix CentOS 7 OpenSCAP files on "([^"]*)"$/) do |host|
+  node = get_target(host)
+  script = '/<\/rear-matter>/a  <platform idref="cpe:/o:centos:centos:7"/>'
+  file = "/usr/share/xml/scap/ssg/content/ssg-rhel7-xccdf.xml"
+  node.run("sed -i '#{script}' #{file}")
 end


Does it only happen in CentOS7? It doesn't happen on CentOS 8?

This is an interesting and open question. In the doubt, I took a conservative approach and chose to use it on CentOS 7 only. We can add it to CentOS 8 too if we see notapplicable there as well.

I am sure it happens in CentOS 6 because of https://forums.centos.org/viewtopic.php?t=50462 .

It also happens in Ubuntu !!! But I did not find the correct fix and gave up after one hour searching. I added a TODO in the code as a way to remember the issue.

Ok, keep in mind it, and maybe add a comment in the card to upgrade our CI to CentOS8, as that could emerge at some moment.

srbarrios · 2020-11-23T08:26:21Z

testsuite/features/secondary/trad_openscap_audit.feature

+  Scenario: Cleanup: remove the openSCAP packages from the traditional client
+    When I remove OpenSCAP dependencies from "sle_client"
+    And I disable SUSE Manager tools repositories on "sle_client"
+    And I disable repository "os_pool_repo os_update_repo" on this "sle_client"


If I recall correctly, a few weeks ago I manage to hide this information from the Gherkin code.
Could you review that there is no method that provides you this action without specifying fake repository names (os_pool_repo os_update_repo) here in the scenario?

No, we do hide this, but for other stuff : Docker, Prometeus, etc.

To be noticed: the existing code was not hiding the details either (And I enable repository "CentOS Base").

I tried to hide it the same way:

And I install packages before testing OpenSCAP

but it resulted complicated, especially due to the fact we reuse steps like I disable SUSE Manager tools repositories that are already doing part of the hiding, but are also used standalone for bootstrapping. The other thing that makes it difficult is that it depends both on OS and on traditional/minion.

I agree this refactor would be beneficial, but I also already lost too much time on openSCAP, which is somehow marginal.

Feel free to write these "hiding" steps (but hiding all repository enable/disable, not only a couple of them). Would be great.

As I said, I will not go behind others' code refactoring it. If you really feel it's ok like that, I will not oppose, I'm just giving my personal opinion :)

srbarrios · 2020-11-23T08:28:05Z

testsuite/features/secondary/trad_openscap_audit.feature

    Given I am on the Systems overview page of this "sle_client"
    When I follow "Audit" in the content area
    And I follow "List Scans" in the content area
    And I click on "Select All"
    And I click on "Remove Selected Scans"
    And I click on "Confirm"
-    Then I should see a "deleted. 0 SCAP Scan(s) retained" text
+    Then I should see a "1 SCAP Scan(s) deleted. 0 SCAP Scan(s) retained" text


These kinds of assumptions are very problematic for idempotency and it's a source of issues if we decide to run the tests in parallel. Please keep that in mind. Note it will not impact if the minion only runs SCAP in this feature.

Agree. Will remove it everywhere. Was done to align to same code on all types of clients, but I aligned the wrong way round.

As a side note, it could not have created problems with parallelism, as it happens on different clients, and each has its own audit screen. But OK.

testsuite/features/secondary/trad_openscap_audit.feature

srbarrios

Giving approval, but I left a few remarks because I don't fully agree with the way we handle some things on that PR. Nothing that I can't live with.

Bischoff requested review from srbarrios, calancha, ktsamis and lkotek November 20, 2020 19:10

github-actions bot added the secondary-features label Nov 20, 2020

Bischoff force-pushed the openscap-centos branch 4 times, most recently from 7491f1f to a9be690 Compare November 21, 2020 04:22

Bischoff changed the title ~~Filter openSCAP results~~ Fix openSCAP results Nov 22, 2020

Bischoff mentioned this pull request Nov 22, 2020

Let the test suite install OpenSCAP packages uyuni-project/sumaform#801

Merged

Bischoff force-pushed the openscap-centos branch 2 times, most recently from 03142cb to ed4b25c Compare November 22, 2020 14:00

Filter the rules to avoid that tested one is on another page

5a79262

Bischoff force-pushed the openscap-centos branch 3 times, most recently from aa013ea to 96bd145 Compare November 22, 2020 14:22

srbarrios reviewed Nov 23, 2020

View reviewed changes

testsuite/features/step_definitions/common_steps.rb Show resolved Hide resolved

srbarrios reviewed Nov 23, 2020

View reviewed changes

testsuite/features/secondary/trad_openscap_audit.feature Show resolved Hide resolved

srbarrios approved these changes Nov 23, 2020

View reviewed changes

Bischoff added 2 commits November 23, 2020 10:11

Idempotency: clean up audit results

3a2cf15

Install correct OpenSCAP packages, and fix them if needed

bb9b72e

Bischoff force-pushed the openscap-centos branch from 96bd145 to bb9b72e Compare November 23, 2020 09:13

juliogonzalez merged commit 06dfff5 into uyuni-project:master Nov 23, 2020

Bischoff deleted the openscap-centos branch November 23, 2020 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix openSCAP results #2880

Fix openSCAP results #2880

Bischoff commented Nov 20, 2020 •

edited

Loading

srbarrios Nov 23, 2020

Bischoff Nov 23, 2020

srbarrios Nov 23, 2020

srbarrios Nov 23, 2020

Bischoff Nov 23, 2020

srbarrios Nov 23, 2020

srbarrios Nov 23, 2020

Bischoff Nov 23, 2020 •

edited

Loading

srbarrios Nov 23, 2020

srbarrios Nov 23, 2020

Bischoff Nov 23, 2020 •

edited

Loading

srbarrios Nov 23, 2020

srbarrios Nov 23, 2020 •

edited

Loading

Bischoff Nov 23, 2020 •

edited

Loading

Bischoff Nov 23, 2020

Bischoff Nov 23, 2020

srbarrios left a comment

Fix openSCAP results #2880

Fix openSCAP results #2880

Conversation

Bischoff commented Nov 20, 2020 • edited Loading

What does this PR change?

Links

Changelogs

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

Bischoff Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bischoff Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srbarrios Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Bischoff Nov 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srbarrios left a comment

Choose a reason for hiding this comment

Bischoff commented Nov 20, 2020 •

edited

Loading

Bischoff Nov 23, 2020 •

edited

Loading

Bischoff Nov 23, 2020 •

edited

Loading

srbarrios Nov 23, 2020 •

edited

Loading

Bischoff Nov 23, 2020 •

edited

Loading