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

Fix in service_autofs_disabled - ansible #10521

Conversation

rumch-se
Copy link
Contributor

@rumch-se rumch-se commented May 2, 2023

Description:

  • Fix of ansible remediation of the rule service_autofs_disabled

Rationale:

I.
The rule uses template service_disabled. There are 2 issues with the current version of this template. 1/The command systemctl in the code block Unit Socket Exists - (i.e. systemctl list-unit-files {{{ DAEMONNAME }}}.socket ) returns more than 1 row - example: UNIT FILE STATE VENDOR PRESET followed by the result. 2/ The code block Disable socket {{{ SERVICENAME }}} raises error when the socket_file_exists.stdout_lines is empty

II.
The template is used by other rules

@openshift-ci openshift-ci bot added the needs-ok-to-test Used by openshift-ci bot. label May 2, 2023
@openshift-ci
Copy link

openshift-ci bot commented May 2, 2023

Hi @rumch-se. 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

github-actions bot commented May 2, 2023

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

Fedora Environment
Open in Gitpod

Oracle Linux 8 Environment
Open in Gitpod

@codeclimate
Copy link

codeclimate bot commented May 2, 2023

Code Climate has analyzed commit b67213d 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.

@Mab879 Mab879 self-assigned this May 2, 2023
@Mab879
Copy link
Member

Mab879 commented May 2, 2023

/ok-to-test

@openshift-ci openshift-ci bot added ok-to-test Used by openshift-ci bot. and removed needs-ok-to-test Used by openshift-ci bot. labels May 2, 2023
@Mab879 Mab879 added the Ansible Ansible remediation update. label May 2, 2023
@Mab879 Mab879 added this to the 0.1.68 milestone May 2, 2023
@@ -19,7 +19,7 @@
meta: noop

- name: "Unit Socket Exists - {{{ DAEMONNAME }}}.socket"
command: systemctl list-unit-files {{{ DAEMONNAME }}}.socket
command: systemctl -q list-unit-files {{{ DAEMONNAME }}}.socket | grep "{{{ DAEMONNAME }}}.socket"
Copy link
Member

Choose a reason for hiding this comment

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

It is not necessary to use grep here. The systemctl command will only show the line if it exists. The return code will be 0 if the socket unit is present or 1 otherwise.

Copy link
Contributor Author

@rumch-se rumch-se May 3, 2023

Choose a reason for hiding this comment

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

Hello @marcusburghardt ,

On ubuntu and suse I have
When we don't have a socket:

u001@rchost:~$ systemctl list-unit-files time-sync.socket
UNIT FILE STATE VENDOR PRESET

0 unit files listed.

When we have a socket
u001@rchost:~$ systemctl list-unit-files libvirtd.socket
UNIT FILE STATE VENDOR PRESET
libvirtd.socket enabled enabled

1 unit files listed.

Whit grep in the first case we have

u001@rchost:$ systemctl -q list-unit-files time-sync.socket | grep time-sync.socket
u001@rchost:
$

Whit grep in the second case case we have
u001@rchost:~$ systemctl -q list-unit-files libvirtd.socket | grep libvirtd.socket
libvirtd.socket enabled enabled

The rule as exists on SUSE does not work.
Have a nice day
Rumen

Copy link
Member

Choose a reason for hiding this comment

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

I see, but grep is still not necessary. See this example:

$ systemctl -q list-unit-files sshd.socket ; echo $?
sshd.socket disabled disabled
0

$ systemctl -q list-unit-files absent.socket ; echo $?
1

You can simplify the command and the conditional which uses its return code.

Copy link
Contributor Author

@rumch-se rumch-se May 24, 2023

Choose a reason for hiding this comment

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

Hello @marcusburghardt
It does not work as expected. I have attached a screen shot taken from my console - Ubuntu 20.04. I think that the command systemctl was changed and after its change we have an additional message "UNIT FILE STATE VENDOR PRESET". Please try on another OS.
Have a nice day
Rumen
socket_issue_1
socket_issue_2

Copy link
Member

Choose a reason for hiding this comment

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

Ok, this change in the systemctl return codes for Ubuntu is new for me.
Even that, since you are using the search for the conditional below, it is still not necessary to use grep here because the output of systemctl command will also include the unit name if it is present.

Copy link
Contributor Author

@rumch-se rumch-se May 25, 2023

Choose a reason for hiding this comment

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

Hello @marcusburghardt

This way of working is not only for Ubuntu, but for SUSE as well (Note: SLE 15 SP 2 - works as before, but SLE 15 SP4 works in the new way). Only via grep I was able to get an empty string when socket does not exist or the name of the socket with its status when the socket exists. You can see that the usage of echo does not remove this "parasite row" - "UNIT FILE...." i.e I did not have a "plain vanilla" return code. May you check how this works on RedHat - different versions - old ones and a new one?

I can add if / else condition which check the product and to use the proposed syntax with grep for Ubuntu and SUSE.

Have a nice day
Rumen

@Mab879 Mab879 removed their assignment May 25, 2023
@jan-cerny jan-cerny modified the milestones: 0.1.68, 0.1.69 May 29, 2023
@marcusburghardt marcusburghardt self-assigned this Jun 2, 2023
@@ -19,7 +19,7 @@
meta: noop

- name: "Unit Socket Exists - {{{ DAEMONNAME }}}.socket"
command: systemctl list-unit-files {{{ DAEMONNAME }}}.socket
command: systemctl -q list-unit-files {{{ DAEMONNAME }}}.socket | grep "{{{ DAEMONNAME }}}.socket"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
command: systemctl -q list-unit-files {{{ DAEMONNAME }}}.socket | grep "{{{ DAEMONNAME }}}.socket"
command: systemctl -q list-unit-files {{{ DAEMONNAME }}}.socket"

This will save the output to the register

@@ -31,7 +31,7 @@
enabled: "no"
state: "stopped"
masked: "yes"
when: '"{{{ DAEMONNAME }}}.socket" in socket_file_exists.stdout_lines[1]'
when: 'socket_file_exists.stdout_lines is search("{{{ DAEMONNAME }}}.socket")'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
when: 'socket_file_exists.stdout_lines is search("{{{ DAEMONNAME }}}.socket")'
when: 'socket_file_exists.stdout_lines is search("{{{ DAEMONNAME }}}.socket", multiline=True)'

Here you are already using the search filter for searching for {{{ DAEMONNAME }}}.socket in the output of the previous command. It is good. You only need to make sure the search is considering multi-lines output.

@rumch-se
Copy link
Contributor Author

rumch-se commented Jun 2, 2023

Hello @marcusburghardt
I have implemented the proposed changes
Have a nice day
Rumen

@marcusburghardt marcusburghardt merged commit 5362bea into ComplianceAsCode:master Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ansible Ansible remediation update. ok-to-test Used by openshift-ci bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants