Newly added test cases : disable/enable audit_logging in libvirtd.log #4421

chunfuwen · 2022-08-15T06:33:29Z

Newly added test cases : disable/enable audit_logging in libvirtd.log

audit_logging = 1 to let audit messages also be sent via libvirt logging infrastructure as well as the auditd logging

Signed-off-by: chunfuwen chwen@redhat.com

chunfuwen · 2022-08-15T06:35:33Z

DATA (filename=output.expected) => NOT FOUND (data sources: variant, test, file)
DATA (filename=stdout.expected) => NOT FOUND (data sources: variant, test, file)
DATA (filename=stderr.expected) => NOT FOUND (data sources: variant, test, file)
PASS 1-type_specific.io-github-autotest-libvirt.conf_file.libvirtd_conf.set_audit_logging.positive_test.enable_audit_log

cleaning libvirtd logs...
DATA (filename=output.expected) => NOT FOUND (data sources: variant, test, file)
DATA (filename=stdout.expected) => NOT FOUND (data sources: variant, test, file)
DATA (filename=stderr.expected) => NOT FOUND (data sources: variant, test, file)
PASS 1-type_specific.io-github-autotest-libvirt.conf_file.libvirtd_conf.set_audit_logging.positive_test.disable_audit_log

chunfuwen · 2022-08-24T00:18:05Z

[xx-185265] [audit_logging] Disable/enable audit_logging in libvirtd.log

chunfuwen · 2022-08-24T00:18:33Z

@yafu-1 , please help take a review

yafu-1 · 2022-08-24T04:44:29Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        vm.start()
+
+        # Check audit daemon service status
+        check_auditd_service()


How about mv 'check_auditd_service()' before start vm and can try to start auditd service if service is not running.

@yafu-1
updated as suggested

yafu-1

LGTM

chunfuwen · 2022-08-30T02:24:06Z

@dzhengfy
please help take a review

vasekhodina

I have selected this PR randomly for my first review. I like how the functions are named and generally that the code doesn't jump between abstraction layers in one function. I have added a few minor suggestions.

vasekhodina · 2022-09-02T17:38:04Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        :return: enable_audit_logging: bool indicating enable/disable audit_logging
+        """
+        log_outputs = "1:file:%s" % log_config_path
+        libvirtd_config.log_outputs = log_outputs


Wouldn't it be sufficient to have libvirtd_config = "1:file:%s" % log_config_path instead of log_outputs var that is not used anywhere else?

vasekhodina · 2022-09-02T17:46:59Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        """
+        Configure audit log level"
+
+        :param params: dict wrapped with params


There is no params parameter to this function. Remove this line.

remove this line

vasekhodina · 2022-09-02T17:48:07Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        Configure audit log level"
+
+        :param params: dict wrapped with params
+        :return: enable_audit_logging: bool indicating enable/disable audit_logging


There is no return statement in this function. The return word here would be better as :param since enable_audit_logging is a parameter to this function.

remove return statement since there is nothing returned

vasekhodina · 2022-09-02T17:57:14Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        status = service_mgr.status(service_name)
+        LOG.debug('Service status is %s', status)
+        if not status:
+            service_mgr.start(service_name)


Isn't the clean code rule of "Functions should do one thing. They should do it well. They should do it only." broken here since we're not only checking but also starting the service?

Wouldn't it be better to move lines 52 and 53 out of this function?
OR
Rename the function to "ensure_auditd_started()"

rename the function name to ensure_auditd_started

vasekhodina · 2022-09-02T18:04:20Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        """
+        Check audit message in libvirtd log file
+        """
+        str_to_grep = "virDomainAudit"


Wouldn't this be better as a function argument?

updated with str_to_grep as function parameter

vasekhodina · 2022-09-02T18:07:18Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        process.run(cmd, ignore_status=True,
+                    shell=True)
+
+    def check_msg_in_libvirtd_log():


Wouldn't it be better to move this function definition and well as all other function definitions outside of the run() function? It would make run() function a lot smaller.

@vasekhodina
Before team members had some discussion about which is better whether function is outside run or inside run.
There is no conclusion for this, and it is decided by individual to trade off this.
1)Outside run, it may need pass more function parameter since those function can not share variables already in run method
2) Inside run, it may look like run is full busy code, but if functions are relative small, it sound ok, and it also can save parameter passing

Ok, I understand. Thank you for the explanation.

vasekhodina · 2022-09-02T18:10:02Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        vm.start()
+        # Check virt type in audit.log
+        check_virt_type_from_audit_log()
+        # Check audit msg in libvirtd log


I think the comment here and on line 100 is redundant and should be deleted. It says the same thing as the function name.

updated by removing comments

chunfuwen · 2022-09-05T07:23:37Z

@vasekhodina
comments are addressed, please double check

vasekhodina · 2022-09-05T11:23:01Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+
+    def ensure_auditd_started():
+        """
+        Check audit service status"


I think it would be good to change the comment to Check audit service status and start it if it's not running.

@vasekhodina updated as suggested

chunfuwen · 2022-09-13T08:11:24Z

@dzhengfy , please help take a review also since there is coming cases, which depends on this

vasekhodina

LGTM

dzhengfy · 2022-09-06T06:42:11Z

libvirt/tests/cfg/conf_file/libvirtd_conf/set_audit_logging.cfg

@@ -0,0 +1,10 @@
+- conf_file.libvirtd_conf.set_audit_logging:


Refer to other naming style, we do not add verb in the fie name. So would you consider to change it like audit_log.cfg

@dzhengfy
I have no idea why file name don't add verb, could you point to me the link?
In libvirt virtualization, update/attach/detach/set/enable/disable are fundamental features and API exposed by libvirt.
So I am confused why not those key actions can be not in filename

dzhengfy · 2022-09-06T06:43:08Z

libvirt/tests/cfg/conf_file/libvirtd_conf/set_audit_logging.cfg

+    variants:
+        - positive_test:
+            variants:
+                - disable_audit_log:


- disable: is clear enough

@dzhengfy
variants name will become part of test case
In this specific situation, the test case is related short, it looks like libvirtd_conf.set_audit_logging.positive_test.enable_audit_log is
not worse than libvirtd_conf.set_audit_logging.positive_test.enable in human friendly and human readable ways

dzhengfy · 2022-09-06T06:43:17Z

libvirt/tests/cfg/conf_file/libvirtd_conf/set_audit_logging.cfg

+            variants:
+                - disable_audit_log:
+                    enable_audit = no
+                - enable_audit_log:


same as above

@dzhengfy
see previous replied comment

dzhengfy · 2022-09-06T06:48:41Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        Check whether virt type: VIRT_CONTROL|VIRT_MACHINE_ID|VIRT_RESOURCE in audit.log
+        """
+        cmd = 'cat /var/log/audit/audit.log |grep -E "type=VIRT_CONTROL|VIRT_MACHINE_ID|VIRT_RESOURCE"'
+        process.system_output(cmd, shell=True, verbose=True)


It seems this line is duplicated. Remove it?

Good catch, remove it

dzhengfy · 2022-09-06T06:54:23Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        cmd = 'cat /var/log/audit/audit.log |grep -E "type=VIRT_CONTROL|VIRT_MACHINE_ID|VIRT_RESOURCE"'
+        process.system_output(cmd, shell=True, verbose=True)
+        if process.system(cmd, ignore_status=True, shell=True):
+            test.fail("Check virt type failed with %s" % cmd)


Before test.fail(), we'd better to log the content of 'cat /var/log/audit/audit.log'

@dzhengfy
/var/log/audit/audit.log' has large numbers of logs, which may distract troubleshooter's mind if all dump into output console
By the way, the command : /var/log/audit/audit.log clearly indicate if you want all log, just cat existed log file:/var/log/audit/audit.log

dzhengfy · 2022-09-06T06:55:36Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        Clean up audit message in log file.
+        """
+        cmd = "> /var/log/audit/audit.log; > %s" % log_config_path
+        process.run(cmd, ignore_status=True,


ignore_status=True should be False because there is no code to check the return result.

@dzhengfy
if this is called at the beginning of test case, log_config_path may not exist at that time, so it may not be proper to check process.run
,in other situation, for example at end of test case, it is proper to check that.
Personally thinking,whether clean up succeed or not has no direct impact on test case itself

By the way, I also met similar situation during my review, see avocado-framework/avocado-vt#3521

dzhengfy · 2022-09-06T07:20:44Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+    enable_audit_logging = "yes" == params.get("enable_audit", "no")
+
+    # Get LibvirtdConfig or VirtQemuConfig
+    log_config_path = os.path.join(data_dir.get_data_dir(), "libvirtd.log")


log file could be put into get_tmp_dir() where you need not to clean it by yourself.

dzhengfy · 2022-09-06T07:29:03Z

libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py

+        :param enable_audit_logging: bool indicating enable/disable audit_logging
+        """
+        libvirtd_config.log_outputs = "1:file:%s" % log_config_path
+        libvirtd_config.log_level = 1


Above two lines are unnecessary because the avocado-vt env_process.py has enabled the debug log by default and you could get the default log file path via utils_misc.get_path(test.debugdir, "libvirtd.log") easily. And you need to define libvirtd_debug_level = '1' in your cfg. That is all.

@dzhengfy
I think we can think of libvirtd enable in specific context.
In this specific test case,
Here libvirtd config is critical part of this case itself, so put each necessary libvirtd config in one block will give reader one whole picture what are needed, rather than distributed the config by define libvirtd_debug_level = '1' in your cfg and utils_misc.get_path(test.debugdir, "libvirtd.log").
In general case, e.g where it just search message from libvirtd.log, it is proper to just use utils_misc.get_path(test.debugdir, "libvirtd.log") to get the path of libvirtd log file, and then grep it from this libvirtd log file

@vasekhodina , what's your opinion ? since I noticed you have some thoughts about clean code

I agree with Chunfu.
I think we can apply one sentence from the Zen of Python: Explicit is better than implicit. So the way I understand it if it's important for the test to have the specific log location, then having it in one block and one var is better.
Anyone reading the code later can see the var log_config_path, but they might not know they can call the function utils_misc.get_path(test.debugdir, "libvirtd.log") to get the log.

Plus the log location is already configured on line 83, so if I would do any changes I would do them there.

audit_logging = 1 to let audit messages also be sent via libvirt logging infrastructure as well as the auditd logging Signed-off-by: chunfuwen <chwen@redhat.com>

vasekhodina

LGTM

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 0074887 to 279e69a Compare August 15, 2022 06:49

yafu-1 reviewed Aug 24, 2022

View reviewed changes

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 279e69a to 1d207f7 Compare August 26, 2022 06:58

yafu-1 approved these changes Aug 30, 2022

View reviewed changes

chunfuwen requested a review from dzhengfy September 1, 2022 01:09

vasekhodina reviewed Sep 2, 2022

View reviewed changes

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 1d207f7 to 97aac82 Compare September 5, 2022 07:20

chunfuwen requested review from vasekhodina and yafu-1 and removed request for vasekhodina and yafu-1 September 5, 2022 07:22

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 97aac82 to 74b702a Compare September 5, 2022 07:25

vasekhodina reviewed Sep 5, 2022

View reviewed changes

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 74b702a to 6c3dc7d Compare September 13, 2022 08:09

chunfuwen requested review from vasekhodina and yafu-1 and removed request for vasekhodina September 13, 2022 08:11

vasekhodina approved these changes Sep 13, 2022

View reviewed changes

dzhengfy reviewed Sep 14, 2022

View reviewed changes

Newly added test cases : disable/enable audit_logging in libvirtd.log

1ebf2a5

audit_logging = 1 to let audit messages also be sent via libvirt logging infrastructure as well as the auditd logging Signed-off-by: chunfuwen <chwen@redhat.com>

chunfuwen force-pushed the add_enable_disable_audit_log_cases branch from 6c3dc7d to 1ebf2a5 Compare September 15, 2022 01:44

chunfuwen requested review from dzhengfy and removed request for yafu-1 September 15, 2022 01:48

chunfuwen requested a review from vasekhodina September 15, 2022 01:48

vasekhodina approved these changes Sep 20, 2022

View reviewed changes

dzhengfy approved these changes Sep 21, 2022

View reviewed changes

dzhengfy merged commit 4874a87 into autotest:master Sep 21, 2022

		@@ -0,0 +1,10 @@
		- conf_file.libvirtd_conf.set_audit_logging:

Newly added test cases : disable/enable audit_logging in libvirtd.log #4421

Newly added test cases : disable/enable audit_logging in libvirtd.log #4421

Conversation

chunfuwen commented Aug 15, 2022

chunfuwen commented Aug 15, 2022

chunfuwen commented Aug 24, 2022

chunfuwen commented Aug 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yafu-1 left a comment

Choose a reason for hiding this comment

chunfuwen commented Aug 30, 2022

vasekhodina left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen commented Sep 13, 2022

vasekhodina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunfuwen Sep 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasekhodina Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

vasekhodina left a comment

Choose a reason for hiding this comment

vasekhodina left a comment •

edited

Loading

chunfuwen Sep 5, 2022 •

edited

Loading

chunfuwen Sep 15, 2022 •

edited

Loading

vasekhodina Sep 20, 2022 •

edited

Loading