Generate the enrollment details on launcher startup #2045

cesarfda · 2025-01-14T16:47:47Z

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:

ee/agent/types/mocks/knapsack.go: Added mock implementations for the new GetEnrollmentDetails and SetEnrollmentDetails methods. [1] [2]
ee/agent/types/mocks/flags.go: Removed several unused mock methods related to osquery endpoints.

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

… and update related methods

…lmentDetails

…rollmentDetails struct to types

…entation

…ith real osquery

…tails

directionless

nice

cmd/launcher/launcher.go

ee/agent/knapsack/knapsack.go

directionless · 2025-01-15T02:38:57Z

ee/tuf/autoupdate.go

@@ -749,3 +771,40 @@ func (ta *TufAutoupdater) cleanUpOldErrors() {
 		)
 	}
 }
+
+// collectAndSetEnrollmentDetails collects the runtime enrollment details for the


Is this duplicated with the one in run?

I think it'd be reasonable to add a function to pkg/osquery/enrollment_details.go that looks like CollectAndSetEnrollmentDetails(ctx context.Context, k types.Knapsack) so we don't have to duplicate it here + below runLauncher 🙂 . (Or maybe CollectAndSetEnrollmentDetails(ctx context.Context, k types.Knapsack, collectTimeout time.Duration, collectRetryInterval time.Duration)?)

directionless · 2025-01-15T02:39:16Z

ee/tuf/autoupdate.go

@@ -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()


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

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?

RebeccaMahany

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

RebeccaMahany · 2025-01-15T16:53:04Z

cmd/launcher/launcher.go

@@ -638,3 +648,46 @@ func runOsqueryVersionCheckAndAddToKnapsack(ctx context.Context, slogger *slog.L
 		"osqueryd_path", osquerydPath,
 	)
 }
+
+func getEnrollmentDetails(ctx context.Context, k types.Knapsack, startupSpan trace.Span, slogger *slog.Logger) service.EnrollmentDetails {


It is better to create a new child span here rather than passing in the startup span -- it will give us better and more specific timing details, and is more consistent with what we do elsewhere. The ctx that you're passing in here already has information about startupSpan, so you can use it to automatically create a child span that hangs off startupSpan:

ctx, span := traces.StartSpan(ctx) defer span.End()

RebeccaMahany · 2025-01-15T16:56:07Z

cmd/launcher/launcher.go

+			)
+		}
+		return err
+	}, 30*time.Second, 5*time.Second); err != nil {


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.)

RebeccaMahany · 2025-01-15T16:57:14Z

cmd/launcher/launcher.go

+		if os.Getenv("LAUNCHER_DEBUG_ENROLL_DETAILS_REQUIRED") == "true" {
+			slogger.Log(ctx, slog.LevelError,
+				"enrollment details required but failed to get them",
+				"err", err,
+			)
+		}


@directionless are we able to rip this out? I don't see it in use anywhere in launcher.

I don't remember what I was debugging with this. Feel free to remove

RebeccaMahany · 2025-01-15T16:58:52Z

ee/agent/types/enrollment.go

+	Unknown         EnrollmentStatus = "unknown"
+)
+
+// Move EnrollmentDetails from service package to types


I think this comment should be updated to describe the struct

RebeccaMahany · 2025-01-15T17:03:51Z

ee/agent/knapsack/knapsack.go

+func (k *knapsack) SetEnrollmentDetails(details types.EnrollmentDetails) error {
+	// Only update if there are actual changes
+	if details != enrollmentDetails {
+		k.slogger.Logger.DebugContext(context.Background(), "updating enrollment details")


We prefer to use the Log method with the corresponding level for consistency. Probably useful to also log the old/new details to see what changed?

k.Slogger().Log(context.Background(), slog.LevelDebug, "updating enrollment details", "old_details", enrollmentDetails, "new_details", details, )

(May need to do fmt.Sprintf("%+v", enrollmentDetails), I forget how well these get stringified inside the logs.)

RebeccaMahany · 2025-01-15T17:07:00Z

ee/tuf/autoupdate.go

@@ -749,3 +771,40 @@ func (ta *TufAutoupdater) cleanUpOldErrors() {
 		)
 	}
 }
+
+// collectAndSetEnrollmentDetails collects the runtime enrollment details for the


I think it'd be reasonable to add a function to pkg/osquery/enrollment_details.go that looks like CollectAndSetEnrollmentDetails(ctx context.Context, k types.Knapsack) so we don't have to duplicate it here + below runLauncher 🙂 . (Or maybe CollectAndSetEnrollmentDetails(ctx context.Context, k types.Knapsack, collectTimeout time.Duration, collectRetryInterval time.Duration)?)

RebeccaMahany · 2025-01-15T17:10:39Z

ee/tuf/autoupdate.go

@@ -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()


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 added 7 commits January 6, 2025 16:46

Refactor enrollment details handling: move EnrollmentDetails to types…

5cb998c

… and update related methods

Update GetEnrollmentDetails to return error and handle it in SetEnrol…

1b258aa

…lmentDetails

Enhance enrollment details retrieval with retry mechanism and move En…

9ac8a8b

…rollmentDetails struct to types

Add SetEnrollmentDetails method to Knapsack interface and mock implem…

ee1a0d8

…entation

Collect and set enrollment details before launcher restart, testing w…

f0a3c8a

…ith real osquery

added edge case coverage and sped up test

25e9a93

Merge remote-tracking branch 'upstream/main' into cesar/enrollment_de…

a9b91ab

…tails

cesarfda linked an issue Jan 14, 2025 that may be closed by this pull request

Include enrollment details in /id response #1986

Open

4 tasks

cesarfda added the features-improvements Features and Improvements label Jan 14, 2025

use context for logging enrollment details update

828f91e

directionless reviewed Jan 15, 2025

View reviewed changes

refactor: extract enrollment details retrieval into a separate function

872e0bf

RebeccaMahany reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate the enrollment details on launcher startup #2045

Generate the enrollment details on launcher startup #2045

cesarfda commented Jan 14, 2025 •

edited

Loading

directionless left a comment

directionless Jan 15, 2025

RebeccaMahany Jan 15, 2025

directionless Jan 15, 2025

RebeccaMahany Jan 15, 2025

RebeccaMahany left a comment

RebeccaMahany Jan 15, 2025

RebeccaMahany Jan 15, 2025

RebeccaMahany Jan 15, 2025

directionless Jan 15, 2025

RebeccaMahany Jan 15, 2025

RebeccaMahany Jan 15, 2025

RebeccaMahany Jan 15, 2025

RebeccaMahany Jan 15, 2025

Generate the enrollment details on launcher startup #2045

Are you sure you want to change the base?

Generate the enrollment details on launcher startup #2045

Conversation

cesarfda commented Jan 14, 2025 • edited Loading

Open Questions:

New Functionality for Enrollment Details:

Updates to Mocks:

Updates to Autoupdate:

New Functionality

Code Structure

Test Improvements

Configuration Options

directionless left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RebeccaMahany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarfda commented Jan 14, 2025 •

edited

Loading