feat: fleet secrets manager #263
Conversation
|
Go test coverage
Total coverage: 58.7% |
jajeffries
force-pushed
the
feat/fleet-secretsmgr
branch
from
f7a6e1e to
08f7806
Compare
There was a problem hiding this comment.
Pull request overview
This pull request introduces Fleet-based secrets management integration, enabling the agent to securely retrieve and manage secrets from a Fleet control plane over MQTT. The implementation adds message schemas for secret operations, implements the FleetSecretsManager with caching and automatic secret updates, and integrates it with the existing Fleet configuration manager. Test improvements include dynamic port allocation for more robust testing.
Changes:
- Added Fleet secrets manager implementation with MQTT-based secret retrieval, caching, and automatic update notifications
- Introduced secrets message schema defining request/response patterns and error codes for Fleet secrets operations
- Integrated Fleet secrets manager with Fleet configuration manager through MQTT topic handlers and OnReadyHooks
- Enhanced MQTT connection with topic-specific message handlers to support secrets-related topics
- Improved test reliability by using dynamic port allocation instead of hardcoded ports
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| agent/secretsmgr/fleet.go | New FleetSecretsManager implementation with MQTT integration, secret caching, and update handling |
| agent/secretsmgr/fleet_test.go | Comprehensive test suite for FleetSecretsManager including processString, handleResponse, and handleUpdateNotification |
| agent/secretsmgr/manager.go | Added "fleet" case to secrets manager factory |
| agent/configmgr/fleet/messages/secrets_messages.go | Message schema definitions for secret requests, responses, and update notifications |
| agent/configmgr/fleet.go | Added BindSecretsManager method and exported FleetConfigManager type for integration |
| agent/configmgr/fleet/connection.go | Added topic handler registration and dispatching for custom MQTT message handlers |
| agent/configmgr/fleet/topics.go | Added secrets request, response, and updated topic templates |
| agent/configmgr/fleet/mocks.go | Added RegisterTopicHandler mock implementation |
| agent/config/types.go | Added FleetSecretsManager configuration type |
| agent/agent.go | Integrated Fleet secrets manager binding before config secret resolution |
| agent/configmgr/fleet_test.go | Improved test with dynamic port allocation |
| agent/otlpbridge/server_test.go | Improved test with dynamic port allocation |
| agent/docker/default_config.yaml | Added Fleet secrets manager configuration with 30s timeout |
| agent/secretsmgr/vault_auth_test.go | Added clarifying comment about Docker port binding test failures |
Comments suppressed due to low confidence (1)
agent/agent.go:210
- The binding of FleetSecretsManager happens before SolveConfigSecrets is called, but the actual MQTT binding happens asynchronously via an OnReadyHook that will only be triggered when the MQTT connection is established (which happens later in configManager.Start at line 252). If SolveConfigSecrets at line 206-208 tries to resolve fleet secrets before the MQTT connection is ready, it will fail because publisher/subscriber won't be bound yet. Consider either: 1) Moving SolveConfigSecrets to after configManager.Start, 2) Adding a mechanism to wait for MQTT readiness before processing secrets, or 3) Document that config/backend secrets cannot use fleet references and only policy secrets can.
// Bind fleet secrets manager to fleet config manager if both are fleet-based
// This needs to happen before SolveConfigSecrets so secrets can be resolved
if a.config.OrbAgent.ConfigManager.Active == "fleet" && a.config.OrbAgent.SecretsManager.Active == "fleet" {
if fleetCM, ok := a.configManager.(*configmgr.FleetConfigManager); ok {
if err := fleetCM.BindSecretsManager(a.secretsManager); err != nil {
a.logger.Error("error binding fleet secrets manager", "error", err)
return err
}
}
}
var err error
if a.config.OrbAgent.Backends,
a.config.OrbAgent.ConfigManager,
err = a.secretsManager.SolveConfigSecrets(a.config.OrbAgent.Backends, a.config.OrbAgent.ConfigManager); err != nil {
return err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
agent/agent.go:210
- There's a timing/ordering issue here. The
SolveConfigSecretsmethod is called beforeconfigManager.Start(), but the Fleet secrets manager requires an MQTT connection to resolve secrets. The MQTT connection is only established whenconfigManager.Start()is called, and the binding of the secrets manager to MQTT happens via anOnReadyHookthat executes after MQTT connects. This means any fleet secret references in the configuration will fail to resolve with "MQTT publisher not bound" error. The binding and secrets resolution need to happen after the MQTT connection is established, not before.
// Bind fleet secrets manager to fleet config manager if both are fleet-based
// This needs to happen before SolveConfigSecrets so secrets can be resolved
if a.config.OrbAgent.ConfigManager.Active == "fleet" && a.config.OrbAgent.SecretsManager.Active == "fleet" {
if fleetCM, ok := a.configManager.(*configmgr.FleetConfigManager); ok {
if err := fleetCM.BindSecretsManager(a.secretsManager); err != nil {
a.logger.Error("error binding fleet secrets manager", "error", err)
return err
}
}
}
var err error
if a.config.OrbAgent.Backends,
a.config.OrbAgent.ConfigManager,
err = a.secretsManager.SolveConfigSecrets(a.config.OrbAgent.Backends, a.config.OrbAgent.ConfigManager); err != nil {
return err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e066f5e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…es without delaying ack
jajeffries
requested review from
grant-nbl,
manrodrigues and
paulstuart
as code owners
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38d6691e0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 785e8426a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This pull request introduces support for integrating the Fleet-based secrets manager with the Fleet configuration manager, enabling secure secrets distribution over MQTT. It adds new types and message schemas for secrets management, updates the Fleet configuration manager to bind and handle secrets, and refactors several types and methods for consistency and extensibility. The changes also include improvements to topic handling in the MQTT connection and add necessary configuration fields for secrets management.
Fleet secrets manager integration
agent/agent.goto bind the Fleet secrets manager to the Fleet configuration manager when both are active, ensuring secrets can be resolved during configuration.BindSecretsManagerinFleetConfigManagerto register MQTT topic handlers and bind the secrets manager for secure communication.Secrets message schema and configuration
agent/configmgr/fleet/messages/secrets_messages.go, defining request, response, and update notification structures.FleetSecretsManagertype and related configuration fields to support Fleet secrets manager options inagent/config/types.go.MQTT connection enhancements
MQTTConnection, allowing dynamic handling of secrets-related MQTT topics. [1] [2] [3] [4] [5]TokenResponseTopicsto include secrets request, response, and update topics for proper routing. [1] [2] [3]Refactoring and consistency
fleetConfigManagertoFleetConfigManagerand updated related constructors and methods for clarity and adherence to Go naming conventions. [1] [2] [3] [4] [5] [6] [7]agent/configmgr/fleet_test.go. [1] [2]