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

refactor: snmp to use gosmi #9518

Merged
merged 75 commits into from
Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from 73 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
2541236
working on gosmi for fields
MyaLongmire Jul 19, 2021
9e20fd0
added comments for next steps
MyaLongmire Jul 20, 2021
ae8fcb3
added mixed letters and numbers
MyaLongmire Jul 21, 2021
2557a8c
more mixed oid support
MyaLongmire Jul 23, 2021
efd12fa
refactor: use gosmi instead of snmptranslate and snmptable
MyaLongmire Aug 20, 2021
d12fd8f
rework: table and fields init finction to point back to parent
MyaLongmire Aug 26, 2021
ac2ccaf
rework: got conversions working
MyaLongmire Aug 30, 2021
5f577a7
added test mib files
MyaLongmire Sep 10, 2021
2ec48c3
rework: unit tests
MyaLongmire Sep 15, 2021
a571d66
refactor: remove tablecache and lock
MyaLongmire Sep 15, 2021
d1097ee
fix: linter fixes
MyaLongmire Sep 15, 2021
e2f2c67
rework: update readme
MyaLongmire Sep 21, 2021
cb7f98a
Delete telegraf.conf
MyaLongmire Sep 16, 2021
e46f8f9
fix: spacing in sample config
Sep 23, 2021
f02f01d
fix: table test init test reworked
MyaLongmire Oct 11, 2021
aa6e5b2
fix: added more mibs for tests
MyaLongmire Oct 11, 2021
f90d915
fix: trying to get mib files working
MyaLongmire Oct 11, 2021
a6fc198
fix: removes unused code
MyaLongmire Oct 12, 2021
4ad5668
fix: remove name from noTranslate test
MyaLongmire Oct 12, 2021
345e591
fix: renaming
MyaLongmire Oct 12, 2021
bdd7029
fix: changed translate error to warning so telegraf can keep running,…
MyaLongmire Oct 12, 2021
9ee6056
fix: call getMibsPath only once and start reqorking testTableBuild
MyaLongmire Oct 12, 2021
f0abade
fix: changed tableBuildWalk test to use a real mib added oids to inte…
MyaLongmire Oct 12, 2021
233fe36
chore: removed leftover print statements
MyaLongmire Oct 12, 2021
ea369d3
chore: removed leftover print statements
MyaLongmire Oct 12, 2021
8ee0c33
fis: trailing name in test
MyaLongmire Oct 12, 2021
ad6712e
fix: added .9 in front of oids in testSnmpInit_noTranslate so they do…
MyaLongmire Oct 12, 2021
94fcfb1
Remove tests we added back during rebase
reimda Oct 13, 2021
61a06fb
fix: removed isTag true from one field of test table init
MyaLongmire Oct 13, 2021
a4344cd
fix: split testTableBuild_walk into two tests. One has the original b…
MyaLongmire Oct 13, 2021
861c212
fix: added the lock back in as gosmi is not thread safe
MyaLongmire Oct 13, 2021
18684c8
chore: moved revision to snmp_v2 plugin and restored the original snm…
MyaLongmire Oct 19, 2021
44263e1
fix: change warning back to err but do not return an error if the oid…
MyaLongmire Oct 19, 2021
200e0e8
fix: move back to snmp
MyaLongmire Nov 15, 2021
71fc43f
fix: remove error checking that never results in an error
MyaLongmire Nov 15, 2021
e7cd245
fix: remove extra comment
MyaLongmire Nov 15, 2021
618cfe1
fix: made TestSnmpInit use a real table
MyaLongmire Nov 15, 2021
74a37c0
fix: unit tests
MyaLongmire Nov 17, 2021
090ca03
fix: server unit tests
MyaLongmire Nov 17, 2021
d94ebc3
fix: flipping line order to keep orginiality
MyaLongmire Nov 17, 2021
fb42465
fix: flipping line order to keep orginiality
MyaLongmire Nov 17, 2021
5a1e57a
fix: fix spelling
MyaLongmire Nov 17, 2021
7315bb7
fix: make the linter happy
MyaLongmire Nov 17, 2021
b8309e7
fix: linter issues
MyaLongmire Nov 17, 2021
1423563
fix: added 9s back to testsnmpinit_notranslate
MyaLongmire Nov 17, 2021
f9b01f9
feat: update readme
MyaLongmire Nov 18, 2021
46c7d9e
fix: remove error return type from getmibspath
MyaLongmire Nov 18, 2021
7bac3e9
fix: readme linter errors
MyaLongmire Nov 18, 2021
33132e0
fix: errors to warns
MyaLongmire Nov 18, 2021
990521b
fix: use index/value pair directly
MyaLongmire Nov 19, 2021
c6663d7
fix: spelling
MyaLongmire Nov 19, 2021
a7b0b99
fix: use index/value pair directly
MyaLongmire Nov 19, 2021
904ef43
fix: readme and linter
MyaLongmire Nov 19, 2021
13677f3
fix: remove submask
MyaLongmire Nov 19, 2021
611bad0
fix: added comments to getmibspath
MyaLongmire Nov 19, 2021
d356f14
chore: removed leftover comment
MyaLongmire Nov 19, 2021
3e6f1b1
fix: changed filepath to error
Nov 23, 2021
2cf2fed
fix: filepath return an error
Nov 23, 2021
295d41b
chore: merge master to resolve conflicts
Nov 23, 2021
9445d02
fix: default path
Nov 24, 2021
054aafd
chore: readme markdown
MyaLongmire Nov 29, 2021
a77e8a3
Merge branch 'master' into snmp-gosmi-rework
MyaLongmire Nov 29, 2021
cddb46d
chore: marjdown linter fixes
MyaLongmire Nov 29, 2021
5d241b5
move getmibspath to internal and append separate paths onto global pa…
MyaLongmire Nov 29, 2021
eec127c
refactor: move all gosmi code from snmp_trap into internal
MyaLongmire Nov 29, 2021
c7257bb
refactor: move gosmi into snmp internal and out of snmp
MyaLongmire Nov 29, 2021
7701f3f
chore: added keys to composite literals
MyaLongmire Nov 29, 2021
d3d973e
chore: mute linter on too many return variables
MyaLongmire Nov 29, 2021
45a424f
fix: initialise nil map
MyaLongmire Nov 29, 2021
d37ae61
chore: renaming and docs
MyaLongmire Nov 30, 2021
194416e
refactor: clean up test
MyaLongmire Nov 30, 2021
2167aac
chore: more doc clarity
MyaLongmire Nov 30, 2021
92d3eef
chore: remove leftover comments
MyaLongmire Nov 30, 2021
4beceeb
Merge branch 'master' into snmp-gosmi-rework
MyaLongmire Nov 30, 2021
890ea6d
fix: added locking
MyaLongmire Nov 30, 2021
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
2 changes: 2 additions & 0 deletions internal/snmp/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ type ClientConfig struct {
Retries int `toml:"retries"`
// Values: 1, 2, 3
Version uint8 `toml:"version"`
// Path to mib files
Path []string `toml:"path"`

// Parameters for Version 1 & 2
Community string `toml:"community"`
Expand Down
183 changes: 183 additions & 0 deletions internal/snmp/translate.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,183 @@
package snmp

import (
"fmt"
"os"
"path/filepath"
"strings"

"github.com/influxdata/telegraf"
"github.com/sleepinggenius2/gosmi"
"github.com/sleepinggenius2/gosmi/types"
)

// must init, append path for each directory, load module for every file
// or gosmi will fail without saying why
func LoadMibsFromPath(paths []string, log telegraf.Logger) error {
gosmi.Init()
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS, this function is called from each inputs.snmp plugin. Is this correct? If so, this means that you call gosmi.Init() multiple time with gosmi having a global state if my interpretation is correct...

If this is the case, chances are high that you get side-effects between the different plugin-instances potentially ranging from race-conditions when adding the modules to mutual overwriting of the loaded plugins when Init() is called multiple times... To reduce those effects you should init gosmi only once (using sync.Once) and guard this loading function with a lock. I don't know SNMP, but let's hope that there are no collisions possible between multiple (contradicting) mib-definitions...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do call gosmi.Init more than once but the library has a built-in check to see if it has already been initialized or not.

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 have also added a mutex

Copy link
Member

@srebhan srebhan Dec 1, 2021

Choose a reason for hiding this comment

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

Hey @MyaLongmire, I don't see any check... When calling gosmi.Init() it calls this function which in turn calls smi.Init() that can be found here. Already at this point, multiple files are reread etc. Additionally internal.Init() is called also modifying data. While I agree that currently nothing "critical" is changed there it won't hurt to be on the safe side by

Suggested change
gosmi.Init()
once.Do(gosmi.Init)

and place var once sync.Once in the package scope, to be sure we call that function only once at least in that package. It would be even better to make sure it's not called elsewhere, but that's probably out of scope for this PR...

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 added in pr 10199

Copy link
Contributor

Choose a reason for hiding this comment

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

gosmi.Init() always calls smi.Init("gosmi") which always calls internal.Init(handleName) with the same handleName. That function does check whether it already has an internal handle of that name. That's why it looked to me like calling gosmi.Init() multiple times is safe.

It's a good idea to be defensive in our code and add sync.Once() anyway. I'm not sure gosmi intends it to be safe to call gosmi.Init() multiple times. I didn't see a guarantee in the docs, so even if it is now it might not be in a future version of gosmi.

var folders []string
for _, mibPath := range paths {
gosmi.AppendPath(mibPath)
folders = append(folders, mibPath)
err := filepath.Walk(mibPath, func(path string, info os.FileInfo, err error) error {
// symlinks are files so we need to double check if any of them are folders
// Will check file vs directory later on
if info.Mode()&os.ModeSymlink != 0 {
link, err := os.Readlink(path)
if err != nil {
log.Warnf("Bad symbolic link %v", link)
}
folders = append(folders, link)
}
return nil
})
if err != nil {
return fmt.Errorf("Filepath could not be walked %v", err)
}
for _, folder := range folders {
err := filepath.Walk(folder, func(path string, info os.FileInfo, err error) error {
// checks if file or directory
if info.IsDir() {
gosmi.AppendPath(path)
} else if info.Mode()&os.ModeSymlink == 0 {
_, err := gosmi.LoadModule(info.Name())
if err != nil {
log.Warnf("Module could not be loaded %v", err)
}
}
return nil
})
if err != nil {
return fmt.Errorf("Filepath could not be walked %v", err)
}
}
folders = []string{}
}
return nil
}

// The following is for snmp_trap
type MibEntry struct {
MibName string
OidText string
}

func TrapLookup(oid string) (e MibEntry, err error) {
var node gosmi.SmiNode
node, err = gosmi.GetNodeByOID(types.OidMustFromString(oid))

// ensure modules are loaded or node will be empty (might not error)
if err != nil {
return e, err
}

e.OidText = node.RenderQualified()

i := strings.Index(e.OidText, "::")
if i == -1 {
return e, fmt.Errorf("not found")
}
e.MibName = e.OidText[:i]
e.OidText = e.OidText[i+2:]
return e, nil
}

// The following is for snmp

func GetIndex(oidNum string, mibPrefix string) (col []string, tagOids map[string]struct{}, err error) {
// first attempt to get the table's tags
tagOids = map[string]struct{}{}

// mimcks grabbing INDEX {} that is returned from snmptranslate -Td MibName
node, err := gosmi.GetNodeByOID(types.OidMustFromString(oidNum))

if err != nil {
return []string{}, map[string]struct{}{}, fmt.Errorf("getting submask: %w", err)
}

for _, index := range node.GetIndex() {
//nolint:staticcheck //assaignment to nil map to keep backwards compatibilty
tagOids[mibPrefix+index.Name] = struct{}{}
}

// grabs all columns from the table
// mimmicks grabbing everything returned from snmptable -Ch -Cl -c public 127.0.0.1 oidFullName
col = node.GetRow().AsTable().ColumnOrder

return col, tagOids, nil
}

//nolint:revive //Too many return variable but necessary
func SnmpTranslateCall(oid string) (mibName string, oidNum string, oidText string, conversion string, err error) {
MyaLongmire marked this conversation as resolved.
Show resolved Hide resolved
var out gosmi.SmiNode
var end string
if strings.ContainsAny(oid, "::") {
// split given oid
// for example RFC1213-MIB::sysUpTime.0
s := strings.Split(oid, "::")
// node becomes sysUpTime.0
node := s[1]
if strings.ContainsAny(node, ".") {
s = strings.Split(node, ".")
// node becomes sysUpTime
node = s[0]
end = "." + s[1]
}

out, err = gosmi.GetNode(node)
if err != nil {
return oid, oid, oid, oid, err
}

oidNum = "." + out.RenderNumeric() + end
} else if strings.ContainsAny(oid, "abcdefghijklnmopqrstuvwxyz") {
//handle mixed oid ex. .iso.2.3
s := strings.Split(oid, ".")
for i := range s {
if strings.ContainsAny(s[i], "abcdefghijklmnopqrstuvwxyz") {
out, err = gosmi.GetNode(s[i])
if err != nil {
return oid, oid, oid, oid, err
}
s[i] = out.RenderNumeric()
}
}
oidNum = strings.Join(s, ".")
out, _ = gosmi.GetNodeByOID(types.OidMustFromString(oidNum))
} else {
out, err = gosmi.GetNodeByOID(types.OidMustFromString(oid))
oidNum = oid
// ensure modules are loaded or node will be empty (might not error)
// do not return the err as the oid is numeric and telegraf can continue
//nolint:nilerr
if err != nil || out.Name == "iso" {
return oid, oid, oid, oid, nil
}
}

tc := out.GetSubtree()

for i := range tc {
// case where the mib doesn't have a conversion so Type struct will be nil
// prevents seg fault
if tc[i].Type == nil {
break
}
switch tc[i].Type.Name {
case "MacAddress", "PhysAddress":
conversion = "hwaddr"
case "InetAddressIPv4", "InetAddressIPv6", "InetAddress", "IPSIpAddress":
conversion = "ipaddr"
}
}

oidText = out.RenderQualified()
i := strings.Index(oidText, "::")
if i == -1 {
return "", oid, oid, oid, fmt.Errorf("not found")
}
mibName = oidText[:i]
oidText = oidText[i+2:] + end

return mibName, oidNum, oidText, conversion, nil
}
24 changes: 8 additions & 16 deletions plugins/inputs/snmp/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,10 @@ The `snmp` input plugin uses polling to gather metrics from SNMP agents.
Support for gathering individual OIDs as well as complete SNMP tables is
included.

## Prerequisites
## Note about Paths

This plugin uses the `snmptable` and `snmptranslate` programs from the
[net-snmp][] project. These tools will need to be installed into the `PATH` in
order to be located. Other utilities from the net-snmp project may be useful
for troubleshooting, but are not directly used by the plugin.

These programs will load available MIBs on the system. Typically the default
directory for MIBs is `/usr/share/snmp/mibs`, but if your MIBs are in a
different location you may need to make the paths known to net-snmp. The
location of these files can be configured in the `snmp.conf` or via the
`MIBDIRS` environment variable. See [`man 1 snmpcmd`][man snmpcmd] for more
information.
Path is a global variable, separate snmp instances will append the specified
path onto the global path variable
MyaLongmire marked this conversation as resolved.
Show resolved Hide resolved

## Configuration

Expand All @@ -38,6 +29,9 @@ information.
## SNMP version; can be 1, 2, or 3.
# version = 2

## Path to mib files
# path = ["/usr/share/snmp/mibs"]
MyaLongmire marked this conversation as resolved.
Show resolved Hide resolved

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

Expand Down Expand Up @@ -260,7 +254,7 @@ oid = "CISCO-POWER-ETHERNET-EXT-MIB::cpeExtPsePortEntPhyIndex"

Partial result (removed agent_host and host columns from all following outputs in this section):

```shell
```text
> ciscoPower,index=1.2 EntPhyIndex=1002i,PortPwrConsumption=6643i 1621460628000000000
> ciscoPower,index=1.6 EntPhyIndex=1006i,PortPwrConsumption=10287i 1621460628000000000
> ciscoPower,index=1.5 EntPhyIndex=1005i,PortPwrConsumption=8358i 1621460628000000000
Expand Down Expand Up @@ -313,7 +307,7 @@ is_tag = true

Result:

```shell
```text
> ciscoPowerEntity,EntPhysicalName=GigabitEthernet1/2,index=1.2 EntPhyIndex=1002i,PortPwrConsumption=6643i 1621461148000000000
> ciscoPowerEntity,EntPhysicalName=GigabitEthernet1/6,index=1.6 EntPhyIndex=1006i,PortPwrConsumption=10287i 1621461148000000000
> ciscoPowerEntity,EntPhysicalName=GigabitEthernet1/5,index=1.5 EntPhyIndex=1005i,PortPwrConsumption=8358i 1621461148000000000
Expand Down Expand Up @@ -357,7 +351,5 @@ interface,agent_host=127.0.0.1,ifDescr=eth0,ifIndex=2,source=example.org ifAdmin
interface,agent_host=127.0.0.1,ifDescr=lo,ifIndex=1,source=example.org ifAdminStatus=1i,ifInDiscards=0i,ifInErrors=0i,ifInNUcastPkts=0i,ifInOctets=51555569i,ifInUcastPkts=339097i,ifInUnknownProtos=0i,ifLastChange=0i,ifMtu=65536i,ifOperStatus=1i,ifOutDiscards=0i,ifOutErrors=0i,ifOutNUcastPkts=0i,ifOutOctets=51555569i,ifOutQLen=0i,ifOutUcastPkts=339097i,ifSpecific=".0.0",ifSpeed=10000000i,ifType=24i 1575509815000000000
```

[net-snmp]: http://www.net-snmp.org/
[man snmpcmd]: http://net-snmp.sourceforge.net/docs/man/snmpcmd.html#lbAK
[metric filtering]: /docs/CONFIGURATION.md#metric-filtering
[metric]: /docs/METRICS.md
Loading