log checkpoint followup + dns checkup tests

pkg/debug/checkups/bboltdb.go

@@ -50,6 +50,6 @@ func (c *bboltdbCheckup) Summary() string {
 	return "N/A"
 }
 
-func (c *bboltdbCheckup) Data() map[string]any {
+func (c *bboltdbCheckup) Data() any {
 	return c.data
 }

pkg/debug/checkups/binary_directory.go

@@ -57,7 +57,7 @@ func (c *BinaryDirectory) Summary() string {
 	return c.summary
 }
 
-func (c *BinaryDirectory) Data() map[string]any {
+func (c *BinaryDirectory) Data() any {
 	return nil
 }
 

pkg/debug/checkups/checkpoint.go

@@ -2,9 +2,7 @@ package checkups
 
 import (
 	"context"
-	"fmt"
 	"io"
-	"strings"
 	"time"
 
 	"github.com/go-kit/kit/log"
@@ -18,16 +16,16 @@ type (
 		Log(keyvals ...interface{}) error
 	}
 
-	checkPointer struct {
+	logCheckPointer struct {
 		logger      logger
 		knapsack    types.Knapsack
 		interrupt   chan struct{}
 		interrupted bool
 	}
 )
 
-func NewCheckupLogger(logger logger, k types.Knapsack) *checkPointer {
-	return &checkPointer{
+func NewCheckupLogger(logger logger, k types.Knapsack) *logCheckPointer {
+	return &logCheckPointer{
 		logger:    log.With(logger, "component", "log checkpoint"),
 		knapsack:  k,
 		interrupt: make(chan struct{}, 1),
@@ -36,7 +34,7 @@ func NewCheckupLogger(logger logger, k types.Knapsack) *checkPointer {
 
 // Run starts a log checkpoint routine. The purpose of this is to
 // ensure we get good debugging information in the logs.
-func (c *checkPointer) Run() error {
+func (c *logCheckPointer) Run() error {
 	ticker := time.NewTicker(time.Minute * 60)
 	defer ticker.Stop()
 
@@ -53,7 +51,7 @@ func (c *checkPointer) Run() error {
 	}
 }
 
-func (c *checkPointer) Interrupt(_ error) {
+func (c *logCheckPointer) Interrupt(_ error) {
 	// Only perform shutdown tasks on first call to interrupt -- no need to repeat on potential extra calls.
 	if c.interrupted {
 		return
@@ -64,28 +62,17 @@ func (c *checkPointer) Interrupt(_ error) {
 	c.interrupt <- struct{}{}
 }
 
-func (c *checkPointer) Once(ctx context.Context) {
+func (c *logCheckPointer) Once(ctx context.Context) {
 	checkups := checkupsFor(c.knapsack, logSupported)
 
 	for _, checkup := range checkups {
 		checkup.Run(ctx, io.Discard)
 
-		logValues := []interface{}{"checkup", checkup.Name()}
-		for k, v := range checkup.Data() {
-			logValues = append(logValues, k, c.summarizeData(v))
-		}
-
-		c.logger.Log(logValues...)
-	}
-}
-
-func (c *checkPointer) summarizeData(data any) any {
-	switch knownValue := data.(type) {
-	case []string:
-		return strings.Join(knownValue, ",")
-	case string, uint, uint64, int, int32, int64:
-		return knownValue
-	default:
-		return fmt.Sprintf("%v", data)
+		level.Debug(c.logger).Log(
+			"checkup", checkup.Name(),
+			"summary", checkup.Summary(),
+			"data", checkup.Data(),
+			"status", checkup.Status(),
+		)
 	}
 }

pkg/debug/checkups/checkpoint_test.go

@@ -29,6 +29,7 @@ func TestInterrupt_Multiple(t *testing.T) {
 	mockKnapsack.On("Autoupdate").Return(true).Maybe()
 	mockKnapsack.On("NotaryServerURL").Return("localhost").Maybe()
 	mockKnapsack.On("LatestOsquerydPath").Return("").Maybe()
+	mockKnapsack.On("ServerProvidedDataStore").Return(nil).Maybe()
 	checkupLogger := NewCheckupLogger(log.NewNopLogger(), mockKnapsack)
 	mockKnapsack.AssertExpectations(t)
 

pkg/debug/checkups/checkups.go

@@ -72,7 +72,7 @@ type checkupInt interface {
 	ExtraFileName() string                                // If this checkup will have extra data, what name should it use in flare
 	Summary() string                                      // Short summary string about the status
 	Status() Status                                       // State of this checkup
-	Data() map[string]any                                 // What data objects exist, if any
+	Data() any                                            // What data objects exist, if any
 }
 
 type targetBits uint8

pkg/debug/checkups/connectivity.go

@@ -86,7 +86,7 @@ func (c *Connectivity) Summary() string {
 	return c.summary
 }
 
-func (c *Connectivity) Data() map[string]any {
+func (c *Connectivity) Data() any {
 	return c.data
 }
 

pkg/debug/checkups/dns.go

@@ -10,20 +10,30 @@ import (
 	"github.com/kolide/launcher/pkg/agent/types"
 )
 
-type dnsCheckup struct {
-	k       types.Knapsack
-	status  Status
-	summary string
-	data    map[string]any
-}
+type (
+	HostResolver interface {
+		LookupHost(ctx context.Context, host string) ([]string, error)
+	}
+	dnsCheckup struct {
+		k        types.Knapsack
+		status   Status
+		summary  string
+		data     map[string]any
+		resolver HostResolver
+	}
+)
 
-func (nc *dnsCheckup) Data() map[string]any  { return nc.data }
-func (nc *dnsCheckup) ExtraFileName() string { return "" }
-func (nc *dnsCheckup) Name() string          { return "DNS Resolution" }
-func (nc *dnsCheckup) Status() Status        { return nc.status }
-func (nc *dnsCheckup) Summary() string       { return nc.summary }
+func (dc *dnsCheckup) Data() any             { return dc.data }
+func (dc *dnsCheckup) ExtraFileName() string { return "" }
+func (dc *dnsCheckup) Name() string          { return "DNS Resolution" }
+func (dc *dnsCheckup) Status() Status        { return dc.status }
+func (dc *dnsCheckup) Summary() string       { return dc.summary }
 
 func (dc *dnsCheckup) Run(ctx context.Context, extraFH io.Writer) error {
+	if dc.resolver == nil {
+		dc.resolver = &net.Resolver{}
+	}
+
 	hosts := []string{
 		dc.k.KolideServerURL(),
 		dc.k.ControlServerURL(),
@@ -34,7 +44,6 @@ func (dc *dnsCheckup) Run(ctx context.Context, extraFH io.Writer) error {
 
 	dc.data = make(map[string]any)
 	attemptedCount, successCount := 0, 0
-	resolver := &net.Resolver{}
 
 	for _, host := range hosts {
 		if len(strings.TrimSpace(host)) == 0 {
@@ -47,7 +56,7 @@ func (dc *dnsCheckup) Run(ctx context.Context, extraFH io.Writer) error {
 			continue
 		}
 
-		ips, err := resolveHost(resolver, hostUrl.Hostname())
+		ips, err := dc.resolveHost(ctx, hostUrl.Hostname())
 		// keep attemptedCount as a separate variable to avoid indicating failures where we didn't even try
 		attemptedCount++
 
@@ -76,11 +85,11 @@ func (dc *dnsCheckup) Run(ctx context.Context, extraFH io.Writer) error {
 	return nil
 }
 
-func resolveHost(resolver *net.Resolver, host string) (string, error) {
-	ctx, cancel := context.WithTimeout(context.Background(), requestTimeout)
+func (dc *dnsCheckup) resolveHost(ctx context.Context, host string) (string, error) {
+	ctx, cancel := context.WithTimeout(ctx, requestTimeout)
 	defer cancel()
 
-	ips, err := resolver.LookupHost(ctx, host)
+	ips, err := dc.resolver.LookupHost(ctx, host)
 
 	if err != nil {
 		return "", err

pkg/debug/checkups/dns_test.go

@@ -0,0 +1,154 @@
+package checkups
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"strings"
+	"testing"
+
+	typesMocks "github.com/kolide/launcher/pkg/agent/types/mocks"
+	"github.com/kolide/launcher/pkg/debug/checkups/mocks"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+)
+
+func Test_dnsCheckup_Run(t *testing.T) {
+	t.Parallel()
+	type fields struct {
+		k        *typesMocks.Knapsack
+		status   Status
+		summary  string
+		data     map[string]any
+		resolver *mocks.HostResolver
+	}
+	type resolution struct {
+		ips []string
+		err error
+	}
+	type args struct {
+		ctx context.Context
+	}
+	tests := []struct {
+		name                  string
+		fields                fields
+		args                  args
+		knapsackReturns       map[string]any
+		onLookupHostReturns   map[string]resolution
+		expectedStatus        Status
+		expectedSuccessCount  int
+		expectedAttemptsCount int
+	}{
+		{
+			name: "happy path",
+			fields: fields{
+				k:        typesMocks.NewKnapsack(t),
+				resolver: mocks.NewHostResolver(t),
+			},
+			args: args{context.Background()},
+			knapsackReturns: map[string]interface{}{
+				"KolideServerURL":      "https://kolide-server.example.com",
+				"ControlServerURL":     "https://control-server.example.com",
+				"TufServerURL":         "https://tuf-server.example.com",
+				"InsecureTransportTLS": false,
+			},
+			onLookupHostReturns: map[string]resolution{
+				"kolide-server.example.com":  resolution{ips: []string{"2607:f8b0:4009:808::200e", "142.250.65.206"}, err: nil},
+				"control-server.example.com": resolution{ips: []string{"111.2.3.4", "111.2.3.5"}, err: nil},
+				"tuf-server.example.com":     resolution{ips: []string{"34.149.84.181"}, err: nil},
+				"google.com":                 resolution{ips: []string{"2620:149:af0::10", "17.253.144.10"}, err: nil},
+				"apple.com":                  resolution{ips: []string{"2607:f8b0:4009:808::200e", "142.250.65.206"}, err: nil},
+			},
+			expectedStatus:        Passing,
+			expectedSuccessCount:  5,
+			expectedAttemptsCount: 5,
+		},
+		{
+			name: "unresolvable host errors are included in data",
+			fields: fields{
+				k:        typesMocks.NewKnapsack(t),
+				resolver: mocks.NewHostResolver(t),
+			},
+			args: args{context.Background()},
+			knapsackReturns: map[string]interface{}{
+				"KolideServerURL":      "https://kolide-server.example.com",
+				"ControlServerURL":     "https://control-server.example.com",
+				"TufServerURL":         "https://tuf-server.example.com",
+				"InsecureTransportTLS": false,
+			},
+			onLookupHostReturns: map[string]resolution{
+				"kolide-server.example.com":  resolution{ips: []string{}, err: fmt.Errorf("Unable to resolve: No Such Host")},
+				"control-server.example.com": resolution{ips: []string{"111.2.3.4", "111.2.3.5"}, err: nil},
+				"tuf-server.example.com":     resolution{ips: []string{"34.149.84.181"}, err: nil},
+				"google.com":                 resolution{ips: []string{"2620:149:af0::10", "17.253.144.10"}, err: nil},
+				"apple.com":                  resolution{ips: []string{"2607:f8b0:4009:808::200e", "142.250.65.206"}, err: nil},
+			},
+			expectedStatus:        Warning,
+			expectedSuccessCount:  4,
+			expectedAttemptsCount: 5,
+		},
+		{
+			name: "unset host values are not counted against checkup",
+			fields: fields{
+				k:        typesMocks.NewKnapsack(t),
+				resolver: mocks.NewHostResolver(t),
+			},
+			args: args{context.Background()},
+			knapsackReturns: map[string]interface{}{
+				"KolideServerURL":      "https://kolide-server.example.com",
+				"ControlServerURL":     "https://control-server.example.com",
+				"TufServerURL":         "",
+				"InsecureTransportTLS": false,
+			},
+			onLookupHostReturns: map[string]resolution{
+				"kolide-server.example.com":  resolution{ips: []string{"2607:f8b0:4009:808::200e", "142.250.65.206"}, err: nil},
+				"control-server.example.com": resolution{ips: []string{"111.2.3.4", "111.2.3.5"}, err: nil},
+				"google.com":                 resolution{ips: []string{"2620:149:af0::10", "17.253.144.10"}, err: nil},
+				"apple.com":                  resolution{ips: []string{"2607:f8b0:4009:808::200e", "142.250.65.206"}, err: nil},
+			},
+			expectedStatus:        Passing,
+			expectedSuccessCount:  4,
+			expectedAttemptsCount: 4,
+		},
+	}
+
+	for _, tt := range tests {
+		tt := tt
+		for mockfunc, mockval := range tt.knapsackReturns {
+			tt.fields.k.On(mockfunc).Return(mockval)
+		}
+
+		for mockhost, mockresolution := range tt.onLookupHostReturns {
+			tt.fields.resolver.On("LookupHost", mock.Anything, mockhost).Return(mockresolution.ips, mockresolution.err).Once()
+		}
+
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			dc := &dnsCheckup{
+				k:        tt.fields.k,
+				status:   tt.fields.status,
+				summary:  tt.fields.summary,
+				data:     tt.fields.data,
+				resolver: tt.fields.resolver,
+			}
+			if err := dc.Run(tt.args.ctx, io.Discard); err != nil {
+				t.Errorf("dnsCheckup.Run() error = %v", err)
+				return
+			}
+
+			gotData, ok := dc.Data().(map[string]any)
+			require.True(t, ok, "expected to be able to typecast data into map[string]any for testing")
+
+			for mockhost, mockresolution := range tt.onLookupHostReturns {
+				if mockresolution.err != nil {
+					require.Contains(t, gotData[mockhost], mockresolution.err.Error(), "expected data for %s to contain the resolution error message", mockhost)
+				} else {
+					require.Contains(t, gotData[mockhost], strings.Join(mockresolution.ips, ","), "expected data to contain the resulting IP addresses")
+				}
+			}
+
+			require.Equal(t, tt.expectedAttemptsCount, gotData["lookup_attempts"], "expected lookup attempts to match")
+			require.Equal(t, tt.expectedSuccessCount, gotData["lookup_successes"], "expected lookup success count to match")
+		})
+	}
+}

pkg/debug/checkups/enroll-secret.go

@@ -70,7 +70,7 @@ func (c *enrollSecretCheckup) Summary() string {
 	return c.summary
 }
 
-func (c *enrollSecretCheckup) Data() map[string]any {
+func (c *enrollSecretCheckup) Data() any {
 	return nil
 }
 

pkg/debug/checkups/gnome-extensions.go

@@ -82,7 +82,7 @@ func (c *gnomeExtensions) Summary() string {
 	return c.summary
 }
 
-func (c *gnomeExtensions) Data() map[string]any {
+func (c *gnomeExtensions) Data() any {
 	return nil
 }
 

pkg/debug/checkups/host.go

@@ -26,7 +26,7 @@ type (
 	}
 )
 
-func (hc *hostInfoCheckup) Data() map[string]any  { return hc.data }
+func (hc *hostInfoCheckup) Data() any             { return hc.data }
 func (hc *hostInfoCheckup) ExtraFileName() string { return "" }
 func (hc *hostInfoCheckup) Name() string          { return "Host Info" }
 func (hc *hostInfoCheckup) Status() Status        { return hc.status }
@@ -45,10 +45,9 @@ func (hc *hostInfoCheckup) Run(ctx context.Context, extraFH io.Writer) error {
 		hc.data["uptime_friendly"] = fmt.Sprintf("ERROR: %s", err.Error())
 	} else {
 		hc.data["uptime_friendly"] = formatUptime(uptimeRaw)
+		hc.data["uptime"] = uptimeRaw
 	}
 
-	hc.data["uptime"] = uptimeRaw
-
 	if runtime.GOOS == "windows" {
 		hc.data["in_modern_standby"] = hc.k.InModernStandby()
 	}