Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use mkdir -p when creating directories #10556

Merged
merged 31 commits into from
May 17, 2023

Conversation

maage
Copy link
Contributor

@maage maage commented May 13, 2023

Description:

This ensure mkdir does not fail if directory already exists.

Also avoid unnecessary tests.

Added simple style changes.

Rationale:

Simplify code.

Review Hints:

Changes are all over unfortunately.

maage added 27 commits May 13, 2023 21:03
@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label May 13, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 13, 2023

Hi @maage. Thanks for your PR.

I'm waiting for a ComplianceAsCode member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@github-actions
Copy link

Start a new ephemeral environment with changes proposed in this pull request:

rhel8 (from CTF) Environment (using Fedora as testing environment)
Open in Gitpod

Fedora Testing Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@github-actions
Copy link

github-actions bot commented May 13, 2023

This datastream diff is auto generated by the check Compare DS/Generate Diff

Click here to see the full diff
bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp' differs.
--- xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp
+++ xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_tmp
@@ -1,9 +1,10 @@
 # Remediation is applicable only in certain platforms
 if ! ( [ "${container:-}" == "bwrap-osbuild" ] ); then
 
-if ! [ -d /tmp/tmp-inst ] ; then
- mkdir --mode 000 /tmp/tmp-inst
-fi
+#!/bin/bash
+
+# shellcheck disable=SC2174
+mkdir -p --mode 000 /tmp/tmp-inst
 chmod 000 /tmp/tmp-inst
 chcon --reference=/tmp /tmp/tmp-inst
 

bash remediation for rule 'xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp' differs.
--- xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp
+++ xccdf_org.ssgproject.content_rule_accounts_polyinstantiated_var_tmp
@@ -1,11 +1,12 @@
 # Remediation is applicable only in certain platforms
 if ! ( [ "${container:-}" == "bwrap-osbuild" ] ); then
 
-if ! [ -d /tmp-inst ] ; then
- mkdir --mode 000 /var/tmp/tmp-inst
-fi
+#!/bin/bash
+
+# shellcheck disable=SC2174
+mkdir -p --mode 000 /var/tmp/tmp-inst
 chmod 000 /var/tmp/tmp-inst
-chcon --reference=/var/tmp/ /var/tmp/tmp-inst
+chcon --reference=/var/tmp /var/tmp/tmp-inst
 
 if ! grep -Eq '^\s*/var/tmp\s+/var/tmp/tmp-inst/\s+level\s+root,adm$' /etc/security/namespace.conf ; then
 if grep -Eq '^\s*/var/tmp\s+' /etc/security/namespace.conf ; then

bash remediation for rule 'xccdf_org.ssgproject.content_rule_sssd_enable_pam_services' differs.
--- xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
+++ xccdf_org.ssgproject.content_rule_sssd_enable_pam_services
@@ -5,7 +5,7 @@
 SSSD_CONF_DIR="/etc/sssd/conf.d/*.conf"
 
 if [ ! -f "$SSSD_CONF" ] && [ ! -f "$SSSD_CONF_DIR" ]; then
- mkdir /etc/sssd
+ mkdir -p /etc/sssd
 touch "$SSSD_CONF"
 fi

@maage
Copy link
Contributor Author

maage commented May 14, 2023

Failures here is good example why something like my #10387 is needed.

Maybe there should be some different test phase / code phrase than "ERROR" to catch these. I think this just means we try to test some rule but it is totally not configured for product. Tests did not fail.

2023-05-13T20:40:06.6231004Z ERROR - Rule 'xccdf_org.ssgproject.content_rule_dir_system_commands_root_owned' isn't present in benchmark 'xccdf_org.ssgproject.content_benchmark_RHEL-8' in '/tmp/ssgts-ds-qa6ravy7'
2023-05-13T20:40:06.6287962Z ERROR - Rule 'xccdf_org.ssgproject.content_rule_dir_system_commands_group_root_owned' isn't present in benchmark 'xccdf_org.ssgproject.content_benchmark_RHEL-8' in '/tmp/ssgts-ds-qa6ravy7'

I'll add fix for mount_option_nodev_nonroot_local_partitions shortly. But it seems to me that it has never worked in containerized env. My test container has /dev with size 64MB and test tries to create 200MB test file to create partition and it fails.

I'll add fix for dir_perms_world_writable_root_owned as it did not specially handle /proc. And I think it should exclude it.

dir_perms_world_writable_root_owned test world_writable_dir_on_nonlocal_fs.fail.sh tries to setup NFS server (and mount_option_nodev_nonroot_local_partitions remote_without_nodev.pass.sh) and it fails in containers because there is no nfsd kernel module and container does not have and most probably never will acquire permissions to load kernel modules. This type of tests should be skipped in containerized envs and tested in virtual environments. And configuration should be per test case as other tests cases might work.

@codeclimate
Copy link

codeclimate bot commented May 14, 2023

Code Climate has analyzed commit 7205a61 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 52.4% (0.0% change).

View more on Code Climate.

@maage
Copy link
Contributor Author

maage commented May 14, 2023

Well, mount_option_nodev_nonroot_local_partitions tests does not work in containerized env as it uses mount and that requires CAP_SYS_ADMIN and it is missing and most probably not going to be there anyways.

@marcusburghardt marcusburghardt self-assigned this May 15, 2023
@marcusburghardt marcusburghardt added this to the 0.1.68 milestone May 15, 2023
@marcusburghardt marcusburghardt added enhancement General enhancements to the project. Test Suite Update in Test Suite. labels May 15, 2023
Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @maage . I see the last commits in this PR are not related to mkdir -p changes. Please, move these last commits to another PR and keep this only about mkdir changes.

Copy link
Member

@marcusburghardt marcusburghardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for these improvements and also thanks for the great analysis in #10556 (comment).

I confirm the errors reported in the containers tests (automatus) can be waived in this PR.

@marcusburghardt marcusburghardt merged commit 38b17cd into ComplianceAsCode:master May 17, 2023
@maage maage deleted the mkdir-p-1 branch May 18, 2023 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancements to the project. needs-ok-to-test Used by openshift-ci bot. Test Suite Update in Test Suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants