Adjust snmp translation to return conversion info.

Also consolidated the translation code to obtain all info with just 1 command execution.

Also split test command mocks out to their own file for cleanliness.

plugins/inputs/snmp/snmp.go

@@ -184,23 +184,21 @@ func (t *Table) init() error {
 		return nil
 	}
 
-	mibPrefix := ""
+	mibName, _, oidText, _, err := snmpTranslate(t.Oid)
-	if err := snmpTranslate(&mibPrefix, &t.Oid, &t.Name); err != nil {
+	if err != nil {
-		return err
+		return Errorf(err, "translating %s", t.Oid)
 	}
+	if t.Name == "" {
+		t.Name = oidText
+	}
+	mibPrefix := mibName + "::"
+	oidFullName := mibPrefix + oidText
 
 	// first attempt to get the table's tags
 	tagOids := map[string]struct{}{}
 	// We have to guess that the "entry" oid is `t.Oid+".1"`. snmptable and snmptranslate don't seem to have a way to provide the info.
-	if out, err := execCmd("snmptranslate", "-m", "all", "-Td", t.Oid+".1"); err == nil {
+	if out, err := execCmd("snmptranslate", "-Td", oidFullName+".1"); err == nil {
 		lines := bytes.Split(out, []byte{'\n'})
-		// get the MIB name if we didn't get it above
-		if mibPrefix == "" {
-			if i := bytes.Index(lines[0], []byte("::")); i != -1 {
-				mibPrefix = string(lines[0][:i+2])
-			}
-		}
-
 		for _, line := range lines {
 			if !bytes.HasPrefix(line, []byte("  INDEX")) {
 				continue
@@ -223,7 +221,7 @@ func (t *Table) init() error {
 	}
 
 	// this won't actually try to run a query. The `-Ch` will just cause it to dump headers.
-	out, err := execCmd("snmptable", "-m", "all", "-Ch", "-Cl", "-c", "public", "127.0.0.1", t.Oid)
+	out, err := execCmd("snmptable", "-Ch", "-Cl", "-c", "public", "127.0.0.1", oidFullName)
 	if err != nil {
 		return Errorf(err, "getting table columns for %s", t.Oid)
 	}
@@ -753,39 +751,106 @@ func fieldConvert(conv string, v interface{}) interface{} {
 		case string:
 			v, _ = strconv.Atoi(vt)
 		}
+		return v, nil
 	}
 
-	return v
+	if conv == "hwaddr" {
+		switch vt := v.(type) {
+		case string:
+			v = net.HardwareAddr(vt).String()
+		case []byte:
+			v = net.HardwareAddr(vt).String()
+		default:
+			return nil, fmt.Errorf("invalid type (%T) for hwaddr conversion", v)
+		}
+	}
+
+	if conv == "ip" {
+		var ipbs []byte
+
+		switch vt := v.(type) {
+		case string:
+			ipbs = []byte(vt)
+		case []byte:
+			ipbs = vt
+		default:
+			return nil, fmt.Errorf("invalid type (%T) for ip conversion", v)
+		}
+
+		switch len(ipbs) {
+		case 4, 16:
+			v = net.IP(ipbs).String()
+		default:
+			return nil, fmt.Errorf("invalid length (%d) for ip conversion", len(ipbs))
+		}
+
+		return v, nil
+	}
+
+	return v, nil
 }
 
 // snmpTranslate resolves the given OID.
-// The contents of the oid parameter will be replaced with the numeric oid value.
+func snmpTranslate(oid string) (mibName string, oidNum string, oidText string, conversion string, err error) {
-// If name is empty, the textual OID value is stored in it. If the textual OID cannot be translated, the numeric OID is stored instead.
+	var out []byte
-// If mibPrefix is non-nil, the MIB in which the OID was found is stored, with a suffix of "::".
+	if strings.ContainsAny(oid, ":abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") {
-func snmpTranslate(mibPrefix *string, oid *string, name *string) error {
+		out, err = execCmd("snmptranslate", "-Td", "-Ob", oid)
-	if strings.ContainsAny(*oid, ":abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ") {
+	} else {
-		out, err := execCmd("snmptranslate", "-m", "all", "-On", *oid)
+		out, err = execCmd("snmptranslate", "-Td", "-Ob", "-m", "all", oid)
-		if err != nil {
+	}
-			return Errorf(err, "translating %s", *oid)
+	if err != nil {
-		}
+		return "", "", "", "", err
-		*oid = string(bytes.TrimSuffix(out, []byte{'\n'}))
 	}
 
-	if *name == "" {
+	bb := bytes.NewBuffer(out)
-		out, err := execCmd("snmptranslate", "-m", "all", *oid)
+
+	oidText, err = bb.ReadString('\n')
+	if err != nil {
+		return "", "", "", "", Errorf(err, "getting OID text")
+	}
+	oidText = oidText[:len(oidText)-1]
+
+	i := strings.Index(oidText, "::")
+	if i == -1 {
+		// was not found in MIB. Value is numeric
+		return "", oidText, oidText, "", nil
+	}
+	mibName = oidText[:i]
+	oidText = oidText[i+2:]
+
+	if i := bytes.Index(bb.Bytes(), []byte("  -- TEXTUAL CONVENTION ")); i != -1 {
+		bb.Next(i + len("  -- TEXTUAL CONVENTION "))
+		tc, err := bb.ReadString('\n')
 		if err != nil {
-			//TODO debug message
+			return "", "", "", "", Errorf(err, "getting textual convention")
-			*name = *oid
+		}
+		tc = tc[:len(tc)-1]
+		switch tc {
+		case "MacAddress", "PhysAddress":
+			conversion = "hwaddr"
+		case "InetAddressIPv4", "InetAddressIPv6":
+			conversion = "ip"
+		}
+	}
+
+	i = bytes.Index(bb.Bytes(), []byte("::= { "))
+	bb.Next(i + len("::= { "))
+	objs, err := bb.ReadString('}')
+	if err != nil {
+		return "", "", "", "", Errorf(err, "getting numeric oid")
+	}
+	objs = objs[:len(objs)-1]
+	for _, obj := range strings.Split(objs, " ") {
+		if len(obj) == 0 {
+			continue
+		}
+		if i := strings.Index(obj, "("); i != -1 {
+			obj = obj[i+1:]
+			oidNum += "." + obj[:strings.Index(obj, ")")]
 		} else {
-			if i := bytes.Index(out, []byte("::")); i != -1 {
+			oidNum += "." + obj
-				if mibPrefix != nil {
-					*mibPrefix = string(out[:i+2])
-				}
-				out = out[i+2:]
-			}
-			*name = string(bytes.TrimSuffix(out, []byte{'\n'}))
 		}
 	}
 
-	return nil
+	return mibName, oidNum, oidText, conversion, nil
 }

plugins/inputs/snmp/snmp_mocks_generate.go

@@ -0,0 +1,94 @@
+// +build generate
+
+package main
+
+import (
+	"bufio"
+	"bytes"
+	"fmt"
+	"os"
+	"os/exec"
+	"strings"
+)
+
+// This file is a generator used to generate the mocks for the commands used by the tests.
+
+// These are the commands to be mocked.
+var mockedCommands = [][]string{
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".1.0.0.0"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".1.0.0.1.1"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".1.0.0.1.2"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", "1.0.0.1.1"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".1.0.0.0.1.1"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".1.0.0.0.1.1.0"},
+	{"snmptranslate", "-Td", "-Ob", "-m", "all", ".999"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::server"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::server.0"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::testTable"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::connections"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::latency"},
+	{"snmptranslate", "-Td", "-Ob", "TEST::hostname"},
+	{"snmptranslate", "-Td", "TEST::testTable.1"},
+	{"snmptable", "-Ch", "-Cl", "-c", "public", "127.0.0.1", "TEST::testTable"},
+}
+
+type mockedCommandResult struct {
+	stdout    string
+	stderr    string
+	exitError bool
+}
+
+func main() {
+	if err := generate(); err != nil {
+		fmt.Fprintf(os.Stderr, "error: %s\n", err)
+		os.Exit(1)
+	}
+}
+
+func generate() error {
+	f, err := os.OpenFile("snmp_mocks_test.go", os.O_RDWR, 0644)
+	if err != nil {
+		return err
+	}
+	br := bufio.NewReader(f)
+	var i int64
+	for l, err := br.ReadString('\n'); err == nil; l, err = br.ReadString('\n') {
+		i += int64(len(l))
+		if l == "// BEGIN GO GENERATE CONTENT\n" {
+			break
+		}
+	}
+	f.Truncate(i)
+	f.Seek(i, 0)
+
+	fmt.Fprintf(f, "var mockedCommandResults = map[string]mockedCommandResult{\n")
+
+	for _, cmd := range mockedCommands {
+		ec := exec.Command(cmd[0], cmd[1:]...)
+		out := bytes.NewBuffer(nil)
+		err := bytes.NewBuffer(nil)
+		ec.Stdout = out
+		ec.Stderr = err
+		ec.Env = []string{
+			"MIBDIRS=+./testdata",
+		}
+
+		var mcr mockedCommandResult
+		if err := ec.Run(); err != nil {
+			if err, ok := err.(*exec.ExitError); !ok {
+				mcr.exitError = true
+			} else {
+				return fmt.Errorf("executing %v: %s", cmd, err)
+			}
+		}
+		mcr.stdout = string(out.Bytes())
+		mcr.stderr = string(err.Bytes())
+		cmd0 := strings.Join(cmd, "\000")
+		mcrv := fmt.Sprintf("%#v", mcr)[5:] // trim `main.` prefix
+		fmt.Fprintf(f, "%#v: %s,\n", cmd0, mcrv)
+	}
+	f.Write([]byte("}\n"))
+	f.Close()
+
+	return exec.Command("gofmt", "-w", "snmp_mocks_test.go").Run()
+}

plugins/inputs/snmp/snmp_mocks_test.go

@@ -0,0 +1,79 @@
+package snmp
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"strings"
+	"testing"
+)
+
+type mockedCommandResult struct {
+	stdout    string
+	stderr    string
+	exitError bool
+}
+
+func mockExecCommand(arg0 string, args ...string) *exec.Cmd {
+	args = append([]string{"-test.run=TestMockExecCommand", "--", arg0}, args...)
+	cmd := exec.Command(os.Args[0], args...)
+	cmd.Stderr = os.Stderr // so the test output shows errors
+	return cmd
+}
+
+// This is not a real test. This is just a way of mocking out commands.
+//
+// Idea based on https://github.com/golang/go/blob/7c31043/src/os/exec/exec_test.go#L568
+func TestMockExecCommand(t *testing.T) {
+	var cmd []string
+	for _, arg := range os.Args {
+		if string(arg) == "--" {
+			cmd = []string{}
+			continue
+		}
+		if cmd == nil {
+			continue
+		}
+		cmd = append(cmd, string(arg))
+	}
+	if cmd == nil {
+		return
+	}
+
+	cmd0 := strings.Join(cmd, "\000")
+	mcr, ok := mockedCommandResults[cmd0]
+	if !ok {
+		cv := fmt.Sprintf("%#v", cmd)[8:] // trim `[]string` prefix
+		fmt.Fprintf(os.Stderr, "Unmocked command. Please add the following to `mockedCommands` in snmp_mocks_generate.go, and then run `go generate`:\n\t%s,\n", cv)
+		os.Exit(1)
+	}
+	fmt.Printf("%s", mcr.stdout)
+	fmt.Fprintf(os.Stderr, "%s", mcr.stderr)
+	if mcr.exitError {
+		os.Exit(1)
+	}
+	os.Exit(0)
+}
+
+func init() {
+	execCommand = mockExecCommand
+}
+
+// BEGIN GO GENERATE CONTENT
+var mockedCommandResults = map[string]mockedCommandResult{
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.1.0.0.0":                    mockedCommandResult{stdout: "TEST::testTable\ntestTable OBJECT-TYPE\n  -- FROM\tTEST\n  MAX-ACCESS\tnot-accessible\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) 0 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.1.0.0.1.1":                  mockedCommandResult{stdout: "TEST::hostname\nhostname OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) 1 1 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.1.0.0.1.2":                  mockedCommandResult{stdout: "TEST::1.2\nanonymous#1 OBJECT-TYPE\n  -- FROM\tTEST\n::= { iso(1) 0 testOID(0) 1 2 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x001.0.0.1.1":                   mockedCommandResult{stdout: "TEST::hostname\nhostname OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) 1 1 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.1.0.0.0.1.1":                mockedCommandResult{stdout: "TEST::server\nserver OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) 1 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.1.0.0.0.1.1.0":              mockedCommandResult{stdout: "TEST::server.0\nserver OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) server(1) 0 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00-m\x00all\x00.999":                        mockedCommandResult{stdout: ".999\n [TRUNCATED]\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::server":                             mockedCommandResult{stdout: "TEST::server\nserver OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) 1 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::server.0":                           mockedCommandResult{stdout: "TEST::server.0\nserver OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) server(1) 0 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::testTable":                          mockedCommandResult{stdout: "TEST::testTable\ntestTable OBJECT-TYPE\n  -- FROM\tTEST\n  MAX-ACCESS\tnot-accessible\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) 0 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::connections":                        mockedCommandResult{stdout: "TEST::connections\nconnections OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tINTEGER\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) 2 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::latency":                            mockedCommandResult{stdout: "TEST::latency\nlatency OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) testTable(0) testTableEntry(1) 3 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00-Ob\x00TEST::hostname":                           mockedCommandResult{stdout: "TEST::hostname\nhostname OBJECT-TYPE\n  -- FROM\tTEST\n  SYNTAX\tOCTET STRING\n  MAX-ACCESS\tread-only\n  STATUS\tcurrent\n::= { iso(1) 0 testOID(0) 1 1 }\n", stderr: "", exitError: false},
+	"snmptranslate\x00-Td\x00TEST::testTable.1":                               mockedCommandResult{stdout: "TEST::testTableEntry\ntestTableEntry OBJECT-TYPE\n  -- FROM\tTEST\n  MAX-ACCESS\tnot-accessible\n  STATUS\tcurrent\n  INDEX\t\t{ server }\n::= { iso(1) 0 testOID(0) testTable(0) 1 }\n", stderr: "", exitError: false},
+	"snmptable\x00-Ch\x00-Cl\x00-c\x00public\x00127.0.0.1\x00TEST::testTable": mockedCommandResult{stdout: "server connections latency \nTEST::testTable: No entries\n", stderr: "", exitError: false},
+}

plugins/inputs/snmp/snmp_test.go

@@ -1,11 +1,9 @@
+//go:generate go run -tags generate snmp_mocks_generate.go
 package snmp
 
 import (
 	"fmt"
 	"net"
-	"os"
-	"os/exec"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -18,77 +16,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func mockExecCommand(arg0 string, args ...string) *exec.Cmd {
-	args = append([]string{"-test.run=TestMockExecCommand", "--", arg0}, args...)
-	cmd := exec.Command(os.Args[0], args...)
-	cmd.Stderr = os.Stderr // so the test output shows errors
-	return cmd
-}
-func TestMockExecCommand(t *testing.T) {
-	var cmd []string
-	for _, arg := range os.Args {
-		if string(arg) == "--" {
-			cmd = []string{}
-			continue
-		}
-		if cmd == nil {
-			continue
-		}
-		cmd = append(cmd, string(arg))
-	}
-	if cmd == nil {
-		return
-	}
-
-	// will not properly handle args with spaces, but it's good enough
-	cmdStr := strings.Join(cmd, " ")
-	switch cmdStr {
-	case "snmptranslate -m all .1.0.0.0":
-		fmt.Printf("TEST::testTable\n")
-	case "snmptranslate -m all .1.0.0.0.1.1":
-		fmt.Printf("server\n")
-	case "snmptranslate -m all .1.0.0.0.1.1.0":
-		fmt.Printf("server.0\n")
-	case "snmptranslate -m all .1.0.0.1.1":
-		fmt.Printf("hostname\n")
-	case "snmptranslate -m all .999":
-		fmt.Printf(".999\n")
-	case "snmptranslate -m all -On TEST::testTable":
-		fmt.Printf(".1.0.0.0\n")
-	case "snmptranslate -m all -On TEST::hostname":
-		fmt.Printf(".1.0.0.1.1\n")
-	case "snmptranslate -m all -On TEST::server":
-		fmt.Printf(".1.0.0.0.1.1\n")
-	case "snmptranslate -m all -On TEST::connections":
-		fmt.Printf(".1.0.0.0.1.2\n")
-	case "snmptranslate -m all -On TEST::latency":
-		fmt.Printf(".1.0.0.0.1.3\n")
-	case "snmptranslate -m all -On TEST::server.0":
-		fmt.Printf(".1.0.0.0.1.1.0\n")
-	case "snmptranslate -m all -Td .1.0.0.0.1":
-		fmt.Printf(`TEST::testTableEntry
-testTableEntry OBJECT-TYPE
-  -- FROM	TEST
-  MAX-ACCESS	not-accessible
-  STATUS	current
-  INDEX		{ server }
-::= { iso(1) 2 testOID(3) testTable(0) 1 }
-`)
-	case "snmptable -m all -Ch -Cl -c public 127.0.0.1 .1.0.0.0":
-		fmt.Printf(`server connections latency 
-TEST::testTable: No entries
-`)
-	default:
-		fmt.Fprintf(os.Stderr, "Command not mocked: `%s`\n", cmdStr)
-		// you get the expected output by running the missing command with `-M testdata` in the plugin directory.
-		os.Exit(1)
-	}
-	os.Exit(0)
-}
-func init() {
-	execCommand = mockExecCommand
-}
-
 type testSNMPConnection struct {
 	host   string
 	values map[string]interface{}
@@ -302,7 +229,7 @@ func TestGetSNMPConnection_v3(t *testing.T) {
 	assert.Equal(t, gs.Version, gosnmp.Version3)
 	sp := gs.SecurityParameters.(*gosnmp.UsmSecurityParameters)
 	assert.Equal(t, "1.2.3.4", gsc.Host())
-	assert.Equal(t, 20, gs.MaxRepetitions)
+	assert.EqualValues(t, 20, gs.MaxRepetitions)
 	assert.Equal(t, "mycontext", gs.ContextName)
 	assert.Equal(t, gosnmp.AuthPriv, gs.MsgFlags&gosnmp.AuthPriv)
 	assert.Equal(t, "myuser", sp.UserName)