Repository: kentik/ktranslate
Pull request #297 (2022-03-21 21:04:20)

Covering gap in crashing metadata; lots of tests about this.
When no extra metadata mibs are passed to device, AND some of them do not return a result, a panic happens. This adds more guards around this case. It also extends the testing system to allow coverage. 

Before:
```
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x40 pc=0x6abd10]

goroutine 19 [running]:
testing.tRunner.func1.2({0x6da1a0, 0x9e4b60})
	/home/pye/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
	/home/pye/go/src/testing/testing.go:1212 +0x218
panic({0x6da1a0, 0x9e4b60})
	/home/pye/go/src/runtime/panic.go:1038 +0x215
github.com/kentik/ktranslate/pkg/kt.(*Mib).GetName(...)
	/home/pye/src/ktranslate/pkg/kt/snmp.go:310
github.com/kentik/ktranslate/pkg/inputs/snmp/metadata.(*DeviceMetadata).poll(0xc0000a5400, {0x846070, 0xc0000be000}, 0x83d738)
	/home/pye/src/ktranslate/pkg/inputs/snmp/metadata/device_metadata.go:115 +0x3f0
github.com/kentik/ktranslate/pkg/inputs/snmp/metadata.TestPoll(0xc000232ea0)
	/home/pye/src/ktranslate/pkg/inputs/snmp/metadata/device_metadata_test.go:41 +0x412
testing.tRunner(0xc000232ea0, 0x7391f8)
	/home/pye/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
	/home/pye/go/src/testing/testing.go:1306 +0x35a
FAIL	github.com/kentik/ktranslate/pkg/inputs/snmp/metadata	0.008s
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/metrics	[no test files]
ok  	github.com/kentik/ktranslate/pkg/inputs/snmp/mibs	0.006s
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/mibs/apc	[no test files]
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/ping	[no test files]
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/traps	[no test files]
ok  	github.com/kentik/ktranslate/pkg/inputs/snmp/util	(cached)
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/util/vendor	[no test files]
FAIL
```

After:
```
(ktenv) pye@ribeye:~/src/ktranslate(main) $ go test ./pkg/inputs/snmp/...
?   	github.com/kentik/ktranslate/pkg/inputs/snmp	[no test files]
ok  	github.com/kentik/ktranslate/pkg/inputs/snmp/metadata	0.004s
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/metrics	[no test files]
ok  	github.com/kentik/ktranslate/pkg/inputs/snmp/mibs	(cached)
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/mibs/apc	[no test files]
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/ping	[no test files]
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/traps	[no test files]
ok  	github.com/kentik/ktranslate/pkg/inputs/snmp/util	(cached)
?   	github.com/kentik/ktranslate/pkg/inputs/snmp/util/vendor	[no test files]
(ktenv) pye@ribeye:~/src/ktranslate(main) $ 
```


Patch:
diff --git a/pkg/inputs/snmp/metadata/device_metadata.go b/pkg/inputs/snmp/metadata/device_metadata.go
index a27bdbc..1d5a7d0 100644
--- a/pkg/inputs/snmp/metadata/device_metadata.go
+++ b/pkg/inputs/snmp/metadata/device_metadata.go
@@ -109,10 +109,10 @@ func (dm *DeviceMetadata) poll(ctx context.Context, server *gosnmp.GoSNMP) (*kt.
 		if len(oidResults) == 0 {
 			missing++
 			if _, ok := dm.missing[oid]; ok {
-				dm.log.Debugf("OID %s failed to return results, Metric Name: %s", oid, mib.Name)
+				dm.log.Debugf("OID %s failed to return results, Metric Name: %s", oid, mib.GetName())
 			} else {
 				dm.missing[oid] = true
-				dm.log.Warnf("OID %s failed to return results, Metric Name: %s", oid, mib.Name)
+				dm.log.Warnf("OID %s failed to return results, Metric Name: %s", oid, mib.GetName())
 			}
 		}
 		for _, result := range oidResults {
diff --git a/pkg/inputs/snmp/util/util.go b/pkg/inputs/snmp/util/util.go
index d388da6..717005a 100644
--- a/pkg/inputs/snmp/util/util.go
+++ b/pkg/inputs/snmp/util/util.go
@@ -100,7 +100,11 @@ func ToInt64(val interface{}) int64 {
 // topology-demo/devicemetrics/main.go.  Prefix must occur in value, otherwise
 // results are undefined (aka "wrong").
 func GetIndex(value, prefix string) string {
-	return value[strings.Index(value, prefix)+len(prefix):]
+	posit := strings.Index(value, prefix) + len(prefix)
+	if len(value) > posit {
+		return value[posit:]
+	}
+	return ""
 }
 
 // walk the OID subtree under a root, returning a slice of varbinds
@@ -125,8 +129,10 @@ func WalkOID(ctx context.Context, device *kt.SnmpDeviceConfig, oid string, serve
 		pollTry{walk: server.WalkAll, sleep: SNMP_POLL_SLEEP_TIME},
 	}
 
-	// If the device says to not use bulkwalkall, trim this out now.
-	if device.NoUseBulkWalkAll {
+	// If we are overriding with a test set.
+	if walker := device.GetTestWalker(); walker != nil {
+		tries = []pollTry{pollTry{walk: walker.WalkAll, sleep: time.Duration(0)}}
+	} else if device.NoUseBulkWalkAll { // If the device says to not use bulkwalkall, trim this out now.
 		tries = tries[1:]
 	}
 
diff --git a/pkg/kt/snmp.go b/pkg/kt/snmp.go
index e9dfa0d..b0733b2 100644
--- a/pkg/kt/snmp.go
+++ b/pkg/kt/snmp.go
@@ -10,6 +10,7 @@ import (
 	"sync"
 	"time"
 
+	"github.com/gosnmp/gosnmp"
 	go_metrics "github.com/kentik/go-metrics"
 	"gopkg.in/yaml.v2"
 )
@@ -161,6 +162,7 @@ type SnmpDeviceConfig struct {
 	InstrumentationName string            `yaml:"instrumentationName,omitempty"`
 	RunPing             bool              `yaml:"response_time,omitempty"`
 	allUserTags         map[string]string
+	walker              SNMPTestWalker
 }
 
 type SnmpTrapConfig struct {
@@ -302,6 +304,9 @@ func (mb *Mib) String() string {
 }
 
 func (mb *Mib) GetName() string { // Tag takes precedince over name if it is present.
+	if mb == nil {
+		return "missing_mib"
+	}
 	if mb.Tag != "" {
 		return mb.Tag
 	}
@@ -587,3 +592,15 @@ func (d *SnmpDeviceConfig) GetUserTags() map[string]string {
 
 	return out
 }
+
+func (d *SnmpDeviceConfig) GetTestWalker() SNMPTestWalker {
+	return d.walker
+}
+
+func (d *SnmpDeviceConfig) SetTestWalker(w SNMPTestWalker) {
+	d.walker = w
+}
+
+type SNMPTestWalker interface {
+	WalkAll(string) ([]gosnmp.SnmpPDU, error)
+}


Test patch:
diff --git a/pkg/inputs/snmp/metadata/device_metadata_test.go b/pkg/inputs/snmp/metadata/device_metadata_test.go
new file mode 100644
index 0000000..a6844ad
--- /dev/null
+++ b/pkg/inputs/snmp/metadata/device_metadata_test.go
@@ -0,0 +1,70 @@
+package metadata
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/gosnmp/gosnmp"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/kentik/ktranslate/pkg/eggs/logger"
+	lt "github.com/kentik/ktranslate/pkg/eggs/logger/testing"
+	"github.com/kentik/ktranslate/pkg/kt"
+)
+
+type testWalker struct {
+	results []gosnmp.SnmpPDU
+	err     error
+	dm      string
+}
+
+func (w testWalker) WalkAll(oid string) ([]gosnmp.SnmpPDU, error) {
+	return w.results, w.err
+}
+
+func TestPoll(t *testing.T) {
+	l := lt.NewTestContextL(logger.NilContext, t)
+	conf := &kt.SnmpDeviceConfig{}
+	conf.SetTestWalker(testWalker{err: errors.New("Error"), dm: ""}) // Non nil error
+	dm := NewDeviceMetadata(nil, conf, kt.NewSnmpDeviceMetric(nil, "deviceA"), l)
+
+	res, err := dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(res.Tables))
+
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(res.Tables))
+
+	conf.SetTestWalker(testWalker{results: []gosnmp.SnmpPDU{}, dm: ""}) // No results.
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(res.Tables))
+
+	// Test random string
+	conf.SetTestWalker(testWalker{results: []gosnmp.SnmpPDU{{Value: "not a byte-slice", Name: ".1.1.2.3"}}, dm: ""}) // string value.
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(res.Tables))
+
+	// Test sysdesc
+	conf.SetTestWalker(testWalker{results: []gosnmp.SnmpPDU{{Value: []byte("sysdesc"), Name: ".1.3.6.1.2.1.1.1.0"}}, dm: ""}) // SysDescr.
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, "sysdesc", res.SysDescr)
+
+	meta := map[string]*kt.Mib{
+		"1.1.1": &kt.Mib{Name: "foo"},
+	}
+	dm = NewDeviceMetadata(meta, conf, kt.NewSnmpDeviceMetric(nil, "deviceA"), l)
+	conf.SetTestWalker(testWalker{results: []gosnmp.SnmpPDU{{Value: "not a byte-slice", Name: ".1.1.1"}}, dm: ""}) // string value.
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 0, len(res.Tables))
+
+	conf.SetTestWalker(testWalker{results: []gosnmp.SnmpPDU{{Value: "not a byte-slice", Name: ".1.1.1.2"}}, dm: ""}) // table.
+	res, err = dm.poll(context.Background(), nil)
+	assert.NoError(t, err)
+	assert.Equal(t, 1, len(res.Tables))
+}
diff --git a/pkg/kt/snmp_test.go b/pkg/kt/snmp_test.go
index 2fc5fbd..d325318 100644
--- a/pkg/kt/snmp_test.go
+++ b/pkg/kt/snmp_test.go
@@ -20,3 +20,17 @@ func TestIsPollReady(t *testing.T) {
 	assert.False(t, mib.IsPollReady()) // Skip the 2nd.
 	assert.False(t, mib.IsPollReady()) // Skip the 2nd.
 }
+
+func TestGetName(t *testing.T) {
+	mib := &Mib{
+		Tag:  "foo",
+		Name: "name",
+	}
+	assert.Equal(t, "foo", mib.GetName())
+	mib = nil
+	assert.Equal(t, "missing_mib", mib.GetName())
+	mib = &Mib{
+		Name: "bar",
+	}
+	assert.Equal(t, "bar", mib.GetName())
+}