From 8d27d324d92a5165aab514e7ec3a13d1a4956229 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 16 Dec 2024 14:24:39 -0800 Subject: [PATCH 1/7] add min interval between presence detections --- ee/localserver/server.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/ee/localserver/server.go b/ee/localserver/server.go index ce4f6a917..0f053f66b 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -60,8 +60,9 @@ type localServer struct { serverKey *rsa.PublicKey serverEcKey *ecdsa.PublicKey - presenceDetector presenceDetector - presenceDetectionMutex sync.Mutex + presenceDetector presenceDetector + presenceDetectionMutex sync.Mutex + lastPresenceDetectionAttempt time.Time } const ( @@ -422,6 +423,21 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler ls.presenceDetectionMutex.Lock() defer ls.presenceDetectionMutex.Unlock() + // if the user fails or cancels the presence detection, we want to wait a bit before trying again + // so that if there are several queued up requests, we don't prompt the user multiple times in a row + // if they keep hitting cancel + + const minTimeBetweenPresenceDetection = 3 * time.Second + if time.Since(ls.lastPresenceDetectionAttempt) < minTimeBetweenPresenceDetection { + ls.slogger.Log(r.Context(), slog.LevelInfo, + "presence detection attempted too soon", + ) + + w.Header().Add(kolideDurationSinceLastPresenceDetectionHeaderKey, presencedetection.DetectionFailedDurationValue.String()) + next.ServeHTTP(w, r) + return + } + // can test this by adding an unauthed endpoint to the mux and running, for example: // curl -i -H "X-Kolide-Presence-Detection-Interval: 10s" -H "X-Kolide-Presence-Detection-Reason: my reason" localhost:12519/id detectionIntervalStr := r.Header.Get(kolidePresenceDetectionIntervalHeaderKey) @@ -461,7 +477,6 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler } durationSinceLastDetection, err := ls.presenceDetector.DetectPresence(reason, detectionIntervalDuration) - if err != nil { ls.slogger.Log(r.Context(), slog.LevelInfo, "presence_detection", @@ -472,6 +487,8 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler ) } + ls.lastPresenceDetectionAttempt = time.Now() + // if there was an error, we still want to return a 200 status code // and send the request through // allow the server to decide what to do based on last detection duration From 492d21e0e1ad7673067400a045a872157203942b Mon Sep 17 00:00:00 2001 From: James Pickett Date: Mon, 16 Dec 2024 14:32:49 -0800 Subject: [PATCH 2/7] more logging --- ee/localserver/server.go | 1 + 1 file changed, 1 insertion(+) diff --git a/ee/localserver/server.go b/ee/localserver/server.go index 0f053f66b..72738ed89 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -431,6 +431,7 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler if time.Since(ls.lastPresenceDetectionAttempt) < minTimeBetweenPresenceDetection { ls.slogger.Log(r.Context(), slog.LevelInfo, "presence detection attempted too soon", + "min_interval", minTimeBetweenPresenceDetection, ) w.Header().Add(kolideDurationSinceLastPresenceDetectionHeaderKey, presencedetection.DetectionFailedDurationValue.String()) From 81b14892080a4c8f966f05bd7a2b41b186a216f4 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Tue, 17 Dec 2024 08:31:33 -0800 Subject: [PATCH 3/7] add test for min interval --- ee/localserver/presence-detection-middleware_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ee/localserver/presence-detection-middleware_test.go b/ee/localserver/presence-detection-middleware_test.go index c6206dcff..6fcb8c88e 100644 --- a/ee/localserver/presence-detection-middleware_test.go +++ b/ee/localserver/presence-detection-middleware_test.go @@ -79,7 +79,7 @@ func TestPresenceDetectionHandler(t *testing.T) { mockPresenceDetector := mocks.NewPresenceDetector(t) if tt.expectDetectPresenceCall { - mockPresenceDetector.On("DetectPresence", mock.AnythingOfType("string"), mock.AnythingOfType("Duration")).Return(tt.durationSinceLastDetection, tt.presenceDetectionError) + mockPresenceDetector.On("DetectPresence", mock.AnythingOfType("string"), mock.AnythingOfType("Duration")).Return(tt.durationSinceLastDetection, tt.presenceDetectionError).Once() } server := &localServer{ @@ -112,6 +112,14 @@ func TestPresenceDetectionHandler(t *testing.T) { require.NotEmpty(t, rr.Header().Get(kolideDurationSinceLastPresenceDetectionHeaderKey)) } require.Equal(t, tt.expectedStatusCode, rr.Code) + + // fire the request one more time, it should not call presence detector again + // because it's less than the minimum interval + mockPresenceDetector = mocks.NewPresenceDetector(t) + server.presenceDetector = mockPresenceDetector + + handlerToTest.ServeHTTP(rr, req) + mockPresenceDetector.AssertNotCalled(t, "DetectPresence", mock.Anything, mock.Anything) }) } } From a43d66baac646d895252be45740fbc25f4c0bcde Mon Sep 17 00:00:00 2001 From: James Pickett Date: Tue, 17 Dec 2024 12:22:47 -0800 Subject: [PATCH 4/7] move min attempt interval into user space struct --- .../presence-detection-middleware_test.go | 8 ---- ee/localserver/request-id.go | 2 +- ee/localserver/server.go | 27 +++--------- ee/presencedetection/presencedetection.go | 28 ++++++++++--- .../presencedetection_test.go | 41 +++++++++++++++++++ 5 files changed, 69 insertions(+), 37 deletions(-) diff --git a/ee/localserver/presence-detection-middleware_test.go b/ee/localserver/presence-detection-middleware_test.go index 6fcb8c88e..b5a5cb1b2 100644 --- a/ee/localserver/presence-detection-middleware_test.go +++ b/ee/localserver/presence-detection-middleware_test.go @@ -112,14 +112,6 @@ func TestPresenceDetectionHandler(t *testing.T) { require.NotEmpty(t, rr.Header().Get(kolideDurationSinceLastPresenceDetectionHeaderKey)) } require.Equal(t, tt.expectedStatusCode, rr.Code) - - // fire the request one more time, it should not call presence detector again - // because it's less than the minimum interval - mockPresenceDetector = mocks.NewPresenceDetector(t) - server.presenceDetector = mockPresenceDetector - - handlerToTest.ServeHTTP(rr, req) - mockPresenceDetector.AssertNotCalled(t, "DetectPresence", mock.Anything, mock.Anything) }) } } diff --git a/ee/localserver/request-id.go b/ee/localserver/request-id.go index aad795413..300835667 100644 --- a/ee/localserver/request-id.go +++ b/ee/localserver/request-id.go @@ -84,7 +84,7 @@ func (ls *localServer) requestIdHandlerFunc(w http.ResponseWriter, r *http.Reque Origin: r.Header.Get("Origin"), Status: status{ EnrollmentStatus: string(enrollmentStatus), - InstanceStatuses: ls.knapsack.InstanceStatuses(), + // InstanceStatuses: ls.knapsack.InstanceStatuses(), }, } response.identifiers = ls.identifiers diff --git a/ee/localserver/server.go b/ee/localserver/server.go index 72738ed89..eab5db8f3 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -60,9 +60,8 @@ type localServer struct { serverKey *rsa.PublicKey serverEcKey *ecdsa.PublicKey - presenceDetector presenceDetector - presenceDetectionMutex sync.Mutex - lastPresenceDetectionAttempt time.Time + presenceDetector presenceDetector + presenceDetectionMutex sync.Mutex } const ( @@ -132,7 +131,9 @@ func New(ctx context.Context, k types.Knapsack, presenceDetector presenceDetecto // curl localhost:40978/acceleratecontrol --data '{"interval":"250ms", "duration":"1s"}' // mux.Handle("/acceleratecontrol", ls.requestAccelerateControlHandler()) // curl localhost:40978/id - // mux.Handle("/id", ls.requestIdHandler()) + mux.Handle("/id", ls.presenceDetectionHandler(ls.requestIdHandler())) + + mux.Handle("/id2", ls.requestIdHandler()) srv := &http.Server{ Handler: otelhttp.NewHandler( @@ -423,22 +424,6 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler ls.presenceDetectionMutex.Lock() defer ls.presenceDetectionMutex.Unlock() - // if the user fails or cancels the presence detection, we want to wait a bit before trying again - // so that if there are several queued up requests, we don't prompt the user multiple times in a row - // if they keep hitting cancel - - const minTimeBetweenPresenceDetection = 3 * time.Second - if time.Since(ls.lastPresenceDetectionAttempt) < minTimeBetweenPresenceDetection { - ls.slogger.Log(r.Context(), slog.LevelInfo, - "presence detection attempted too soon", - "min_interval", minTimeBetweenPresenceDetection, - ) - - w.Header().Add(kolideDurationSinceLastPresenceDetectionHeaderKey, presencedetection.DetectionFailedDurationValue.String()) - next.ServeHTTP(w, r) - return - } - // can test this by adding an unauthed endpoint to the mux and running, for example: // curl -i -H "X-Kolide-Presence-Detection-Interval: 10s" -H "X-Kolide-Presence-Detection-Reason: my reason" localhost:12519/id detectionIntervalStr := r.Header.Get(kolidePresenceDetectionIntervalHeaderKey) @@ -488,8 +473,6 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler ) } - ls.lastPresenceDetectionAttempt = time.Now() - // if there was an error, we still want to return a 200 status code // and send the request through // allow the server to decide what to do based on last detection duration diff --git a/ee/presencedetection/presencedetection.go b/ee/presencedetection/presencedetection.go index 394f3aae9..abba7c52f 100644 --- a/ee/presencedetection/presencedetection.go +++ b/ee/presencedetection/presencedetection.go @@ -7,15 +7,18 @@ import ( ) const ( - DetectionFailedDurationValue = -1 * time.Second - DetectionTimeout = 1 * time.Minute + DetectionFailedDurationValue = -1 * time.Second + DetectionTimeout = 1 * time.Minute + DefaultMinDetectionAttmemptInterval = 3 * time.Second ) type PresenceDetector struct { lastDetection time.Time - mutext sync.Mutex + mutex sync.Mutex // detector is an interface to allow for mocking in tests - detector detectorIface + detector detectorIface + lastDetectionAttempt time.Time + minDetectionAttemptInterval time.Duration } // just exists for testing purposes @@ -32,8 +35,8 @@ func (d *detector) Detect(reason string) (bool, error) { // DetectPresence checks if the user is present by detecting the presence of a user. // It returns the duration since the last detection. func (pd *PresenceDetector) DetectPresence(reason string, detectionInterval time.Duration) (time.Duration, error) { - pd.mutext.Lock() - defer pd.mutext.Unlock() + pd.mutex.Lock() + defer pd.mutex.Unlock() if pd.detector == nil { pd.detector = &detector{} @@ -44,7 +47,20 @@ func (pd *PresenceDetector) DetectPresence(reason string, detectionInterval time return time.Since(pd.lastDetection), nil } + minDetetionInterval := pd.minDetectionAttemptInterval + if minDetetionInterval == 0 { + minDetetionInterval = DefaultMinDetectionAttmemptInterval + } + + // if the user fails or cancels the presence detection, we want to wait a bit before trying again + // so that if there are several queued up requests, we don't prompt the user multiple times in a row + // if they keep hitting cancel + if time.Since(pd.lastDetectionAttempt) < minDetetionInterval { + return time.Since(pd.lastDetection), nil + } + success, err := pd.detector.Detect(reason) + pd.lastDetectionAttempt = time.Now() if err != nil { // if we got an error, we behave as if there have been no successful detections in the past return DetectionFailedDurationValue, fmt.Errorf("detecting presence: %w", err) diff --git a/ee/presencedetection/presencedetection_test.go b/ee/presencedetection/presencedetection_test.go index e8dbb603d..4fbd95ab4 100644 --- a/ee/presencedetection/presencedetection_test.go +++ b/ee/presencedetection/presencedetection_test.go @@ -9,6 +9,7 @@ import ( "github.com/kolide/launcher/ee/presencedetection/mocks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) func TestPresenceDetector_DetectPresence(t *testing.T) { @@ -82,6 +83,18 @@ func TestPresenceDetector_DetectPresence(t *testing.T) { initialLastDetectionUTC: time.Now().UTC(), expectError: true, }, + { + // this should never happen, but it is here for completeness + name: "subsequent detection inside min detection attempt interval", + interval: 0, + detector: func(t *testing.T) detectorIface { + d := mocks.NewDetectorIface(t) + d.On("Detect", mock.AnythingOfType("string")).Return(false, nil).Once() + return d + }, + initialLastDetectionUTC: time.Now().UTC(), + expectError: true, + }, } for _, tt := range tests { @@ -106,3 +119,31 @@ func TestPresenceDetector_DetectPresence(t *testing.T) { }) } } + +func TestPresenceDetector_AttemptInterval(t *testing.T) { + t.Parallel() + + d := mocks.NewDetectorIface(t) + d.On("Detect", mock.AnythingOfType("string")).Return(true, nil).Once() + + pd := &PresenceDetector{ + detector: d, + minDetectionAttemptInterval: 500 * time.Millisecond, + } + + interval, err := pd.DetectPresence("this is a test", 0) + assert.NoError(t, err) + + require.Equal(t, time.Duration(0), interval, + "interval should be 0 since detection just happened", + ) + + d = mocks.NewDetectorIface(t) + pd.detector = d + + interval, err = pd.DetectPresence("this is a test", 0) + assert.NoError(t, err) + require.Greater(t, interval, time.Duration(0), + "interval should be greater than 0 since some time passed since last detection", + ) +} From bded4956da4b403311f23da105df7106ff0ad2fd Mon Sep 17 00:00:00 2001 From: James Pickett Date: Tue, 17 Dec 2024 12:25:40 -0800 Subject: [PATCH 5/7] remove debugging stuff --- ee/localserver/request-id.go | 2 +- ee/localserver/server.go | 4 +--- ee/presencedetection/presencedetection_test.go | 12 ------------ 3 files changed, 2 insertions(+), 16 deletions(-) diff --git a/ee/localserver/request-id.go b/ee/localserver/request-id.go index 300835667..aad795413 100644 --- a/ee/localserver/request-id.go +++ b/ee/localserver/request-id.go @@ -84,7 +84,7 @@ func (ls *localServer) requestIdHandlerFunc(w http.ResponseWriter, r *http.Reque Origin: r.Header.Get("Origin"), Status: status{ EnrollmentStatus: string(enrollmentStatus), - // InstanceStatuses: ls.knapsack.InstanceStatuses(), + InstanceStatuses: ls.knapsack.InstanceStatuses(), }, } response.identifiers = ls.identifiers diff --git a/ee/localserver/server.go b/ee/localserver/server.go index eab5db8f3..1db2243d3 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -131,9 +131,7 @@ func New(ctx context.Context, k types.Knapsack, presenceDetector presenceDetecto // curl localhost:40978/acceleratecontrol --data '{"interval":"250ms", "duration":"1s"}' // mux.Handle("/acceleratecontrol", ls.requestAccelerateControlHandler()) // curl localhost:40978/id - mux.Handle("/id", ls.presenceDetectionHandler(ls.requestIdHandler())) - - mux.Handle("/id2", ls.requestIdHandler()) + // mux.Handle("/id", ls.presenceDetectionHandler(ls.requestIdHandler())) srv := &http.Server{ Handler: otelhttp.NewHandler( diff --git a/ee/presencedetection/presencedetection_test.go b/ee/presencedetection/presencedetection_test.go index 4fbd95ab4..ed7eaf156 100644 --- a/ee/presencedetection/presencedetection_test.go +++ b/ee/presencedetection/presencedetection_test.go @@ -83,18 +83,6 @@ func TestPresenceDetector_DetectPresence(t *testing.T) { initialLastDetectionUTC: time.Now().UTC(), expectError: true, }, - { - // this should never happen, but it is here for completeness - name: "subsequent detection inside min detection attempt interval", - interval: 0, - detector: func(t *testing.T) detectorIface { - d := mocks.NewDetectorIface(t) - d.On("Detect", mock.AnythingOfType("string")).Return(false, nil).Once() - return d - }, - initialLastDetectionUTC: time.Now().UTC(), - expectError: true, - }, } for _, tt := range tests { From 7b1c07292be73242c50a12108ab1d14e09d1c324 Mon Sep 17 00:00:00 2001 From: James Pickett Date: Tue, 17 Dec 2024 12:27:42 -0800 Subject: [PATCH 6/7] fix test --- ee/localserver/server.go | 2 +- ee/presencedetection/presencedetection_test.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ee/localserver/server.go b/ee/localserver/server.go index 1db2243d3..46a725988 100644 --- a/ee/localserver/server.go +++ b/ee/localserver/server.go @@ -131,7 +131,7 @@ func New(ctx context.Context, k types.Knapsack, presenceDetector presenceDetecto // curl localhost:40978/acceleratecontrol --data '{"interval":"250ms", "duration":"1s"}' // mux.Handle("/acceleratecontrol", ls.requestAccelerateControlHandler()) // curl localhost:40978/id - // mux.Handle("/id", ls.presenceDetectionHandler(ls.requestIdHandler())) + // mux.Handle("/id", ls.requestIdHandler()) srv := &http.Server{ Handler: otelhttp.NewHandler( diff --git a/ee/presencedetection/presencedetection_test.go b/ee/presencedetection/presencedetection_test.go index ed7eaf156..48065d8a4 100644 --- a/ee/presencedetection/presencedetection_test.go +++ b/ee/presencedetection/presencedetection_test.go @@ -134,4 +134,7 @@ func TestPresenceDetector_AttemptInterval(t *testing.T) { require.Greater(t, interval, time.Duration(0), "interval should be greater than 0 since some time passed since last detection", ) + + // should not have been called since we are within the minDetectionAttemptInterval + d.AssertNotCalled(t, "Detect", mock.Anything) } From 9a4ad0cca528d335d70190daa7530b4d7713205f Mon Sep 17 00:00:00 2001 From: James Pickett Date: Tue, 17 Dec 2024 12:29:59 -0800 Subject: [PATCH 7/7] better test --- ee/presencedetection/presencedetection_test.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ee/presencedetection/presencedetection_test.go b/ee/presencedetection/presencedetection_test.go index 48065d8a4..b17d933f5 100644 --- a/ee/presencedetection/presencedetection_test.go +++ b/ee/presencedetection/presencedetection_test.go @@ -119,9 +119,9 @@ func TestPresenceDetector_AttemptInterval(t *testing.T) { minDetectionAttemptInterval: 500 * time.Millisecond, } + // first detection, should prompt user for new detection interval, err := pd.DetectPresence("this is a test", 0) assert.NoError(t, err) - require.Equal(t, time.Duration(0), interval, "interval should be 0 since detection just happened", ) @@ -129,6 +129,8 @@ func TestPresenceDetector_AttemptInterval(t *testing.T) { d = mocks.NewDetectorIface(t) pd.detector = d + // second detection, should NOT prompt user for new detection + // since within min interval interval, err = pd.DetectPresence("this is a test", 0) assert.NoError(t, err) require.Greater(t, interval, time.Duration(0), @@ -137,4 +139,14 @@ func TestPresenceDetector_AttemptInterval(t *testing.T) { // should not have been called since we are within the minDetectionAttemptInterval d.AssertNotCalled(t, "Detect", mock.Anything) + + time.Sleep(1 * time.Second) + + // third detection, should prompt user for new detection since minDetectionAttemptInterval has passed + d.On("Detect", mock.AnythingOfType("string")).Return(true, nil).Once() + interval, err = pd.DetectPresence("this is a test", 0) + assert.NoError(t, err) + require.Equal(t, time.Duration(0), interval, + "interval should be 0 since detection just happened", + ) }