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

pipefail not really compatible with grep #6779

Merged

Conversation

brett060102
Copy link
Contributor

The pipefail option traps on all non 0 reurn codes, but grep exits
with 1 if the pattern is not found. So, just disable that ansible-lint
check if the shell snippet is using grep.

Description:

  • Disable ansible-lint pipefail test, if going grep

Rationale:

  • The pipefail option traps on all non 0 reurn codes, but grep exits with 1 if the pattern is not found. So, just disable that ansible-lint check if the shell snippet is using grep.

  • Without this the ansible play will hit pipefail trap if pattern is not found, which pretty much breaks, if not found, add cases.

  • Fixes # Issue number here (e.g. Updating sysctl XCCDF naming #26) or remove this line if no issue exists.

The pipefail option traps on all non 0 reurn codes, but grep exits
with 1 if the pattern is not found. So, just disable that ansible-lint
check if the shell snippet is using grep.
@openshift-ci-robot
Copy link
Collaborator

Hi @brett060102. 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.

@openshift-ci-robot openshift-ci-robot added the needs-ok-to-test Used by openshift-ci bot. label Mar 30, 2021
@openscap-ci
Copy link
Collaborator

openscap-ci commented Mar 30, 2021

Changes identified:
Rules:
 no_host_based_files
 no_user_host_based_files
 accounts_passwords_pam_tally2
 set_password_hashing_min_rounds_logindefs
 smartcard_pam_enabled
 accounts_no_uid_except_zero
 aide_verify_acls
 aide_verify_ext_attributes

Show details

Rule no_host_based_files:
 Ansible remediation changed.
Rule no_user_host_based_files:
 Ansible remediation changed.
Rule accounts_passwords_pam_tally2:
 Ansible remediation changed.
Rule set_password_hashing_min_rounds_logindefs:
 Ansible remediation changed.
Rule smartcard_pam_enabled:
 Ansible remediation changed.
Rule accounts_no_uid_except_zero:
 Ansible remediation changed.
Rule aide_verify_acls:
 Ansible remediation changed.
Rule aide_verify_ext_attributes:
 Ansible remediation changed.

Recommended tests to execute:
 build_product rhel8
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-rhel8-ds.xml no_host_based_files
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-rhel8-ds.xml no_user_host_based_files
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-rhel8-ds.xml accounts_no_uid_except_zero
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-rhel8-ds.xml aide_verify_acls
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-rhel8-ds.xml aide_verify_ext_attributes
 build_product sle12
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-sle12-ds.xml accounts_passwords_pam_tally2
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-sle12-ds.xml set_password_hashing_min_rounds_logindefs
 tests/test_suite.py rule --libvirt qemu:///system test-suite-vm --remediate-using ansible --datastream build/ssg-sle12-ds.xml smartcard_pam_enabled

@brett060102
Copy link
Contributor Author

We did find and fix this in our development branch, but I dropped that part of the fix when doing #6730

@@ -5,8 +5,7 @@
# disruption = low
- block:
- name: "Find local mount points"
shell: |
set -o pipefail
shell: | #noqa 306 we don't care about grep failure in this case
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately the yaml libraries strip out comments when processing them.
The exception for ansible-lint should be added here: https://github.com/ComplianceAsCode/content/blob/master/tests/ansible-lint_config.yml

@vojtapolasek
Copy link
Collaborator

Here is an alternative approach which does not require removal of ansible-lint checks. If it works.
https://stackoverflow.com/questions/6550484/prevent-grep-returning-an-error-when-input-doesnt-match

@brett060102
Copy link
Contributor Author

@vojtapolasek I know about the workaround options, but they look messy to me.
I am trying to come up with something that will be at least somewhat clean.
Would like to avoid turning off the test globally if possible.

@brett060102 brett060102 closed this Apr 1, 2021
@brett060102 brett060102 deleted the fix_ansible_link_fixes branch April 1, 2021 13:47
@brett060102 brett060102 restored the fix_ansible_link_fixes branch April 1, 2021 13:48
@brett060102 brett060102 reopened this Apr 1, 2021
@brett060102
Copy link
Contributor Author

@vojtapolasek Think this should be OK, now. In pam_options/ansible.template and set_password_hashing_min_rounds_logindefs/ansible/shared.yml I tried usimh a single line command since there is no piping used there, but ansible-lint still objected. This code now makes more sense than the original. The "| cat" was just being used to eat the "1" return code from grep, the execute "true" on failure is really a better way to do that. Thanks for your time on this.

@brett060102
Copy link
Contributor Author

@vojtapolasek should be done messing with this now.

@brett060102
Copy link
Contributor Author

@vojtapolasek How does this one look now?

@vojtapolasek
Copy link
Collaborator

I am checking it, give me a bit more time, there are other warnings and I need to filter them out.

@vojtapolasek
Copy link
Collaborator

I think it looks good. Thank you for fixing this.

@vojtapolasek vojtapolasek merged commit e5919b5 into ComplianceAsCode:master Apr 7, 2021
@yuumasato yuumasato added this to the 0.1.56 milestone Apr 7, 2021
@brett060102
Copy link
Contributor Author

@vojtapolasek Thank you.

@brett060102 brett060102 deleted the fix_ansible_link_fixes branch June 28, 2023 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants