Refactor handling of content #148

matejak · 2021-06-04T15:59:57Z

This PR aims to process content in a way that is backend-agnostic, i.e. it can work for the automated kickstart installations as well as interactive ones. It is separated into multiple commits - most of them are small, but the main one is quite extensive.

The fetch is performed in a two-step manner that was present in the GUI code, when it is started in a separate thread, and then the processing code then waits for the fetching thread - this may be an overkill for a plain kickstart installation, but it doesn't cause any problems.
Interactions with the backend are performed by callbacks - at this time it is a possibility to report status, definition of what to do after fetching stops, and finally definition of how to handle errors by means of a function that accepts an exception instance.
The unified pipeline includes content fetching, fingerprint verification, file extraction and file identification.

A class containing a content set has been introduced - it contains a list of discovered files (fetched, present on the system, or extracted from an archive) that have been identified by the oscap scanner. That class allows to work with the content more easily.

pep8speaks · 2021-06-04T16:00:02Z

Hello @matejak! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file org_fedora_oscap/content_discovery.py:

Line 123:100: E501 line too long (101 > 99 characters)
Line 145:100: E501 line too long (114 > 99 characters)

In the file org_fedora_oscap/ks/oscap.py:

Line 403:13: E125 continuation line with same indent as next logical line
Line 403:13: E128 continuation line under-indented for visual indent
Line 441:100: E501 line too long (103 > 99 characters)

In the file tests/test_content_handling.py:

Line 32:100: E501 line too long (101 > 99 characters)

Comment last updated at 2021-06-17 13:57:45 UTC

openscap-ci · 2021-06-04T16:00:13Z

Build finished.

openscap-ci · 2021-06-07T13:33:55Z

Build finished.

openscap-ci · 2021-06-07T14:35:29Z

Build finished.

jan-cerny · 2021-06-09T08:51:32Z

org_fedora_oscap/model.py

+        for net_prefix in data_fetch.NET_URL_PREFIXES)
+
+
+class Model:


Please add documentation strings on classes and public methods.

jan-cerny · 2021-06-09T08:54:00Z

org_fedora_oscap/model.py

+        self.content_uri_path = path
+        self.content_uri_scheme = scheme
+
+    def fetch_content(self, cert, what_if_fail):


What about merging it with fetch_files?

That makes sense now, but what if you would like to introduce support for fetching tailoring? Then, it would make sense to keep those separately.

jan-cerny · 2021-06-09T09:01:12Z

org_fedora_oscap/model.py

+                                            hash_obj)
+        if digest != fingerprint:
+            log.error(
+                "File {dest_filename} failed integrity check - assumed a "


jan-cerny · 2021-06-09T09:03:17Z

org_fedora_oscap/model.py

+        return content
+
+
+class ObtainedContent:


I feel that it somehow overlaps with OSCAPData class (forms ks/oscap.py).

This one aims to assist the gathered files discovery - files can be downloaded directly, or they can be extracted. This class enables a user to quickly understand what is available, and whether the current set of contents is usable for further processing.

jan-cerny · 2021-06-09T09:05:26Z

tests/test_data_fetch.py

+def test_fetch_local(tmp_path):
+    source_path = pathlib.Path(__file__).absolute()
+    dest_path = tmp_path / "dest"
+    data_fetch.fetch_data("file://" + str(source_path), dest_path)


So if I am an user when I run the installation where do I put the files or how do I pass files to Anaconda?

You can use this only if those files are already there. I can imagine usage when the content filename is not guessed properly (e.g. RHEL8 vs RHV), or when the installation image contains the content as a specific location.

jan-cerny · 2021-06-09T09:38:59Z

org_fedora_oscap/model.py

+        for net_prefix in data_fetch.NET_URL_PREFIXES)
+
+
+class Model:


Can we think of a better name?

jan-cerny · 2021-06-09T09:45:46Z

org_fedora_oscap/model.py

+                "Couldn't find a valid datastream or a valid XCCDF-OVAL file tuple "
+                "among the available content")
+            raise content_handling.ContentHandlingError(msg)
+        if elligible_main_content[0]:


this perhaps require a comment or better unpacking the tuple on line into 2 variables that would have good names and changing the condition.

or one can avoid the tuple completely.

openscap-ci · 2021-06-09T14:38:10Z

Build finished.

openscap-ci · 2021-06-09T14:43:53Z

Build finished.

openscap-ci · 2021-06-10T11:42:14Z

Build finished.

openscap-ci · 2021-06-10T12:38:04Z

Build finished.

openscap-ci · 2021-06-10T15:06:44Z

Build finished.

jan-cerny

It mostly works well.

See the comments below.

Please check the unit tests, they don't pass for me.

jan-cerny · 2021-06-11T11:15:56Z

org_fedora_oscap/content_discovery.py

+
+class ContentBringer:
+    CONTENT_DOWNLOAD_LOCATION = pathlib.Path(common.INSTALLATION_CONTENT_DIR)
+    DEFAULT_CONTENT = f"{common.SSG_DIR}/{common.SSG_CONTENT}"


I would rename it to make it clear that it's the path to the datastream from the scap-security-guide RPM, eg. DEFAULT_SSG_DATA_STREAM_PATH.

jan-cerny · 2021-06-11T11:18:02Z

org_fedora_oscap/content_discovery.py

+        Args:
+            what_if_fail: Callback accepting exception as an argument that
+                should handle them in the calling layer.
+            cert: HTTPS certificates


certificates paths? names?

jan-cerny · 2021-06-11T11:58:54Z

org_fedora_oscap/content_discovery.py

+                msg = f"Couldn't find '{content_path}' among the available content"
+            else:
+                msg = (
+                    "File '{content_path}' is not a valid datastream "


jan-cerny · 2021-06-11T12:18:13Z

org_fedora_oscap/content_discovery.py

+                f"When dealing with {attribute_name}, "
+                f"there was already the {old_value.name} when setting the new {new_value.name}")


If I provide an RPM containing multiple SCAP source data streams this causes the UI to crash. It's expected to fail in this situation, but it shouldn't crash.

However, providing https://github.com/OpenSCAP/oscap-anaconda-addon/blob/rhel8-branch/testing_files/separate-scap-files.zip causes the addon to crash as well because of CPE OVAL (ssg-rhel8-cpe-oval.xml) gets into a conflict with check OVAL (ssg-rhel8-oval.xml). I think it shouldn't happen, CPE OVAL checks can be a valid part of oscap evaluation.

jan-cerny · 2021-06-11T12:51:36Z

tests/test_content_handling.py

+
+
+def test_init_invalid_file_path():
+    with pytest.raises(ch.DataStreamHandlingError) as excinfo:


The DataStreamHandlingError has been removed by #147 together with some other stuff from content_handling.py which were replaced by SCAPContentHandler class. It looks like this file had conflicts with #147 which should be resolved in a different way,

jan-cerny · 2021-06-11T12:52:34Z

org_fedora_oscap/ks/oscap.py

-                progressQ.send_quit(1)
-                while True:
-                    time.sleep(100000)
+            msg = ["Wrong configuration detected!"]


msgs would be a more descriptive name because this is a list

Another weird thing is the absence of translations.

org_fedora_oscap/gui/spokes/oscap.py

That function has the same interface as the remote fetching one.

sudo is only needed when RPMs are installed.

Notably introduced the _terminate function that extracts all the installation termination-related boilerplate.

The class unifies the content workflow between kickstart and GUI installations. It provides methods to work with the content in non-interactive and interactive modes using a system of callbacks.

Anaconda will want to perform fetch, but if we install via GUI, a fetch has already been performed, and the addon should be able to acknowledge the work that has been done on the GUI side.

- Added docstringsu - Improved naming. - Removed redundant arguments. - Split large method into two smaller.

jan-cerny

I have tried multiple different content from the testing_files directory and it seems to work. Great improvement.

I have only one small nitpick.

jan-cerny · 2021-06-17T13:25:56Z

org_fedora_oscap/gui/spokes/oscap.py


        try:
+            if actually_fetched_content:
+                self.content_bringer.use_downloaded_content(content)
+            log.info(f"Opening SCAP content at {self._addon_data.preinst_content_path} with tailoring {self._addon_data.preinst_tailoring_path}")


I'm sorry but now I realized that self._addon_data.preinst_tailoring_path isn't set when tailoring isn't present so you would get: "Opening SCAP content at /tmp/bzfn with tailoring" in that situation which would be confusing for people reading the log.

Fixed by a rebase.

Co-authored-by: Jan Černý <jcerny@redhat.com>

Tests became obsolete because of earlier refactoring.

- Multiple OVAL files in an archive cause no longer an error. - Multiple instances of other filetypes result in a content-related error. - Unexpected errors don't crash the addon.

- mock is part of the Python standard library. - scanner is used in unit tests.

scrutinizer-notifier · 2021-06-17T13:57:52Z

A new inspection was created.

scrutinizer-notifier · 2021-06-17T13:59:14Z

The inspection completed: 41 updated code elements

jan-cerny

amazing

matejak added this to the 1.2 milestone Jun 4, 2021

matejak force-pushed the gathering_refactoring branch from 51b522f to 7abf7eb Compare June 7, 2021 14:35

jan-cerny reviewed Jun 9, 2021

View reviewed changes

matejak force-pushed the gathering_refactoring branch from 78b0c76 to 15db68c Compare June 9, 2021 14:43

matejak marked this pull request as ready for review June 9, 2021 14:43

jan-cerny self-assigned this Jun 10, 2021

matejak force-pushed the gathering_refactoring branch from 15db68c to f2a7550 Compare June 10, 2021 11:29

matejak force-pushed the gathering_refactoring branch from d3d3802 to bd4d62e Compare June 10, 2021 15:06

jan-cerny reviewed Jun 11, 2021

View reviewed changes

matejak force-pushed the gathering_refactoring branch from 4ae7ab0 to cea80b8 Compare June 16, 2021 15:45

matejak added 11 commits June 17, 2021 11:01

Slightly refactor handling of downloaded content.

2ea54ee

Added a function for local content fetching.

b0b7f00

That function has the same interface as the remote fetching one.

Enabled building images without sudo when possible.

e3ff5b4

sudo is only needed when RPMs are installed.

Extracted rule data gathering.

6632e53

Various improvements.

dfdc41c

Notably introduced the _terminate function that extracts all the installation termination-related boilerplate.

Introduced the Model class.

3164e02

The class unifies the content workflow between kickstart and GUI installations. It provides methods to work with the content in non-interactive and interactive modes using a system of callbacks.

Fied GUI installs.

4a85918

Anaconda will want to perform fetch, but if we install via GUI, a fetch has already been performed, and the addon should be able to acknowledge the work that has been done on the GUI side.

Improved code based on review feedback.

3764f40

- Added docstringsu - Improved naming. - Removed redundant arguments. - Split large method into two smaller.

Pulled more functionality to the Model class.

ecdfff1

Refactored handling of kickstart install abort.

4574d94

Rebranded the model class.

5c116dc

matejak force-pushed the gathering_refactoring branch from cea80b8 to 01e7afd Compare June 17, 2021 11:40

jan-cerny reviewed Jun 17, 2021

View reviewed changes

matejak and others added 5 commits June 17, 2021 15:57

Improve code style.

ff1cc83

Co-authored-by: Jan Černý <jcerny@redhat.com>

Cleaned up tests.

76cf404

Tests became obsolete because of earlier refactoring.

Increased robustness of content handling.

670ba00

- Multiple OVAL files in an archive cause no longer an error. - Multiple instances of other filetypes result in a content-related error. - Unexpected errors don't crash the addon.

Update gating test configuration.

9e0dd09

- mock is part of the Python standard library. - scanner is used in unit tests.

Come up with a better name for the certificate path.

1732af6

matejak force-pushed the gathering_refactoring branch from 3c32b4d to 1732af6 Compare June 17, 2021 13:57

jan-cerny approved these changes Jun 17, 2021

View reviewed changes

jan-cerny merged commit b7f2374 into OpenSCAP:rhel8-branch Jun 17, 2021

matejak mentioned this pull request Jun 21, 2021

The file:// URI is not supported in the oscap GUI spoke. #79

Closed

		f"When dealing with {attribute_name}, "
		f"there was already the {old_value.name} when setting the new {new_value.name}")



		def test_init_invalid_file_path():
		with pytest.raises(ch.DataStreamHandlingError) as excinfo:

Refactor handling of content #148

Refactor handling of content #148

Conversation

matejak commented Jun 4, 2021

pep8speaks commented Jun 4, 2021 • edited Loading

Comment last updated at 2021-06-17 13:57:45 UTC

openscap-ci commented Jun 4, 2021

openscap-ci commented Jun 7, 2021

openscap-ci commented Jun 7, 2021

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

openscap-ci commented Jun 9, 2021

openscap-ci commented Jun 9, 2021

openscap-ci commented Jun 10, 2021

openscap-ci commented Jun 10, 2021

openscap-ci commented Jun 10, 2021

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

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

scrutinizer-notifier commented Jun 17, 2021

scrutinizer-notifier commented Jun 17, 2021

jan-cerny left a comment

Choose a reason for hiding this comment

pep8speaks commented Jun 4, 2021 •

edited

Loading