diff --git a/managed/models/agent_helpers.go b/managed/models/agent_helpers.go index 6578fc2f816..2eabb7ec8d6 100644 --- a/managed/models/agent_helpers.go +++ b/managed/models/agent_helpers.go @@ -331,6 +331,58 @@ func FindAgents(q *reform.Querier, filters AgentFilters) ([]*Agent, error) { return agents, nil } +// IsInternalPgQANAgent reports whether the Agent is the QAN Agent of PMM Server's own PostgreSQL +// Service. +// +// Keyed on the Service alone, not the pmm-agent: Service names are unique, so the name already +// excludes a remote instance's QAN Agent (RDS/Azure discovery attaches those to PMM Server's own +// pmm-agent too). Adding pmm_agent_id == PMMServerAgentID would also be wrong, because +// PMMServerAgentID is a mutable process global, reassigned in HA setup and from the pmm-agent +// config file. +func IsInternalPgQANAgent(q *reform.Querier, agent *Agent) (bool, error) { + if agent.AgentType != QANPostgreSQLPgStatementsAgentType { + return false, nil + } + + serviceID := pointer.GetString(agent.ServiceID) + if serviceID == "" { + return false, nil + } + + service, err := FindServiceByID(q, serviceID) + if err != nil { + return false, err + } + + return service.ServiceName == PMMServerPostgreSQLServiceName, nil +} + +// FindInternalPgQANAgent returns the QAN Agent of PMM Server's own PostgreSQL Service. +// +// It returns NotFound when PMM Server has no such Service, which is the normal state in HA mode +// where PMM Server runs against an external PostgreSQL and the fixtures do not create it. +func FindInternalPgQANAgent(q *reform.Querier) (*Agent, error) { + service, err := FindServiceByName(q, PMMServerPostgreSQLServiceName) + if err != nil { + return nil, err + } + + // Queried directly rather than through FindAgents, which re-validates a ServiceID filter with + // FindServiceByID -- a third round trip for the Service row just loaded above. + structs, err := q.SelectAllFrom(AgentTable, "WHERE service_id = $1 AND agent_type = $2 ORDER BY agent_id", + service.ServiceID, QANPostgreSQLPgStatementsAgentType) + if err != nil { + return nil, err + } + if len(structs) == 0 { + return nil, status.Errorf(codes.NotFound, "QAN Agent for the %q Service not found.", PMMServerPostgreSQLServiceName) + } + + agent := DecryptAgent(*structs[0].(*Agent)) //nolint:forcetypeassert + + return &agent, nil +} + // FindAgentByID finds Agent by ID. func FindAgentByID(q *reform.Querier, id string) (*Agent, error) { if id == "" { @@ -1208,12 +1260,23 @@ func (p *ChangeAgentParams) AffectsConnection() bool { } // ChangeAgent changes agent parameters based on agent type. -func ChangeAgent(q *reform.Querier, agentID string, params *ChangeAgentParams) (*Agent, error) { //nolint:cyclop,maintidx +func ChangeAgent(q *reform.Querier, agentID string, params *ChangeAgentParams) (*Agent, error) { row, err := FindAgentByID(q, agentID) if err != nil { return nil, err } + return ApplyAgentChange(q, row, params) +} + +// ApplyAgentChange changes agent parameters on an already-loaded Agent row, based on agent type. +// +// Callers that already had to load the row to inspect it before changing it (e.g. to check its +// type or a precondition) can pass it here directly, instead of ChangeAgent re-fetching the same +// row from the database. +func ApplyAgentChange(q *reform.Querier, row *Agent, params *ChangeAgentParams) (*Agent, error) { //nolint:cyclop,gocognit,maintidx + var err error + // Handle common fields first if params.Enabled != nil { row.Disabled = !(*params.Enabled) diff --git a/managed/services/inventory/agents.go b/managed/services/inventory/agents.go index 69c8b9acefe..d3b00fd38ba 100644 --- a/managed/services/inventory/agents.go +++ b/managed/services/inventory/agents.go @@ -18,7 +18,6 @@ package inventory import ( "context" - "os" "strings" "github.com/AlekSi/pointer" @@ -234,7 +233,7 @@ func (as *AgentsService) ChangeNodeExporter(ctx context.Context, agentID string, MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.NodeExporterType, params) if err != nil { return nil, err } @@ -334,7 +333,7 @@ func (as *AgentsService) ChangeMySQLdExporter(ctx context.Context, agentID strin ConnectionTimeout: duration.OptionalFromProto(p.ConnectionTimeout), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.MySQLdExporterType, params) if err != nil { return nil, err } @@ -438,7 +437,7 @@ func (as *AgentsService) ChangeMongoDBExporter( ConnectionTimeout: duration.OptionalFromProto(p.ConnectionTimeout), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.MongoDBExporterType, params) if err != nil { return nil, err } @@ -541,7 +540,7 @@ func (as *AgentsService) ChangeQANMySQLPerfSchemaAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANMySQLPerfSchemaAgentType, params) if err != nil { return nil, err } @@ -647,7 +646,7 @@ func (as *AgentsService) ChangeQANMySQLSlowlogAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANMySQLSlowlogAgentType, params) if err != nil { return nil, err } @@ -741,7 +740,7 @@ func (as *AgentsService) ChangePostgresExporter( ConnectionTimeout: duration.OptionalFromProto(p.ConnectionTimeout), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.PostgresExporterType, params) if err != nil { return nil, err } @@ -832,7 +831,7 @@ func (as *AgentsService) ChangeValkeyExporter(ctx context.Context, agentID strin ConnectionTimeout: duration.OptionalFromProto(p.ConnectionTimeout), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.ValkeyExporterType, params) if err != nil { return nil, err } @@ -929,7 +928,7 @@ func (as *AgentsService) ChangeQANMongoDBProfilerAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANMongoDBProfilerAgentType, params) if err != nil { return nil, err } @@ -1026,7 +1025,7 @@ func (as *AgentsService) ChangeQANMongoDBMongologAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANMongoDBMongologAgentType, params) if err != nil { return nil, err } @@ -1113,7 +1112,7 @@ func (as *AgentsService) ChangeProxySQLExporter( ConnectionTimeout: duration.OptionalFromProto(p.ConnectionTimeout), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.ProxySQLExporterType, params) if err != nil { return nil, err } @@ -1210,25 +1209,10 @@ func (as *AgentsService) ChangeQANPostgreSQLPgStatementsAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANPostgreSQLPgStatementsAgentType, params) if err != nil { return nil, err } - // Check if we're trying to modify the internal PostgreSQL QAN agent and if the environment variable is set - envVar, exists := os.LookupEnv(env.EnableInternalPgQAN) - if exists && envVar != "" { - a, err := models.FindAgentByID(as.db.Querier, agentID) - if err != nil { - return nil, status.Errorf(codes.NotFound, "agent with ID %q not found", agentID) - } - if pointer.GetString(a.PMMAgentID) == models.PMMServerAgentID { - return nil, status.Errorf( - codes.FailedPrecondition, - "QAN for PMM's internal PostgreSQL server is set to %s via an environment variable.", - envVar, - ) - } - } pgStatementsAgent, ok := agent.(*inventoryv1.QANPostgreSQLPgStatementsAgent) if !ok { @@ -1324,7 +1308,7 @@ func (as *AgentsService) ChangeQANPostgreSQLPgStatMonitorAgent( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.QANPostgreSQLPgStatMonitorAgentType, params) if err != nil { return nil, err } @@ -1409,7 +1393,7 @@ func (as *AgentsService) ChangeRDSExporter(ctx context.Context, agentID string, MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.RDSExporterType, params) if err != nil { return nil, err } @@ -1506,7 +1490,7 @@ func (as *AgentsService) ChangeExternalExporter( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.ExternalExporterType, params) if err != nil { return nil, err } @@ -1593,7 +1577,7 @@ func (as *AgentsService) ChangeAzureDatabaseExporter( MetricsResolutions: convertMetricsResolutions(p.MetricsResolutions), } - agent, err := as.executeAgentChange(ctx, agentID, params) + agent, err := as.executeAgentChange(ctx, agentID, models.AzureDatabaseExporterType, params) if err != nil { return nil, err } @@ -1621,7 +1605,7 @@ func (as *AgentsService) ChangeNomadAgent(ctx context.Context, agentID string, p SkipConnectionCheck: true, } - agent, err := as.executeAgentChange(ctx, agentID, changeParams) + agent, err := as.executeAgentChange(ctx, agentID, models.NomadAgentType, changeParams) if err != nil { return nil, err } @@ -1712,7 +1696,7 @@ func (as *AgentsService) ChangeRTAMongoDBAgent( changeParams.RTAOptions = models.RTAOptionsFromRequest(p.RtaOptions) } - ag, err := as.executeAgentChange(ctx, agentID, changeParams) + ag, err := as.executeAgentChange(ctx, agentID, models.RTAMongoDBAgentType, changeParams) if err != nil { return nil, err } @@ -1768,6 +1752,51 @@ func unexpectedAgentTypeError(agent inventoryv1.Agent) error { return status.Errorf(codes.Internal, "unexpected agent type %T", agent) } +// checkInternalPgQANEnvOverride rejects a request that would flip the enabled state of the QAN agent +// of PMM's internal PostgreSQL server away from the state pinned by the PMM_ENABLE_INTERNAL_PG_QAN +// environment variable. +// +// The agent argument is the stored row, before the requested change is applied. +// +// The guard deliberately stays in the service layer rather than moving into +// models.ApplyAgentChange: Server.handleInternalQANToggle is the legitimate actor for this exact +// state and calls ApplyAgentChange directly, so a guard down there would make the settings API trip +// over its own pin. +func checkInternalPgQANEnvOverride(q *reform.Querier, agent *models.Agent, enable *bool) error { + // Only a request that actually flips the enabled state can contradict the variable. + if enable == nil || *enable == !agent.Disabled { + return nil + } + + // Read before IsInternalPgQANAgent, which costs a query: with the variable unset -- the default + // -- nothing is pinned and there is nothing to check. + enabledByEnv, lookupErr := env.LookupBool(env.EnableInternalPgQAN) + if enabledByEnv == nil && lookupErr == nil { + return nil + } + + internal, err := models.IsInternalPgQANAgent(q, agent) + if err != nil || !internal { + return err + } + + if lookupErr != nil { + // pmm-managed-init rejects an unparsable value before PMM Server starts, so reaching here + // means that validation was bypassed. The intent to pin is clear even though the value is + // not, so refuse rather than silently unpin. + return status.Errorf(codes.FailedPrecondition, "QAN for PMM's internal PostgreSQL server is configured via an environment variable: %s.", lookupErr) + } + if *enable == *enabledByEnv { + return nil + } + + return status.Errorf( + codes.FailedPrecondition, + "QAN for PMM's internal PostgreSQL server is set to %t via an environment variable.", + *enabledByEnv, + ) +} + // Helper function to convert custom labels from protobuf to model format. func convertCustomLabels(customLabels *common.StringMap) *map[string]string { if customLabels != nil { @@ -1814,11 +1843,36 @@ func convertMetricsResolutions(mrs *common.MetricsResolutions) *models.ChangeMet } // Helper function to execute agent change and build response. -func (as *AgentsService) executeAgentChange(ctx context.Context, agentID string, params *models.ChangeAgentParams) (inventoryv1.Agent, error) { //nolint:ireturn +// +// The expectedType argument is the agent type that the calling Change*Agent method knows how to +// convert. The inventory API picks that method from the request payload and not from the type of the +// agent being changed, so a request can name an agent of any type. Checking the type here, inside +// the transaction, turns that into a rejected request; without it the change is committed and only +// then fails the type assertion in the caller, leaving the agent modified, pmm-agent not notified +// and the client with an internal error. +// +// The expectedType argument restates what the caller's own type assertion on the result already says, and the +// compiler cannot tie the two together: keep them in sync, or a valid request becomes InvalidArgument. +func (as *AgentsService) executeAgentChange(ctx context.Context, agentID string, expectedType models.AgentType, params *models.ChangeAgentParams) (inventoryv1.Agent, error) { //nolint:ireturn,lll var agent inventoryv1.Agent err := as.db.InTransactionContext(ctx, nil, func(tx *reform.TX) error { - updatedAgent, err := models.ChangeAgent(tx.Querier, agentID, params) + // Returning an error rolls the transaction back, so a rejected request leaves the agent untouched. + currentAgent, err := models.FindAgentByID(tx.Querier, agentID) + if err != nil { + return err + } + + if currentAgent.AgentType != expectedType { + return status.Errorf(codes.InvalidArgument, "Agent with ID %s has type %s, expected %s.", agentID, currentAgent.AgentType, expectedType) + } + + err = checkInternalPgQANEnvOverride(tx.Querier, currentAgent, params.Enabled) + if err != nil { + return err + } + + updatedAgent, err := models.ApplyAgentChange(tx.Querier, currentAgent, params) if err != nil { return err } diff --git a/managed/services/inventory/agents_test.go b/managed/services/inventory/agents_test.go index 9dd8430d355..665482ae4ff 100644 --- a/managed/services/inventory/agents_test.go +++ b/managed/services/inventory/agents_test.go @@ -17,6 +17,7 @@ package inventory import ( "context" + "fmt" "reflect" "testing" "time" @@ -921,53 +922,281 @@ func TestAgents(t *testing.T) { }) } +// internalPgQANAgent returns the QAN agent of PMM's internal PostgreSQL created by the fixtures. +// It is looked up instead of named by a predicted ID, because the fixture IDs shift as soon as an +// agent is added to models.setupPMMServerAgents ahead of it. +func internalPgQANAgent(t *testing.T, as *AgentsService) *models.Agent { + t.Helper() + + agent, err := models.FindInternalPgQANAgent(as.db.Querier) + require.NoError(t, err) + + return agent +} + func TestChangeQANPostgreSQLPgStatementsAgentWithEnvVar(t *testing.T) { - t.Run("FailWhenEnvVarSet", func(t *testing.T) { + // PMM_ENABLE_INTERNAL_PG_QAN is read twice on a real server: once by the fixtures, which create + // the QAN agent of PMM's internal PostgreSQL enabled or disabled accordingly, and once by the + // guard under test. Subtests therefore decide on the variable before calling setup(t), so that + // the stored state and the pinned state agree the way they do on a server, and so that an + // ambient value in the developer's or CI environment cannot change what the fixtures create. + + t.Run("RejectDisablingWhilePinnedEnabled", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "true") + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + require.False(t, agent.Disabled, "the fixtures create the agent enabled when the variable is true") + + _, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(false), + LogLevel: inventoryv1.LogLevel_LOG_LEVEL_DEBUG.Enum(), + }) + tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, + "QAN for PMM's internal PostgreSQL server is set to true via an environment variable."), err) + + // A rejected request must not leave any of the requested changes behind. + stored, err := models.FindAgentByID(as.db.Querier, agent.AgentID) + require.NoError(t, err) + assert.False(t, stored.Disabled) + assert.Nil(t, stored.LogLevel) + }) + + t.Run("RejectEnablingWhilePinnedDisabled", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "false") _, as, _, teardown, ctx, _ := setup(t) t.Cleanup(func() { teardown(t) }) - // Set the environment variable + agent := internalPgQANAgent(t, as) + require.True(t, agent.Disabled) + + _, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(true), + LogLevel: inventoryv1.LogLevel_LOG_LEVEL_DEBUG.Enum(), + }) + tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, + "QAN for PMM's internal PostgreSQL server is set to false via an environment variable."), err) + + stored, err := models.FindAgentByID(as.db.Querier, agent.AgentID) + require.NoError(t, err) + assert.True(t, stored.Disabled) + assert.Nil(t, stored.LogLevel) + }) + + t.Run("AllowRequestThatDoesNotChangeTheEnabledState", func(t *testing.T) { + // Nothing reconciles the stored state with the variable after the fixtures ran, so an + // operator who adds PMM_ENABLE_INTERNAL_PG_QAN=true to an existing installation ends up with + // a disabled agent and a variable that says enabled. A request echoing the current state + // back, as a UI PATCH does, changes nothing and must not take unrelated parameters down + // with it. + tests.UnsetEnv(t, env.EnableInternalPgQAN) + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + require.True(t, agent.Disabled) + t.Setenv(env.EnableInternalPgQAN, "true") + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID) - // Try to change the internal PostgreSQL QAN agent (pmm-server's agent) - // The agent with ID "00000000-0000-4000-8000-000000000004" is the internal PostgreSQL QAN agent - _, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, "00000000-0000-4000-8000-000000000004", &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ - Enable: new(false), + changed, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(false), + LogLevel: inventoryv1.LogLevel_LOG_LEVEL_DEBUG.Enum(), }) + require.NoError(t, err) + assert.True(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) + assert.Equal(t, inventoryv1.LogLevel_LOG_LEVEL_DEBUG, changed.GetQanPostgresqlPgstatementsAgent().LogLevel) + }) - // Expect a FailedPrecondition error - tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, "QAN for PMM's internal PostgreSQL server is set to true via an environment variable."), err) + t.Run("AllowMovingTowardsThePinnedState", func(t *testing.T) { + tests.UnsetEnv(t, env.EnableInternalPgQAN) + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + require.True(t, agent.Disabled) + + t.Setenv(env.EnableInternalPgQAN, "true") + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID) + + // The variable pins a state, it does not forbid reaching it. + changed, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(true), + }) + require.NoError(t, err) + assert.False(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) }) - t.Run("SucceedWhenEnvVarNotSet", func(t *testing.T) { + t.Run("AllowParametersUnrelatedToTheEnabledState", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "true") _, as, _, teardown, ctx, _ := setup(t) t.Cleanup(func() { teardown(t) }) - // Ensure the environment variable is not set - // (It shouldn't be set by default, but we explicitly unset it to be safe) - t.Setenv(env.EnableInternalPgQAN, "") + agent := internalPgQANAgent(t, as) + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID) - // Mock the state update request - as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, "pmm-server") + changed, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + LogLevel: inventoryv1.LogLevel_LOG_LEVEL_DEBUG.Enum(), + MaxQueryLength: new(int32(2048)), + }) + require.NoError(t, err) + assert.Equal(t, inventoryv1.LogLevel_LOG_LEVEL_DEBUG, changed.GetQanPostgresqlPgstatementsAgent().LogLevel) + assert.Equal(t, int32(2048), changed.GetQanPostgresqlPgstatementsAgent().MaxQueryLength) + assert.False(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) + }) + + t.Run("RejectRequestThroughParamsOfAnotherAgentType", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "true") + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + + // The inventory API picks the Change*Agent method from the request payload and not from the + // type of the agent being changed, so any of them can be pointed at the internal QAN agent. + // Such a request is refused outright, before the change is applied. + _, err := as.ChangeQANPostgreSQLPgStatMonitorAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatMonitorAgentParams{ + Enable: new(false), + LogLevel: inventoryv1.LogLevel_LOG_LEVEL_DEBUG.Enum(), + }) + tests.AssertGRPCError(t, status.New(codes.InvalidArgument, fmt.Sprintf("Agent with ID %s has type %s, expected %s.", + agent.AgentID, models.QANPostgreSQLPgStatementsAgentType, models.QANPostgreSQLPgStatMonitorAgentType)), err) + + stored, err := models.FindAgentByID(as.db.Querier, agent.AgentID) + require.NoError(t, err) + assert.False(t, stored.Disabled) + assert.Nil(t, stored.LogLevel) + }) - // Try to change the internal PostgreSQL QAN agent - agent, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, "00000000-0000-4000-8000-000000000004", &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + t.Run("AllowRemotePostgreSQLUnderPMMServersAgent", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "true") + ss, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + // RDS and Azure discovery attach the QAN agent of a remote PostgreSQL instance to PMM + // Server's own pmm-agent, so agent type and pmm-agent alone do not identify the internal + // QAN agent. QAN for such an instance has nothing to do with PMM's own database and stays + // switchable while the variable is set. + ps, err := ss.AddPostgreSQL(ctx, &models.AddDBMSServiceParams{ + ServiceName: "remote-postgres", + NodeID: models.PMMServerNodeID, + Address: new("127.0.0.1"), + Port: new(uint16(5432)), + }) + require.NoError(t, err) + + // One state update for adding the QAN agent, another one for changing it. + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID).Times(2) + + added, err := as.AddQANPostgreSQLPgStatementsAgent(ctx, &inventoryv1.AddQANPostgreSQLPgStatementsAgentParams{ + PmmAgentId: models.PMMServerAgentID, + ServiceId: ps.ServiceId, + Username: "username", + SkipConnectionCheck: true, + }) + require.NoError(t, err) + + changed, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, added.GetQanPostgresqlPgstatementsAgent().AgentId, + &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(false), + }) + require.NoError(t, err) + assert.True(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) + }) + + t.Run("AllowOtherAgentTypesOfPMMServer", func(t *testing.T) { + t.Setenv(env.EnableInternalPgQAN, "true") + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + // The postgres_exporter of PMM's internal PostgreSQL, running under the same pmm-agent as + // the QAN agent that the variable pins. The variable covers QAN only. + pgExporters, err := models.FindAgents(as.db.Querier, models.AgentFilters{ + PMMAgentID: models.PMMServerAgentID, + AgentType: new(models.PostgresExporterType), + }) + require.NoError(t, err) + require.Len(t, pgExporters, 1) + + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID) + + changed, err := as.ChangePostgresExporter(ctx, pgExporters[0].AgentID, &inventoryv1.ChangePostgresExporterParams{ Enable: new(false), }) + require.NoError(t, err) + assert.True(t, changed.GetPostgresExporter().Disabled) + }) + + t.Run("RejectWhenValueIsNotABool", func(t *testing.T) { + // pmm-managed-init refuses to start PMM Server on an unparsable value, so reaching the guard + // with one means that validation was bypassed. The intent to pin the state is clear even + // though the value is not, so the change is refused rather than silently unpinned. + t.Setenv(env.EnableInternalPgQAN, "not-a-bool") + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + require.True(t, agent.Disabled) + + _, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(true), + }) + tests.AssertGRPCError(t, status.New(codes.FailedPrecondition, + `QAN for PMM's internal PostgreSQL server is configured via an environment variable: `+ + `invalid value "not-a-bool" for environment variable PMM_ENABLE_INTERNAL_PG_QAN.`), err) - // Should succeed + stored, err := models.FindAgentByID(as.db.Querier, agent.AgentID) require.NoError(t, err) - assert.True(t, agent.GetQanPostgresqlPgstatementsAgent().Disabled) + assert.True(t, stored.Disabled) + }) - // Change it back to enabled - as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, "pmm-server") - agent, err = as.ChangeQANPostgreSQLPgStatementsAgent(ctx, "00000000-0000-4000-8000-000000000004", &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + t.Run("AllowAnyChangeWhenVariableNotSet", func(t *testing.T) { + tests.UnsetEnv(t, env.EnableInternalPgQAN) + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + agent := internalPgQANAgent(t, as) + as.state.(*mockAgentsStateUpdater).On("RequestStateUpdate", ctx, models.PMMServerAgentID).Times(2) + + changed, err := as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ Enable: new(true), }) + require.NoError(t, err) + assert.False(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) + + changed, err = as.ChangeQANPostgreSQLPgStatementsAgent(ctx, agent.AgentID, &inventoryv1.ChangeQANPostgreSQLPgStatementsAgentParams{ + Enable: new(false), + }) + require.NoError(t, err) + assert.True(t, changed.GetQanPostgresqlPgstatementsAgent().Disabled) + }) +} + +func TestChangeAgentRejectsAgentOfAnotherType(t *testing.T) { + t.Run("PostgresExporterThroughNodeExporterParams", func(t *testing.T) { + _, as, _, teardown, ctx, _ := setup(t) + t.Cleanup(func() { teardown(t) }) + + // Pointing a Change*Agent method at an agent of a different type used to commit the change + // and then fail the type assertion in the caller, so the agent ended up modified, pmm-agent + // was never told and the client got codes.Internal. The request is now refused instead. + pgExporters, err := models.FindAgents(as.db.Querier, models.AgentFilters{ + PMMAgentID: models.PMMServerAgentID, + AgentType: new(models.PostgresExporterType), + }) + require.NoError(t, err) + require.Len(t, pgExporters, 1) + + _, err = as.ChangeNodeExporter(ctx, pgExporters[0].AgentID, &inventoryv1.ChangeNodeExporterParams{ + Enable: new(false), + }) + tests.AssertGRPCError(t, status.New(codes.InvalidArgument, fmt.Sprintf("Agent with ID %s has type %s, expected %s.", + pgExporters[0].AgentID, models.PostgresExporterType, models.NodeExporterType)), err) - // Should succeed + stored, err := models.FindAgentByID(as.db.Querier, pgExporters[0].AgentID) require.NoError(t, err) - assert.False(t, agent.GetQanPostgresqlPgstatementsAgent().Disabled) + assert.False(t, stored.Disabled) }) } diff --git a/managed/services/server/server.go b/managed/services/server/server.go index 60d885b2c17..328bdd60be2 100644 --- a/managed/services/server/server.go +++ b/managed/services/server/server.go @@ -421,7 +421,7 @@ func (s *Server) GetSettings(ctx context.Context, _ *serverv1.GetSettingsRequest // In HA mode, internal QAN is always disabled as PostgreSQL is external disabledInternalPgQan = true } else { - internalPgQanAgent, err := s.getInternalPgQANAgent(dbCtx) + internalPgQanAgent, err := models.FindInternalPgQANAgent(dbCtx) if err != nil { // if we can't get the agent, log the error and set it to disabled. s.l.Errorf("failed to get internal pgQAN agent: %v", err) @@ -630,20 +630,6 @@ func (s *Server) ChangeSettings(ctx context.Context, req *serverv1.ChangeSetting }, nil } -func (s *Server) getInternalPgQANAgent(q *reform.Querier) (*models.Agent, error) { - agents, err := models.FindAgents(q, models.AgentFilters{ - PMMAgentID: models.PMMServerAgentID, - AgentType: new(models.QANPostgreSQLPgStatementsAgentType), - }) - if err != nil { - return nil, fmt.Errorf("failed to find agents: %w", err) - } - if len(agents) == 0 { - return nil, errors.New("internal pgQAN agent not found") - } - return agents[0], nil -} - func (s *Server) handleInternalQANToggle(ctx context.Context, q *reform.Querier, enableInternalPgQan *bool) (bool, error) { if s.haService.Params().Enabled { if *enableInternalPgQan { @@ -654,15 +640,12 @@ func (s *Server) handleInternalQANToggle(ctx context.Context, q *reform.Querier, return true, nil } - internalQanAgent, err := s.getInternalPgQANAgent(q) + internalQanAgent, err := models.FindInternalPgQANAgent(q) if err != nil { return false, fmt.Errorf("failed to get QAN agent: %w", err) } - if internalQanAgent == nil { - return false, errors.New("internal QAN agent not found") - } - newAgent, err := models.ChangeAgent(q, internalQanAgent.AgentID, &models.ChangeAgentParams{ + newAgent, err := models.ApplyAgentChange(q, internalQanAgent, &models.ChangeAgentParams{ Enabled: enableInternalPgQan, }) if err != nil { diff --git a/managed/utils/env/env.go b/managed/utils/env/env.go index 5e641cdcaef..e6b46cf57e5 100644 --- a/managed/utils/env/env.go +++ b/managed/utils/env/env.go @@ -17,6 +17,7 @@ package env import ( + "fmt" "os" "strconv" "strings" @@ -48,19 +49,36 @@ const ( ClickHouseConfig = "PMM_CLICKHOUSE_CONFIG" ) -// GetBool returns the boolean value of the environment variable. -// Returns false if the variable is not set or cannot be parsed as boolean. -// It does not return errors since it assumes that validation has already been done during startup. -func GetBool(key string) bool { +// LookupBool returns the boolean value of the environment variable. It tells the three states +// of an optional boolean apart: +// - (nil, nil) when the variable is not set, +// - (value, nil) when it holds a boolean, +// - (nil, error) when it is set to something that is not a boolean. +// +// The error lets a caller decide what an unparsable value means for it, instead of having that +// choice made here. The envvars.ParseEnvVars parser reports the same values as configuration +// errors, and pmm-managed-init refuses to start PMM Server when it does. +func LookupBool(key string) (*bool, error) { v, ok := os.LookupEnv(key) if !ok { - return false + return nil, nil //nolint:nilnil } b, err := strconv.ParseBool(v) if err != nil { + return nil, fmt.Errorf("invalid value %q for environment variable %s", v, key) + } + return &b, nil +} + +// GetBool returns the boolean value of the environment variable. +// Returns false if the variable is not set or cannot be parsed as boolean. +// It does not return errors since it assumes that validation has already been done during startup. +func GetBool(key string) bool { + b, _ := LookupBool(key) + if b == nil { return false } - return b + return *b } // GetStringSlice returns the string slice value of the environment variable. diff --git a/managed/utils/env/env_test.go b/managed/utils/env/env_test.go index 4768e6f8a11..c9f0fb4b763 100644 --- a/managed/utils/env/env_test.go +++ b/managed/utils/env/env_test.go @@ -19,8 +19,81 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestLookupBool(t *testing.T) { + tests := []struct { + name string + envValue string + set bool + expected *bool + wantErr bool + }{ + { + name: "not set", + expected: nil, + }, + { + name: "true", + envValue: "true", + set: true, + expected: new(true), + }, + { + name: "false", + envValue: "false", + set: true, + expected: new(false), + }, + { + name: "1", + envValue: "1", + set: true, + expected: new(true), + }, + { + name: "0", + envValue: "0", + set: true, + expected: new(false), + }, + { + // Set but empty is a misconfiguration, not the same as unset: envvars.ParseEnvVars + // reports it as a configuration error too. + name: "set to empty string", + envValue: "", + set: true, + wantErr: true, + }, + { + name: "not a boolean", + envValue: "yes", + set: true, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + const key = "TEST_LOOKUP_BOOL" + if tt.set { + t.Setenv(key, tt.envValue) + } + + result, err := LookupBool(key) + if tt.wantErr { + require.Error(t, err) + assert.Nil(t, result) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestGetBool(t *testing.T) { tests := []struct { name string diff --git a/managed/utils/tests/env.go b/managed/utils/tests/env.go new file mode 100644 index 00000000000..c9e0b40f6c7 --- /dev/null +++ b/managed/utils/tests/env.go @@ -0,0 +1,42 @@ +// Copyright (C) 2023 Percona LLC +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package tests + +import ( + "os" + "testing" +) + +// UnsetEnv removes an environment variable for the duration of the test, restoring it afterwards. +// +// There is no counterpart to t.Setenv for unsetting, and setting a variable to an empty string is +// not the same thing: envvars.ParseEnvVars rejects an empty boolean as a configuration error, so a +// test that needs the "not set at all" case has to actually unset it. +func UnsetEnv(t *testing.T, key string) { + t.Helper() + + value, ok := os.LookupEnv(key) + if !ok { + return + } + + // Registers the cleanup that puts the original value back, then drops it for this test. + t.Setenv(key, value) + err := os.Unsetenv(key) + if err != nil { + t.Fatalf("failed to unset %s: %s", key, err) + } +}