Skip to content

Commit

Permalink
move min attempt interval into user space struct
Browse files Browse the repository at this point in the history
  • Loading branch information
James-Pickett committed Dec 17, 2024
1 parent 81b1489 commit a43d66b
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 37 deletions.
8 changes: 0 additions & 8 deletions ee/localserver/presence-detection-middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
2 changes: 1 addition & 1 deletion ee/localserver/request-id.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 5 additions & 22 deletions ee/localserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
28 changes: 22 additions & 6 deletions ee/presencedetection/presencedetection.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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{}
Expand All @@ -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)
Expand Down
41 changes: 41 additions & 0 deletions ee/presencedetection/presencedetection_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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",
)
}

0 comments on commit a43d66b

Please sign in to comment.