[console-monitor] Implement Console Logging feature - #409
nats-nokia wants to merge 4 commits into
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Hi, there are workflow run(s) waiting for approval, you may be first-time contributor. I will notify maintainers to help approve once PR is approved. Thanks! ---Powered by SONiC BuildBot
|
ae0dc70 to
a1e8158
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
a1e8158 to
9f2a782
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
9f2a782 to
12e3332
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| self.log_fd = os.open( | ||
| self.log_file_path, | ||
| os.O_WRONLY | os.O_CREAT | os.O_APPEND, | ||
| 0o644, |
There was a problem hiding this comment.
Console logs capture everything typed/echoed at the console, including credentials entered at login/enable prompts. 0o644 lets any local user read them. Consider 0o640 or 0o600.
There was a problem hiding this comment.
sure, corrected the file permission to 0o640. Thank you.
Signed-off-by: Natarajan Subbiramani <natarajan.subbiramani@nokia.com>
modified logrotate default values size as 10M, count as 10 Added more test coverage Signed-off-by: Natarajan Subbiramani <natarajan.subbiramani@nokia.com>
Signed-off-by: Natarajan Subbiramani <natarajan.subbiramani@nokia.com>
Signed-off-by: Natarajan Subbiramani <natarajan.subbiramani@nokia.com>
12e3332 to
1101b6f
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| return ( | ||
| f"{log_file} {{\n" | ||
| f" missingok\n" | ||
| f" notifempty\n" |
There was a problem hiding this comment.
should we have some validation checks for these arguments before making the log rotate config? or is it assumed that yang validation will catch it ?
There was a problem hiding this comment.
Yes, yang validation would catch any invalid arguments.
ediwibowo-msft
left a comment
ediwibowo-msft
left a comment
There was a problem hiding this comment.
Reviewed the console logging feature. Overall design is solid — O_APPEND + copytruncate is the right combination so the proxy fd survives rotation without reopening, and logging device output rather than raw PTM input sensibly avoids capturing un-echoed input. A few points below, one functional.
| old_config.get("log_file") != new_config.get("log_file") or | ||
| old_config.get("logrotate_size") != new_config.get("logrotate_size") or | ||
| old_config.get("logrotate_count") != new_config.get("logrotate_count")): | ||
| log.warning(f"DCE: [{link_id}] Logging config changed, proxy will restart") |
There was a problem hiding this comment.
Changing only logrotate_size/logrotate_count will bounce the proxy and drop the active console session, even though the proxy's _load_config never reads these values (only baud, logging_enabled, log_file). Because _get_all_configs now includes the logrotate params in the cached dict, the outer new_config != old_config check restarts the link for a logrotate-only edit — but _sync_logrotate_configs already regenerates the conf, which is all that's needed. Consider keeping logrotate-only params out of the restart-detection config so those edits only rewrite the logrotate file without restarting the proxy. (No test currently asserts a logrotate-only change avoids restart.)
| # This glob-based reconciliation assumes console-monitor owns every | ||
| # LOGROTATE_CONF_PREFIX file under LOGROTATE_DIR. | ||
| existing_links = set() | ||
| pattern = os.path.join(LOGROTATE_DIR, f"{LOGROTATE_CONF_PREFIX}*") |
There was a problem hiding this comment.
This reconciliation deletes any /etc/logrotate.d/console-* file not in the desired set. The console- prefix is generic, and as the comment above acknowledges, this assumes console-monitor owns every matching file. If another package ever ships /etc/logrotate.d/console-<something>, it would be silently removed. A more specific prefix (e.g. console-monitor- or sonic-console-) would make ownership unambiguous and keep this safe.
|
|
||
| def _sync(self) -> None: | ||
| """Sync services with CONFIG_DB""" | ||
| self._sync_logrotate_configs() |
There was a problem hiding this comment.
_sync_logrotate_configs() runs before _check_feature_enabled(), so logrotate confs are written even when console_mgmt is disabled and no proxy is running. Harmless due to missingok, but inconsistent when the feature is off there's nothing to rotate, and arguably the confs should be cleaned up rather than created.
|
|
||
| if self.logging_enabled and self.log_file_path: | ||
| try: | ||
| self.log_fd = os.open( |
There was a problem hiding this comment.
log_file_path comes straight from CONFIG_DB and is opened as root with O_CREAT|O_APPEND, following symlinks to an arbitrary path. CONFIG_DB is admin-trusted so risk is low, but since this runs with elevated privileges it'd be worth hardening — e.g. confining the path to /var/log or adding O_NOFOLLOW — to avoid a stray symlink redirecting root writes.
4 participants
console-monitor DCE reads console logging filename, logrotate parameters from CONSOLE_PORT table and creates logrotate conf for corresponding line number. If filename and logrotate are not provided, default values will be used.
console-monitor proxy service opens the log file name in append mode and keep logging the characters read from console device, if logging is enabled.
Related PR from other sub modules:
sonic-net/sonic-buildimage#28411
sonic-net/sonic-utilities#4685