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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions libvirt/tests/cfg/conf_file/libvirtd_conf/set_audit_logging.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
- conf_file.libvirtd_conf.set_audit_logging:
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

type = set_audit_logging
start_vm = no
variants:
- positive_test:
variants:
- disable_audit_log:
Copy link
Contributor

Choose a reason for hiding this comment

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

- disable: is clear enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

enable_audit = no
- enable_audit_log:
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dzhengfy
see previous replied comment

enable_audit = yes
102 changes: 102 additions & 0 deletions libvirt/tests/src/conf_file/libvirtd_conf/set_audit_logging.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import logging
import os

from avocado.utils import process
from avocado.utils import service

from virttest import data_dir
from virttest import utils_libvirtd
from virttest import utils_split_daemons

from virttest.utils_test import libvirt

from virttest.utils_config import LibvirtdConfig
from virttest.utils_config import VirtQemudConfig

LOG = logging.getLogger('avocado.' + __name__)


def run(test, params, env):
"""
Test Disable/enable audit_logging in libvirtd.log.

1) Enable/disable audit_logging in libvirtd.conf;
2) Restart libvirtd daemon;
3) Check if audit logging as expected;
"""
# Here it needs manipulate libvird config as test step, so it doesn't use default config
# enabled by avocado-vt framework at start
def config_libvirtd_log(enable_audit_logging=False):
"""
Configure audit log level"

: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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

@vasekhodina vasekhodina Sep 20, 2022

Choose a reason for hiding this comment

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

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.

libvirtd_config.audit_level = 1
if enable_audit_logging:
libvirtd_config.audit_logging = 1
utils_libvirtd.Libvirtd('virtqemud').restart()

def ensure_auditd_started():
"""
Check audit service status and start it if it's not running
"""
service_name = 'auditd'
service_mgr = service.ServiceManager()
status = service_mgr.status(service_name)
LOG.debug('Service status is %s', status)
if not status:
service_mgr.start(service_name)

Choose a reason for hiding this comment

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

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()"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename the function name to ensure_auditd_started


def check_virt_type_from_audit_log():
"""
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"'
if process.system(cmd, ignore_status=True, shell=True):
test.fail("Check virt type failed with %s" % cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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


def clean_up_audit_log_file():
"""
Clean up audit message in log file.
"""
cmd = "> /var/log/audit/audit.log; > %s" % log_config_path
process.run(cmd, ignore_status=False,
shell=True)

def check_msg_in_libvirtd_log(str_to_grep):
"""
Check audit message in libvirtd log file

param: str_to_grep: captured message in string
"""
if not libvirt.check_logfile(str_to_grep, log_config_path, str_in_log=enable_audit_logging):
test.fail("Check message log:%s failed in log file:%s" % (str_to_grep, log_config_path))

vm_name = params.get("main_vm", "avocado-vt-vm1")
vm = env.get_vm(vm_name)
enable_audit_logging = "yes" == params.get("enable_audit", "no")

# Get LibvirtdConfig or VirtQemuConfig
log_config_path = os.path.join(data_dir.get_tmp_dir(), "libvirtd.log")
libvirtd_config = VirtQemudConfig() if utils_split_daemons.is_modular_daemon() else LibvirtdConfig()

try:
# Need stop VM first
if vm.is_alive():
vm.destroy()
config_libvirtd_log(enable_audit_logging)
# Clean up old audit message from log file
clean_up_audit_log_file()

# Check and start audit daemon service if necessary
ensure_auditd_started()

vm.start()
check_virt_type_from_audit_log()
check_msg_in_libvirtd_log("virDomainAudit")
finally:
libvirtd_config.restore()
utils_libvirtd.Libvirtd('virtqemud').restart()