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.

Sign up for GitHub

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

Hidden agent id flag #160

Merged
merged 4 commits into from
Dec 23, 2022
Merged

Hidden agent id flag #160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c647868
Add new hidden agent-id flag
arbulu89 Dec 22, 2022
fcdf523
Add new agent-id flag docs
arbulu89 Dec 22, 2022
1d595d2
Set test values as suite params
arbulu89 Dec 23, 2022
3fd5e5f
Rename agent-id by force-agent-id
arbulu89 Dec 23, 2022
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
12 changes: 10 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,13 +224,21 @@ You can install it with `go install github.com/vektra/mockery/v2`.

> Be sure to add the `mockery` binary to your `$PATH` environment variable so that `make` can find it. That usually comes with configuring `$GOPATH`, `$GOBIN`, and adding the latter to your `$PATH`.



> Please note that the `trento agent` component requires to be running on
> the OS (_not_ inside a container) so, while it is technically possible to run `trento agent`
> commands in the container, it makes little sense because most of its internals
> require direct access to the host of the HA Cluster components.

## Fake Agent ID

In some circunstances, having a fake Agent ID might be useful, specially during development and testing stages. The hidden `force-agent-id` flag is available for that.

Here an example on how to use it:

`./trento-agent start --force-agent-id "800ddd9b-8497-493f-b9fa-1bd6c9afb230"`

> Don't use this flag on production systems, as the agent ID must be unique by definition and any change affects the whole Trento usage.

## Fact gathering plugin system

A plugin system is available in the Agent, in order to add new fact gathering options, so it can run user created checks in the server side.
Expand Down
15 changes: 13 additions & 2 deletions cmd/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"time"

"github.com/pkg/errors"
"github.com/spf13/afero"
"github.com/spf13/viper"
"github.com/trento-project/agent/internal/agent"
"github.com/trento-project/agent/internal/discovery"
Expand All @@ -20,7 +21,7 @@ func validatePeriod(durationFlag string, minValue time.Duration) error {
return nil
}

func LoadConfig() (*agent.Config, error) {
func LoadConfig(fileSystem afero.Fs) (*agent.Config, error) {
minPeriodValues := map[string]time.Duration{
"cluster-discovery-period": discovery.ClusterDiscoveryMinPeriod,
"sapsystem-discovery-period": discovery.SAPDiscoveryMinPeriod,
Expand Down Expand Up @@ -51,10 +52,19 @@ func LoadConfig() (*agent.Config, error) {
return nil, errors.New("api-key is required, cannot start agent")
}

agentID := viper.GetString("force-agent-id")
if agentID == "" {
id, err := agent.GetAgentID(fileSystem)
if err != nil {
return nil, errors.Wrap(err, "could not get the agent ID")
}
agentID = id
}

collectorConfig := &collector.Config{
ServerURL: viper.GetString("server-url"),
APIKey: apiKey,
AgentID: "",
AgentID: agentID,
}

discoveryPeriodsConfig := &discovery.DiscoveriesPeriodConfig{
Expand All @@ -72,6 +82,7 @@ func LoadConfig() (*agent.Config, error) {
}

return &agent.Config{
AgentID: agentID,
InstanceName: hostname,
DiscoveriesConfig: discoveriesConfig,
// Feature flag to enable the facts engine
Expand Down
66 changes: 52 additions & 14 deletions cmd/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,21 @@ import (
"testing"
"time"

"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/stretchr/testify/suite"
"github.com/trento-project/agent/internal/agent"
"github.com/trento-project/agent/internal/discovery"
"github.com/trento-project/agent/internal/discovery/collector"
"github.com/trento-project/agent/test/helpers"
)

type AgentCmdTestSuite struct {
suite.Suite
cmd *cobra.Command
cmd *cobra.Command
fileSystem afero.Fs
hostname string
expectedConfig *agent.Config
}

func TestAgentCmdTestSuite(t *testing.T) {
Expand All @@ -41,12 +46,10 @@ func (suite *AgentCmdTestSuite) SetupTest() {
cmd.SetOut(&b)

suite.cmd = cmd
}

func (suite *AgentCmdTestSuite) TearDownTest() {
_ = suite.cmd.Execute()

expectedConfig := &agent.Config{
suite.fileSystem = helpers.MockMachineIDFile()
suite.hostname = "some-hostname"
suite.expectedConfig = &agent.Config{
AgentID: "some-agent-id",
InstanceName: "some-hostname",
DiscoveriesConfig: &discovery.DiscoveriesConfig{
SSHAddress: "some-ssh-address",
Expand All @@ -60,19 +63,13 @@ func (suite *AgentCmdTestSuite) TearDownTest() {
CollectorConfig: &collector.Config{
ServerURL: "http://serverurl",
APIKey: "some-api-key",
AgentID: "",
AgentID: "some-agent-id",
},
},
FactsEngineEnabled: false,
FactsServiceURL: "amqp://guest:guest@localhost:5672",
PluginsFolder: "/usr/etc/trento/plugins/",
}

config, err := LoadConfig()
config.InstanceName = "some-hostname"
suite.NoError(err)

suite.EqualValues(expectedConfig, config)
}

func (suite *AgentCmdTestSuite) TestConfigFromFlags() {
Expand All @@ -86,7 +83,16 @@ func (suite *AgentCmdTestSuite) TestConfigFromFlags() {
"--subscription-discovery-period=900s",
"--server-url=http://serverurl",
"--api-key=some-api-key",
"--force-agent-id=some-agent-id",
})

_ = suite.cmd.Execute()

config, err := LoadConfig(suite.fileSystem)
config.InstanceName = suite.hostname
suite.NoError(err)

suite.EqualValues(suite.expectedConfig, config)
}

func (suite *AgentCmdTestSuite) TestConfigFromEnv() {
Expand All @@ -98,8 +104,40 @@ func (suite *AgentCmdTestSuite) TestConfigFromEnv() {
os.Setenv("TRENTO_SUBSCRIPTION_DISCOVERY_PERIOD", "900s")
os.Setenv("TRENTO_SERVER_URL", "http://serverurl")
os.Setenv("TRENTO_API_KEY", "some-api-key")
os.Setenv("TRENTO_FORCE_AGENT_ID", "some-agent-id")

_ = suite.cmd.Execute()

config, err := LoadConfig(suite.fileSystem)
config.InstanceName = suite.hostname
suite.NoError(err)

suite.EqualValues(suite.expectedConfig, config)
}

func (suite *AgentCmdTestSuite) TestConfigFromFile() {
os.Setenv("TRENTO_CONFIG", "../test/fixtures/config/agent.yaml")

_ = suite.cmd.Execute()

config, err := LoadConfig(suite.fileSystem)
config.InstanceName = suite.hostname
suite.NoError(err)

suite.EqualValues(suite.expectedConfig, config)
}

func (suite *AgentCmdTestSuite) TestAgentIDLoaded() {
suite.cmd.SetArgs([]string{
"start",
"--ssh-address=some-ssh-address",
"--api-key=some-api-key",
})

_ = suite.cmd.Execute()

config, err := LoadConfig(suite.fileSystem)
suite.NoError(err)
suite.Equal(helpers.DummyAgentID, config.AgentID)
suite.Equal(helpers.DummyAgentID, config.DiscoveriesConfig.CollectorConfig.AgentID)
}
3 changes: 2 additions & 1 deletion cmd/id.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cmd
import (
"os"

"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/trento-project/agent/internal/agent"
)
Expand All @@ -12,7 +13,7 @@ func NewAgentIDCmd() *cobra.Command {
Use: "id",
Short: "Print the agent identifier",
RunE: func(cmd *cobra.Command, args []string) error {
agentID, err := agent.GetAgentID()
agentID, err := agent.GetAgentID(afero.NewOsFs())
Copy link
Member

Choose a reason for hiding this comment

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

This PR opens the need to refactor also id command I guess, because as of now it will always get the real agent id.

What I would expect is that when ./trento-agent id is executed it returns either what has been configured in agent's config file or the real agent id.

As an addition I wouldn't expect ./trento-agent id --force-agent-id=blaa, as it would be fancy 😅

Finally we'd need to take care of having the same defaults for both the commands, otherwise in one case viper would complain about not having some expected flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't go in this direction. This force-agent-id is a hidden flag for development/test purposes, and it shouldn't be used together with the id command, which is used to get the real id. It is only for the start function, which is the one that it makes sense.
Using ./trento-agent id --force-agent-id=blaa doesn't make any sense.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah true, that's just for test/dev so if that's how it has to be I can live with this little inconsistency 😉

Using ./trento-agent id --force-agent-id=blaa doesn't make any sense.

Yes, we're saying the same about this 😄

if err != nil {
return err
}
Expand Down
18 changes: 16 additions & 2 deletions cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/afero"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/viper"
Expand Down Expand Up @@ -54,6 +55,7 @@ func NewStartCmd() *cobra.Command {
"http://localhost",
"Trento server URL",
)

startCmd.Flags().
String(
"api-key",
Expand All @@ -69,6 +71,7 @@ func NewStartCmd() *cobra.Command {
10*time.Second,
"Cluster discovery mechanism loop period in seconds",
)

startCmd.Flags().
DurationVarP(
&sapSystemDiscoveryPeriod,
Expand All @@ -77,6 +80,7 @@ func NewStartCmd() *cobra.Command {
10*time.Second,
"SAP systems discovery mechanism loop period in seconds",
)

startCmd.Flags().
DurationVarP(
&cloudDiscoveryPeriod,
Expand All @@ -85,6 +89,7 @@ func NewStartCmd() *cobra.Command {
10*time.Second,
"Cloud discovery mechanism loop period in seconds",
)

startCmd.Flags().
DurationVarP(
&hostDiscoveryPeriod,
Expand All @@ -93,6 +98,7 @@ func NewStartCmd() *cobra.Command {
10*time.Second,
"Host discovery mechanism loop period in seconds",
)

startCmd.Flags().
DurationVarP(
&subscriptionDiscoveryPeriod,
Expand All @@ -101,10 +107,16 @@ func NewStartCmd() *cobra.Command {
900*time.Second,
"Subscription discovery mechanism loop period in seconds",
)

err := startCmd.Flags().
MarkHidden("subscription-discovery-period")
if err != nil {
panic(err)
}

startCmd.Flags().
String("force-agent-id", "", "Agent ID. Used to mock the real ID for development purposes")
err = startCmd.Flags().
MarkHidden("force-agent-id")
if err != nil {
panic(err)
}
Expand All @@ -115,11 +127,13 @@ func NewStartCmd() *cobra.Command {
if err != nil {
panic(err)
}

startCmd.Flags().String("facts-service-url", "amqp://guest:guest@localhost:5672", "Facts service queue url")
err = startCmd.Flags().MarkHidden("facts-service-url")
if err != nil {
panic(err)
}

return startCmd
}

Expand All @@ -131,7 +145,7 @@ func start(*cobra.Command, []string) {
signals := make(chan os.Signal, 1)
signal.Notify(signals, syscall.SIGINT, syscall.SIGTERM)

config, err := LoadConfig()
config, err := LoadConfig(afero.NewOsFs())
if err != nil {
log.Fatal("Failed to create the agent configuration: ", err)
}
Expand Down
16 changes: 3 additions & 13 deletions internal/agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"time"

"github.com/google/uuid"
"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/afero"
"golang.org/x/sync/errgroup"
Expand All @@ -21,18 +20,17 @@ import (
const machineIDPath = "/etc/machine-id"

var (
fileSystem = afero.NewOsFs() //nolint
trentoNamespace = uuid.Must(uuid.Parse("fb92284e-aa5e-47f6-a883-bf9469e7a0dc")) //nolint
)

type Agent struct {
agentID string
config *Config
collectorClient collector.Client
discoveries []discovery.Discovery
}

type Config struct {
AgentID string
InstanceName string
DiscoveriesConfig *discovery.DiscoveriesConfig
FactsEngineEnabled bool
Expand All @@ -42,13 +40,6 @@ type Config struct {

// NewAgent returns a new instance of Agent with the given configuration
func NewAgent(config *Config) (*Agent, error) {
agentID, err := GetAgentID()
if err != nil {
return nil, errors.Wrap(err, "could not get the agent ID")
}

config.DiscoveriesConfig.CollectorConfig.AgentID = agentID

collectorClient := collector.NewCollectorClient(config.DiscoveriesConfig.CollectorConfig)

discoveries := []discovery.Discovery{
Expand All @@ -60,15 +51,14 @@ func NewAgent(config *Config) (*Agent, error) {
}

agent := &Agent{
agentID: agentID,
config: config,
collectorClient: collectorClient,
discoveries: discoveries,
}
return agent, nil
}

func GetAgentID() (string, error) {
func GetAgentID(fileSystem afero.Fs) (string, error) {
machineIDBytes, err := afero.ReadFile(fileSystem, machineIDPath)
if err != nil {
return "", err
Expand Down Expand Up @@ -121,7 +111,7 @@ func (a *Agent) Start(ctx context.Context) error {

gathererRegistry.AddGatherers(gatherersFromPlugins)

c := factsengine.NewFactsEngine(a.agentID, a.config.FactsServiceURL, *gathererRegistry)
c := factsengine.NewFactsEngine(a.config.AgentID, a.config.FactsServiceURL, *gathererRegistry)

g.Go(func() error {
log.Info("Starting fact gathering service...")
Expand Down
Loading