Add option to enable installation of individual ansible playbooks per rule #7039

ggbecker · 2021-05-24T15:27:16Z

Description:

Add option to enable installation of individual ansible tasks per rule.

openshift-ci · 2021-05-24T15:27:17Z

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

jan-cerny · 2021-05-25T14:06:10Z

CMakeLists.txt

 option(SSG_SEPARATE_SCAP_FILES_ENABLED "If enabled, separate SCAP files (OVAL, XCCDF, CPE dict, ...) will be installed alongside the source data-streams" TRUE)
 option(SSG_ANSIBLE_PLAYBOOKS_ENABLED "If enabled, Ansible Playbooks for each profile will be built and installed." TRUE)
 option(SSG_BASH_SCRIPTS_ENABLED "If enabled, Bash remediation scripts for each profile will be built and installed." TRUE)
+option(SSG_ANSIBLE_TASKS_ENABLED "If enabled, Ansible Tasks for each rule will be installed." FALSE)


What about guarding the call of the ssg_build_ansible_playbooks macro by this option?

it makes sense

can you please check if this is what you had in mind? 1ec2353

jan-cerny · 2021-05-25T14:06:43Z

CMakeLists.txt

 option(SSG_SEPARATE_SCAP_FILES_ENABLED "If enabled, separate SCAP files (OVAL, XCCDF, CPE dict, ...) will be installed alongside the source data-streams" TRUE)
 option(SSG_ANSIBLE_PLAYBOOKS_ENABLED "If enabled, Ansible Playbooks for each profile will be built and installed." TRUE)
 option(SSG_BASH_SCRIPTS_ENABLED "If enabled, Bash remediation scripts for each profile will be built and installed." TRUE)
+option(SSG_ANSIBLE_TASKS_ENABLED "If enabled, Ansible Tasks for each rule will be installed." FALSE)


In fact, these files are Ansible Playbooks, so using the term "tasks" is misleading.

I understand that... but how should I call them since SSG_ANSIBLE_PLAYBOOKS_ENABLED is already being used?

maybe SSG_ANSIBLE_INDIVIDUAL_PLAYBOOKS_ENABLED or SSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED?

I've changed to SSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED, see: 81f9051#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR51

jan-cerny · 2021-05-25T14:08:16Z

cmake/SSGCommon.cmake

+    if(SSG_ANSIBLE_TASKS_ENABLED)
+        install(
+            CODE "
+            file(GLOB ROLE_FILES \"${CMAKE_BINARY_DIR}/${PRODUCT}/playbooks/*.yml\") \n


In fact, these files are Ansible Playbooks and you also have used the misleading word "tasks" for it before so I think using the term "role" for them increases confusion.

that's correct... it was a copy paste problem

I have also changed the folder's name that holds all the ansible playbooks from tasks to playbooks. So files will be installed to ansible/playbooks/

JAORMX · 2021-05-26T05:16:59Z

/retest

CMakeLists.txt

cmake/SSGCommon.cmake

jan-cerny · 2021-05-27T09:01:12Z

cmake/SSGCommon.cmake

+            CODE "
+            file(GLOB PLAYBOOK_PER_RULE_FILES \"${CMAKE_BINARY_DIR}/${PRODUCT}/playbooks/*\") \n
+            if(NOT IS_ABSOLUTE ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/playbooks)
+                file(INSTALL DESTINATION \"\${CMAKE_INSTALL_PREFIX}/${SSG_ANSIBLE_ROLE_INSTALL_DIR}/playbooks/${PRODUCT}\"


I'm sorry, I had a comment but I apparently didn't submit it or I misclicked. My comment was that the ${SSG_ANSIBLE_ROLE_INSTALL_DIR} is already used to install profile playbooks. But the downstream Linux distributions will want to place the rule playbooks to a different subpackage than the profile playbooks. Therefore I think it might be better to put the rule playbooks to a different directory than ${SSG_ANSIBLE_ROLE_INSTALL_DIR}. What is your opinion on that?

I understand your concern, but it would feel really strange to have ansible playbooks on a different folder than {SSG_ANSIBLE_ROLE_INSTALL_DIR} (which is usually scap-security-guide/ansible). That's why a created a subfolder called playbooks where this files would be installed. I guess the fact that downstream Linux distributions will use these files on a different subpackage doesn't necessarily mean we have to install these files on a different top-level directory.

but it would feel really strange to have ansible playbooks on a different folder than {SSG_ANSIBLE_ROLE_INSTALL_DIR}

It depends, something like /usr/share/scap-security-guide/rule_ansible_playbooks/ could work.

I guess the fact that downstream Linux distributions will use these files on a different subpackage doesn't necessarily mean we have to install these files on a different top-level directory.

That's correct, it's certainly possible to pick the right items for the (sub)packages in spec file.

I mean I think it's fine in the current form, I was curious if we could come up with something possibly better or not.

As the profile playbooks installed in ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/ are actually playbooks (not roles), how about installing the rule playbooks in ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/rule_playbooks/${PRODUCT}?

I agree that the naming is a bit confusing here. I guess it's better to have explicit than some hidden meaning. I'll update it accordingly. Thanks

jan-cerny · 2021-05-27T11:53:51Z

/retest

yuumasato

Just running cmake I get the following warning for each product:

CMake Warning (dev) at cmake/SSGCommon.cmake:804 (add_dependencies):          
  Policy CMP0046 is not set: Error on non-existent dependency in                                                                                             
  add_dependencies.  Run "cmake --help-policy CMP0046" for policy details.    
  Use the cmake_policy command to set the policy and suppress this warning.

  The dependency target "generate-chromium-ansible-playbooks" of target
  "chromium-content" does not exist.
Call Stack (most recent call first):
  chromium/CMakeLists.txt:6 (ssg_build_product)
This warning is for project developers.  Use -Wno-dev to suppress it.

The problem is that SSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED being false by default, disables ssg_build_ansible_playbooks(${PRODUCT}) which would add the custom_target generate-${PRODUCT}-ansible-playbooks.

ggbecker · 2021-05-27T13:27:57Z

Just running cmake I get the following warning for each product:

CMake Warning (dev) at cmake/SSGCommon.cmake:804 (add_dependencies):          
  Policy CMP0046 is not set: Error on non-existent dependency in                                                                                             
  add_dependencies.  Run "cmake --help-policy CMP0046" for policy details.    
  Use the cmake_policy command to set the policy and suppress this warning.

  The dependency target "generate-chromium-ansible-playbooks" of target
  "chromium-content" does not exist.
Call Stack (most recent call first):
  chromium/CMakeLists.txt:6 (ssg_build_product)
This warning is for project developers.  Use -Wno-dev to suppress it.

The problem is that SSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED being false by default, disables ssg_build_ansible_playbooks(${PRODUCT}) which would add the custom_target generate-${PRODUCT}-ansible-playbooks.

That's why I had initially an OR logic expression in the condition for ssg_build_ansible_playbooks call... I guess I'll have to reevaluate some things. Thanks for noticing that.

yuumasato · 2021-05-27T13:36:24Z

That's why I had initially an OR logic expression in the condition for ssg_build_ansible_playbooks call... I guess I'll have to reevaluate some things. Thanks for noticing that.

I think the AND makes sense. A solution could be to declare the custom target directly in the ssg_build_product macro, like in:

content/cmake/SSGCommon.cmake

Line 755 in a705438

add_custom_target(${PRODUCT}-content)

ggbecker · 2021-05-27T14:04:42Z

That's why I had initially an OR logic expression in the condition for ssg_build_ansible_playbooks call... I guess I'll have to reevaluate some things. Thanks for noticing that.

I think the AND makes sense. A solution could be to declare the custom target directly in the ssg_build_product macro, like in:

content/cmake/SSGCommon.cmake

Line 755 in a705438

add_custom_target(${PRODUCT}-content)

hopefully that's what you had in mind :) f7bb68f

yuumasato

hopefully that's what you had in mind :) f7bb68f

Almost there, see my comments.

cmake/SSGCommon.cmake

ggbecker · 2021-05-27T14:29:37Z

hopefully that's what you had in mind :) f7bb68f

Almost there, see my comments.

Okay, done in 141dce8 (amended commit)

yuumasato · 2021-05-27T15:37:16Z

hopefully that's what you had in mind :) f7bb68f

Almost there, see my comments.

Okay, done in 141dce8 (amended commit)

Now I get warnings when doing this, :(
cmake -DSSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED:BOOLEAN=ON ../

@ggbecker I apologize, my suggestion doesn't work as expected. add_dependencies() only works on targets, not files.

I think you'll need to have a better look on why this dependency is added here:

content/cmake/SSGCommon.cmake

Lines 805 to 808 in 141dce8

    
                   add_dependencies( 
        
                       ${PRODUCT}-content 
        
                       generate-${PRODUCT}-ansible-playbooks 
        
                   )

To me it looks like it should be moved closer to where it is defined.

ggbecker · 2021-05-31T14:26:40Z

hopefully that's what you had in mind :) f7bb68f

Almost there, see my comments.

Okay, done in 141dce8 (amended commit)

Now I get warnings when doing this, :(
cmake -DSSG_ANSIBLE_PLAYBOOKS_PER_RULE_ENABLED:BOOLEAN=ON ../

@ggbecker I apologize, my suggestion doesn't work as expected. add_dependencies() only works on targets, not files.

I think you'll need to have a better look on why this dependency is added here:

content/cmake/SSGCommon.cmake

Lines 805 to 808 in 141dce8

add_dependencies(

${PRODUCT}-content

generate-${PRODUCT}-ansible-playbooks

)

To me it looks like it should be moved closer to where it is defined.

I brought the dependency definition closer to the target definition. That might fix the issue, although I'm not entirely sure if this would cause any undesired side effect. See: 02df119

yuumasato · 2021-05-31T15:44:58Z

@ggbecker The rule playbooks were not being built with 02df119.
I made a PR to your branch.
It would also be good to squash the last few commits to clean up a bit this back and forth with the target and dependencies.

ggbecker · 2021-05-31T15:53:42Z

@ggbecker The rule playbooks were not being built with 02df119.
I made a PR to your branch.

Thank you very much.

It would also be good to squash the last few commits to clean up a bit this back and forth with the target and dependencies.

I definitely will :D

yuumasato

LGTM, I just have one comment about the install path of rule playbooks.

yuumasato · 2021-06-01T09:04:58Z

cmake/SSGCommon.cmake

+            CODE "
+            file(GLOB PLAYBOOK_PER_RULE_FILES \"${CMAKE_BINARY_DIR}/${PRODUCT}/playbooks/*\") \n
+            if(NOT IS_ABSOLUTE ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/playbooks)
+                file(INSTALL DESTINATION \"\${CMAKE_INSTALL_PREFIX}/${SSG_ANSIBLE_ROLE_INSTALL_DIR}/playbooks/${PRODUCT}\"


As the profile playbooks installed in ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/ are actually playbooks (not roles), how about installing the rule playbooks in ${SSG_ANSIBLE_ROLE_INSTALL_DIR}/rule_playbooks/${PRODUCT}?

A dependency on rule playbooks target was being added from a conditional branch related to profile playbooks. It caused issues when building profile playbooks but not rule playbooks, the rule playbooks target would not exist, but still be added as dependency. Co-authored-by: Watson Sato <[email protected]>

ggbecker · 2021-06-01T13:43:46Z

LGTM, I just have one comment about the install path of rule playbooks.

I have updated the installing directory.

yuumasato · 2021-06-01T14:57:19Z

@JAORMX Hey man, I see a a lot of "didn't match expected results" in the rhcos e2e, are these somehow expected?
I don't see how changes in this PR would cause it.
The expected result for the audit_rules_etc_group_open rule didn't match. Expected 'PASS', Got 'FAIL'

yuumasato · 2021-06-01T16:38:18Z

/retest

yuumasato · 2021-06-02T07:58:37Z

Test results look good now, :)

Add option to enable installation of individual ansible tasks per rule.

7283a29

ggbecker added the Ansible Ansible remediation update. label May 24, 2021

ggbecker added this to the 0.1.57 milestone May 24, 2021

openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label May 24, 2021

jan-cerny reviewed May 25, 2021

View reviewed changes

jan-cerny changed the title ~~Add option to enable installation of individual ansible tasks per rule.~~ Add option to enable installation of individual ansible tasks per rule May 25, 2021

Rename ansible per rule cmake option.

81f9051

ggbecker force-pushed the individual-ansible-tasks branch 2 times, most recently from fadda7a to 035fe7a Compare May 25, 2021 15:43

ggbecker marked this pull request as ready for review May 25, 2021 15:48

openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label May 25, 2021

ggbecker force-pushed the individual-ansible-tasks branch from 035fe7a to b033813 Compare May 25, 2021 15:49

jan-cerny reviewed May 27, 2021

View reviewed changes

ggbecker added 2 commits May 27, 2021 12:58

Guard build of playbooks per rule by a new CMake Option.

2f424af

Print message for CMake option enable ansible playbooks per rule.

406a49b

ggbecker force-pushed the individual-ansible-tasks branch from b033813 to a705438 Compare May 27, 2021 10:58

yuumasato requested changes May 27, 2021

View reviewed changes

cmake/SSGCommon.cmake Outdated Show resolved Hide resolved

cmake/SSGCommon.cmake Show resolved Hide resolved

ggbecker force-pushed the individual-ansible-tasks branch from f7bb68f to 141dce8 Compare May 27, 2021 14:28

ggbecker changed the title ~~Add option to enable installation of individual ansible tasks per rule~~ Add option to enable installation of individual ansible playbooks per rule May 31, 2021

ggbecker force-pushed the individual-ansible-tasks branch from 605f4a3 to f637c88 Compare May 31, 2021 16:09

yuumasato approved these changes Jun 1, 2021

View reviewed changes

ggbecker and others added 2 commits June 1, 2021 14:49

Fix path of gathered ansible playbooks per rule.

5a185a6

ggbecker force-pushed the individual-ansible-tasks branch from f637c88 to 8b99c9c Compare June 1, 2021 12:50

yuumasato merged commit 04055c6 into ComplianceAsCode:master Jun 2, 2021

ggbecker mentioned this pull request Jun 8, 2021

Only enable ansible/yaml lint tests when playbooks are built #7099

Merged

Add option to enable installation of individual ansible playbooks per rule #7039

Add option to enable installation of individual ansible playbooks per rule #7039

Uh oh!

Conversation

ggbecker commented May 24, 2021

Description:

Uh oh!

openshift-ci bot commented May 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggbecker May 25, 2021 • 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

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 May 25, 2021 • 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!

JAORMX commented May 26, 2021

Uh oh!

Uh oh!

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!

jan-cerny commented May 27, 2021

Uh oh!

yuumasato left a comment • 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!

ggbecker commented May 27, 2021

Uh oh!

yuumasato commented May 27, 2021

Uh oh!

ggbecker commented May 27, 2021

Uh oh!

yuumasato left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ggbecker commented May 27, 2021

Uh oh!

yuumasato commented May 27, 2021

Uh oh!

ggbecker commented May 31, 2021

Uh oh!

yuumasato commented May 31, 2021

Uh oh!

ggbecker commented May 31, 2021

Uh oh!

yuumasato left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggbecker commented Jun 1, 2021

Uh oh!

yuumasato commented Jun 1, 2021

ggbecker May 25, 2021 •

edited

Loading

ggbecker May 25, 2021 •

edited

Loading

yuumasato left a comment •

edited

Loading