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

Generate the enrollment details on launcher startup #2045

Closed
wants to merge 12 commits into from

Conversation

cesarfda
Copy link
Contributor

@cesarfda cesarfda commented Jan 14, 2025

This pull request introduces new functionality for managing enrollment details and updates several mocks in the knapsack package. The key changes include adding methods for setting and getting enrollment details, moving EnrollmentDetails to a new location, and updating the mocks accordingly.

Open Questions:

  • For autoupdate tests, I changed the tests to use a real osquery binary to validate the GetOsqEnrollDetails. I believe I could achieve similar coverage using Mock.Querier in the tests so we don't make the tests flaky by introducing a dependency.
  • In autoupdate I chose to get the enrollment details right before the restart. There is also the option of moving that into other steps of the Autoupdater assuming the enrollment details don't change between steps.
  • Also, with the current setup running before the restart the timeout used for the GetEnrollmentDetails backoff might add additional time to the restart, I set the default to 30 seconds because it was what was used when the code lived in the osquery extension but I could make it shorter so we don't hold the restart for too long.

New Functionality for Enrollment Details:

  • cmd/launcher/launcher.go: Added logic to retrieve and set runtime enrollment details, including a retry mechanism with backoff for fetching osquery enrollment details.
  • ee/agent/knapsack/knapsack.go: Introduced SetEnrollmentDetails and GetEnrollmentDetails methods to manage enrollment details.
  • ee/agent/types/enrollment.go: Moved EnrollmentDetails struct from the service package to the types package, and defined EnrollmentStatus constants.
  • ee/agent/types/knapsack.go: Updated the Knapsack interface to include methods for managing enrollment details.

Updates to Mocks:

These changes enhance the functionality and maintainability of the knapsack package by providing a standardized way to handle enrollment details and updating the mocks to reflect the new methods.

Updates to Autoupdate:

New Functionality

  • Added collectAndSetEnrollmentDetails method to preserve system state during updates
  • Integrated configurable backoff mechanism for enrollment collection
  • Enhanced restart process with enrollment data preservation

Code Structure

  • ee/tuf/autoupdate.go:
    • Added enrollment collection before launcher restarts
    • Implemented configurable backoff timing
    • Enhanced error handling and logging

Test Improvements

  • ee/tuf/autoupdate_test.go:
    • Added real osqueryd binary integration
    • Implemented configurable test timeouts
    • Enhanced test coverage for enrollment collection
    • Added table-driven tests for various scenarios

Configuration Options

  • Added WithOsquerierBackoff for flexible timing control
  • Implemented production-ready default values
  • Added test-specific timing configurations

@cesarfda cesarfda linked an issue Jan 14, 2025 that may be closed by this pull request
4 tasks
@cesarfda cesarfda added the features-improvements Features and Improvements label Jan 14, 2025
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.

nice

cmd/launcher/launcher.go Outdated Show resolved Hide resolved
ee/agent/knapsack/knapsack.go Outdated Show resolved Hide resolved
ee/tuf/autoupdate.go Outdated Show resolved Hide resolved
@@ -528,6 +541,15 @@ func (ta *TufAutoupdater) checkForUpdate(binariesToCheck []autoupdatableBinary)
if updatedVersion, ok := updatesDownloaded[binaryLauncher]; ok {
// Only reload if we're not using a localdev path
if ta.knapsack.LocalDevelopmentPath() == "" {
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'll defer to becca, but I'm not sure this is the right place

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking...we have both LauncherVersion and OsqueryVersion inside the enrollment details, so that's why I'd wanted it updated after autoupdate. If we want to keep it here for that reason, then it probably makes more sense to only update EnrollmentDetails.LauncherVersion and EnrollmentDetails.OsqueryVersion -- we don't need to regenerate the rest of the enrollment details at this time. What do you think?

Copy link
Contributor

@RebeccaMahany RebeccaMahany left a comment

Choose a reason for hiding this comment

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

Nice work! Left a couple comments, will continue to keep an eye on the discussion about where we want to update launcher/osquery version 🙂

cmd/launcher/launcher.go Outdated Show resolved Hide resolved
)
}
return err
}, 30*time.Second, 5*time.Second); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can delay launcher startup for up to 30 seconds -- I think we probably don't want to do that. Maybe we should call getEnrollmentDetails in a goroutine so that it doesn't halt startup? (We would probably want to make knapsack.enrollmentDetails nullable -- var enrollmentDetails *types.EnrollmentDetails -- and then have extension.Enroll wait for those details to be available.)

cmd/launcher/launcher.go Outdated Show resolved Hide resolved
ee/agent/types/enrollment.go Outdated Show resolved Hide resolved
ee/agent/knapsack/knapsack.go Outdated Show resolved Hide resolved
ee/tuf/autoupdate.go Outdated Show resolved Hide resolved
@@ -528,6 +541,15 @@ func (ta *TufAutoupdater) checkForUpdate(binariesToCheck []autoupdatableBinary)
if updatedVersion, ok := updatesDownloaded[binaryLauncher]; ok {
// Only reload if we're not using a localdev path
if ta.knapsack.LocalDevelopmentPath() == "" {
ctx := context.Background()
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking...we have both LauncherVersion and OsqueryVersion inside the enrollment details, so that's why I'd wanted it updated after autoupdate. If we want to keep it here for that reason, then it probably makes more sense to only update EnrollmentDetails.LauncherVersion and EnrollmentDetails.OsqueryVersion -- we don't need to regenerate the rest of the enrollment details at this time. What do you think?

@cesarfda
Copy link
Contributor Author

I'm starting over to make the changes simpler. I'll also add a requirement of not caching the quick look up part of the call and focus the caching only on the osquery details

@cesarfda cesarfda closed this Jan 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
features-improvements Features and Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include enrollment details in /id response
3 participants