From dfa2d838554d9c0c318c76a45c8bd82128fc3bd5 Mon Sep 17 00:00:00 2001 From: Zachary Wasserman Date: Tue, 18 Apr 2017 10:39:50 -0700 Subject: [PATCH] Update online status calculation to use per-host intervals (#1494) Replaces the existing calculation that uses a global online interval. This method was lacking due to the fact that different hosts may have different checkin intervals set. The new calculation uses `min(distributed_interval, config_tls_refresh) + 30` as the interval. This is calculated with the stored values for each host. Closes #1321 --- CHANGELOG.md | 4 + server/datastore/datastore_hosts_test.go | 60 +++---- server/datastore/datastore_targets_test.go | 184 +++++++++++---------- server/datastore/datastore_test.go | 1 + server/datastore/inmem/hosts.go | 4 +- server/datastore/mysql/hosts.go | 37 ++--- server/datastore/mysql/targets.go | 16 +- server/kolide/hosts.go | 25 ++- server/kolide/hosts_test.go | 42 ++++- server/kolide/options.go | 8 - server/kolide/targets.go | 32 ++-- server/mock/datastore_hosts.go | 6 +- server/service/endpoint_hosts.go | 19 +-- server/service/endpoint_targets.go | 8 +- server/service/logging_options.go | 16 -- server/service/metrics_options.go | 14 -- server/service/service_hosts.go | 6 +- server/service/service_options.go | 73 -------- server/service/service_options_test.go | 134 --------------- server/service/service_targets.go | 8 +- 20 files changed, 234 insertions(+), 463 deletions(-) delete mode 100644 server/service/service_options_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e8d9bf92dc..6910833d2c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +* Improve online status detection. + + The Kolide server now tracks the `distributed_interval` and `config_tls_refresh` values for each individual host (these can be different if they are set via flagfile and not through Kolide), to ensure that online status is represented as accurately as possible. + * Kolide server now requires `--auth_jwt_key` to be specified at startup. If no JWT key is provided by the user, the server will print a new suggested random JWT key for use. diff --git a/server/datastore/datastore_hosts_test.go b/server/datastore/datastore_hosts_test.go index abb4589d45..62201ef486 100644 --- a/server/datastore/datastore_hosts_test.go +++ b/server/datastore/datastore_hosts_test.go @@ -526,7 +526,7 @@ func testGenerateHostStatusStatistics(t *testing.T, ds kolide.Datastore) { mockClock := clock.NewMockClock() - online, offline, mia, new, err := ds.GenerateHostStatusStatistics(mockClock.Now(), 60) + online, offline, mia, new, err := ds.GenerateHostStatusStatistics(mockClock.Now()) assert.Nil(t, err) assert.Equal(t, uint(0), online) assert.Equal(t, uint(0), offline) @@ -534,87 +534,67 @@ func testGenerateHostStatusStatistics(t *testing.T, ds kolide.Datastore) { assert.Equal(t, uint(0), new) // Online - _, err = ds.NewHost(&kolide.Host{ + h, err := ds.NewHost(&kolide.Host{ ID: 1, OsqueryHostID: "1", - UUID: "1", NodeKey: "1", DetailUpdateTime: mockClock.Now().Add(-30 * time.Second), SeenTime: mockClock.Now().Add(-30 * time.Second), - UpdateCreateTimestamps: kolide.UpdateCreateTimestamps{ - CreateTimestamp: kolide.CreateTimestamp{CreatedAt: mockClock.Now()}, - }, }) - assert.Nil(t, err) + require.Nil(t, err) + h.DistributedInterval = 15 + h.ConfigTLSRefresh = 30 + require.Nil(t, ds.SaveHost(h)) // Online - _, err = ds.NewHost(&kolide.Host{ + h, err = ds.NewHost(&kolide.Host{ ID: 2, OsqueryHostID: "2", - UUID: "2", NodeKey: "2", DetailUpdateTime: mockClock.Now().Add(-1 * time.Minute), SeenTime: mockClock.Now().Add(-1 * time.Minute), - UpdateCreateTimestamps: kolide.UpdateCreateTimestamps{ - CreateTimestamp: kolide.CreateTimestamp{CreatedAt: mockClock.Now()}, - }, }) - assert.Nil(t, err) + require.Nil(t, err) + h.DistributedInterval = 60 + h.ConfigTLSRefresh = 3600 + require.Nil(t, ds.SaveHost(h)) // Offline - _, err = ds.NewHost(&kolide.Host{ + h, err = ds.NewHost(&kolide.Host{ ID: 3, OsqueryHostID: "3", - UUID: "3", NodeKey: "3", DetailUpdateTime: mockClock.Now().Add(-1 * time.Hour), SeenTime: mockClock.Now().Add(-1 * time.Hour), - UpdateCreateTimestamps: kolide.UpdateCreateTimestamps{ - CreateTimestamp: kolide.CreateTimestamp{CreatedAt: mockClock.Now()}, - }, }) - assert.Nil(t, err) + require.Nil(t, err) + h.DistributedInterval = 300 + h.ConfigTLSRefresh = 300 + require.Nil(t, ds.SaveHost(h)) // MIA - _, err = ds.NewHost(&kolide.Host{ + h, err = ds.NewHost(&kolide.Host{ ID: 4, OsqueryHostID: "4", - UUID: "4", NodeKey: "4", DetailUpdateTime: mockClock.Now().Add(-35 * (24 * time.Hour)), SeenTime: mockClock.Now().Add(-35 * (24 * time.Hour)), - UpdateCreateTimestamps: kolide.UpdateCreateTimestamps{ - CreateTimestamp: kolide.CreateTimestamp{CreatedAt: mockClock.Now()}, - }, }) - assert.Nil(t, err) + require.Nil(t, err) - // With an online interval of 60, both the host that checked in a minute ago - // as well as the host that checked in 30 seconds ago should both be online - online, offline, mia, new, err = ds.GenerateHostStatusStatistics(mockClock.Now(), 2*time.Minute) + online, offline, mia, new, err = ds.GenerateHostStatusStatistics(mockClock.Now()) assert.Nil(t, err) assert.Equal(t, uint(2), online) assert.Equal(t, uint(1), offline) assert.Equal(t, uint(1), mia) assert.Equal(t, uint(4), new) - // With an online interval of 10, no hosts should be online - online, offline, mia, new, err = ds.GenerateHostStatusStatistics(mockClock.Now(), 20*time.Second) + online, offline, mia, new, err = ds.GenerateHostStatusStatistics(mockClock.Now().Add(1 * time.Hour)) assert.Nil(t, err) assert.Equal(t, uint(0), online) assert.Equal(t, uint(3), offline) assert.Equal(t, uint(1), mia) assert.Equal(t, uint(4), new) - - // With an online interval of 3600 seconds (60 minutes), the host that checked - // in 30 seconds ago, a minute ago, and 60 minutes ago should all appear to be - // online - online, offline, mia, new, err = ds.GenerateHostStatusStatistics(mockClock.Now(), 2*time.Hour) - assert.Nil(t, err) - assert.Equal(t, uint(3), online) - assert.Equal(t, uint(0), offline) - assert.Equal(t, uint(1), mia) - assert.Equal(t, uint(4), new) } func testMarkHostSeen(t *testing.T, ds kolide.Datastore) { diff --git a/server/datastore/datastore_targets_test.go b/server/datastore/datastore_targets_test.go index 0904e0891e..b54b1515be 100644 --- a/server/datastore/datastore_targets_test.go +++ b/server/datastore/datastore_targets_test.go @@ -1,6 +1,7 @@ package datastore import ( + "strconv" "testing" "time" @@ -17,73 +18,30 @@ func testCountHostsInTargets(t *testing.T, ds kolide.Datastore) { mockClock := clock.NewMockClock() - h1, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "1", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "foo.local", - NodeKey: "1", - UUID: "1", - }) - require.Nil(t, err) - require.Nil(t, ds.MarkHostSeen(h1, mockClock.Now())) + hostCount := 0 + initHost := func(seenTime time.Time, distributedInterval uint, configTLSRefresh uint) *kolide.Host { + hostCount += 1 + h, err := ds.NewHost(&kolide.Host{ + OsqueryHostID: strconv.Itoa(hostCount), + DetailUpdateTime: mockClock.Now(), + SeenTime: mockClock.Now(), + NodeKey: strconv.Itoa(hostCount), + DistributedInterval: distributedInterval, + ConfigTLSRefresh: configTLSRefresh, + }) + require.Nil(t, err) + require.Nil(t, ds.MarkHostSeen(h, seenTime)) + return h + } - h2, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "2", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "bar.local", - NodeKey: "2", - UUID: "2", - }) - require.Nil(t, err) - // make this host "offline" - require.Nil(t, ds.MarkHostSeen(h2, mockClock.Now().Add(-1*time.Hour))) - - h3, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "3", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "baz.local", - NodeKey: "3", - UUID: "3", - }) - require.Nil(t, err) - require.Nil(t, ds.MarkHostSeen(h3, mockClock.Now().Add(-5*time.Second))) - - h4, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "4", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "xxx.local", - NodeKey: "4", - UUID: "4", - }) - require.Nil(t, err) - require.Nil(t, ds.MarkHostSeen(h4, mockClock.Now())) - - h5, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "5", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "yyy.local", - NodeKey: "5", - UUID: "5", - }) - require.Nil(t, err) - require.Nil(t, ds.MarkHostSeen(h5, mockClock.Now())) - - h6, err := ds.NewHost(&kolide.Host{ - OsqueryHostID: "6", - DetailUpdateTime: time.Now(), - SeenTime: time.Now(), - HostName: "zzz.local", - NodeKey: "6", - UUID: "6", - }) - require.Nil(t, err) + // Checks in every + h1 := initHost(mockClock.Now().Add(-1*time.Second), 10, 60) + h2 := initHost(mockClock.Now().Add(-1*time.Hour), 30, 7200) + h3 := initHost(mockClock.Now().Add(-5*time.Second), 20, 20) + h4 := initHost(mockClock.Now().Add(-47*time.Second), 10, 10) + h5 := initHost(mockClock.Now(), 5, 5) const thirtyDaysAndAMinuteAgo = -1 * (30*24*60 + 1) - require.Nil(t, ds.MarkHostSeen(h6, mockClock.Now().Add(thirtyDaysAndAMinuteAgo*time.Minute))) + h6 := initHost(mockClock.Now().Add(thirtyDaysAndAMinuteAgo*time.Minute), 3600, 3600) l1, err := ds.NewLabel(&kolide.Label{ Name: "label foo", @@ -109,54 +67,114 @@ func testCountHostsInTargets(t *testing.T, ds kolide.Datastore) { assert.Nil(t, err) } - metrics, err := ds.CountHostsInTargets(nil, []uint{l1.ID, l2.ID}, mockClock.Now(), 30*time.Minute) + metrics, err := ds.CountHostsInTargets(nil, []uint{l1.ID, l2.ID}, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(6), metrics.TotalHosts) - assert.Equal(t, uint(1), metrics.OfflineHosts) - assert.Equal(t, uint(4), metrics.OnlineHosts) + assert.Equal(t, uint(2), metrics.OfflineHosts) + assert.Equal(t, uint(3), metrics.OnlineHosts) assert.Equal(t, uint(1), metrics.MissingInActionHosts) - metrics, err = ds.CountHostsInTargets([]uint{h1.ID, h2.ID}, []uint{l1.ID, l2.ID}, mockClock.Now(), 30*time.Minute) + metrics, err = ds.CountHostsInTargets([]uint{h1.ID, h2.ID}, []uint{l1.ID, l2.ID}, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(6), metrics.TotalHosts) - assert.Equal(t, uint(1), metrics.OfflineHosts) - assert.Equal(t, uint(4), metrics.OnlineHosts) + assert.Equal(t, uint(2), metrics.OfflineHosts) + assert.Equal(t, uint(3), metrics.OnlineHosts) assert.Equal(t, uint(1), metrics.MissingInActionHosts) - metrics, err = ds.CountHostsInTargets([]uint{h1.ID, h2.ID}, nil, mockClock.Now(), 30*time.Minute) + metrics, err = ds.CountHostsInTargets([]uint{h1.ID, h2.ID}, nil, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(2), metrics.TotalHosts) assert.Equal(t, uint(1), metrics.OnlineHosts) assert.Equal(t, uint(1), metrics.OfflineHosts) assert.Equal(t, uint(0), metrics.MissingInActionHosts) - metrics, err = ds.CountHostsInTargets([]uint{h1.ID}, []uint{l2.ID}, mockClock.Now(), 30*time.Minute) + metrics, err = ds.CountHostsInTargets([]uint{h1.ID}, []uint{l2.ID}, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(4), metrics.TotalHosts) - assert.Equal(t, uint(4), metrics.OnlineHosts) + assert.Equal(t, uint(3), metrics.OnlineHosts) + assert.Equal(t, uint(1), metrics.OfflineHosts) + assert.Equal(t, uint(0), metrics.MissingInActionHosts) + + metrics, err = ds.CountHostsInTargets(nil, nil, mockClock.Now()) + require.Nil(t, err) + assert.Equal(t, uint(0), metrics.TotalHosts) + assert.Equal(t, uint(0), metrics.OnlineHosts) assert.Equal(t, uint(0), metrics.OfflineHosts) assert.Equal(t, uint(0), metrics.MissingInActionHosts) - metrics, err = ds.CountHostsInTargets(nil, nil, mockClock.Now(), 30*time.Minute) + metrics, err = ds.CountHostsInTargets([]uint{}, []uint{}, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(0), metrics.TotalHosts) assert.Equal(t, uint(0), metrics.OnlineHosts) assert.Equal(t, uint(0), metrics.OfflineHosts) assert.Equal(t, uint(0), metrics.MissingInActionHosts) // Advance clock so all hosts are offline - mockClock.AddTime(1 * time.Hour) - metrics, err = ds.CountHostsInTargets(nil, []uint{l1.ID, l2.ID}, mockClock.Now(), 30*time.Minute) + mockClock.AddTime(2 * time.Minute) + metrics, err = ds.CountHostsInTargets(nil, []uint{l1.ID, l2.ID}, mockClock.Now()) require.Nil(t, err) - require.NotNil(t, metrics) assert.Equal(t, uint(6), metrics.TotalHosts) assert.Equal(t, uint(0), metrics.OnlineHosts) assert.Equal(t, uint(5), metrics.OfflineHosts) assert.Equal(t, uint(1), metrics.MissingInActionHosts) } + +func testHostStatus(t *testing.T, ds kolide.Datastore) { + if ds.Name() == "inmem" { + t.Skip("inmem is being deprecated, test skipped") + } + + mockClock := clock.NewMockClock() + + h, err := ds.EnrollHost("1", 24) + require.Nil(t, err) + + // Make host no longer appear new + mockClock.AddTime(36 * time.Hour) + + expectOnline := kolide.TargetMetrics{TotalHosts: 1, OnlineHosts: 1} + expectOffline := kolide.TargetMetrics{TotalHosts: 1, OfflineHosts: 1} + expectMIA := kolide.TargetMetrics{TotalHosts: 1, MissingInActionHosts: 1} + + var testCases = []struct { + seenTime time.Time + distributedInterval uint + configTLSRefresh uint + metrics kolide.TargetMetrics + }{ + {mockClock.Now().Add(-30 * time.Second), 10, 3600, expectOnline}, + {mockClock.Now().Add(-45 * time.Second), 10, 3600, expectOffline}, + {mockClock.Now().Add(-30 * time.Second), 3600, 10, expectOnline}, + {mockClock.Now().Add(-45 * time.Second), 3600, 10, expectOffline}, + + {mockClock.Now().Add(-70 * time.Second), 60, 60, expectOnline}, + {mockClock.Now().Add(-91 * time.Second), 60, 60, expectOffline}, + + {mockClock.Now().Add(-1 * time.Second), 10, 10, expectOnline}, + {mockClock.Now().Add(-1 * time.Minute), 10, 10, expectOffline}, + {mockClock.Now().Add(-31 * 24 * time.Hour), 10, 10, expectMIA}, + + // Ensure behavior is reasonable if we don't have the values + {mockClock.Now().Add(-1 * time.Second), 0, 0, expectOnline}, + {mockClock.Now().Add(-1 * time.Minute), 0, 0, expectOffline}, + {mockClock.Now().Add(-31 * 24 * time.Hour), 0, 0, expectMIA}, + } + + for _, tt := range testCases { + t.Run("", func(t *testing.T) { + // Save interval values + h.DistributedInterval = tt.distributedInterval + h.ConfigTLSRefresh = tt.configTLSRefresh + require.Nil(t, ds.SaveHost(h)) + + // Mark seen + require.Nil(t, ds.MarkHostSeen(h, tt.seenTime)) + + // Verify status + metrics, err := ds.CountHostsInTargets([]uint{h.ID}, []uint{}, mockClock.Now()) + require.Nil(t, err) + assert.Equal(t, tt.metrics, metrics) + }) + } +} diff --git a/server/datastore/datastore_test.go b/server/datastore/datastore_test.go index 2629570d07..8752f666c5 100644 --- a/server/datastore/datastore_test.go +++ b/server/datastore/datastore_test.go @@ -77,6 +77,7 @@ var testFunctions = [...]func(*testing.T, kolide.Datastore){ testMigrationStatus, testUnicode, testCountHostsInTargets, + testHostStatus, testResetOptions, testIdentityProvider, } diff --git a/server/datastore/inmem/hosts.go b/server/datastore/inmem/hosts.go index 61a7210f1f..9a3d06e3aa 100644 --- a/server/datastore/inmem/hosts.go +++ b/server/datastore/inmem/hosts.go @@ -113,7 +113,7 @@ func (d *Datastore) ListHosts(opt kolide.ListOptions) ([]*kolide.Host, error) { return hosts, nil } -func (d *Datastore) GenerateHostStatusStatistics(now time.Time, onlineInterval time.Duration) (online, offline, mia, new uint, err error) { +func (d *Datastore) GenerateHostStatusStatistics(now time.Time) (online, offline, mia, new uint, err error) { d.mtx.Lock() defer d.mtx.Unlock() @@ -122,7 +122,7 @@ func (d *Datastore) GenerateHostStatusStatistics(now time.Time, onlineInterval t new++ } - status := host.Status(now, onlineInterval) + status := host.Status(now) switch status { case kolide.StatusMIA: mia++ diff --git a/server/datastore/mysql/hosts.go b/server/datastore/mysql/hosts.go index 3014604378..43997ae987 100644 --- a/server/datastore/mysql/hosts.go +++ b/server/datastore/mysql/hosts.go @@ -314,32 +314,19 @@ func (d *Datastore) ListHosts(opt kolide.ListOptions) ([]*kolide.Host, error) { return hosts, nil } -func (d *Datastore) GenerateHostStatusStatistics(now time.Time, onlineInterval time.Duration) (online, offline, mia, new uint, e error) { - sqlStatement := ` - SELECT ( - SELECT count(id) - FROM hosts - WHERE DATE_ADD(seen_time, INTERVAL 30 DAY) <= ? - ) AS mia, - ( - SELECT count(id) - FROM hosts - WHERE DATE_ADD(seen_time, INTERVAL ? SECOND) <= ? - AND DATE_ADD(seen_time, INTERVAL 30 DAY) >= ? - ) AS offline, - ( - SELECT count(id) - FROM hosts - WHERE DATE_ADD(seen_time, INTERVAL ? SECOND) > ? - ) AS online, - ( - SELECT count(id) - FROM hosts - WHERE DATE_ADD(created_at, INTERVAL 1 DAY) >= ? - ) AS new +func (d *Datastore) GenerateHostStatusStatistics(now time.Time) (online, offline, mia, new uint, e error) { + // The logic in this function should remain synchronized with + // host.Status and CountHostsInTargets + + sqlStatement := fmt.Sprintf(` + SELECT + COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL 30 DAY) <= ? THEN 1 ELSE 0 END), 0) mia, + COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL LEAST(distributed_interval, config_tls_refresh) + %d SECOND) <= ? AND DATE_ADD(seen_time, INTERVAL 30 DAY) >= ? THEN 1 ELSE 0 END), 0) offline, + COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL LEAST(distributed_interval, config_tls_refresh) + %d SECOND) > ? THEN 1 ELSE 0 END), 0) online, + COALESCE(SUM(CASE WHEN DATE_ADD(created_at, INTERVAL 1 DAY) >= ? THEN 1 ELSE 0 END), 0) new FROM hosts LIMIT 1; - ` + `, kolide.OnlineIntervalBuffer, kolide.OnlineIntervalBuffer) counts := struct { MIA uint `db:"mia"` @@ -347,7 +334,7 @@ func (d *Datastore) GenerateHostStatusStatistics(now time.Time, onlineInterval t Online uint `db:"online"` New uint `db:"new"` }{} - err := d.db.Get(&counts, sqlStatement, now, onlineInterval.Seconds(), now, now, onlineInterval.Seconds(), now, now) + err := d.db.Get(&counts, sqlStatement, now, now, now, now, now) if err != nil && err != sql.ErrNoRows { e = errors.Wrap(err, "generating host statistics") return diff --git a/server/datastore/mysql/targets.go b/server/datastore/mysql/targets.go index 924e346b5e..be68511a77 100644 --- a/server/datastore/mysql/targets.go +++ b/server/datastore/mysql/targets.go @@ -1,6 +1,7 @@ package mysql import ( + "fmt" "time" "github.com/jmoiron/sqlx" @@ -8,23 +9,26 @@ import ( "github.com/pkg/errors" ) -func (d *Datastore) CountHostsInTargets(hostIDs []uint, labelIDs []uint, now time.Time, onlineInterval time.Duration) (kolide.TargetMetrics, error) { +func (d *Datastore) CountHostsInTargets(hostIDs []uint, labelIDs []uint, now time.Time) (kolide.TargetMetrics, error) { + // The logic in this function should remain synchronized with + // host.Status and GenerateHostStatusStatistics + if len(hostIDs) == 0 && len(labelIDs) == 0 { // No need to query if no targets selected return kolide.TargetMetrics{}, nil } - sql := ` + sql := fmt.Sprintf(` SELECT COUNT(*) total, COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL 30 DAY) <= ? THEN 1 ELSE 0 END), 0) mia, - COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL ? SECOND) <= ? AND DATE_ADD(seen_time, INTERVAL 30 DAY) >= ? THEN 1 ELSE 0 END), 0) offline, - COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL ? SECOND) > ? THEN 1 ELSE 0 END), 0) online, + COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL LEAST(distributed_interval, config_tls_refresh) + %d SECOND) <= ? AND DATE_ADD(seen_time, INTERVAL 30 DAY) >= ? THEN 1 ELSE 0 END), 0) offline, + COALESCE(SUM(CASE WHEN DATE_ADD(seen_time, INTERVAL LEAST(distributed_interval, config_tls_refresh) + %d SECOND) > ? THEN 1 ELSE 0 END), 0) online, COALESCE(SUM(CASE WHEN DATE_ADD(created_at, INTERVAL 1 DAY) >= ? THEN 1 ELSE 0 END), 0) new FROM hosts h WHERE (id IN (?) OR (id IN (SELECT DISTINCT host_id FROM label_query_executions WHERE label_id IN (?) AND matches = 1))) AND NOT deleted -` +`, kolide.OnlineIntervalBuffer, kolide.OnlineIntervalBuffer) // Using -1 in the ID slices for the IN clause allows us to include the // IN clause even if we have no IDs to use. -1 will not match the @@ -39,7 +43,7 @@ func (d *Datastore) CountHostsInTargets(hostIDs []uint, labelIDs []uint, now tim queryHostIDs = append(queryHostIDs, int(id)) } - query, args, err := sqlx.In(sql, now, onlineInterval.Seconds(), now, now, onlineInterval.Seconds(), now, now, queryHostIDs, queryLabelIDs) + query, args, err := sqlx.In(sql, now, now, now, now, now, queryHostIDs, queryLabelIDs) if err != nil { return kolide.TargetMetrics{}, errors.Wrap(err, "sqlx.In CountHostsInTargets") } diff --git a/server/kolide/hosts.go b/server/kolide/hosts.go index 76c87f9e7e..ac9f95d9ef 100644 --- a/server/kolide/hosts.go +++ b/server/kolide/hosts.go @@ -24,6 +24,11 @@ const ( // OfflineDuration if a host hasn't been in communition for this // period it is considered MIA. MIADuration = 30 * 24 * time.Hour + + // OnlineIntervalBuffer is the additional time in seconds to add to the + // online interval to avoid flapping of hosts that check in a bit later + // than their expected checkin interval. + OnlineIntervalBuffer = 30 ) type HostStore interface { @@ -35,8 +40,10 @@ type HostStore interface { EnrollHost(osqueryHostId string, nodeKeySize int) (*Host, error) AuthenticateHost(nodeKey string) (*Host, error) MarkHostSeen(host *Host, t time.Time) error - GenerateHostStatusStatistics(now time.Time, onlineInterval time.Duration) (online, offline, mia, new uint, err error) SearchHosts(query string, omit ...uint) ([]*Host, error) + // GenerateHostStatusStatistics retrieves the count of online, offline, + // MIA and new hosts. + GenerateHostStatusStatistics(now time.Time) (online, offline, mia, new uint, err error) // DistributedQueriesForHost retrieves the distributed queries that the // given host should run. The result map is a mapping from campaign ID // to query text. @@ -145,11 +152,23 @@ func RandomText(keySize int) (string, error) { return base64.StdEncoding.EncodeToString(key), nil } -func (h *Host) Status(now time.Time, onlineInterval time.Duration) string { +// Status calculates the online status of the host +func (h *Host) Status(now time.Time) string { + // The logic in this function should remain synchronized with + // GenerateHostStatusStatistics and CountHostsInTargets + + onlineInterval := h.ConfigTLSRefresh + if h.DistributedInterval < h.ConfigTLSRefresh { + onlineInterval = h.DistributedInterval + } + + // Add a small buffer to prevent flapping + onlineInterval += OnlineIntervalBuffer + switch { case h.SeenTime.Add(MIADuration).Before(now): return StatusMIA - case h.SeenTime.Add(onlineInterval).Before(now): + case h.SeenTime.Add(time.Duration(onlineInterval) * time.Second).Before(now): return StatusOffline default: return StatusOnline diff --git a/server/kolide/hosts_test.go b/server/kolide/hosts_test.go index 354d01ac60..ea2647353d 100644 --- a/server/kolide/hosts_test.go +++ b/server/kolide/hosts_test.go @@ -47,19 +47,43 @@ func TestResetHosts(t *testing.T) { func TestHostStatus(t *testing.T) { mockClock := clock.NewMockClock() - host := Host{} + var testCases = []struct { + seenTime time.Time + distributedInterval uint + configTLSRefresh uint + status string + }{ + {mockClock.Now().Add(-30 * time.Second), 10, 3600, StatusOnline}, + {mockClock.Now().Add(-45 * time.Second), 10, 3600, StatusOffline}, + {mockClock.Now().Add(-30 * time.Second), 3600, 10, StatusOnline}, + {mockClock.Now().Add(-45 * time.Second), 3600, 10, StatusOffline}, - host.SeenTime = mockClock.Now() - assert.Equal(t, StatusOnline, host.Status(mockClock.Now(), 60*time.Second)) + {mockClock.Now().Add(-70 * time.Second), 60, 60, StatusOnline}, + {mockClock.Now().Add(-91 * time.Second), 60, 60, StatusOffline}, - host.SeenTime = mockClock.Now().Add(-1 * time.Minute) - assert.Equal(t, StatusOnline, host.Status(mockClock.Now(), 60*time.Second)) + {mockClock.Now().Add(-1 * time.Second), 10, 10, StatusOnline}, + {mockClock.Now().Add(-1 * time.Minute), 10, 10, StatusOffline}, + {mockClock.Now().Add(-31 * 24 * time.Hour), 10, 10, StatusMIA}, - host.SeenTime = mockClock.Now().Add(-1 * time.Hour) - assert.Equal(t, StatusOffline, host.Status(mockClock.Now(), 60*time.Second)) + // Ensure behavior is reasonable if we don't have the values + {mockClock.Now().Add(-1 * time.Second), 0, 0, StatusOnline}, + {mockClock.Now().Add(-1 * time.Minute), 0, 0, StatusOffline}, + {mockClock.Now().Add(-31 * 24 * time.Hour), 0, 0, StatusMIA}, + } + + for _, tt := range testCases { + t.Run("", func(t *testing.T) { + // Save interval values + h := Host{ + DistributedInterval: tt.distributedInterval, + ConfigTLSRefresh: tt.configTLSRefresh, + SeenTime: tt.seenTime, + } + + assert.Equal(t, tt.status, h.Status(mockClock.Now())) + }) + } - host.SeenTime = mockClock.Now().Add(-35 * (24 * time.Hour)) // 35 days - assert.Equal(t, StatusMIA, host.Status(mockClock.Now(), 60*time.Second)) } func TestHostIsNew(t *testing.T) { diff --git a/server/kolide/options.go b/server/kolide/options.go index 4931fc3d84..cff9f64824 100644 --- a/server/kolide/options.go +++ b/server/kolide/options.go @@ -6,7 +6,6 @@ import ( "encoding/json" "fmt" "strings" - "time" ) // OptionStore interface describes methods to access datastore @@ -38,13 +37,6 @@ type OptionService interface { // ModifyOptions will change values of the options in OptionRequest. Note // passing ReadOnly options will cause an error. ModifyOptions(ctx context.Context, req OptionRequest) ([]Option, error) - // ExpectedCheckinInterval returns how often we should expect to hear from a - // host. By maintaining a known list of osquery configuration options which - // influence the interval that osqueryd hosts check-in to a TLS server, we - // can deduce a minimum amount of time that we should expect to hear from an - // osqueryd agent if it is online. This is currently two times the most - // frequent check-in interval. - ExpectedCheckinInterval(ctx context.Context) (time.Duration, error) // ResetOptions resets all options to their default values ResetOptions(ctx context.Context) ([]Option, error) } diff --git a/server/kolide/targets.go b/server/kolide/targets.go index a1f98d2cd2..ddf5bf7033 100644 --- a/server/kolide/targets.go +++ b/server/kolide/targets.go @@ -10,18 +10,25 @@ type TargetSearchResults struct { Labels []Label } -// TargetMetrics contains information about the state -// of hosts that are tracked by the app +// TargetMetrics contains information about the online status of a set of +// hosts. type TargetMetrics struct { + // TotalHosts is the total hosts in any status. It should equal + // OnlineHosts + OfflineHosts + MissingInActionHosts. TotalHosts uint `db:"total"` - // OnlineHosts have updated within the last 30 minutes + // OnlineHosts is the count of hosts that have checked in within their + // expected checkin interval (based on the configuration interval + // values, see Host.Status()). OnlineHosts uint `db:"online"` - // OfflineHosts are hosts that haven't updated in 30 minutes + // OfflineHosts is the count of hosts that have not checked in within + // their expected interval. OfflineHosts uint `db:"offline"` - // MissingInActionHosts are hosts that haven't had an update for more - // than thirty days + // MissingInActionHosts is the count of hosts that have not checked in + // within the last 30 days. MissingInActionHosts uint `db:"mia"` - NewHosts uint `db:"new"` + // NewHosts is the count of hosts that have enrolled in the last 24 + // hours. + NewHosts uint `db:"new"` } type TargetService interface { @@ -30,16 +37,15 @@ type TargetService interface { // (hosts and label) which match the supplied search query. SearchTargets(ctx context.Context, query string, selectedHostIDs []uint, selectedLabelIDs []uint) (*TargetSearchResults, error) - // CountHostsInTargets returns the count of hosts in the selected - // targets. The first return uint is the total number of hosts in the - // targets. The second return uint is the total online hosts. The third - // returned uint is the total number of hosts that have been offline for more - // than 30 days. (Missing in action) + // CountHostsInTargets returns the metrics of the hosts in the provided + // label and explicit host IDs. CountHostsInTargets(ctx context.Context, hostIDs []uint, labelIDs []uint) (*TargetMetrics, error) } type TargetStore interface { - CountHostsInTargets(hostIDs []uint, labelIDs []uint, now time.Time, onlineInterval time.Duration) (TargetMetrics, error) + // CountHostsInTargets returns the metrics of the hosts in the provided + // label and explicit host IDs. + CountHostsInTargets(hostIDs []uint, labelIDs []uint, now time.Time) (TargetMetrics, error) } type TargetType int diff --git a/server/mock/datastore_hosts.go b/server/mock/datastore_hosts.go index ba56e75ca1..8200490b97 100644 --- a/server/mock/datastore_hosts.go +++ b/server/mock/datastore_hosts.go @@ -26,7 +26,7 @@ type AuthenticateHostFunc func(nodeKey string) (*kolide.Host, error) type MarkHostSeenFunc func(host *kolide.Host, t time.Time) error -type GenerateHostStatusStatisticsFunc func(now time.Time, onlineInterval time.Duration) (online uint, offline uint, mia uint, new uint, err error) +type GenerateHostStatusStatisticsFunc func(now time.Time) (online uint, offline uint, mia uint, new uint, err error) type SearchHostsFunc func(query string, omit ...uint) ([]*kolide.Host, error) @@ -107,9 +107,9 @@ func (s *HostStore) MarkHostSeen(host *kolide.Host, t time.Time) error { return s.MarkHostSeenFunc(host, t) } -func (s *HostStore) GenerateHostStatusStatistics(now time.Time, onlineInterval time.Duration) (online uint, offline uint, mia uint, new uint, err error) { +func (s *HostStore) GenerateHostStatusStatistics(now time.Time) (online uint, offline uint, mia uint, new uint, err error) { s.GenerateHostStatusStatisticsFuncInvoked = true - return s.GenerateHostStatusStatisticsFunc(now, onlineInterval) + return s.GenerateHostStatusStatisticsFunc(now) } func (s *HostStore) SearchHosts(query string, omit ...uint) ([]*kolide.Host, error) { diff --git a/server/service/endpoint_hosts.go b/server/service/endpoint_hosts.go index 3ceedf7a85..823a896398 100644 --- a/server/service/endpoint_hosts.go +++ b/server/service/endpoint_hosts.go @@ -6,7 +6,6 @@ import ( "github.com/go-kit/kit/endpoint" "github.com/kolide/kolide/server/kolide" - "github.com/pkg/errors" ) type hostResponse struct { @@ -15,10 +14,10 @@ type hostResponse struct { DisplayText string `json:"display_text"` } -func hostResponseForHost(ctx context.Context, svc kolide.Service, host *kolide.Host, onlineInterval time.Duration) (*hostResponse, error) { +func hostResponseForHost(ctx context.Context, svc kolide.Service, host *kolide.Host) (*hostResponse, error) { return &hostResponse{ Host: *host, - Status: host.Status(time.Now(), onlineInterval), + Status: host.Status(time.Now()), DisplayText: host.HostName, }, nil } @@ -46,12 +45,7 @@ func makeGetHostEndpoint(svc kolide.Service) endpoint.Endpoint { return getHostResponse{Err: err}, nil } - onlineInterval, err := svc.ExpectedCheckinInterval(ctx) - if err != nil { - return nil, errors.Wrap(err, "getting expected check-in interval") - } - - resp, err := hostResponseForHost(ctx, svc, host, onlineInterval) + resp, err := hostResponseForHost(ctx, svc, host) if err != nil { return getHostResponse{Err: err}, nil } @@ -85,14 +79,9 @@ func makeListHostsEndpoint(svc kolide.Service) endpoint.Endpoint { return listHostsResponse{Err: err}, nil } - onlineInterval, err := svc.ExpectedCheckinInterval(ctx) - if err != nil { - return nil, errors.Wrap(err, "getting expected check-in interval") - } - hostResponses := make([]hostResponse, len(hosts), len(hosts)) for i, host := range hosts { - h, err := hostResponseForHost(ctx, svc, host, onlineInterval) + h, err := hostResponseForHost(ctx, svc, host) if err != nil { return listHostsResponse{Err: err}, nil } diff --git a/server/service/endpoint_targets.go b/server/service/endpoint_targets.go index 3d2b1a8950..3d16b0f4d6 100644 --- a/server/service/endpoint_targets.go +++ b/server/service/endpoint_targets.go @@ -6,7 +6,6 @@ import ( "github.com/go-kit/kit/endpoint" "github.com/kolide/kolide/server/kolide" - "github.com/pkg/errors" ) //////////////////////////////////////////////////////////////////////////////// @@ -65,17 +64,12 @@ func makeSearchTargetsEndpoint(svc kolide.Service) endpoint.Endpoint { Labels: []labelSearchResult{}, } - onlineInterval, err := svc.ExpectedCheckinInterval(ctx) - if err != nil { - return searchTargetsResponse{Err: errors.Wrap(err, "getting expected check-in interval")}, nil - } - for _, host := range results.Hosts { targets.Hosts = append(targets.Hosts, hostSearchResult{ hostResponse{ Host: host, - Status: host.Status(time.Now(), onlineInterval), + Status: host.Status(time.Now()), }, host.HostName, }, diff --git a/server/service/logging_options.go b/server/service/logging_options.go index a5cda346fb..425d5ceb0b 100644 --- a/server/service/logging_options.go +++ b/server/service/logging_options.go @@ -41,22 +41,6 @@ func (mw loggingMiddleware) ModifyOptions(ctx context.Context, req kolide.Option return options, err } -func (mw loggingMiddleware) ExpectedCheckinInterval(ctx context.Context) (time.Duration, error) { - var ( - interval time.Duration - err error - ) - defer func(begin time.Time) { - mw.logger.Log( - "method", "ExpectedCheckinInterval", - "err", err, - "took", time.Since(begin), - ) - }(time.Now()) - interval, err = mw.Service.ExpectedCheckinInterval(ctx) - return interval, err -} - func (mw loggingMiddleware) ResetOptions(ctx context.Context) ([]kolide.Option, error) { var ( options []kolide.Option diff --git a/server/service/metrics_options.go b/server/service/metrics_options.go index ca145ec311..6fb2d8333a 100644 --- a/server/service/metrics_options.go +++ b/server/service/metrics_options.go @@ -49,17 +49,3 @@ func (mw metricsMiddleware) ResetOptions(ctx context.Context) ([]kolide.Option, options, err = mw.Service.ResetOptions(ctx) return options, err } - -func (mw metricsMiddleware) ExpectedCheckinInterval(ctx context.Context) (time.Duration, error) { - var ( - interval time.Duration - err error - ) - defer func(begin time.Time) { - lvs := []string{"method", "ExpectedCheckinInterval", "error", fmt.Sprint(err != nil)} - mw.requestCount.With(lvs...).Add(1) - mw.requestLatency.With(lvs...).Observe(time.Since(begin).Seconds()) - }(time.Now()) - interval, err = mw.Service.ExpectedCheckinInterval(ctx) - return interval, err -} diff --git a/server/service/service_hosts.go b/server/service/service_hosts.go index c4e6a55403..1b8e39afa5 100644 --- a/server/service/service_hosts.go +++ b/server/service/service_hosts.go @@ -15,11 +15,7 @@ func (svc service) GetHost(ctx context.Context, id uint) (*kolide.Host, error) { } func (svc service) GetHostSummary(ctx context.Context) (*kolide.HostSummary, error) { - onlineInterval, err := svc.ExpectedCheckinInterval(ctx) - if err != nil { - return nil, err - } - online, offline, mia, new, err := svc.ds.GenerateHostStatusStatistics(svc.clock.Now(), onlineInterval) + online, offline, mia, new, err := svc.ds.GenerateHostStatusStatistics(svc.clock.Now()) if err != nil { return nil, err } diff --git a/server/service/service_options.go b/server/service/service_options.go index 0ac806695f..9f0b6b5b57 100644 --- a/server/service/service_options.go +++ b/server/service/service_options.go @@ -2,15 +2,10 @@ package service import ( "context" - "time" "github.com/kolide/kolide/server/kolide" - "github.com/pkg/errors" ) -const expectedCheckinIntervalMultiplier = 2 -const minimumExpectedCheckinInterval = 10 * time.Second - func (svc service) ResetOptions(ctx context.Context) ([]kolide.Option, error) { return svc.ds.ResetOptions() } @@ -29,71 +24,3 @@ func (svc service) ModifyOptions(ctx context.Context, req kolide.OptionRequest) } return req.Options, nil } - -func (svc service) ExpectedCheckinInterval(ctx context.Context) (time.Duration, error) { - interval := uint(0) - found := false - - osqueryIntervalOptionNames := []string{ - "distributed_interval", - "logger_tls_period", - } - - for _, option := range osqueryIntervalOptionNames { - // for each option which is known to hold a TLS check-in interval, try to - // fetch it - opt, err := svc.ds.OptionByName(option) - if err != nil { - // if the option is not set, try the next known option - if _, ok := err.(kolide.NotFoundError); ok { - continue - } - // if some other error occured when getting the option, we want to return - // that - return 0, err - } - - // try to cast the option as a uint. if this fails, the option has likely been set incorrectly - var val uint - switch v := opt.Value.Val.(type) { - case int: - val = uint(v) - case uint: - val = v - case uint64: - val = uint(v) - case float64: - val = uint(v) - default: - return 0, errors.New("Option is not a number: " + opt.Name) - } - - // If an option has not been found yet, we want to save this interval. - // If an option HAS been found already and this one is less, we want to - // save that as our new minimum check-in interval. - if !found || val < interval { - found = true - interval = val - } - } - - // if we never found any interval options set, the default distributed - // interval is 60, so we use that - if !found { - interval = 60 - } - - // The interval is multiplied to ensure that we are being generous in - // the calculation if the host is a bit slower than the interval to - // check in. This prevents flapping of the online status. - calculatedInterval := time.Duration(interval) * time.Second * expectedCheckinIntervalMultiplier - - // We use a minimum threshold here to ensure that online status does - // not flap when the interval is set very low. - if calculatedInterval < minimumExpectedCheckinInterval { - calculatedInterval = minimumExpectedCheckinInterval - } - - // return the lowest interval that we found - return calculatedInterval, nil -} diff --git a/server/service/service_options_test.go b/server/service/service_options_test.go deleted file mode 100644 index 5b51007da8..0000000000 --- a/server/service/service_options_test.go +++ /dev/null @@ -1,134 +0,0 @@ -package service - -import ( - "context" - "testing" - "time" - - "github.com/kolide/kolide/server/config" - "github.com/kolide/kolide/server/datastore/inmem" - "github.com/kolide/kolide/server/kolide" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestExpectedCheckinInterval(t *testing.T) { - ds, err := inmem.New(config.TestConfig()) - require.Nil(t, err) - require.Nil(t, ds.MigrateData()) - svc, err := newTestService(ds, nil) - require.Nil(t, err) - ctx := context.Background() - - var distributedInterval uint - var distributedIntervalID uint - var loggerTlsPeriod uint - var loggerTlsPeriodID uint - - updateLocalOptionValues := func(opts []kolide.Option) { - for _, option := range opts { - if option.Name == "distributed_interval" { - distributedInterval = uint(option.Value.Val.(int)) - distributedIntervalID = option.ID - } - if option.Name == "logger_tls_period" { - loggerTlsPeriod = uint(option.Value.Val.(int)) - loggerTlsPeriodID = option.ID - } - } - } - - options, err := svc.GetOptions(ctx) - require.Nil(t, err) - updateLocalOptionValues(options) - require.Equal(t, 10, int(distributedInterval)) - require.Equal(t, 10, int(loggerTlsPeriod)) - interval, err := svc.ExpectedCheckinInterval(ctx) - require.Nil(t, err) - assert.Equal(t, 10*time.Second*expectedCheckinIntervalMultiplier, interval) - - options, err = svc.ModifyOptions(ctx, kolide.OptionRequest{ - Options: []kolide.Option{ - kolide.Option{ - ID: distributedIntervalID, - Name: "distributed_interval", - Value: kolide.OptionValue{ - Val: 50, - }, - Type: kolide.OptionTypeInt, - ReadOnly: false, - }, - kolide.Option{ - ID: loggerTlsPeriodID, - Name: "logger_tls_period", - Value: kolide.OptionValue{ - Val: 100, - }, - Type: kolide.OptionTypeInt, - ReadOnly: false, - }, - }, - }, - ) - require.Nil(t, err) - - options, err = svc.GetOptions(ctx) - require.Nil(t, err) - updateLocalOptionValues(options) - require.Equal(t, 50, int(distributedInterval)) - require.Equal(t, 100, int(loggerTlsPeriod)) - interval, err = svc.ExpectedCheckinInterval(ctx) - require.Nil(t, err) - assert.Equal(t, 50*time.Second*expectedCheckinIntervalMultiplier, interval) - - options, err = svc.ModifyOptions(ctx, kolide.OptionRequest{ - Options: []kolide.Option{ - kolide.Option{ - ID: loggerTlsPeriodID, - Name: "logger_tls_period", - Value: kolide.OptionValue{ - Val: 20, - }, - Type: kolide.OptionTypeInt, - ReadOnly: false, - }, - }, - }, - ) - require.Nil(t, err) - - options, err = svc.GetOptions(ctx) - require.Nil(t, err) - updateLocalOptionValues(options) - require.Equal(t, 50, int(distributedInterval)) - require.Equal(t, 20, int(loggerTlsPeriod)) - interval, err = svc.ExpectedCheckinInterval(ctx) - require.Nil(t, err) - assert.Equal(t, 20*time.Second*expectedCheckinIntervalMultiplier, interval) - - // Set the interval low enough to hit the minimum threshold - options, err = svc.ModifyOptions(ctx, kolide.OptionRequest{ - Options: []kolide.Option{ - kolide.Option{ - ID: loggerTlsPeriodID, - Name: "logger_tls_period", - Value: kolide.OptionValue{ - Val: 2, - }, - Type: kolide.OptionTypeInt, - ReadOnly: false, - }, - }, - }, - ) - require.Nil(t, err) - - options, err = svc.GetOptions(ctx) - require.Nil(t, err) - updateLocalOptionValues(options) - require.Equal(t, 50, int(distributedInterval)) - require.Equal(t, 2, int(loggerTlsPeriod)) - interval, err = svc.ExpectedCheckinInterval(ctx) - require.Nil(t, err) - assert.Equal(t, minimumExpectedCheckinInterval, interval) -} diff --git a/server/service/service_targets.go b/server/service/service_targets.go index d2bdaaa7d7..d9e1736109 100644 --- a/server/service/service_targets.go +++ b/server/service/service_targets.go @@ -4,7 +4,6 @@ import ( "context" "github.com/kolide/kolide/server/kolide" - "github.com/pkg/errors" ) func (svc service) SearchTargets(ctx context.Context, query string, selectedHostIDs []uint, selectedLabelIDs []uint) (*kolide.TargetSearchResults, error) { @@ -29,12 +28,7 @@ func (svc service) SearchTargets(ctx context.Context, query string, selectedHost } func (svc service) CountHostsInTargets(ctx context.Context, hostIDs []uint, labelIDs []uint) (*kolide.TargetMetrics, error) { - onlineInterval, err := svc.ExpectedCheckinInterval(ctx) - if err != nil { - return nil, errors.Wrap(err, "getting expected check-in interval") - } - - metrics, err := svc.ds.CountHostsInTargets(hostIDs, labelIDs, svc.clock.Now(), onlineInterval) + metrics, err := svc.ds.CountHostsInTargets(hostIDs, labelIDs, svc.clock.Now()) if err != nil { return nil, err }