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

feat: adds agent config command that returns the agent configuration #4671

Merged
merged 7 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions cmd/cli/agent/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package agent

import (
"fmt"

"github.com/spf13/cobra"

"github.com/bacalhau-project/bacalhau/cmd/util"
"github.com/bacalhau-project/bacalhau/cmd/util/flags/cliflags"
"github.com/bacalhau-project/bacalhau/cmd/util/output"
"github.com/bacalhau-project/bacalhau/pkg/config/types"
"github.com/bacalhau-project/bacalhau/pkg/publicapi/client/v2"
)

func NewConfigCmd() *cobra.Command {
o := output.NonTabularOutputOptions{
Format: output.YAMLFormat,
Pretty: true,
}
configCmd := &cobra.Command{
Use: "config",
Short: "Get the agent's configuration.",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
cfg, err := util.SetupRepoConfig(cmd)
if err != nil {
return fmt.Errorf("failed to setup repo: %w", err)
}
api, err := util.GetAPIClientV2(cmd, cfg)
if err != nil {
return fmt.Errorf("failed to create api client: %w", err)
}
return run(cmd, api, o)
},
}
configCmd.Flags().AddFlagSet(cliflags.OutputNonTabularFormatFlags(&o))
return configCmd
}

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)
}
Comment on lines +40 to +48
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error handling and add timeout.

While the implementation is functional, consider these improvements:

  1. Add context timeout for the API call
  2. 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"
)

46 changes: 46 additions & 0 deletions cmd/cli/agent/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
//go:build unit || !integration

package agent_test

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

func TestConfigSuite(t *testing.T) {
suite.Run(t, new(ConfigSuite))
}

type ConfigSuite struct {
cmdtesting.BaseSuite
}

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)
}
Comment on lines +25 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance test coverage for JSON output.

While the basic functionality is tested, consider adding more assertions:

  1. Verify sensitive data redaction (tokens, keys)
  2. Test more configuration fields beyond just orchestrator.Enabled
  3. 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 (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)
s.Require().True(cfg.Orchestrator.Enabled)
}
1 change: 1 addition & 0 deletions cmd/cli/agent/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ func NewCmd() *cobra.Command {
cmd.AddCommand(NewAliveCmd())
cmd.AddCommand(NewNodeCmd())
cmd.AddCommand(NewVersionCmd())
cmd.AddCommand(NewConfigCmd())
return cmd
}
3 changes: 2 additions & 1 deletion pkg/config/types/bacalhau.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ type Bacalhau struct {
Logging Logging `yaml:"Logging,omitempty" json:"Logging,omitempty"`
UpdateConfig UpdateConfig `yaml:"UpdateConfig,omitempty" json:"UpdateConfig,omitempty"`
FeatureFlags FeatureFlags `yaml:"FeatureFlags,omitempty" json:"FeatureFlags,omitempty"`
DisableAnalytics bool `yaml:"DisableAnalytics,omitempty" json:"DisableAnalytics,omitempty"`
// DisableAnalytics, when true, disables sharing anonymous analytics data with the Bacalhau development team
DisableAnalytics bool `yaml:"DisableAnalytics,omitempty" json:"DisableAnalytics,omitempty"`
}

// Copy returns a deep copy of the Bacalhau configuration.
Expand Down
10 changes: 9 additions & 1 deletion pkg/publicapi/apimodels/agent.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package apimodels

import "github.com/bacalhau-project/bacalhau/pkg/models"
import (
"github.com/bacalhau-project/bacalhau/pkg/config/types"
"github.com/bacalhau-project/bacalhau/pkg/models"
)

// IsAliveResponse is the response to the IsAlive request.
type IsAliveResponse struct {
Expand Down Expand Up @@ -30,3 +33,8 @@ type GetAgentNodeResponse struct {
BaseGetResponse
*models.NodeState
}

type GetAgentConfigResponse struct {
BaseGetResponse
Config types.Bacalhau `json:"config"`
}
6 changes: 6 additions & 0 deletions pkg/publicapi/client/v2/api_agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,9 @@ func (c *Agent) Node(ctx context.Context, req *apimodels.GetAgentNodeRequest) (*
err := c.client.Get(ctx, "/api/v1/agent/node", req, &res)
return &res, err
}

func (c *Agent) Config(ctx context.Context) (*apimodels.GetAgentConfigResponse, error) {
var res apimodels.GetAgentConfigResponse
err := c.client.Get(ctx, "/api/v1/agent/config", &apimodels.BaseGetRequest{}, &res)
return &res, err
}
18 changes: 15 additions & 3 deletions pkg/publicapi/endpoint/agent/endpoint.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package agent

import (
"fmt"
"net/http"

"github.com/labstack/echo/v4"
Expand Down Expand Up @@ -113,7 +114,7 @@ func (e *Endpoint) debug(c echo.Context) error {
return c.JSON(http.StatusOK, debugInfoMap)
}

// debug godoc
// config godoc
//
// @ID agent/config
// @Summary Returns the current configuration of the node.
Expand All @@ -123,6 +124,17 @@ func (e *Endpoint) debug(c echo.Context) error {
// @Failure 500 {object} string
// @Router /api/v1/agent/config [get]
func (e *Endpoint) config(c echo.Context) error {
cfg := e.bacalhauConfig
return c.JSON(http.StatusOK, cfg)
cfg, err := e.bacalhauConfig.Copy()
if err != nil {
return echo.NewHTTPError(http.StatusInternalServerError, fmt.Sprintf("could not copy bacalhau config: %s", err))
}
if cfg.Compute.Auth.Token != "" {
cfg.Compute.Auth.Token = "<redacted>"
}
if cfg.Orchestrator.Auth.Token != "" {
cfg.Orchestrator.Auth.Token = "<redacted>"
}
Comment on lines +131 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance sensitive data redaction.

Consider the following improvements to the redaction logic:

  1. Use a constant for the redaction text
  2. 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 keys

Committable suggestion was skipped due to low confidence.

return c.JSON(http.StatusOK, apimodels.GetAgentConfigResponse{
Config: cfg,
})
}
52 changes: 52 additions & 0 deletions pkg/publicapi/endpoint/agent/endpoint_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
//go:build unit || !integration

package agent

import (
"encoding/json"
"net/http"
"net/http/httptest"
"testing"

"github.com/labstack/echo/v4"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/bacalhau-project/bacalhau/pkg/config/types"
"github.com/bacalhau-project/bacalhau/pkg/publicapi/apimodels"
)

// 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>")
}
Comment on lines +19 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider enhancing test coverage with additional scenarios.

The current test effectively verifies the redaction of sensitive tokens. However, consider these improvements:

  1. Add table-driven tests to cover various configuration scenarios
  2. Include negative test cases (e.g., malformed config)
  3. Verify non-sensitive fields remain unchanged
  4. Add Content-Type header validation
  5. 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.

7 changes: 4 additions & 3 deletions test_integration/1_orchestrator_basic_config_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@ package test_integration
import (
"context"
"fmt"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"github.com/testcontainers/testcontainers-go/exec"
"strings"
"testing"
)

type OrchestratorBasicConfigSuite struct {
Expand Down Expand Up @@ -65,7 +66,7 @@ func (s *OrchestratorBasicConfigSuite) TestOrchestratorNodeUpAndEnabled() {
marshalledOutput, err := s.unmarshalJSONString(agentConfigOutput, JSONObject)
s.Require().NoErrorf(err, "Error unmarshalling response: %q", err)

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)
}
Comment on lines 68 to 71
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider expanding test coverage.

Given the PR objectives and discussions, consider adding test cases for:

  1. Verifying both JSON and YAML output formats
  2. Ensuring sensitive data is properly masked
  3. 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...
}


Expand Down
5 changes: 3 additions & 2 deletions test_integration/2_orchestrator_config_override_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ package test_integration
import (
"context"
"fmt"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/suite"
)

type OrchestratorConfigOverrideSuite struct {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ package test_integration
import (
"context"
"fmt"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/suite"
)

type OrchestratorConfigOverrideAndFlagSuite struct {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ package test_integration
import (
"context"
"fmt"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/suite"
)

type OrchestratorConfigOverrideAndFlagAndConfigFlagSuite struct {
Expand Down Expand Up @@ -67,7 +68,7 @@ func (s *OrchestratorConfigOverrideAndFlagAndConfigFlagSuite) TestConfigOverride
unmarshalledAgentOutput, err := s.unmarshalJSONString(agentConfigOutput, JSONObject)
s.Require().NoErrorf(err, "Error unmarshalling response: %q", err)

webuiEnabled := unmarshalledAgentOutput.(map[string]interface{})["WebUI"].(map[string]interface{})["Enabled"].(bool)
webuiEnabled := unmarshalledAgentOutput.(map[string]interface{})["config"].(map[string]interface{})["WebUI"].(map[string]interface{})["Enabled"].(bool)
s.Require().Truef(webuiEnabled, "Expected orchestrator to be enabled, got: %t", webuiEnabled)
}

Expand Down
7 changes: 4 additions & 3 deletions test_integration/5_orchestrator_no_config_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@ package test_integration

import (
"context"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"strings"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/suite"
)

type OrchestratorNoConfigSuite struct {
Expand Down Expand Up @@ -56,7 +57,7 @@ func (s *OrchestratorNoConfigSuite) TestStartingOrchestratorNodeWithConfigFile()
unmarshalledOutput, err := s.unmarshalJSONString(agentConfigOutput, JSONObject)
s.Require().NoErrorf(err, "Error unmarshalling response: %q", err)

unmarshalledOutputMap := unmarshalledOutput.(map[string]interface{})
unmarshalledOutputMap := unmarshalledOutput.(map[string]interface{})["config"].(map[string]interface{})

orchestratorEnabled := unmarshalledOutputMap["Orchestrator"].(map[string]interface{})["Enabled"].(bool)
s.Require().Truef(orchestratorEnabled, "Expected orchestrator to be enabled, got: %t", orchestratorEnabled)
Expand Down
7 changes: 4 additions & 3 deletions test_integration/6_jobs_basic_runs_scenarios_suite_test.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
package test_integration

import (
"bacalhau/integration_tests/utils"
"context"
"fmt"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
"strings"
"testing"
"time"

"bacalhau/integration_tests/utils"
"github.com/google/uuid"
"github.com/stretchr/testify/suite"
)

type JobsBasicRunsScenariosSuite struct {
Expand Down
Loading