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.

Sign up for GitHub

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
Merged

Don't log DNS queries by default #1368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 6 additions & 1 deletion cmd/limactl/hostagent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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 😞

if logrus.GetLevel() == logrus.DebugLevel {
logrus.SetLevel(logrus.TraceLevel)
} else {
logrus.SetLevel(logrus.DebugLevel)
}
}
10 changes: 5 additions & 5 deletions pkg/hostagent/dns/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func (s *Server) Shutdown() {
}

func newStaticClientConfig(ips []string) (*dns.ClientConfig, error) {
logrus.Debugf("newStaticClientConfig creating config for the the following IPs: %v", ips)
logrus.Tracef("newStaticClientConfig creating config for the the following IPs: %v", ips)
s := ``
for _, ip := range ips {
s += fmt.Sprintf("nameserver %s\n", ip)
Expand Down Expand Up @@ -152,7 +152,7 @@ func (h *Handler) handleQuery(w dns.ResponseWriter, req *dns.Msg) {
)
defer w.Close()
reply.SetReply(req)
logrus.Debugf("handleQuery received DNS query: %v", req)
logrus.Tracef("handleQuery received DNS query: %v", req)
for _, q := range req.Question {
hdr := dns.RR_Header{
Name: q.Name,
Expand Down Expand Up @@ -315,7 +315,7 @@ func (h *Handler) handleQuery(w dns.ResponseWriter, req *dns.Msg) {
}

func (h *Handler) handleDefault(w dns.ResponseWriter, req *dns.Msg) {
logrus.Debugf("handleDefault for %v", req)
logrus.Tracef("handleDefault for %v", req)
for _, client := range h.clients {
for _, srv := range h.clientConfig.Servers {
addr := fmt.Sprintf("%s:%s", srv, h.clientConfig.Port)
Expand All @@ -325,7 +325,7 @@ func (h *Handler) handleDefault(w dns.ResponseWriter, req *dns.Msg) {
continue
}
if h.truncate {
logrus.Debugf("handleDefault truncating reply: %v", reply)
logrus.Tracef("handleDefault truncating reply: %v", reply)
reply.Truncate(truncateSize)
}
if err = w.WriteMsg(reply); err != nil {
Expand All @@ -337,7 +337,7 @@ func (h *Handler) handleDefault(w dns.ResponseWriter, req *dns.Msg) {
var reply dns.Msg
reply.SetReply(req)
if h.truncate {
logrus.Debugf("handleDefault truncating reply: %v", reply)
logrus.Tracef("handleDefault truncating reply: %v", reply)
reply.Truncate(truncateSize)
}
if err := w.WriteMsg(&reply); err != nil {
Expand Down