Refactor EnrollOrbit/EnrollHost - #30872
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change refactors host enrollment methods and their usages to use a functional options pattern instead of positional parameters. It updates method signatures, internal logic, and all relevant call sites in the codebase and tests. No new features or control flow changes are introduced; only the enrollment API usage is modernized. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Datastore
Note over Caller,Datastore: Host enrollment using functional options
Caller->>Datastore: EnrollHost(ctx, WithEnrollHostOsqueryHostID(...), ...)
Datastore->>Datastore: Apply options to config struct
Datastore->>Datastore: Perform enrollment logic using config fields
Datastore-->>Caller: Return Host or error
sequenceDiagram
participant Caller
participant Datastore
Note over Caller,Datastore: Orbit enrollment using functional options
Caller->>Datastore: EnrollOrbit(ctx, WithEnrollOrbitHostInfo(...), ...)
Datastore->>Datastore: Apply options to config struct
Datastore->>Datastore: Perform enrollment logic using config fields
Datastore-->>Caller: Return Host or error
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #30872 +/- ##
==========================================
+ Coverage 64.17% 64.20% +0.02%
==========================================
Files 1889 1890 +1
Lines 185003 185085 +82
Branches 5361 5361
==========================================
+ Hits 118729 118825 +96
+ Misses 56935 56923 -12
+ Partials 9339 9337 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/datastore/mysql/apple_mdm_test.go (1)
4751-4755: fix the “maunual-host” typotiny spelling slip in the hostname string—may trip up future greps.
- fleet.WithEnrollOrbitHostInfo(fleet.OrbitHostInfo{HardwareSerial: manualSerial, Platform: "darwin", HardwareUUID: manualUUID, Hostname: "maunual-host"}), + fleet.WithEnrollOrbitHostInfo(fleet.OrbitHostInfo{HardwareSerial: manualSerial, Platform: "darwin", HardwareUUID: manualUUID, Hostname: "manual-host"}),server/fleet/datastore.go (1)
1049-1052: Excellent consistency with the functional options pattern!This method signature change matches the
EnrollHostrefactoring above, which provides nice consistency across the enrollment methods. The functional options pattern will make it much easier to add new enrollment parameters in the future.Consider updating the comment to reflect the new approach:
-// EnrollOrbit will enroll a new orbit instance. +// EnrollOrbit will enroll a new orbit instance using the provided functional options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
server/datastore/mysql/apple_mdm_test.go(3 hunks)server/datastore/mysql/hosts.go(2 hunks)server/datastore/mysql/hosts_test.go(19 hunks)server/datastore/mysql/labels_test.go(2 hunks)server/datastore/mysql/policies_test.go(4 hunks)server/datastore/mysql/targets_test.go(1 hunks)server/datastore/mysqlredis/hosts.go(1 hunks)server/datastore/mysqlredis/hosts_test.go(5 hunks)server/fleet/datastore.go(1 hunks)server/fleet/orbit.go(1 hunks)server/fleet/osquery.go(2 hunks)server/mock/datastore.go(1 hunks)server/mock/datastore_mock.go(2 hunks)server/service/integration_core_test.go(1 hunks)server/service/integration_mdm_test.go(1 hunks)server/service/orbit.go(1 hunks)server/service/osquery.go(1 hunks)server/test/enrollment.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
server/datastore/mysql/apple_mdm_test.go (2)
undefined
<retrieved_learning>
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
</retrieved_learning>
<retrieved_learning>
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
</retrieved_learning>
server/datastore/mysql/hosts_test.go (1)
undefined
<retrieved_learning>
Learnt from: getvictor
PR: #30589
File: ee/server/service/hostidentity/depot/depot.go:104-119
Timestamp: 2025-07-08T16:06:54.576Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the security concern where shared challenges allow certificate revocation (lines 104-119) is a known issue that will be addressed in a later feature, not an immediate concern to fix.
</retrieved_learning>
🧬 Code Graph Analysis (11)
server/datastore/mysql/targets_test.go (1)
server/fleet/osquery.go (2)
WithEnrollHostOsqueryHostID(98-102)WithEnrollHostNodeKey(119-123)
server/mock/datastore.go (1)
server/fleet/orbit.go (1)
DatastoreEnrollOrbitOption(120-120)
server/test/enrollment.go (1)
server/fleet/orbit.go (4)
WithEnrollOrbitHostInfo(130-134)OrbitHostInfo(89-109)WithEnrollOrbitNodeKey(137-141)WithEnrollOrbitTeamID(144-148)
server/datastore/mysql/policies_test.go (1)
server/fleet/osquery.go (3)
WithEnrollHostOsqueryHostID(98-102)WithEnrollHostNodeKey(119-123)WithEnrollHostTeamID(126-130)
server/service/integration_core_test.go (1)
server/fleet/orbit.go (4)
WithEnrollOrbitHostInfo(130-134)OrbitHostInfo(89-109)WithEnrollOrbitNodeKey(137-141)WithEnrollOrbitTeamID(144-148)
server/datastore/mysql/apple_mdm_test.go (1)
server/fleet/orbit.go (4)
WithEnrollOrbitMDMEnabled(123-127)WithEnrollOrbitHostInfo(130-134)OrbitHostInfo(89-109)WithEnrollOrbitNodeKey(137-141)
server/datastore/mysqlredis/hosts_test.go (2)
server/mock/datastore_mock.go (1)
EnrollHostFunc(754-754)server/fleet/osquery.go (5)
DatastoreEnrollHostOption(88-88)DatastoreEnrollHostConfig(77-85)WithEnrollHostOsqueryHostID(98-102)WithEnrollHostNodeKey(119-123)WithEnrollHostCooldown(133-137)
server/fleet/osquery.go (1)
server/fleet/app.go (1)
Duration(835-837)
server/service/orbit.go (2)
server/fleet/orbit.go (4)
WithEnrollOrbitMDMEnabled(123-127)WithEnrollOrbitHostInfo(130-134)WithEnrollOrbitNodeKey(137-141)WithEnrollOrbitTeamID(144-148)server/fleet/app.go (1)
MDM(153-229)
server/mock/datastore_mock.go (3)
server/fleet/osquery.go (1)
DatastoreEnrollHostOption(88-88)server/fleet/hosts.go (1)
Host(274-388)server/fleet/orbit.go (1)
DatastoreEnrollOrbitOption(120-120)
server/datastore/mysql/labels_test.go (1)
server/fleet/osquery.go (2)
WithEnrollHostOsqueryHostID(98-102)WithEnrollHostNodeKey(119-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (scripts, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (macos-latest)
- GitHub Check: publish
- GitHub Check: build-binaries
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript)
- GitHub Check: test-preview (ubuntu-latest)
- GitHub Check: test-packaging (ubuntu-latest, local)
🔇 Additional comments (42)
server/datastore/mysql/targets_test.go (1)
285-288: Good refactoring to functional options patternThe change from positional parameters to functional options is clean and follows the established pattern. Using
fleet.WithEnrollHostOsqueryHostID("1")andfleet.WithEnrollHostNodeKey("key1")makes the code more readable and maintainable.server/datastore/mysql/labels_test.go (2)
118-121: Clean functional options implementationThe refactoring to use
fleet.WithEnrollHostOsqueryHostID(fmt.Sprint(i))andfleet.WithEnrollHostNodeKey(fmt.Sprint(i))is well done. Usingfmt.Sprint(i)to generate unique identifiers for each host in the test loop is a good approach for creating distinct test data.
928-931: Consistent functional options usageThe change to use functional options with
fleet.WithEnrollHostOsqueryHostID("0")andfleet.WithEnrollHostNodeKey("0")is consistent with the overall refactoring pattern. The test remains focused on its purpose while benefiting from the improved API design.server/service/orbit.go (1)
168-173: Nice refactoring to functional options pattern!This change improves the code by replacing positional parameters with functional options, making it more maintainable and extensible. The implementation correctly passes all the original parameters through their respective option functions while maintaining the same functionality.
The functional options pattern is a great choice here because it:
- Makes the code more readable and self-documenting
- Allows for easier extension in the future
- Reduces the risk of parameter ordering mistakes
server/service/integration_mdm_test.go (1)
9672-9679: Nice refactoring to functional options pattern!This change makes the code much more readable and maintainable. The functional options clearly express what each parameter does - enabling MDM, providing host info, and setting the node key. This is a great improvement over positional parameters, especially for a method that could have multiple optional configurations.
The test logic remains intact while the API becomes more self-documenting. Well done!
server/datastore/mysql/policies_test.go (7)
524-528: Great refactoring work!The conversion from positional parameters to functional options makes the code more readable and maintainable. The use of
fleet.WithEnrollHostOsqueryHostID,fleet.WithEnrollHostNodeKey, andfleet.WithEnrollHostTeamIDclearly shows what each parameter represents.
530-534: Perfect consistency!Another clean conversion using the functional options pattern. The parameters are clearly defined and the team assignment is explicit.
536-540: Excellent pattern adherence!The functional options approach continues to be applied consistently across all the test cases. This makes the code much more self-documenting.
1585-1589: Looks good!The refactoring maintains the same functionality while improving code clarity. The team assignment is clearly visible through the functional option.
1660-1664: Nice work!Another clean conversion that maintains the original behavior while using the new functional options pattern.
1669-1673: Well done!The functional options pattern is consistently applied here, making the team assignment explicit and readable.
1683-1686: Perfect completion!This final conversion shows how the functional options pattern gracefully handles cases where not all parameters are needed - here we're enrolling a host without a team assignment, and the code clearly shows only the required options.
server/mock/datastore.go (1)
25-27: Perfect refactoring of the mock method signature.The updated method signature correctly matches the new functional options pattern. The mock implementation appropriately maintains its stub behavior while conforming to the refactored interface.
server/test/enrollment.go (1)
15-22: Excellent migration to functional options pattern.The enrollment call has been properly refactored to use the new functional options. The parameters are correctly mapped:
- Host information (hardware UUID and serial) through
WithEnrollOrbitHostInfo- Node key through
WithEnrollOrbitNodeKey- Team ID through
WithEnrollOrbitTeamIDThis maintains the same functionality while providing a more flexible and maintainable API.
server/service/integration_core_test.go (1)
10286-10293: The functional options pattern is implemented correctly and improves code readability.The refactoring successfully modernizes the enrollment API by replacing positional parameters with named functional options. This makes the code more maintainable and self-documenting. The usage of
WithEnrollOrbitHostInfo,WithEnrollOrbitNodeKey, andWithEnrollOrbitTeamIDfollows the established pattern from the fleet package.One small consideration: the
*h.OsqueryHostIDdereference on line 10288 assumes the pointer is not nil. In a test context this should be safe, but it's worth being aware of this assumption.server/service/osquery.go (1)
143-151: Nice refactoring to functional options!This is a great improvement to the code. The functional options pattern makes the method call much more readable and maintainable. Each parameter is clearly labeled, making it easy to understand what each value represents:
WithEnrollHostMDMEnabled- clearly shows MDM configurationWithEnrollHostOsqueryHostID- identifies the host identifierWithEnrollHostHardwareUUIDandWithEnrollHostHardwareSerial- hardware detailsWithEnrollHostNodeKey- the node key for authenticationWithEnrollHostTeamID- team assignmentWithEnrollHostCooldown- enrollment cooldown periodThis refactoring will make it much easier to add new enrollment options in the future without breaking existing code. The context parameter is correctly passed as the first argument, and all the necessary parameters are preserved from the original implementation.
server/datastore/mysql/apple_mdm_test.go (2)
4598-4602: enroll orbit call migrated correctlythe new options-based call matches the updated signature and clearly documents each flag passed in. nice work.
5863-5872: second enrollment call also looks goodoptions are passed in a clear, self-describing way. consistent with the new pattern.
server/datastore/mysqlredis/hosts.go (1)
137-138: Clean refactoring to functional options patternThis refactoring successfully updates the method signature to use variadic functional options while maintaining the exact same behavior. The underlying logic for Redis operations remains unchanged, which aligns perfectly with the PR objective of structural improvements without functional changes.
server/fleet/orbit.go (3)
111-117: Well-structured configuration structThe
DatastoreEnrollOrbitConfigstruct nicely encapsulates all the enrollment parameters that were previously passed individually. This provides a clear, organized way to handle the configuration data.
119-120: Proper functional option type definitionThe
DatastoreEnrollOrbitOptionfunction type follows Go conventions for functional options patterns perfectly.
122-148: Excellent implementation of functional optionsAll four option functions (
WithEnrollOrbitMDMEnabled,WithEnrollOrbitHostInfo,WithEnrollOrbitNodeKey, andWithEnrollOrbitTeamID) follow the standard functional options pattern correctly. Each returns a closure that modifies the appropriate field in the configuration struct. This approach provides:
- Better API flexibility for future extensions
- Clear, readable parameter setting
- Type safety for all parameters
server/fleet/datastore.go (1)
1044-1047: Great refactoring to functional options pattern!This change to use functional options is a solid improvement that will make the code more maintainable and extensible. The functional options pattern is a well-established Go idiom for handling optional parameters.
You might consider updating the comment to mention the new functional options approach:
-// EnrollHost will enroll a new host with the given identifier, setting the node key, and team. Implementations of -// this method should respect the provided host enrollment cooldown, by returning an error if the host has enrolled -// within the cooldown period. +// EnrollHost will enroll a new host using the provided functional options. Implementations of +// this method should respect the provided host enrollment cooldown, by returning an error if the host has enrolled +// within the cooldown period.server/datastore/mysql/hosts_test.go (5)
1678-1702: Great refactoring to functional options pattern!The conversion from positional parameters to functional options improves code readability and maintainability. The options like
fleet.WithEnrollHostOsqueryHostID,fleet.WithEnrollHostNodeKey,fleet.WithEnrollHostTeamID, andfleet.WithEnrollHostCooldownmake the intent of each parameter crystal clear.
1718-1742: Consistent application of functional options pattern.The refactoring is applied consistently across these test functions, maintaining the same test logic while improving API clarity.
5588-5606: Clean conversion to functional options.The enrollment calls are now more self-documenting with explicit option functions rather than positional parameters.
8370-8431: Excellent handling of both EnrollHost and EnrollOrbit conversions.The refactoring properly handles both enrollment methods, with
EnrollHostusing options likefleet.WithEnrollHostHardwareUUIDandEnrollOrbitusingfleet.WithEnrollOrbitHostInfoandfleet.WithEnrollOrbitNodeKey. The struct initialization forfleet.OrbitHostInfois well-organized.
8903-9450: Comprehensive and consistent refactoring across all enrollment scenarios.The extensive test coverage for various enrollment scenarios (hosts with/without serials, duplicate serials, mixed osquery/orbit enrollments, MDM-enabled hosts) has been successfully converted to use functional options. The test logic remains intact while the API calls are now much more readable and maintainable.
The use of structured options like
fleet.WithEnrollOrbitHostInfowith properly initializedfleet.OrbitHostInfostructs containing fields likeHardwareUUID,HardwareSerial,ComputerName, etc., makes the test cases very clear in their intent.server/datastore/mysqlredis/hosts_test.go (6)
29-38: Mock function correctly implements functional options patternThe mock
EnrollHostFuncproperly creates a config struct, applies all provided options, and uses the configured values. This implementation correctly follows the functional options pattern established in the codebase.
75-79: Consistent usage of functional options for host enrollmentThe call to
EnrollHostproperly uses the functional options pattern with appropriate option functions for osquery host ID, node key, and cooldown configuration.
84-88: Proper implementation of functional optionsThis enrollment call correctly uses the functional options pattern, maintaining consistency with the refactored API design.
98-102: Functional options applied correctlyThe host enrollment call uses the appropriate option functions and follows the established pattern consistently.
112-116: Consistent API usage maintainedThis enrollment call properly implements the functional options pattern, using the same consistent approach as other calls in the test.
139-143: Final enrollment call follows pattern correctlyThe last
EnrollHostcall in the test maintains consistency with the functional options pattern and uses the appropriate option functions.server/datastore/mysql/hosts.go (2)
2027-2037: Great refactoring to functional options pattern!The
EnrollOrbitmethod has been successfully refactored to use the functional options pattern. The implementation correctly creates a configuration struct and applies the provided options. This approach makes the API more flexible and extensible while maintaining clean code structure.The variable extraction from the config struct is well-organized and maintains the same parameter names for consistency with the previous implementation.
2192-2205: Excellent implementation of functional options pattern!The
EnrollHostmethod refactoring follows the same clean pattern asEnrollOrbit. The configuration struct approach with option application is implemented correctly, and all the original parameters are properly extracted from the config.This refactoring improves the API design by making it more extensible - new options can be added in the future without breaking existing code. The functional options pattern is a well-established Go idiom that enhances maintainability.
server/fleet/osquery.go (4)
3-6: Good addition of required importThe addition of the "time" import is necessary for the
time.Durationtype used in theCooldownfield of the new configuration struct.
76-85: Well-designed configuration structThe
DatastoreEnrollHostConfigstruct effectively encapsulates all the enrollment parameters with appropriate field types. The use of*uintforTeamIDallows for nil to represent "no team assignment," which is a good design choice.
87-88: Proper functional option type definitionThe
DatastoreEnrollHostOptiontype correctly follows the functional options pattern by accepting a pointer to the config struct.
90-137: Consistent and well-implemented option functionsAll option functions follow the same pattern and are properly implemented:
- Clear naming convention with "WithEnrollHost" prefix
- Comprehensive documentation comments
- Correct closure implementation returning the functional option type
- Each function sets the appropriate field in the configuration struct
This is a clean implementation of the functional options pattern that will improve code maintainability and readability as mentioned in the PR objectives.
server/mock/datastore_mock.go (2)
754-756: Function signatures updated correctly for functional options pattern.The function types have been properly updated to accept variadic functional options instead of explicit parameters. This change aligns well with the broader refactor to adopt the functional options pattern across the codebase.
6058-6063: Mock method implementations updated consistently.The mock implementations for both
EnrollHostandEnrollOrbitmethods have been updated to properly delegate to the new function signatures with variadic options. The tracking logic for invocations and locking behavior remain unchanged, which is exactly what we'd expect from a mock refactor.Also applies to: 6065-6070
|
|
||
| // EnrollHost enrolls the osquery agent to Fleet. | ||
| func (ds *Datastore) EnrollHost(ctx context.Context, isMDMEnabled bool, osqueryHostID, hardwareUUID, hardwareSerial, nodeKey string, teamID *uint, cooldown time.Duration) (*fleet.Host, error) { | ||
| func (ds *Datastore) EnrollHost(ctx context.Context, opts ...fleet.DatastoreEnrollHostOption) (*fleet.Host, error) { |
There was a problem hiding this comment.
Nit: Should be renamed to EnrollOsquery at some point.
2 participants
Fixes #30473
Refactore Datastore.EnrollHost and Datastore.EnrollOrbit methods to use functional options. Doing this refactor before adding new options to those methods. This should make the code more maintainable and easier to understand.
No functional changes here. Just refactoring.
Checklist for submitter
Summary by CodeRabbit
Refactor
New Features