Skip to content

Don't log DNS queries by default #1368

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

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

jandubois
Copy link
Member

Local server names/addresses are often considered confidential and should not by default be logged. Their loglevel has been lowered to TRACE.

The host agent code now switches to TRACE level logging when the instance is started via limactl --debug start ....

Fixes #1367

Local server names/addresses are often considered confidential and
should not by default be logged. Their loglevel has been lowered to
TRACE.

The host agent code now switches to TRACE level logging when the instance
is started via `limactl --debug start ...`.

Signed-off-by: Jan Dubois <[email protected]>
@jandubois
Copy link
Member Author

@Nino-K Can you take a look if this makes sense to you?

Copy link
Contributor

@Nino-K Nino-K left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -122,5 +122,10 @@ func initLogrus(stderr io.Writer) {
logrus.SetOutput(stderr)
// JSON logs are parsed in pkg/hostagent/events.Watcher()
logrus.SetFormatter(new(logrus.JSONFormatter))
logrus.SetLevel(logrus.DebugLevel)
// HostAgent logging is one level more verbose than the start command itself
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Idk, I thought it was intentionally set to always log at DEBUG because the output goes to ha.stderr.log and doesn't pollute the terminal output: e88241e.

Which made sense to me: I don't normally want the output of limactl --debug start in the terminal, but I only look at ha.stderr.log when something is wrong, so always getting DEBUG output there is useful, so you don't have to restart your VM with --debug to get full logs.

So assuming this was intentional, I extended it to include TRACE level output when you run with --debug, as there is no way to specify the logging level for HA separately from the main logging level.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC that was just my silly workaround to avoid bother implementing --log-level propagation 🤦

Copy link
Member Author

@jandubois jandubois Feb 15, 2023

Choose a reason for hiding this comment

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

IIRC that was just my silly workaround to avoid bother implementing --log-level propagation

Log-level propagation is implemented (and I tested that it works when you remove the explicit override to DEBUG in the HA code):

lima/pkg/start/start.go

Lines 127 to 129 in 5b9df0e

if logrus.GetLevel() >= logrus.DebugLevel {
args = append(args, "--debug")
}

I continue to think that always having debug logging active in ha.stderr.log is useful, even though it feels inconsistent. I just don't want to include DNS lookups in it by default.

So on a practical level this PR works for me. If we want to regularize the settings, I think we need a separate log-level for HA that should default to DEBUG. But this is just adding complexity for little benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Log-level propagation is implemented

I just checked, and the propagation was already implemented by the time the override was added. So it still feels intentional to me. 😄

Copy link
Member

Choose a reason for hiding this comment

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

Propagation of custom log level is not implemented

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know what you mean by that. We only have INFO and DEBUG log levels in limactl, and that level is propagated to the HA, but then overridden to always be DEBUG.

Anyways, I'm lost now at what you want to do.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you are right, I've forgotten my own coding design 😞

@AkihiroSuda AkihiroSuda merged commit f086bc4 into lima-vm:master Feb 15, 2023
@AkihiroSuda AkihiroSuda added this to the v0.15.0 milestone Feb 15, 2023
@jandubois jandubois deleted the dns-trace branch February 15, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS logging should be opt-in
3 participants