restructure templates by vojtapolasek · Pull Request #6303 · ComplianceAsCode/content

vojtapolasek · 2020-10-27T15:57:41Z

Description:

for each existing template:
- create a new directory within shared/templates named after the template
- move template files to the directory and name them according to the language (oval, bash etc.)
- create a file template.py and move the preprocessing function (formerly decorated function in ssg/templates.py) into this file, call the function preprocess. If there is no real preprocessing needed, just skip this step.
- create file template.yml and declare supported languages within this file. This prevents situation with missing files for language implementation etc.
rewrite the file ssg/templates.py:
- create a new Class "template" for holding of some data, currently very simple, will be glad to extend it
- remove decorated functions
- modify the function "preprocess_data" to use new way of loading preprocessing functions
drop the "permissions" template, it was no longer used and it was even lacking the decorated function
update the part about creating of templates within the developer guide

Rationale:

This restructuring has the following benefits:

easier template creation - no modification of ssg/templates.py needed
the directory structure is easier to navigate
I believe this can lead to effective testing of templates with SSG test suite, not implemented yet

openshift-ci-robot · 2020-10-27T15:57:43Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

pep8speaks · 2020-10-27T15:57:55Z

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

In the file ssg/templates.py:

Line 47:17: E128 continuation line under-indented for visual indent
Line 51:17: E128 continuation line under-indented for visual indent
Line 57:35: E261 at least two spaces before inline comment

Comment last updated at 2020-11-23 17:53:15 UTC

openscap-ci · 2020-10-27T15:58:01Z

Changes identified:
Others:
Changes in Python files.

Show details

Others:
Python file shared/templates/accounts_password/template.py is newly added.
Python file shared/templates/audit_file_contents/template.py is newly added.
Python file shared/templates/audit_rules_login_events/template.py is newly added.
Python file shared/templates/audit_rules_path_syscall/template.py is newly added.
Python file shared/templates/audit_rules_privileged_commands/template.py is newly added.
Python file shared/templates/audit_rules_usergroup_modification/template.py is newly added.
Python file shared/templates/auditd_lineinfile/template.py is newly added.
Python file shared/templates/file_groupowner/template.py is newly added.
Python file shared/templates/file_owner/template.py is newly added.
Python file shared/templates/file_permissions/template.py is newly added.
Python file shared/templates/grub2_bootloader_argument/template.py is newly added.
Python file shared/templates/mount/template.py is newly added.
Python file shared/templates/mount_option/template.py is newly added.
Python file shared/templates/mount_option_remote_filesystems/template.py is newly added.
Python file shared/templates/package_installed/template.py is newly added.
Python file shared/templates/sebool/template.py is newly added.
Python file shared/templates/service_disabled/template.py is newly added.
Python file shared/templates/service_enabled/template.py is newly added.
Python file shared/templates/shell_lineinfile/template.py is newly added.
Python file shared/templates/sshd_lineinfile/template.py is newly added.
Python file shared/templates/sysctl/template.py is newly added.
Python file shared/templates/timer_enabled/template.py is newly added.
Python file shared/templates/yamlfile_value/template.py is newly added.
Python abstract syntax tree change found in ssg/templates.py.

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

vojtapolasek · 2020-10-27T15:58:32Z

/test all

redhatrises · 2020-10-27T16:20:47Z

I like this idea, but I would wouldn't follow the existing rule concepts of having separate directories for OVAL, bash, ansible, etc. Those directories make the project less approachable and readable. Not having the separate directories would make this easier for people. @carlosmmatos would you agree?

yuumasato · 2020-10-27T20:17:10Z

I like this idea, but I would wouldn't follow the existing rule concepts of having separate directories for OVAL, bash, ansible, etc.

They are not in directories, the files are just called oval, bash, ansible, ANACONDA, ignition, ...
BTW, @vojtapolasek the templates for Anaconda are all caps, :)

I think I'd name the files oval.template, ...

vojtapolasek · 2020-10-29T11:17:10Z

I added the .template extension to files in the template folder as requested. Also anaconda remediations are now in lower case.

carlosmmatos · 2020-10-29T13:32:59Z

I like this approach and using the .template file name syntax. It's more in line with a current control structure so this should be fairly simple to follow from a user or dev perspective.

ggbecker · 2020-10-29T17:10:52Z

ssg/templates.py

+
+    def preprocess(self, parameters, lang):
+        preprocess_mod = None  # temporarily imported module containing preprocessing function
+        spec = importlib.util.spec_from_file_location(


I have an idea about the templates.py file in the template directory. When I don't want to preprocess the data aka I don't need to change the input in any way, why not to just skip this logic (e.g capturing some exception that the file doesn't exist) and input the raw data to the jinja template? This way we don't need to have dummy templates.py in every template directory.

I am not against this idea. But can't we use these files also for documentation purposes? I am talking about your readthedocs initiative.

I'm not sure if it adds real value having an empty function without any comments

I like the idea of being able to skip e.g. the Python file, and I think that concerning documentation, we could aim at having e.g. a template.rst file that would contain the relevant snippet. In other words, I think that we don't need a function as a carrier of documentation.

ggbecker · 2020-10-29T17:22:17Z

Regarding the CI failure in RHEL7:

    import importlib.util
ImportError: No module named util

The function importlib.utilmodule_from_spec was introduced in python 3.5 according to https://docs.python.org/3/library/importlib.html#importlib.util.module_from_spec

So we cannot use it in RHEL7 (it has python-2.7.5). Maybe this can help:

import imp
 module = imp.load_source(module_name.split('.')[-1], filename)
 return module

from: https://stackoverflow.com/questions/45350363/backport-of-importlib-for-python-2-7-from-3-6

yuumasato

Looks pretty good.

yuumasato · 2020-10-30T10:50:56Z

ssg/templates.py

+        # TODO: Remove this right after the variables in templates are renamed
+        # to lowercase


Hmm, are the uppercase variables in the template intentional or a remnant of the past?

I would assume that the intention was to avoid naming conflicting and that the variable came from the input template data. But back then the templating system was different. I don't think it makes much sense nowadays.

Yes, it's a remainder from the previous system. We can rename the variables in the templates to lowercase now and then we will be able to remove this part of the code.

I see. I propose to remove this obsolete thing in a subsequent PR.

jan-cerny · 2020-11-02T11:23:03Z

ssg/templates.py

 import sys
-import re
-from xml.sax.saxutils import unescape
+import imp


The docs say that it's deprecated: https://docs.python.org/3/library/imp.html

Well, that's the problem of being compatible with older versions of Python.So far it works.

what if we detect which version of python is being executed and have a separate code for each version. I know that it sounds cumbersome, but when we stop using python2 we can just get rid of the older implementation.

jan-cerny · 2020-11-02T11:25:43Z

ssg/templates.py

+        # TODO: Remove this right after the variables in templates are renamed
+        # to lowercase


Yes, it's a remainder from the previous system. We can rename the variables in the templates to lowercase now and then we will be able to remove this part of the code.

jan-cerny · 2020-11-02T11:32:20Z

ssg/templates.py

+        # scan directory structure and dynamically create list of templates
+        for item in os.listdir(self.templates_dir):


Currently there is a mechanism implemented by @template decorator that was created to prevent situation that a template should exist but it doesn't exist. I assume with your changes this situation isn't prevented and it can happen.

Hm, you are right. In my implementation, languages supported by a template are determined based on the fact if certain files exist / do not exist. Maybe every template could have a small yaml file (template.yml). There could be list of supported languages and documentation. What do you think?

I think that detection based on existence of actual files is more robust. The decorator, on the other hand, was easier to implement, and it could be used in generating documentation.

Brainstorming: I think that there is room for a decorator that could take care of template arguments. Imagine that you omit an essential argument - it would get a descriptive error, and if you make a typo in a name of optional template argument, you would also get an error that an unknown argument was supplied.

@vojtapolasek In other words in depends on whether we think that the situation that a template is expected to exist but doesn't exist is likely to happen and is believed to cause problems to content authors and developers. In the original PR #4809 it was a concern (see #4809 (comment)) and since we discussed that it is a problem we introduced #4834 that prevents this situation.

I personally thought that it isn't a big problem because unwanted removal of template files can be prevented by good PR reviews so we don't need such a mechanism. But, I would like to have opinions of @yuumasato and @matejak who were concerned about it back then.

@matejak I think you're confused. Can you explain what do you mean by "I think that detection based on existence of actual files is more robust"?

I think I was worried that we would run into issues when silently failing to generate content while migrating to the new templating issue.

I personally thought that it isn't a big problem because unwanted removal of template files can be prevented by good PR reviews so we don't need such a mechanism.

I agree with that, but another aspect to consider is that people may have their own templates locally.
Basing the template generation on the existence of the file makes it simple to add new languages too.
One just needs to drop the file into the directory, no need to edit any python code.

Would it make sense to add debug messages for the languages detected for each template?

Another problem was that people when use the template in the rule.yml they expect that if they use a template which isn't available they will get an error message.

OK let me summarize and propose solutions:

currently, we do not expect rule authors to specify particular languages which they want to use with the template. We assume they want to use all available languages and they somehow know which of them are available. This information was tracked in the documentation and also in the function decorator.

Currently, we try to blindly build all templates for all languages and if it fails, it is okay and we silently skip it because these cases were covered by the decorator. We fail only in the case when the language was mentioned in the decorator but the language file is missing.

This PR removes the explicit naming of supported languages within the decorator and automatically finds out which languages are supported by examining the directory structure. It expects that rule authors are aware of the fact that if a file is not available, the language variant is simply not built. I don't know if it is acceptable. I think that the case where template somewhere claims which exact languages it supports is desired and we should keep it. It is kind of contract between template authors and rule authors.
I see two options how to solve this:

Ask rule authors to be explicit while naming languages used with the template. Then we would just check if the template has the right language files and if not, fail the build.

If we want to keep this automagic way of detecting which languages are available, we should somewhere keep track of supported languages. This will detect the case when a language file for a template is removed but the template claims it supports it.

I am inclining to the option 1. It just makes things crystal clear.

I don't like the option 1, as the aim of the templates is to provide scalable functionality - if you write a rule that is templated, your work is done, and as new languages are added current ones are improved, the rule gets improved for free, without any action needed.
If I get it correctly, option 1 means that if a new language is introduced, a massive PR is to be expected, adding that language to rule yamls that use the template.

At the same time, I think that it is useful to throw an error if a support for a particular language was requested, but the corresponding template doesn't exist, or if a support for a particular language was not excluded, but the template instantiation failed.

Therefore, I would lean towards option 2, as it is more scalable, and I don't recall any problems related to discrepancies between author expectations and template availability.

I'm also leaning forward to option 2 and I agree with @matejak. How do you imagine " somewhere keep track of supported languages"?

We could have a template.yml for each templat in its directory where these languages would be listed. Any additional data for the template could be listed there later.

jan-cerny · 2020-11-02T12:11:33Z

I like the effort very much.

I think in future we can also move the documentation closer to the template code.

matejak · 2020-11-03T12:00:56Z

I like this PR very much as well.

I think in future we can also move the documentation closer to the template code.

Certainly, especially when #6299 is merged. But that would be a topic for another PR.

jan-cerny · 2020-11-06T09:03:50Z

@vojtapolasek you have conflicts

remove template function callbacks rewrite data preprocessing to use new structure

add .template extension e.g. oval.template

if no preprocessing of template parameters is needed, template.py does not need to be present

vojtapolasek · 2020-11-23T10:43:59Z

Now every template needs to declare which languages it supports within the template.yml file.

vojtapolasek · 2020-11-23T17:54:38Z

Updated documentation. I copied the new chapter also into the newer developer guide which is connected with readthedocs.

jan-cerny · 2020-11-25T09:42:18Z

/test e2e-aws-ocp4-cis-node

jan-cerny · 2020-11-25T12:23:08Z

@vojtapolasek Excellent!

vojtapolasek added this to the 0.1.53 milestone Oct 27, 2020

openshift-ci-robot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 27, 2020

openshift-ci-robot added the needs-rebase Used by openshift-ci bot. label Oct 29, 2020

vojtapolasek force-pushed the friendlier_templates_2 branch 2 times, most recently from 8932403 to bfb2d8f Compare October 29, 2020 11:15

openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Oct 29, 2020

vojtapolasek marked this pull request as ready for review October 29, 2020 11:18

openshift-ci-robot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 29, 2020

ggbecker reviewed Oct 29, 2020

View reviewed changes

yuumasato reviewed Oct 30, 2020

View reviewed changes

vojtapolasek force-pushed the friendlier_templates_2 branch from 40b9f47 to 242aaeb Compare October 30, 2020 15:45

jan-cerny reviewed Nov 2, 2020

View reviewed changes

yuumasato modified the milestones: 0.1.53, 0.1.54 Nov 5, 2020

vojtapolasek force-pushed the friendlier_templates_2 branch from 242aaeb to b6533e6 Compare November 6, 2020 14:32

matejak mentioned this pull request Nov 16, 2020

Added simple lineinfile template #6389

Merged

openshift-ci-robot added the needs-rebase Used by openshift-ci bot. label Nov 20, 2020

rewrite templates.py

26d4600

remove template function callbacks rewrite data preprocessing to use new structure

vojtapolasek added 11 commits November 23, 2020 10:04

restructure the templates folder

16b9dd2

removed no longer used template

2511e1b

remove forgotten debug functionality

6df1eff

update docs on creating of new templates

31b0f7a

change names of anaconda remediation files to lower case

421e0ec

fix pep8

cc7ee84

remove no longer used template decorator function

bd41c88

rename template language files

a120ad6

add .template extension e.g. oval.template

update documentation

ca25268

rewrite for compatibility with python 2.7

1d72458

remove template.py if not needed

3a0cc88

if no preprocessing of template parameters is needed, template.py does not need to be present

vojtapolasek force-pushed the friendlier_templates_2 branch from 31ff0a8 to 3a0cc88 Compare November 23, 2020 09:04

openshift-ci-robot removed the needs-rebase Used by openshift-ci bot. label Nov 23, 2020

vojtapolasek added 2 commits November 23, 2020 11:41

implement support for template.yml

c90938e

add template.yml files

7d25be5

vojtapolasek added 3 commits November 23, 2020 11:52

update documentation

57b4048

remove no longer valid statement that template.py has to exist

ccdc777

copy documentation also into the readthedocs format

887d160

vojtapolasek requested a review from ggbecker November 23, 2020 17:54

jan-cerny self-assigned this Nov 25, 2020

jan-cerny merged commit e9c47c0 into ComplianceAsCode:master Nov 25, 2020

		# TODO: Remove this right after the variables in templates are renamed
		# to lowercase

		# scan directory structure and dynamically create list of templates
		for item in os.listdir(self.templates_dir):

Comments

Conversation

vojtapolasek commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Rationale:

Uh oh!

openshift-ci-robot commented Oct 27, 2020

Uh oh!

pep8speaks commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-11-23 17:53:15 UTC

Uh oh!

openscap-ci commented Oct 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vojtapolasek commented Oct 27, 2020

Uh oh!

redhatrises commented Oct 27, 2020

Uh oh!

yuumasato commented Oct 27, 2020

Uh oh!

vojtapolasek commented Oct 29, 2020

Uh oh!

carlosmmatos commented Oct 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggbecker commented Oct 29, 2020

Uh oh!

yuumasato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtapolasek Nov 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matejak Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vojtapolasek commented Oct 27, 2020 •

edited

Loading

pep8speaks commented Oct 27, 2020 •

edited

Loading

openscap-ci commented Oct 27, 2020 •

edited

Loading

vojtapolasek Nov 6, 2020 •

edited

Loading

matejak Nov 11, 2020 •

edited

Loading