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

fix: Add option to select translator #10802

Merged
merged 14 commits into from
Mar 18, 2022
Merged

fix: Add option to select translator #10802

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
2796f50
fix: Add option to select translator
reimda Mar 10, 2022
5f73405
Clean up and disable linter
reimda Mar 10, 2022
5c8e2ea
use translator in ifname processor
reimda Mar 11, 2022
6f664a5
Use exported gosmi translator constructor
reimda Mar 14, 2022
dac215c
disable linter
reimda Mar 14, 2022
c51e199
Add translator setting to snmp_trap plugin
reimda Mar 14, 2022
bf61b86
Add missing file
reimda Mar 15, 2022
cdaadd9
gofmt
reimda Mar 15, 2022
997ad84
Remove unused test functions
reimda Mar 15, 2022
8c83ee8
ignore confusing naming linter warning
reimda Mar 15, 2022
8c01814
Switch to agent level translator setting instead of per-plugin setting
reimda Mar 17, 2022
73bb2dd
Add missing file
reimda Mar 17, 2022
83929a5
fake change to trigger github after its outage
reimda Mar 17, 2022
a3a4c1c
Use switch to make the linter happy
reimda Mar 17, 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
5 changes: 5 additions & 0 deletions agent/agent.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/influxdata/telegraf"
"github.com/influxdata/telegraf/config"
"github.com/influxdata/telegraf/internal"
"github.com/influxdata/telegraf/internal/snmp"
"github.com/influxdata/telegraf/models"
"github.com/influxdata/telegraf/plugins/serializers/influx"
)
Expand Down Expand Up @@ -186,6 +187,10 @@ func (a *Agent) Run(ctx context.Context) error {
// initPlugins runs the Init function on plugins.
func (a *Agent) initPlugins() error {
for _, input := range a.Config.Inputs {
// Share the snmp translator setting with plugins that need it.
if tp, ok := input.Input.(snmp.TranslatorPlugin); ok {
tp.SetTranslator(a.Config.Agent.Translator)
}
err := input.Init()
if err != nil {
return fmt.Errorf("could not initialize input %s: %v",
Expand Down
12 changes: 12 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,8 @@ type AgentConfig struct {

Hostname string
OmitHostname bool

Translator string `toml:"translator"`
}

// InputNames returns a list of strings of the configured inputs.
Expand Down Expand Up @@ -418,6 +420,11 @@ var agentConfig = `
hostname = ""
## If set to true, do no set the "host" tag in the telegraf agent.
omit_hostname = false

## Translator for SNMP OIDs
## Valid values are "netsnmp" which runs snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library.
Hipska marked this conversation as resolved.
Show resolved Hide resolved
# translator = "netsnmp"
`

var outputHeader = `
Expand Down Expand Up @@ -855,6 +862,11 @@ func (c *Config) LoadConfigData(data []byte) error {
c.Tags["host"] = c.Agent.Hostname
}

// Set snmp agent translator default
if c.Agent.Translator == "" {
c.Agent.Translator = "netsnmp"
}

if len(c.UnusedFields) > 0 {
return fmt.Errorf("line %d: configuration specified the fields %q, but they weren't used", tbl.Line, keys(c.UnusedFields))
}
Expand Down
2 changes: 1 addition & 1 deletion internal/snmp/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ type ClientConfig struct {
// Path to mib files
Path []string `toml:"path"`
// Translator implementation
Translator string `toml:"translator"`
Translator string `toml:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed to be exported.

Suggested change
Translator string `toml:"-"`
translatorType string


// Parameters for Version 1 & 2
Community string `toml:"community"`
Expand Down
7 changes: 2 additions & 5 deletions plugins/inputs/snmp/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@ path onto the global path variable
## SNMP version; can be 1, 2, or 3.
# version = 2

## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"

## Path to mib files
## Used by the gosmi translator.
## To add paths when translating with netsnmp, use the MIBDIRS environment variable
# path = ["/usr/share/snmp/mibs"]

## SNMP community string.
Expand Down
18 changes: 9 additions & 9 deletions plugins/inputs/snmp/snmp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,9 @@ const sampleConfig = `
## SNMP version; can be 1, 2, or 3.
# version = 2

## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"

## Path to mib files
## Only used with GoSMI translator
## Used by the gosmi translator.
## To add paths when translating with netsnmp, use the MIBDIRS environment variable
Hipska marked this conversation as resolved.
Show resolved Hide resolved
# path = ["/usr/share/snmp/mibs"]

## Agent host tag; the tag used to reference the source host
Expand Down Expand Up @@ -120,16 +116,21 @@ type Snmp struct {
translator Translator
}

func (s *Snmp) SetTranslator(name string) {
s.Translator = name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about calling this InitTranslator and initiating the translator, returning error on failure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is kind of an ugly hack to share the agent's translator setting with the plugins. I didn't want to add more error handling code to the agent to handle an error where it calls this function. There's already a well defined way for inputs to report config errors to the agent- Init func's return value.


func (s *Snmp) Init() error {
var err error

if s.Translator == "gosmi" {
s.translator, err = NewGosmiTranslator(s.Path, s.Log)
if err != nil {
return err
}
} else {
} else if s.Translator == "netsnmp" {
s.translator = NewNetsnmpTranslator()
} else {
return fmt.Errorf("invalid translator value")
}

s.connectionCache = make([]snmpConnection, len(s.Agents))
Expand Down Expand Up @@ -356,7 +357,6 @@ func init() {
Timeout: config.Duration(5 * time.Second),
Version: 2,
Path: []string{"/usr/share/snmp/mibs"},
Translator: "netsnmp",
Community: "public",
},
}
Expand Down
27 changes: 22 additions & 5 deletions plugins/inputs/snmp/snmp_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,6 @@ func TestSampleConfig(t *testing.T) {
Community: "public",
MaxRepetitions: 10,
Retries: 3,
Translator: "netsnmp",
},
Name: "snmp",
}
Expand Down Expand Up @@ -179,6 +178,9 @@ func TestSnmpInit(t *testing.T) {
Fields: []Field{
{Oid: "TEST::hostname"},
},
ClientConfig: snmp.ClientConfig{
Translator: "netsnmp",
},
reimda marked this conversation as resolved.
Show resolved Hide resolved
}

err := s.Init()
Expand Down Expand Up @@ -218,6 +220,9 @@ func TestSnmpInit_noTranslate(t *testing.T) {
{Oid: ".1.1.1.6"},
}},
},
ClientConfig: snmp.ClientConfig{
Translator: "netsnmp",
},
}

err := s.Init()
Expand Down Expand Up @@ -267,10 +272,11 @@ func TestGetSNMPConnection_v2(t *testing.T) {
s := &Snmp{
Agents: []string{"1.2.3.4:567", "1.2.3.4", "udp://127.0.0.1"},
ClientConfig: snmp.ClientConfig{
Timeout: config.Duration(3 * time.Second),
Retries: 4,
Version: 2,
Community: "foo",
Timeout: config.Duration(3 * time.Second),
Retries: 4,
Version: 2,
Community: "foo",
Translator: "netsnmp",
},
}
err := s.Init()
Expand Down Expand Up @@ -308,6 +314,9 @@ func TestGetSNMPConnectionTCP(t *testing.T) {

s := &Snmp{
Agents: []string{"tcp://127.0.0.1:56789"},
ClientConfig: snmp.ClientConfig{
Translator: "netsnmp",
},
}
err := s.Init()
require.NoError(t, err)
Expand Down Expand Up @@ -348,6 +357,7 @@ func TestGetSNMPConnection_v3(t *testing.T) {
EngineID: "myengineid",
EngineBoots: 1,
EngineTime: 2,
Translator: "netsnmp",
},
}
err := s.Init()
Expand Down Expand Up @@ -396,6 +406,7 @@ func TestGetSNMPConnection_v3_blumenthal(t *testing.T) {
EngineID: "myengineid",
EngineBoots: 1,
EngineTime: 2,
Translator: "netsnmp",
},
},
},
Expand All @@ -417,6 +428,7 @@ func TestGetSNMPConnection_v3_blumenthal(t *testing.T) {
EngineID: "myengineid",
EngineBoots: 1,
EngineTime: 2,
Translator: "netsnmp",
},
},
},
Expand All @@ -438,6 +450,7 @@ func TestGetSNMPConnection_v3_blumenthal(t *testing.T) {
EngineID: "myengineid",
EngineBoots: 1,
EngineTime: 2,
Translator: "netsnmp",
},
},
},
Expand All @@ -459,6 +472,7 @@ func TestGetSNMPConnection_v3_blumenthal(t *testing.T) {
EngineID: "myengineid",
EngineBoots: 1,
EngineTime: 2,
Translator: "netsnmp",
},
},
},
Expand Down Expand Up @@ -494,6 +508,9 @@ func TestGetSNMPConnection_v3_blumenthal(t *testing.T) {
func TestGetSNMPConnection_caching(t *testing.T) {
s := &Snmp{
Agents: []string{"1.2.3.4", "1.2.3.5", "1.2.3.5"},
ClientConfig: snmp.ClientConfig{
Translator: "netsnmp",
},
}
err := s.Init()
require.NoError(t, err)
Expand Down
7 changes: 2 additions & 5 deletions plugins/inputs/snmp_trap/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,9 @@ path onto the global path variable
##
# service_address = "udp://:162"
##
## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"
##
## Path to mib files
## Used by the gosmi translator.
## To add paths when translating with netsnmp, use the MIBDIRS environment variable
# path = ["/usr/share/snmp/mibs"]
##
## Deprecated in 1.20.0; no longer running snmptranslate
Expand Down
17 changes: 9 additions & 8 deletions plugins/inputs/snmp_trap/snmp_trap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type SnmpTrap struct {
ServiceAddress string `toml:"service_address"`
Timeout config.Duration `toml:"timeout" deprecated:"1.20.0;unused option"`
Version string `toml:"version"`
Translator string `toml:"translator"`
Translator string `toml:"-"`
Path []string `toml:"path"`

// Settings for version 3
Expand Down Expand Up @@ -59,11 +59,6 @@ var sampleConfig = `
##
# service_address = "udp://:162"
##
## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"
##
## Path to mib files
# path = ["/usr/share/snmp/mibs"]
##
Expand Down Expand Up @@ -104,21 +99,27 @@ func init() {
ServiceAddress: "udp://:162",
Path: []string{"/usr/share/snmp/mibs"},
Version: "2c",
Translator: "netsnmp",
}
})
}

func (s *SnmpTrap) SetTranslator(name string) {
s.Translator = name
}

func (s *SnmpTrap) Init() error {
var err error
if s.Translator == "gosmi" {
s.translator, err = newGosmiTranslator(s.Path, s.Log)
if err != nil {
return err
}
} else {
} else if s.Translator == "netsnmp" {
s.translator = newNetsnmpTranslator()
} else {
return fmt.Errorf("invalid translator value")
}

if err != nil {
s.Log.Errorf("Could not get path %v", err)
}
Expand Down
1 change: 1 addition & 0 deletions plugins/inputs/snmp_trap/snmp_trap_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1286,6 +1286,7 @@ func TestReceiveTrap(t *testing.T) {
AuthPassword: tt.authPass,
PrivProtocol: tt.privProto,
PrivPassword: tt.privPass,
Translator: "netsnmp",
}

require.NoError(t, s.Init())
Expand Down
5 changes: 0 additions & 5 deletions plugins/processors/ifname/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@ Telegraf minimum version: Telegraf 1.15.0
## SNMP version; can be 1, 2, or 3.
# version = 2

## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"

## SNMP community string.
# community = "public"

Expand Down
17 changes: 3 additions & 14 deletions plugins/processors/ifname/ifname.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,6 @@ var sampleConfig = `
## SNMP version; can be 1, 2, or 3.
# version = 2

## Translator for OIDs
## Valid values are "netsnmp" which calls snmptranslate and snmptable,
## and "gosmi" which uses the gosmi library
# translator = "netsnmp"

## SNMP community string.
# community = "public"

Expand Down Expand Up @@ -139,15 +134,9 @@ func (d *IfName) Init() error {
return fmt.Errorf("parsing SNMP client config: %w", err)
}

if d.Translator == "gosmi" {
var err error
d.translator, err = si.NewGosmiTranslator(d.Path, d.Log)
if err != nil {
return err
}
} else {
d.translator = si.NewNetsnmpTranslator()
}
// Since OIDs in this plugin are always numeric there is no need
// to translate.
d.translator = si.NewNetsnmpTranslator()
Comment on lines +137 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

Then why initiating it anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because it needs to be something. I don't expect it to be used. We should consider changing ifname so it doesn't call table Init() at all. Then we won't need a translator at all.

Copy link
Contributor

@Hipska Hipska Mar 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As said before, better would be the table.Init not expect a translator as argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about that @reimda?

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 don't see how we can make table.Init() not expect a translator. Here in ifname, Init() shouldn't need to look up anything in the mibs because we only use numeric oids, but in the main use case in the snmp input allows names so it does need to use mibs.

Maybe here in ifname we should change the code to manually initialize the table and then not call Init() at all. That would probably mean removing makeTableNoMock() completely and changing makeTable() to fill in all the fields of snmp.Table that table.Init() would.

Copy link
Contributor

@Hipska Hipska Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Easiest way would be by adding translator to the struct. But don't know if that is a good thing memory-wise.

Edit: It looks like the call of Init shouldn't even be needed, as we already initialise all necessary parameters. The only problem remaining would be the call of Table.Build that expects a translator, but it would not be called in case of ifName plugin usage, so I'm wondering if you can just pass a null value?


return nil
}
Expand Down