Skip to content

Conversation

@Mab879
Copy link
Member

@Mab879 Mab879 commented Oct 15, 2021

Description:

Escaped the $ in rsyslog_encrypt_offload_*

Rationale:

@Mab879 Mab879 added the bugfix Fixes to reported bugs. label Oct 15, 2021
@Mab879 Mab879 added this to the 0.1.59 milestone Oct 15, 2021
@vojtapolasek
Copy link
Collaborator

The fix won't be that simple unfortunately. It needs to be fixed within one of macros. Ideally it would be nice to use the regex_escape filter.
http://ansible-docs.readthedocs.io/zh/stable-2.0/rst/playbooks_filters.html

@Mab879 Mab879 force-pushed the rsyslog_encrypt_offload_fix_7741 branch from 1554dcd to b8d6799 Compare October 18, 2021 18:52
@Mab879
Copy link
Member Author

Mab879 commented Oct 18, 2021

/retest-required

@Mab879 Mab879 marked this pull request as draft October 18, 2021 21:16
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 18, 2021
@vojtapolasek
Copy link
Collaborator

Just a thought, is there actually a case where we would NOT want the regex to be escaped? Would it make sense to do this implicitly?

@yuumasato
Copy link
Member

Just a thought, is there actually a case where we would NOT want the regex to be escaped? Would it make sense to do this implicitly?

I think it makes sense to always escape the parameter.
The ansible_set_config_file_dir macro docs doesn't make it clear that parameter can be a regex.
And although it is used to construct a line_regex for ansible_lineinfile macro, the parameter is not expected to be a regular expression.

Also, the escape_regex argument is not clear about what regex it is escaping.
If we go the way of explicit option, I'd suggest to change it to escape_parameter, and rename parameter to parameter_regex.

@yuumasato
Copy link
Member

Also, could having parameter as a regular expression have unintended consequences such as adding a value to multiple parameters?

@Mab879 Mab879 closed this Oct 21, 2021
@Mab879 Mab879 force-pushed the rsyslog_encrypt_offload_fix_7741 branch from b8d6799 to 20491c7 Compare October 21, 2021 19:08
@Mab879 Mab879 reopened this Oct 25, 2021
Copy link
Member

@yuumasato yuumasato left a comment

Choose a reason for hiding this comment

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

LGTM, I have just a few nitpicks, :P

@Mab879 Mab879 marked this pull request as ready for review October 29, 2021 17:15
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Used by openshift-ci bot. label Oct 29, 2021
@yuumasato yuumasato self-assigned this Nov 1, 2021
@yuumasato
Copy link
Member

/retest

@Mab879
Copy link
Member Author

Mab879 commented Nov 1, 2021

I'm having a hard time getting regexp: '{{ {{{ regex }}} | regex_escape }}' to play nice with build system and Ansible lint. Not sure how best to make it work.

@Mab879 Mab879 requested a review from vojtapolasek November 2, 2021 13:33
@yuumasato
Copy link
Member

Another thing I also noticed is that the OVAL checks for rsyslog_encrypt_offload_defaultnetstreamdriver and related don't handle cases when /etc/rsyslog.conf has the parameter witt correct, but /etc/rsyslog.d/somefile.conf` has the parameter with wrong value.

I guess we want to fail in these cases? As the Ansible remediation always removes configurations in /etc/rsyslog.d/.
And unfortunately the Bash remediation is a bit behind and only checks for /etc/rsyslog.conf.

But I think these are out of the original scope of this PR and can be handled later...

@yuumasato yuumasato dismissed jan-cerny’s stale review November 4, 2021 13:05

The isuse pointed out by Jan has been addressed.

@yuumasato
Copy link
Member

/retest

@yuumasato
Copy link
Member

@Mab879 LGTM, before I merge, could you address the ansible-lint issues?

@openshift-ci
Copy link

openshift-ci bot commented Nov 4, 2021

@Mab879: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-ocp4-e8 c29550e link true /test e2e-aws-ocp4-e8
ci/prow/e2e-aws-ocp4-cis-node c29550e link true /test e2e-aws-ocp4-cis-node

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@yuumasato yuumasato merged commit 1720f0e into ComplianceAsCode:master Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix Fixes to reported bugs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rules rsyslog_encrypt_offload_* have wrong regex

4 participants