feat: adds agent config command that returns the agent configuration
#4671
Conversation
There was a problem hiding this comment.
Nice. I believe this command will be very useful.
I have few remarks/concerns:
- It looks like this PR has broken the
/api/v1/agent/configendpoint. The response is now the binary encoding of the config{"Config":"QVBJOgogICAgSG9zdDogMC4wLjAuMAogICAgUG9yd...... I know about that since some of the TestContainers tests are failing - Are there any potential sensitive data that we might need to mask from that config? Maybe any connection strings with passwords in them, or similar items ?
- Should we add few tests to protect this feature?
Yes, this is to be expected given the body of the request is now
Yes, several fields of the config may contain tokens, for example the Compute config here. My personal opinion is that config files shouldn't contain sensitive data - it should live somewhere else. That point aside, I am not sure excluding them from the (returned) config is the right choice, perhaps it may be simpler to require auth to query this endpoint? Again, open to alternatives.
100%. Can you point me to the appropriate package to writing such tests? |
|
The API should just return json representation of the config exactly the same how it works today. No binary and no need for nesting inside a I agree with Jamil about having to mask sensitive data. The only fields we have today are |
I think it better to return the json representation of the YAML config, since it is much more standard API behavior. Although Json is a subset of YAML, and not the other way around, I believe the current structure of the config should convert to JSON with now issues.
I think requiring authentication is for sure needed for this endpoint, regardless if it contains sensitive data (exposing configuration publicly is a big security attack vector.)
I was thinking of a basic unit test that lives beside the |
Sounds good, will make the change.
Also sounds good, will change.
Sounds good to me. |
I've noticed that some commands already have this flag: Maybe in this situation |
|
@jamlo in fe291ae I have (attempted to) fix the integration tests. I've been unable to verify this locally as the (new) integration tests fail on my Ubuntu host with the following error message: I suspect I either need to downgrade my client version or upgrade the version required by the testing framework. wdyt? |
|
@frrist For the docker API version, you can configure that in the Tests when you run it locally here . I've set the APi version since Github Actions does not have the latest version, so I pin it. Btw, I checked the TestContainers tests, and they are running fine on CI after you did the changes, except for one Test that is failing due to how I am calling curl (I should call it to only output the response body). Since these tests are not a blocker to merge yet, I will fix it when you merge the PR. |
WalkthroughA new command for retrieving the agent's configuration has been added to the Bacalhau project. This includes the implementation of a Cobra command in Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
pkg/publicapi/client/v2/api_agent.go (1)
33-38: Consider adding support for sensitive data redaction.While the implementation follows the established pattern in the codebase, there's a need to handle sensitive data in the configuration response. Consider:
- Adding options to the request to control sensitive data visibility
- Implementing server-side redaction of sensitive fields
Here's a suggested approach:
-func (c *Agent) Config(ctx context.Context) (*apimodels.GetAgentConfigResponse, error) { +func (c *Agent) Config(ctx context.Context, req *apimodels.GetAgentConfigRequest) (*apimodels.GetAgentConfigResponse, error) { var res apimodels.GetAgentConfigResponse - err := c.client.Get(ctx, "/api/v1/agent/config", &apimodels.BaseGetRequest{}, &res) + err := c.client.Get(ctx, "/api/v1/agent/config", req, &res) return &res, err }Where
GetAgentConfigRequestcould include:type GetAgentConfigRequest struct { apimodels.BaseGetRequest IncludeSensitive bool `json:"include_sensitive"` }cmd/cli/agent/config_test.go (2)
5-15: Consider grouping imports for better readability.While all necessary imports are present, consider grouping them with blank lines between standard library, third-party, and internal packages.
import ( "encoding/json" "testing" + "github.com/stretchr/testify/suite" "gopkg.in/yaml.v3" + cmdtesting "github.com/bacalhau-project/bacalhau/cmd/testing" "github.com/bacalhau-project/bacalhau/cmd/util/output" "github.com/bacalhau-project/bacalhau/pkg/config/types" )
1-46: Consider restructuring tests for better maintainability.The test suite could benefit from the following architectural improvements:
- Use table-driven tests to cover multiple config scenarios
- Create test helpers for common assertions to reduce code duplication
- Add a separate test file for integration tests that verify the actual API interaction
- Consider using a mock config provider for more controlled testing
Example structure:
type configTestCase struct { name string args []string expectedFields map[string]interface{} expectError bool } func (s *ConfigSuite) TestConfigWithCases() { cases := []configTestCase{ { name: "default yaml output", args: []string{"agent", "config"}, expectedFields: map[string]interface{}{ "Orchestrator.Enabled": true, "APIServer.Host": "localhost", }, }, // Add more test cases } for _, tc := range cases { s.Run(tc.name, func() { // Test implementation }) } }cmd/cli/agent/config.go (1)
15-38: Add documentation about output format options.While the command implementation is solid, it would be helpful to document the available output formats in the command's description.
Apply this change:
configCmd := &cobra.Command{ Use: "config", - Short: "Get the agent's configuration.", + Short: "Get the agent's configuration in YAML or JSON format.", + Long: `Get the agent's configuration. + +The output format can be controlled using the --format flag: + - yaml: Human-readable YAML format (default) + - json: JSON format + +Example: + bacalhau agent config + bacalhau agent config --format json`, Args: cobra.NoArgs,test_integration/5_orchestrator_no_config_suite_test.go (1)
Line range hint
60-77: Consider adding test cases for sensitive data handling.The test validates basic configuration fields but doesn't verify the handling of sensitive data, which was a concern raised in the PR comments.
Would you like me to help generate additional test cases for:
- Verifying that sensitive fields are properly masked
- Testing authentication requirements for the config endpoint
pkg/publicapi/endpoint/agent/endpoint.go (2)
Line range hint
117-126: Update API documentation to match implementation.The godoc needs the following improvements:
- The @success response type should be
apimodels.GetAgentConfigResponseinstead oftypes.Bacalhau.- Add a note about sensitive data redaction in the @description.
Update the godoc block as follows:
// config godoc // // @ID agent/config // @Summary Returns the current configuration of the node. +// @Description Returns the node configuration with sensitive information (auth tokens) redacted. // @Tags Ops // @Produce json -// @Success 200 {object} types.Bacalhau +// @Success 200 {object} apimodels.GetAgentConfigResponse // @Failure 500 {object} string // @Router /api/v1/agent/config [get]
127-130: Add validation after config copy.Consider validating the copied configuration before proceeding with redaction to ensure data integrity.
cfg, err := e.bacalhauConfig.Copy() if err != nil { return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("could not copy bacalhau config: %s", err)) } +if cfg == nil { + return echo.NewHTTPError(http.StatusInternalServerError, "copied config is nil") +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
cmd/cli/agent/config.go(1 hunks)cmd/cli/agent/config_test.go(1 hunks)cmd/cli/agent/root.go(1 hunks)pkg/config/types/bacalhau.go(1 hunks)pkg/publicapi/apimodels/agent.go(2 hunks)pkg/publicapi/client/v2/api_agent.go(1 hunks)pkg/publicapi/endpoint/agent/endpoint.go(3 hunks)pkg/publicapi/endpoint/agent/endpoint_test.go(1 hunks)test_integration/1_orchestrator_basic_config_suite_test.go(2 hunks)test_integration/2_orchestrator_config_override_suite_test.go(1 hunks)test_integration/3_orchestrator_config_override_and_flag_suite_test.go(1 hunks)test_integration/4_orchestrator_config_override_and_flag_and_config_flag_suite_test.go(2 hunks)test_integration/5_orchestrator_no_config_suite_test.go(2 hunks)test_integration/6_jobs_basic_runs_scenarios_suite_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- pkg/config/types/bacalhau.go
- test_integration/2_orchestrator_config_override_suite_test.go
- test_integration/3_orchestrator_config_override_and_flag_suite_test.go
- test_integration/6_jobs_basic_runs_scenarios_suite_test.go
🔇 Additional comments (18)
cmd/cli/agent/root.go (1)
19-19: LGTM! Command registration follows established patterns.
The addition of NewConfigCmd() is clean and consistent with the existing command structure.
Let's verify the command implementation:
✅ Verification successful
Let me gather more information about the config command implementation and its test coverage.
Implementation verified and properly tested
The command implementation is well-structured and meets the requirements:
- Properly handles both JSON and YAML output formats
- Uses the API client to fetch configuration securely
- Has comprehensive test coverage for both output formats
- Follows the project's patterns for command implementation
- Properly handles errors and provides clear error messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the config command implementation and its test coverage
# Test 1: Check if NewConfigCmd is properly defined
ast-grep --pattern 'func NewConfigCmd() *cobra.Command'
# Test 2: Verify test coverage
rg -l "TestConfig.*" --type go
Length of output: 2199
Script:
#!/bin/bash
# Check the implementation of the run function and test coverage
# Get the run function implementation
ast-grep --pattern 'func run(cmd *cobra.Command, api $_,$_) error {
$$$
}'
# Get the test implementation
cat cmd/cli/agent/config_test.go
Length of output: 1990
pkg/publicapi/apimodels/agent.go (2)
3-6: LGTM: Import changes are appropriate
The added import for types package is necessary for the new GetAgentConfigResponse struct.
37-40: Add documentation and consider sensitive data handling
The struct definition looks good, but there are a few considerations:
- Add documentation comment for the struct explaining its purpose and usage
- Consider adding field tags or implementing custom marshaling to handle sensitive data redaction, as discussed in the PR comments
Let's verify if the Bacalhau type contains sensitive fields:
Suggested documentation and potential sensitive data handling:
+// GetAgentConfigResponse represents the response for agent configuration retrieval.
+// Note: This response may contain sensitive information and should be handled accordingly.
type GetAgentConfigResponse struct {
BaseGetResponse
- Config types.Bacalhau `json:"config"`
+ Config types.Bacalhau `json:"config" sensitive:"true"`
}pkg/publicapi/client/v2/api_agent.go (1)
33-38: Verify error handling consistency with other endpoints.
Let's ensure the error handling is consistent with other endpoints and the configuration endpoint is properly implemented.
✅ Verification successful
Let me gather more information about the error handling patterns in similar client methods to ensure consistency.
Let me try one more verification to check the error handling patterns in similar client methods using a different pattern.
Error handling is consistent with other Agent client methods
The implementation follows the same error handling pattern used across other Agent client methods (Alive, Version, Node):
- Declares a response variable of the appropriate type
- Uses
client.Getwith consistent parameter ordering - Returns the response pointer and error directly
The endpoint is also properly defined in the API schema (swagger.json) and has corresponding test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check error handling patterns across the codebase
echo "Checking error handling patterns in API endpoints..."
rg -A 2 "func.*Config.*context.Context.*error"
# Verify the endpoint implementation
echo "Checking configuration endpoint implementation..."
rg -A 5 '"/api/v1/agent/config"'
Length of output: 2811
Script:
#!/bin/bash
# Check error handling in other client methods in the same package
echo "Checking error handling in other client methods..."
ast-grep --pattern 'func \(c \*Agent\) $_\(ctx context.Context\) \(\$_, error\) {
$$$
}'
# Check the client.Get implementation
echo "Checking client.Get implementation..."
ast-grep --pattern 'func \(c \*$_\) Get\(ctx context.Context, $$$\) error {'
Length of output: 367
Script:
#!/bin/bash
# Check other methods in the Agent client
echo "Checking other Agent client methods..."
rg -A 3 "func \(c \*Agent\)" pkg/publicapi/client/v2/
# Check the client implementation used by Agent
echo "Checking client implementation..."
rg -A 3 "type.*client.*struct" pkg/publicapi/client/v2/
Length of output: 1796
cmd/cli/agent/config_test.go (3)
1-4: LGTM! Build tags and package declaration are correct.
The build tags appropriately separate unit tests from integration tests, and the external test package is suitable for black-box testing.
17-23: LGTM! Test suite setup is well-structured.
The test suite is properly initialized using testify/suite, and the embedding of BaseSuite provides common testing functionality.
37-46: 🛠️ Refactor suggestion
Enhance test coverage for YAML output and verify potential issues.
Similar to the JSON test, this could benefit from more comprehensive assertions. Additionally, we should verify that the command handles various edge cases correctly.
func (s *ConfigSuite) TestConfigYAMLOutput() {
// NB: the default output is yaml, thus we don't specify it here.
_, out, err := s.ExecuteTestCobraCommand("agent", "config")
s.Require().NoError(err, "Could not request config with yaml output.")
var cfg types.Bacalhau
err = yaml.Unmarshal([]byte(out), &cfg)
s.Require().NoError(err, "Could not unmarshal the output into yaml - %+v", out)
+ // Verify structure and essential fields
s.Require().True(cfg.Orchestrator.Enabled)
+ s.Require().NotEmpty(cfg.APIServer.Host, "API server host should be set")
+ s.Require().NotEmpty(cfg.APIServer.Port, "API server port should be set")
+
+ // Verify sensitive data is redacted
+ s.Require().Equal("[REDACTED]", cfg.Compute.IPFS.SwarmKey, "SwarmKey should be redacted")
+
+ // Verify YAML structure
+ s.Require().Contains(out, "APIServer:", "YAML should contain APIServer section")
+ s.Require().Contains(out, "Compute:", "YAML should contain Compute section")
}
+
+func (s *ConfigSuite) TestConfigEdgeCases() {
+ // Test with invalid output format
+ _, _, err := s.ExecuteTestCobraCommand("agent", "config", "--output", "invalid")
+ s.Require().Error(err, "Should error on invalid output format")
+
+ // Test with both --pretty and --output=json
+ _, out, err := s.ExecuteTestCobraCommand("agent", "config", "--output", "json", "--pretty")
+ s.Require().NoError(err)
+ s.Require().Contains(out, "\n ", "JSON output should be pretty-printed")
+}Let's verify the command implementation to ensure it properly handles these test cases:
cmd/cli/agent/config.go (2)
1-13: LGTM! Package and imports are well organized.
The imports cover all necessary functionality for implementing the agent config command.
24-34: Consider handling sensitive data in configuration.
Based on PR discussions, there's a need to handle sensitive data in the configuration. Consider implementing redaction of sensitive fields before returning the configuration.
Let's check if there are any sensitive fields in the configuration type:
Consider implementing a configuration sanitizer that:
- Identifies sensitive fields (tokens, keys, credentials)
- Redacts or masks these values before display
- Optionally requires authentication for viewing full config
pkg/publicapi/endpoint/agent/endpoint_test.go (2)
1-17: LGTM! Build tags and imports are well-organized.
The build tags are correctly configured for unit testing, and the imports cover all necessary testing and API dependencies.
40-49: Verify JSON format aligns with API requirements.
The test confirms that the endpoint returns JSON format as discussed in the PR comments. Let's verify the complete API implementation:
✅ Verification successful
Let me gather more information about the JSON response format implementation.
Let me check one final aspect about the response handling and model structure.
Implementation correctly handles JSON format and sensitive data
The verification confirms that:
- The endpoint is properly configured to return JSON using
echo.MIMEApplicationJSONcontent type - Response is structured using
GetAgentConfigResponsemodel with proper JSON tags - Sensitive auth tokens are consistently redacted before being sent in the response
- The test correctly validates both JSON parsing and data redaction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the API consistently returns JSON format and handles sensitive data properly
# Test 1: Check if the API endpoint is consistently using JSON
echo "Checking for JSON response handling..."
rg -l "json\." pkg/publicapi/endpoint/agent/
# Test 2: Verify sensitive field handling across the codebase
echo "Checking sensitive field handling..."
rg "Token.*redact" pkg/publicapi/
Length of output: 694
Script:
#!/bin/bash
# Check the endpoint implementation and response type
echo "Checking endpoint implementation..."
rg -A 5 "func.*Config" pkg/publicapi/endpoint/agent/endpoint.go
# Check the API models for response structure
echo "Checking API models..."
rg "type.*GetAgentConfigResponse" pkg/publicapi/
Length of output: 322
Script:
#!/bin/bash
# Check the API model structure and handler implementation
echo "Checking API model structure..."
cat pkg/publicapi/apimodels/agent.go
echo "Checking handler implementation..."
cat pkg/publicapi/endpoint/agent/endpoint.go
Length of output: 5080
test_integration/5_orchestrator_no_config_suite_test.go (2)
7-9: LGTM! Import organization looks good.
The import statements are properly organized and grouped.
60-60: Verify the new config response structure across all endpoints.
The change to access config via ["config"] key aligns with the PR's changes to the /api/v1/agent/config endpoint. However, let's verify this structure is consistent across all related endpoints.
✅ Verification successful
Let me gather more information about the response structure by checking the implementation of the endpoint and related tests.
Based on the gathered information, I can now provide the final response:
The config response structure with ["config"] key is consistent across all endpoints and tests.
The implementation and test files show that:
- The
/api/v1/agent/configendpoint consistently returns the configuration under theconfigkey - All test files (
test_integration/1_orchestrator_basic_config_suite_test.goandtest_integration/5_orchestrator_no_config_suite_test.go) access the config using["config"]key - The endpoint implementation in
pkg/publicapi/endpoint/agent/endpoint.goreturns the config directly, which is then wrapped with theconfigkey at the API layer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of config response structure across endpoints
# and test files.
# Test 1: Check for other endpoint definitions that might need similar updates
echo "Checking API endpoint definitions..."
rg -A 3 "/api/v1/agent/config"
# Test 2: Check for similar test patterns that might need updates
echo "Checking test patterns..."
ast-grep --pattern 'unmarshalledOutput.(map[string]interface{})["config"]'
Length of output: 3905
Script:
#!/bin/bash
# Check the implementation of the config endpoint
echo "Checking config endpoint implementation..."
rg -A 10 "func.*config\(.*echo\.Context\)" pkg/publicapi/endpoint/agent/endpoint.go
# Check all test files for config response structure
echo "Checking test assertions on config response..."
rg -A 5 "unmarshalledOutput|marshalledOutput" test_integration/
Length of output: 14969
test_integration/4_orchestrator_config_override_and_flag_and_config_flag_suite_test.go (2)
8-10: LGTM! Import organization improved.
The reorganization of imports improves code readability.
71-71: 🛠️ Refactor suggestion
Verify the new config structure and add error handling.
The test correctly verifies the new config structure with the additional config key. However, the deep type assertions could be made more robust.
Consider adding type assertion error handling:
-webuiEnabled := unmarshalledAgentOutput.(map[string]interface{})["config"].(map[string]interface{})["WebUI"].(map[string]interface{})["Enabled"].(bool)
+config, ok := unmarshalledAgentOutput.(map[string]interface{})
+s.Require().True(ok, "Expected agent output to be a map")
+configSection, ok := config["config"].(map[string]interface{})
+s.Require().True(ok, "Expected config section to be a map")
+webUI, ok := configSection["WebUI"].(map[string]interface{})
+s.Require().True(ok, "Expected WebUI section to be a map")
+webuiEnabled, ok := webUI["Enabled"].(bool)
+s.Require().True(ok, "Expected Enabled to be a boolean")Let's verify the new config structure is consistent across the codebase:
✅ Verification successful
Let me search for more context about the config structure and type assertions in the codebase.
Let me check the WebUI config structure definition to understand the expected types.
Let me check one more thing about the test context to understand the unmarshal pattern being used.
Based on all the gathered context, I can now provide the final response:
Add error handling for WebUI config type assertions
The review comment is correct. The code performs multiple type assertions without error checking, while dealing with JSON response from an HTTP endpoint. The suggested refactoring with gradual type assertions and error handling is appropriate because:
- The WebUI config structure is well-defined in both legacy (
pkg/config_legacy/types/node.go) and current (pkg/config/types/webui.go) implementations - The code is part of an integration test that makes HTTP requests, where response validation is crucial
- This is the only instance of such deep type assertions in the test files, making it a good candidate for improvement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the config structure is consistent across tests
# Expect: All test files should use the same path to access WebUI.Enabled
# Search for WebUI config access patterns in test files
rg -A 2 'WebUI.*Enabled' test_integration/
Length of output: 555
Script:
#!/bin/bash
# Search for similar config structure access patterns
rg -A 2 'unmarshalledAgentOutput.*config' test_integration/
# Search for type assertions with error handling patterns
rg -A 2 'ok\s*:=.*\.(map\[string\]interface{})' test_integration/
# Look for WebUI config structure definition
rg -l 'type.*WebUI.*struct'
Length of output: 883
Script:
#!/bin/bash
# Check WebUI config structure in both legacy and current implementations
cat pkg/config_legacy/types/node.go pkg/config/types/webui.go
# Look for other test files that might be using similar unmarshal patterns
rg -A 2 'json\.Unmarshal.*AgentOutput' test_integration/
Length of output: 5298
Script:
#!/bin/bash
# Get more context around the unmarshalledAgentOutput usage
rg -B 5 -A 5 'unmarshalledAgentOutput' test_integration/4_orchestrator_config_override_and_flag_and_config_flag_suite_test.go
# Check if there are any other similar test files
fd -e go . test_integration/ -x grep -l "AgentOutput"
Length of output: 1065
pkg/publicapi/endpoint/agent/endpoint.go (1)
137-139: LGTM! Response structure follows API conventions.
The response is correctly wrapped in apimodels.GetAgentConfigResponse and uses proper HTTP status codes.
test_integration/1_orchestrator_basic_config_suite_test.go (2)
6-8: LGTM! Clean import organization.
The import statements are now properly organized and duplicates have been removed.
69-69: Verify API contract alignment.
The test has been updated to access the orchestrator configuration through a nested structure config.Orchestrator.Enabled. Let's verify this matches the new API contract and documentation.
✅ Verification successful
Let me gather more information about the config structure by searching for the configuration type definitions.
Let me search specifically for the Orchestrator configuration structure to verify the API contract.
Let me check the Bacalhau configuration structure to verify how the Orchestrator config is nested.
Based on the gathered information, I can now provide the final response:
API contract is correctly implemented in the test
The test's access pattern config.Orchestrator.Enabled aligns with the official API contract. This is verified by:
- The Bacalhau configuration structure in
pkg/config/types/bacalhau.gowhich showsOrchestratorfield as a top-level field - The
Orchestratorstruct inpkg/config/types/orchestrator.gocontaining theEnabledboolean field - Consistent usage across other parts of the codebase, including CLI flags and documentation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the API contract for agent config endpoint
# Expected: Find documentation or other tests that confirm the new structure
# Test 1: Look for API documentation
echo "Searching for API documentation..."
fd -g '*.md' -x rg -l 'api/v1/agent/config'
# Test 2: Look for similar test patterns
echo "Searching for similar test patterns..."
rg -A 2 'api/v1/agent/config' --type go
# Test 3: Look for struct definitions
echo "Searching for config structure definitions..."
ast-grep --pattern 'type $_ struct {
Config struct {
Orchestrator struct {
Enabled bool
$$$
}
$$$
}
$$$
}'
Length of output: 2651
Script:
#!/bin/bash
# Search for Orchestrator config structure definitions
echo "Searching for Orchestrator config structures..."
ast-grep --pattern 'type $_ struct {
Orchestrator struct {
Enabled bool
$$$
}
$$$
}'
# Search for config related type definitions
echo "Searching for config type definitions..."
rg -l "type.*Config.*struct" --type go
# Look at the actual config implementation
echo "Examining config implementation..."
rg -A 5 "type.*Config.*struct" --type go
Length of output: 39121
Script:
#!/bin/bash
# Search for Orchestrator-related configuration structures
echo "Searching for Orchestrator configuration..."
rg -A 10 "type.*struct.*{.*Orchestrator.*}" --type go
# Search for the agent config endpoint implementation
echo "Searching for agent config endpoint implementation..."
rg -A 10 "func.*config.*echo\.Context.*" pkg/publicapi/endpoint/agent/endpoint.go
# Look for Orchestrator-related fields in configuration
echo "Searching for Orchestrator configuration fields..."
rg -A 3 "Orchestrator.*struct" --type go
Length of output: 3535
Script:
#!/bin/bash
# Search for Bacalhau configuration structure
echo "Searching for Bacalhau config structure..."
rg -A 15 "type Bacalhau struct" pkg/config/types/
# Search for other test files accessing the config
echo "Searching for config access patterns in tests..."
rg -A 3 'config.*Orchestrator.*Enabled' --type go
Length of output: 3753
| func (s *ConfigSuite) TestConfigJSONOutput() { | ||
| _, out, err := s.ExecuteTestCobraCommand( | ||
| "agent", "config", "--output", string(output.JSONFormat), "--pretty=false", | ||
| ) | ||
| s.Require().NoError(err, "Could not request config with json output.") | ||
|
|
||
| var cfg types.Bacalhau | ||
| err = json.Unmarshal([]byte(out), &cfg) | ||
| s.Require().NoError(err, "Could not unmarshal the output into json - %+v", err) | ||
| s.Require().True(cfg.Orchestrator.Enabled) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage for JSON output.
While the basic functionality is tested, consider adding more assertions:
- Verify sensitive data redaction (tokens, keys)
- Test more configuration fields beyond just
orchestrator.Enabled - Validate the structure and format of the JSON output
func (s *ConfigSuite) TestConfigJSONOutput() {
_, out, err := s.ExecuteTestCobraCommand(
"agent", "config", "--output", string(output.JSONFormat), "--pretty=false",
)
s.Require().NoError(err, "Could not request config with json output.")
var cfg types.Bacalhau
err = json.Unmarshal([]byte(out), &cfg)
s.Require().NoError(err, "Could not unmarshal the output into json - %+v", err)
+ // Verify structure and essential fields
s.Require().True(cfg.Orchestrator.Enabled)
+ s.Require().NotEmpty(cfg.APIServer.Host, "API server host should be set")
+ s.Require().NotEmpty(cfg.APIServer.Port, "API server port should be set")
+
+ // Verify sensitive data is redacted
+ s.Require().Equal("[REDACTED]", cfg.Compute.IPFS.SwarmKey, "SwarmKey should be redacted")
+
+ // Verify JSON structure
+ s.Require().Contains(out, "\"APIServer\":", "JSON should contain APIServer section")
+ s.Require().Contains(out, "\"Compute\":", "JSON should contain Compute section")
}Committable suggestion was skipped due to low confidence.
| func run(cmd *cobra.Command, api client.API, o output.NonTabularOutputOptions) error { | ||
| ctx := cmd.Context() | ||
| response, err := api.Agent().Config(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot get agent config: %w", err) | ||
| } | ||
|
|
||
| return output.OutputOneNonTabular[types.Bacalhau](cmd, o, response.Config) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and add timeout.
While the implementation is functional, consider these improvements:
- Add context timeout for the API call
- Enhance error handling for empty or invalid responses
Consider applying these changes:
func run(cmd *cobra.Command, api client.API, o output.NonTabularOutputOptions) error {
- ctx := cmd.Context()
+ ctx, cancel := context.WithTimeout(cmd.Context(), 30*time.Second)
+ defer cancel()
+
response, err := api.Agent().Config(ctx)
if err != nil {
return fmt.Errorf("cannot get agent config: %w", err)
}
+ if response == nil || response.Config == nil {
+ return fmt.Errorf("received empty configuration from agent")
+ }
return output.OutputOneNonTabular[types.Bacalhau](cmd, o, response.Config)
}Don't forget to add the imports:
import (
"context"
"time"
)| // TestEndpointConfigRedactFields asserts that auth tokens in the config are redacted. | ||
| func TestEndpointConfigRedactFields(t *testing.T) { | ||
| router := echo.New() | ||
|
|
||
| // populate the fields that should be redacted with "secret" values. | ||
| NewEndpoint(EndpointParams{ | ||
| Router: router, | ||
| BacalhauConfig: types.Bacalhau{ | ||
| Orchestrator: types.Orchestrator{ | ||
| Auth: types.OrchestratorAuth{ | ||
| Token: "super-secret-orchestrator-token", | ||
| }, | ||
| }, | ||
| Compute: types.Compute{ | ||
| Auth: types.ComputeAuth{ | ||
| Token: "super-secret-orchestrator-token", | ||
| }, | ||
| }, | ||
| }, | ||
| }) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/api/v1/agent/config", nil) | ||
| rr := httptest.NewRecorder() | ||
| router.ServeHTTP(rr, req) | ||
|
|
||
| require.Equal(t, http.StatusOK, rr.Code) | ||
|
|
||
| // assert the secret values are not present. | ||
| var payload apimodels.GetAgentConfigResponse | ||
| err := json.NewDecoder(rr.Body).Decode(&payload) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, payload.Config.Orchestrator.Auth.Token, "<redacted>") | ||
| assert.Equal(t, payload.Config.Compute.Auth.Token, "<redacted>") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing test coverage with additional scenarios.
The current test effectively verifies the redaction of sensitive tokens. However, consider these improvements:
- Add table-driven tests to cover various configuration scenarios
- Include negative test cases (e.g., malformed config)
- Verify non-sensitive fields remain unchanged
- Add Content-Type header validation
- Assert the complete JSON structure matches the API model
Here's a suggested enhancement:
func TestEndpointConfigRedactFields(t *testing.T) {
+ tests := []struct {
+ name string
+ config types.Bacalhau
+ expectedCode int
+ expectRedacted bool
+ }{
+ {
+ name: "should redact sensitive fields",
+ config: types.Bacalhau{
+ Orchestrator: types.Orchestrator{
+ Auth: types.OrchestratorAuth{
+ Token: "secret-token",
+ },
+ },
+ // Add more test fields here
+ },
+ expectedCode: http.StatusOK,
+ expectRedacted: true,
+ },
+ // Add more test cases
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ router := echo.New()
+ NewEndpoint(EndpointParams{
+ Router: router,
+ BacalhauConfig: tt.config,
+ })
+
+ req := httptest.NewRequest(http.MethodGet, "/api/v1/agent/config", nil)
+ rr := httptest.NewRecorder()
+ router.ServeHTTP(rr, req)
+
+ require.Equal(t, tt.expectedCode, rr.Code)
+ require.Equal(t, "application/json", rr.Header().Get("Content-Type"))
+
+ var payload apimodels.GetAgentConfigResponse
+ err := json.NewDecoder(rr.Body).Decode(&payload)
+ require.NoError(t, err)
+
+ // Verify sensitive fields are redacted
+ if tt.expectRedacted {
+ assert.Equal(t, "<redacted>", payload.Config.Orchestrator.Auth.Token)
+ assert.Equal(t, "<redacted>", payload.Config.Compute.Auth.Token)
+ }
+
+ // Verify non-sensitive fields remain unchanged
+ assert.Equal(t, tt.config.Orchestrator.Host, payload.Config.Orchestrator.Host)
+ // Add more field verifications
+ })
+ }
}Committable suggestion was skipped due to low confidence.
| if cfg.Compute.Auth.Token != "" { | ||
| cfg.Compute.Auth.Token = "<redacted>" | ||
| } | ||
| if cfg.Orchestrator.Auth.Token != "" { | ||
| cfg.Orchestrator.Auth.Token = "<redacted>" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance sensitive data redaction.
Consider the following improvements to the redaction logic:
- Use a constant for the redaction text
- Consider redacting other potential sensitive fields (e.g., API keys, certificates)
+const (
+ redactedText = "<redacted>"
+)
-if cfg.Compute.Auth.Token != "" {
- cfg.Compute.Auth.Token = "<redacted>"
-}
-if cfg.Orchestrator.Auth.Token != "" {
- cfg.Orchestrator.Auth.Token = "<redacted>"
-}
+// Helper function to redact sensitive strings
+func redactIfNotEmpty(s string) string {
+ if s != "" {
+ return redactedText
+ }
+ return s
+}
+
+// Redact sensitive information
+cfg.Compute.Auth.Token = redactIfNotEmpty(cfg.Compute.Auth.Token)
+cfg.Orchestrator.Auth.Token = redactIfNotEmpty(cfg.Orchestrator.Auth.Token)
+// TODO: Consider redacting other sensitive fields like:
+// - API keys
+// - Certificates
+// - Private keysCommittable suggestion was skipped due to low confidence.
|
|
||
| orchestratorEnabled := marshalledOutput.(map[string]interface{})["Orchestrator"].(map[string]interface{})["Enabled"].(bool) | ||
| orchestratorEnabled := marshalledOutput.(map[string]interface{})["config"].(map[string]interface{})["Orchestrator"].(map[string]interface{})["Enabled"].(bool) | ||
| s.Require().Truef(orchestratorEnabled, "Expected orchestrator to be enabled, got: %t", orchestratorEnabled) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding test coverage.
Given the PR objectives and discussions, consider adding test cases for:
- Verifying both JSON and YAML output formats
- Ensuring sensitive data is properly masked
- Testing error scenarios (e.g., invalid config, missing permissions)
Example test structure:
func (s *OrchestratorBasicConfigSuite) TestAgentConfigOutputFormats() {
// Test JSON output
jsonOutput, err := s.executeCommandInDefaultJumpbox([]string{
"bacalhau", "agent", "config", "--output=json",
})
s.Require().NoError(err)
// Verify JSON structure...
// Test YAML output
yamlOutput, err := s.executeCommandInDefaultJumpbox([]string{
"bacalhau", "agent", "config", "--output=yaml",
})
s.Require().NoError(err)
// Verify YAML structure...
}
func (s *OrchestratorBasicConfigSuite) TestSensitiveDataHandling() {
output, err := s.executeCommandInDefaultJumpbox([]string{
"bacalhau", "agent", "config",
})
s.Require().NoError(err)
// Verify sensitive fields are masked...
}
Example:
Summary by CodeRabbit
New Features
Bug Fixes
Tests