From a4f853af8e0a95712c4c79c28ecf63dbf28c2893 Mon Sep 17 00:00:00 2001 From: ThinkChaos Date: Thu, 29 Aug 2024 20:04:16 -0400 Subject: [PATCH] refactor: make QueryLoggingResolver read the hostname on creation Remove that code from util and the use of globals. Partly move away from calling the value a "hostname" since it's more of an instance ID, especially if you run more than one per host. --- querylog/database_writer.go | 2 +- querylog/file_writer.go | 2 +- querylog/logger_writer.go | 3 +- querylog/logger_writer_test.go | 1 - querylog/writer.go | 1 + resolver/query_logging_resolver.go | 46 +++++++++++++++++++----- resolver/query_logging_resolver_test.go | 22 ++++++++++-- server/server.go | 4 ++- util/hostname.go | 40 --------------------- util/hostname_test.go | 48 ------------------------- 10 files changed, 63 insertions(+), 106 deletions(-) delete mode 100644 util/hostname.go delete mode 100644 util/hostname_test.go diff --git a/querylog/database_writer.go b/querylog/database_writer.go index 678ff811f..54ae3a6c1 100644 --- a/querylog/database_writer.go +++ b/querylog/database_writer.go @@ -176,7 +176,7 @@ func (d *DatabaseWriter) Write(entry *LogEntry) { EffectiveTLDP: eTLD, Answer: entry.Answer, ResponseCode: entry.ResponseCode, - Hostname: util.HostnameString(), + Hostname: entry.BlockyInstance, } d.lock.Lock() diff --git a/querylog/file_writer.go b/querylog/file_writer.go index 1cb00cab9..545e99d6d 100644 --- a/querylog/file_writer.go +++ b/querylog/file_writer.go @@ -115,7 +115,7 @@ func createQueryLogRow(logEntry *LogEntry) []string { logEntry.ResponseCode, logEntry.ResponseType, logEntry.QuestionType, - util.HostnameString(), + logEntry.BlockyInstance, } } diff --git a/querylog/logger_writer.go b/querylog/logger_writer.go index 786bc6bc4..c2ec0822c 100644 --- a/querylog/logger_writer.go +++ b/querylog/logger_writer.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/0xERR0R/blocky/log" - "github.com/0xERR0R/blocky/util" "github.com/sirupsen/logrus" ) @@ -40,7 +39,7 @@ func LogEntryFields(entry *LogEntry) logrus.Fields { "question_type": entry.QuestionType, "answer": entry.Answer, "duration_ms": entry.DurationMs, - "hostname": util.HostnameString(), + "instance": entry.BlockyInstance, }) } diff --git a/querylog/logger_writer_test.go b/querylog/logger_writer_test.go index 38b136c54..b5053641f 100644 --- a/querylog/logger_writer_test.go +++ b/querylog/logger_writer_test.go @@ -51,7 +51,6 @@ var _ = Describe("LoggerWriter", func() { Expect(fields).Should(HaveKeyWithValue("duration_ms", entry.DurationMs)) Expect(fields).Should(HaveKeyWithValue("question_type", entry.QuestionType)) Expect(fields).Should(HaveKeyWithValue("response_code", entry.ResponseCode)) - Expect(fields).Should(HaveKey("hostname")) Expect(fields).ShouldNot(HaveKey("client_names")) Expect(fields).ShouldNot(HaveKey("question_name")) diff --git a/querylog/writer.go b/querylog/writer.go index 0d83534af..e68483ac0 100644 --- a/querylog/writer.go +++ b/querylog/writer.go @@ -15,6 +15,7 @@ type LogEntry struct { QuestionType string QuestionName string Answer string + BlockyInstance string } type Writer interface { diff --git a/resolver/query_logging_resolver.go b/resolver/query_logging_resolver.go index 456d98c73..44eb92e1f 100644 --- a/resolver/query_logging_resolver.go +++ b/resolver/query_logging_resolver.go @@ -2,6 +2,10 @@ package resolver import ( "context" + "errors" + "fmt" + "os" + "strings" "time" "github.com/0xERR0R/blocky/config" @@ -25,8 +29,9 @@ type QueryLoggingResolver struct { NextResolver typed - logChan chan *querylog.LogEntry - writer querylog.Writer + logChan chan *querylog.LogEntry + writer querylog.Writer + instanceID string } func GetQueryLoggingWriter(ctx context.Context, cfg config.QueryLog) (querylog.Writer, error) { @@ -58,7 +63,7 @@ func GetQueryLoggingWriter(ctx context.Context, cfg config.QueryLog) (querylog.W } // NewQueryLoggingResolver returns a new resolver instance -func NewQueryLoggingResolver(ctx context.Context, cfg config.QueryLog) *QueryLoggingResolver { +func NewQueryLoggingResolver(ctx context.Context, cfg config.QueryLog) (*QueryLoggingResolver, error) { logger := log.PrefixedLog(queryLoggingResolverType) var writer querylog.Writer @@ -86,14 +91,20 @@ func NewQueryLoggingResolver(ctx context.Context, cfg config.QueryLog) *QueryLog cfg.Type = config.QueryLogTypeConsole } + instanceID, err := readInstanceID("/etc/hostname") + if err != nil { + return nil, err + } + logChan := make(chan *querylog.LogEntry, logChanCap) resolver := QueryLoggingResolver{ configurable: withConfig(&cfg), typed: withType(queryLoggingResolverType), - logChan: logChan, - writer: writer, + logChan: logChan, + writer: writer, + instanceID: instanceID, } go resolver.writeLog(ctx) @@ -103,7 +114,7 @@ func NewQueryLoggingResolver(ctx context.Context, cfg config.QueryLog) *QueryLog go resolver.periodicCleanUp(ctx) } - return &resolver + return &resolver, nil } // triggers periodically cleanup of old log files @@ -170,9 +181,10 @@ func (r *QueryLoggingResolver) createLogEntry(request *model.Request, response * start time.Time, durationMs int64, ) *querylog.LogEntry { entry := querylog.LogEntry{ - Start: start, - ClientIP: "0.0.0.0", - ClientNames: []string{"none"}, + Start: start, + ClientIP: "0.0.0.0", + ClientNames: []string{"none"}, + BlockyInstance: r.instanceID, } for _, f := range r.cfg.Fields { @@ -227,3 +239,19 @@ func (r *QueryLoggingResolver) writeLog(ctx context.Context) { } } } + +func readInstanceID(file string) (string, error) { + // Prefer /etc/hostname over os.Hostname to allow easy differentiation in a Docker Swarm + // See details in https://github.com/0xERR0R/blocky/pull/756 + hn, fErr := os.ReadFile(file) + if fErr == nil { + return strings.TrimSpace(string(hn)), nil + } + + hostname, osErr := os.Hostname() + if osErr == nil { + return hostname, nil + } + + return "", fmt.Errorf("cannot determine instance ID: %w", errors.Join(fErr, osErr)) +} diff --git a/resolver/query_logging_resolver_test.go b/resolver/query_logging_resolver_test.go index ce8c8978d..f979afde6 100644 --- a/resolver/query_logging_resolver_test.go +++ b/resolver/query_logging_resolver_test.go @@ -42,6 +42,7 @@ var _ = Describe("QueryLoggingResolver", func() { var ( sut *QueryLoggingResolver sutConfig config.QueryLog + err error m *mockResolver tmpDir *TmpFolder mockRType ResponseType @@ -61,8 +62,6 @@ var _ = Describe("QueryLoggingResolver", func() { ctx, cancelFn = context.WithCancel(context.Background()) DeferCleanup(cancelFn) - var err error - sutConfig, err = config.WithDefaults[config.QueryLog]() Expect(err).Should(Succeed()) @@ -76,7 +75,8 @@ var _ = Describe("QueryLoggingResolver", func() { sutConfig.SetDefaults() // not called when using a struct literal } - sut = NewQueryLoggingResolver(ctx, sutConfig) + sut, err = NewQueryLoggingResolver(ctx, sutConfig) + Expect(err).Should(Succeed()) m = &mockResolver{ ResolveFn: func(context.Context, *Request) (*Response, error) { @@ -441,6 +441,22 @@ var _ = Describe("QueryLoggingResolver", func() { }) }) }) + + Describe("Hostname function tests", func() { + It("should use the given file if it exists", func() { + expected := "TestName" + tmpFile := NewTmpFolder("hostname").CreateStringFile("filetest1", expected+" \n") + + Expect(readInstanceID(tmpFile.Path)).Should(Equal(expected)) + }) + + It("should fallback to os.Hostname", func() { + expected, err := os.Hostname() + Expect(err).Should(Succeed()) + + Expect(readInstanceID("/var/empty/nonexistent")).Should(Equal(expected)) + }) + }) }) func readCsv(file string) ([][]string, error) { diff --git a/server/server.go b/server/server.go index 0ca7217f2..b33b85433 100644 --- a/server/server.go +++ b/server/server.go @@ -304,12 +304,14 @@ func createQueryResolver( upstreamTree, utErr := resolver.NewUpstreamTreeResolver(ctx, cfg.Upstreams, bootstrap) blocking, blErr := resolver.NewBlockingResolver(ctx, cfg.Blocking, redisClient, bootstrap) clientNames, cnErr := resolver.NewClientNamesResolver(ctx, cfg.ClientLookup, cfg.Upstreams, bootstrap) + queryLogging, qlErr := resolver.NewQueryLoggingResolver(ctx, cfg.QueryLog) condUpstream, cuErr := resolver.NewConditionalUpstreamResolver(ctx, cfg.Conditional, cfg.Upstreams, bootstrap) hostsFile, hfErr := resolver.NewHostsFileResolver(ctx, cfg.HostsFile, bootstrap) err := multierror.Append( multierror.Prefix(utErr, "upstream tree resolver: "), multierror.Prefix(blErr, "blocking resolver: "), + multierror.Prefix(qlErr, "query logging resolver: "), multierror.Prefix(cnErr, "client names resolver: "), multierror.Prefix(cuErr, "conditional upstream resolver: "), multierror.Prefix(hfErr, "hosts file resolver: "), @@ -324,7 +326,7 @@ func createQueryResolver( resolver.NewECSResolver(cfg.ECS), clientNames, resolver.NewEDEResolver(cfg.EDE), - resolver.NewQueryLoggingResolver(ctx, cfg.QueryLog), + queryLogging, resolver.NewMetricsResolver(cfg.Prometheus), resolver.NewRewriterResolver(cfg.CustomDNS.RewriterConfig, resolver.NewCustomDNSResolver(cfg.CustomDNS)), hostsFile, diff --git a/util/hostname.go b/util/hostname.go deleted file mode 100644 index 81bb033e1..000000000 --- a/util/hostname.go +++ /dev/null @@ -1,40 +0,0 @@ -package util - -import ( - "os" - "strings" -) - -//nolint:gochecknoglobals -var ( - hostname string - hostnameErr error -) - -const hostnameFile string = "/etc/hostname" - -//nolint:gochecknoinits -func init() { - getHostname(hostnameFile) -} - -// Direct replacement for os.Hostname -func Hostname() (string, error) { - return hostname, hostnameErr -} - -// Only return the hostname(may be empty if there was an error) -func HostnameString() string { - return hostname -} - -func getHostname(location string) { - hostname, hostnameErr = os.Hostname() - - if hn, err := os.ReadFile(location); err == nil { - hostname = strings.TrimSpace(string(hn)) - hostnameErr = nil - - return - } -} diff --git a/util/hostname_test.go b/util/hostname_test.go deleted file mode 100644 index 5d0274cb1..000000000 --- a/util/hostname_test.go +++ /dev/null @@ -1,48 +0,0 @@ -package util - -import ( - "os" - "strings" - - "github.com/0xERR0R/blocky/helpertest" - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" -) - -var _ = Describe("Hostname function tests", func() { - When("file is present", func() { - var tmpDir *helpertest.TmpFolder - - BeforeEach(func() { - tmpDir = helpertest.NewTmpFolder("hostname") - }) - - It("should be used", func() { - tmpFile := tmpDir.CreateStringFile("filetest1", "TestName ") - - getHostname(tmpFile.Path) - - fhn, err := os.ReadFile(tmpFile.Path) - Expect(err).Should(Succeed()) - - hn, err := Hostname() - Expect(err).Should(Succeed()) - - Expect(hn).Should(Equal(strings.TrimSpace(string(fhn)))) - }) - }) - - When("file is not present", func() { - It("should use os.Hostname", func() { - getHostname("/does-not-exist") - - _, err := Hostname() - Expect(err).Should(Succeed()) - - ohn, err := os.Hostname() - Expect(err).Should(Succeed()) - - Expect(HostnameString()).Should(Equal(ohn)) - }) - }) -})