Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

include arch and os in local server headers #1877

Conversation

James-Pickett
Copy link
Contributor

@James-Pickett James-Pickett commented Oct 2, 2024

In the event that presence detection is requested for a non-supported OS, return default failure duration -1s. Always include OS and Arch in headers.

@James-Pickett James-Pickett changed the title include arch and os in headers include arch and os in local server headers Oct 2, 2024
@James-Pickett James-Pickett marked this pull request as ready for review October 2, 2024 22:38
directionless
directionless previously approved these changes Oct 2, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like forward progress

@@ -415,13 +416,14 @@ func (ls *localServer) presenceDetectionHandler(next http.Handler) http.Handler

// presence detection is only supported on macos currently
if runtime.GOOS != "darwin" {
w.Header().Add(kolideDurationSinceLastPresenceDetectionHeaderKey, presencedetection.DetectionFailedDurationValue.String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this, we'll need to figure out how to differentiate between "not supported" and "timeout" or "error"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted it so we only return the presence response header when presence detection is actually requested.

That might give us enough data for some logic like

if failure && os == "linux"
  // we no it's not supported

but maybe we need something more concrete like a failure_reason header

directionless
directionless previously approved these changes Oct 3, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still pretty sure we're going to need to iterate. But I think it's okay to do it post merge.

@directionless
Copy link
Contributor

Let's see, I think we have the following conditions:

  • presence detection supported, success
  • presence detection supported, failure / timeout (Are there failures other than timeout? Maybe someone can click no)
  • presence detection not supported
  • old launcher so it's just 🤷 (eg: no headers at all)

It might additionally be interesting to track the reasons it failed. At least, if there are reasons we can track.

So we probably do need some kind of header for why it failed. And one reason can be unsupported (Or, if we don't like enums, we can have a boolean unsupported header and a string error header. But I think either is probably fine)

@James-Pickett James-Pickett added this pull request to the merge queue Oct 3, 2024
Merged via the queue into kolide:main with commit 8726e75 Oct 3, 2024
31 checks passed
@James-Pickett James-Pickett deleted the james/include-os-arch-headers-presence-detection branch October 3, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants