fleet icon indicating copy to clipboard operation
fleet copied to clipboard

add headers denoting capabilities between fleet server / desktop / orbit

Open roperzh opened this issue 3 years ago • 8 comments

This adds a new mechanism to allow us to handle compatibility issues between Orbit, Fleet Server and Fleet Desktop.

The general idea is to always send a custom header of the form:

fleet-capabilities-header = "X-Fleet-Capabilities:" capabilities
capabilities              = capability * (,)
capability                = string

Both from the server to the clients (Orbit, Fleet Desktop) and vice-versa. For an example, see: https://github.com/fleetdm/fleet/commit/8c0bbdd291f54e03e19766bcdfead0fb8067f60c

Also, the following applies:

  • Backwards compat: if the header is not present, assume that orbit/fleet doesn't have the capability
  • The current capabilities endpoint will be removed

Motivation

This solution is trying to solve the following problems:

  • We have three independent processes communicating with each other (Fleet Desktop, Orbit and Fleet Server). Each process can be updated independently, and therefore we need a way for each process to know what features are supported by its peers.
  • We originally implemented a dedicated API endpoint in the server that returned a list of the capabilities (or "features") enabled, we found this, and any other server-only solution (like API versioning) to be insufficient because:
    • There are cases in which the server also needs to know which features are supported by its clients
    • Clients needed to poll for changes to detect if the capabilities supported by the server change, by sending the capabilities on each request we have a much cleaner way to handling different responses.
  • We are also introducing an unauthenticated endpoint to get the server features, this gives us flexibility if we need to implement different authentication mechanisms, and was one of the pitfalls of the first implementation.

Related to https://github.com/fleetdm/fleet/issues/7929

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • [x] Added/updated tests
  • [x] Manual QA for all new/changed functionality

roperzh avatar Sep 19 '22 16:09 roperzh

Codecov Report

Base: 60.72% // Head: 60.78% // Increases project coverage by +0.05% :tada:

Coverage data is based on head (2d96930) compared to base (ecaff07). Patch coverage: 83.96% of modified lines in pull request are covered.

Additional details and impacted files
@@                  Coverage Diff                   @@
##           orbit-fleet-enroll    #7833      +/-   ##
======================================================
+ Coverage               60.72%   60.78%   +0.05%     
======================================================
  Files                     434      436       +2     
  Lines                   41629    41719      +90     
======================================================
+ Hits                    25281    25359      +78     
- Misses                  13949    13961      +12     
  Partials                 2399     2399              
Impacted Files Coverage Δ
orbit/cmd/orbit/orbit.go 1.37% <0.00%> (-0.01%) :arrow_down:
server/fleet/app.go 0.00% <ø> (ø)
server/service/client.go 14.90% <0.00%> (ø)
server/service/orbit_client.go 0.00% <0.00%> (ø)
server/service/orbit.go 5.43% <38.46%> (+5.43%) :arrow_up:
server/service/base_client.go 71.42% <94.73%> (+6.56%) :arrow_up:
server/service/endpoint_utils.go 81.26% <95.45%> (+0.96%) :arrow_up:
server/contexts/capabilities/capabilities.go 100.00% <100.00%> (ø)
server/fleet/capabilities.go 100.00% <100.00%> (ø)
server/service/device_client.go 75.00% <100.00%> (+0.71%) :arrow_up:
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Sep 19 '22 22:09 codecov-commenter

@roperzh

So if I got it right, for https://github.com/fleetdm/fleet/pull/7536 the workflow will look something like:

  1. Issue request to the /desktop end-point.
  2. If fleet server is running an old version, I'll get an 404 or something similar.
  3. Check the capabilities headers to make sure the /desktop capability is not supported (will need to add this).
  4. If not supported, fall back to /policies end-point, otherwise throw up.

juan-fdz-hawa avatar Sep 20 '22 13:09 juan-fdz-hawa

So if I got it right, for #7536 the workflow will look something like:

1. Issue request to the `/desktop` end-point.

2. If fleet server is running an old version, I'll get an 404 or something similar.

3. Check the capabilities headers to make sure the `/desktop` capability is not supported (will need to add this).

4. If not supported, fall back to `/policies` end-point, otherwise throw up.

@juan-fdz-hawa I think that's right, the request doesn't necessarily have to be to the /desktop endpoint (all responses for device endpoints will include the header), but yeah 👍

do you think this approach makes sense?

roperzh avatar Sep 20 '22 15:09 roperzh

Regarding https://github.com/fleetdm/fleet/pull/7833#issuecomment-1252525023

Am guessing Juan does not need the capabilities feature? If the endpoint exists (doesn't return 404) then use it. If it returns 404 use the old one.

lucasmrod avatar Sep 20 '22 15:09 lucasmrod

Regarding #7833 (comment)

Am guessing Juan does not need the capabilities feature? If the endpoint exists (doesn't return 404) then use it. If it returns 404 use the old one.

I think we do if we want to play it safe, if we got 404 and not supported then we know the server is running an old version. If we get an 404 and is supported then we know that there's a bug somewhere (maybe a typo in the URL)

juan-fdz-hawa avatar Sep 20 '22 15:09 juan-fdz-hawa

@juan-fdz-hawa @lucasmrod @chiiph folks, based on the feedback here and a chat with Lucas, I have updated the PR with:

  • A map of server features for Orbit, and a different, separate map for Fleet Desktop
  • Two dedicated "ping" endpoints, one for Orbit and one for Fleet these:
    • Are publicly accessible (not authenticated)
    • Contain the capabilities header
  • Logic to populate the server context with the capabilities sent by the clients

Would you be so kind to take another look? Thanks!

roperzh avatar Sep 20 '22 19:09 roperzh

this is how I'm using this functionality in the token rotation branch: https://github.com/fleetdm/fleet/pull/7779/commits/8c0bbdd291f54e03e19766bcdfead0fb8067f60c cc: @juan-fdz-hawa

roperzh avatar Sep 20 '22 20:09 roperzh



Test summary

208 0 0 0Flakiness 1


Run details

Project Fleet
Status Passed
Commit ceb0a552a4 ℹ️
Started Sep 23, 2022 9:10 PM
Ended Sep 23, 2022 9:26 PM
Duration 15:55 💡
OS Linux Ubuntu - 20.04
Browser Electron 94

View run in Cypress Dashboard ➡️


Flakiness

cypress/integration/premium/team_admin.spec.ts Flakiness
1 Premium tier - Team Admin user > Manage schedules page > edit a team's scheduled query successfully

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cypress[bot] avatar Sep 23 '22 19:09 cypress[bot]