Expose v4/v6-only connection-schemes through GosnmpWrapper #8804

jaydizz · 2021-02-04T16:26:21Z

Hi,
This PR fixes #7879.

I modified the Gosnmpwrapper to expose tcp{4,6} and udp{4,6} as connection-scheme and pass them to Gosnmp.
This way, one can indirectly specify the address-family to be used for snmp-requests.
I updated the according README-files.
In my small test-setup everything is working as intended.

However, I did not find a unit-test for the snmp input-plugin or the wrapper to alter, so I assume this small 4 line change can (hopefully) be reviewed by hand.
This is my first PR on a go-project ever, so please be gentle! :)

Have a nice day.

Required for all PRs:

Associated README.md updated.
Has appropriate unit tests.

telegraf-tiger

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

jaydizz · 2021-02-04T16:29:57Z

!signed-cla

telegraf-tiger

🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

jaydizz · 2021-02-04T16:31:12Z

!signed-cla

telegraf-tiger

🤝 ✅ CLA has been signed. Thank you!

Hipska · 2021-02-04T16:34:59Z

plugins/inputs/snmp/README.md

  ##   example: agents = ["udp://127.0.0.1:161"]
-  ##            agents = ["tcp://127.0.0.1:161"]


I would keep the tcp example.

I have re-added the example.

Hipska · 2021-02-04T16:36:28Z

plugins/inputs/snmp/README.md

@@ -22,8 +22,11 @@ information.
 ```toml
 [[inputs.snmp]]
  ## Agent addresses to retrieve values from.
+  ##   format:  agents = ["<scheme>://<hostname>:<port>"]


I would mention that scheme and port are optional.

Done, thank you!

Hipska · 2021-02-04T16:45:48Z

Could you check your markup for the checkbox? Should be [x].
Could you also add "Fixes #7879"?

Hipska · 2021-02-04T16:49:58Z

internal/snmp/wrapper.go

+	case "udp4":
+		gs.Transport = "udp4"
+	case "udp6":
+		gs.Transport = "udp6"
 	case "", "udp":


I assume the empty string can be removed?

Suggested change

case "", "udp":

case "udp":

Gosnmp will default to "udp" if no scheme is specified.
However, in userconfig we can omit the "udp://" which will then cause the switch-case to fall through to the default-case.

Regarding #7879:
The default case is reached when an invalid scheme is specified, but telegraf does not output the error nor stop execution. I will look into it further.

Correct, but isn't that handled by this part?

telegraf/internal/snmp/wrapper.go

Lines 158 to 160 in 3b8df55

if !strings.Contains(agent, "://") {

agent = "udp://" + agent

}

yes, you're right.
I removed the line.

Hipska · 2021-02-04T17:15:55Z

plugins/inputs/snmp/snmp.go

@@ -24,9 +24,15 @@ import (

 const description = `Retrieves SNMP values from remote agents`
 const sampleConfig = `
+  [[inputs.snmp]]


I think this line addition is causing the test suite to fail..

I removed the line.
Thank you for your patience!

Hipska · 2021-02-04T17:31:12Z

internal/snmp/wrapper.go

+	case "udp4":
+		gs.Transport = "udp4"
+	case "udp6":
+		gs.Transport = "udp6"
+	case "udp":
 		gs.Transport = "udp"


Maybe keep a more logical ordering?

Suggested change

case "udp4":

gs.Transport = "udp4"

case "udp6":

gs.Transport = "udp6"

case "udp":

gs.Transport = "udp"

case "udp":

gs.Transport = "udp"

case "udp4":

gs.Transport = "udp4"

case "udp6":

gs.Transport = "udp6"

Hipska · 2021-02-04T20:43:23Z

internal/snmp/wrapper.go

 	switch u.Scheme {
-	case "tcp":
-		gs.Transport = "tcp"
-	case "tcp4":
-		gs.Transport = "tcp4"
-	case "tcp6":
-		gs.Transport = "tcp6"
-	case "udp":
-		gs.Transport = "udp"
-	case "udp4":
-		gs.Transport = "udp4"
-	case "udp6":
-		gs.Transport = "udp6"
+	case "tcp", "tcp4", "tcp6", "udp", "udp4", "udp6":
+		gs.Transport = u.Scheme
 	default:
 		return fmt.Errorf("unsupported scheme: %v", u.Scheme)
 	}


This got me thinking, why even bother checking it? This block could be replaced with gs.Transport = u.Scheme and gosnmp will handle it further when connecting. So the snmp input plugin will always support all the schemes that gosnmp is supporting..

Hi and thank you for you reviews! This is a pleasant experience!

I thought the same thing but I am not sure of the side-effects that the remaining schemes like unix-sockets have. I am not sure if this plays well with windows or if it would play well with non-privileged users on Unix.
After all, the only schemes we do not support with the above code are "unix", "unixgram" and "unixpacket".
Usually I prefer to just expose everything the library has to offer and let the user handle the aftermath, but in this case I am a little intimidated by the size of the project here.
I think it would be better to limit the connection possibilities to the "well-known" ones.

What do you think of that?

My opinion is still to let gosnmp handle it. I'm calling @reimda in to hear his opinion.

I did a little digging and I think I know what the intention behind the check in gosnmpwrapper was:
gosnmp will happily take arbitrary values as connetion scheme and not complain about them. The first time the code can fail due to a wrong connection scheme is when gs.Connect in plugins/input/snmp.go is called.

However, the error raised by gosnmp is quite descriptive of what is going on:
agent upd4://test: setting up connection: error establishing connection to host: dial upd4:test: unknown network upd
Additionally, this is error is handeled here (line 576 ongoing):

if err := gs.Connect(); err != nil { return nil, fmt.Errorf("setting up connection: %w", err) }

So indeed: There should be no problem in letting gosnmp handle the error and just feeding it with anything inu.Scheme.
Additionally this is more future-proof... maybe gosnmp will add some more schemes later on.

Correct indeed, that's what I meant.

Currently I am looking into why the schemes ip, ip4 and ip6 always seem to fail and return UnknownNetworkError.
I traced it down to dial.go in the go-sources line 175.

167 func parseNetwork(ctx context.Context, network string, needsProto bool) (afnet string, proto int, err error) { 168 i := last(network, ':') 169 if i < 0 { // no colon 170 switch network { 171 case "tcp", "tcp4", "tcp6": 172 case "udp", "udp4", "udp6": 173 case "ip", "ip4", "ip6": 174 if needsProto { 175 return "", 0, UnknownNetworkError(network) 176 } 177 case "unix", "unixgram", "unixpacket": 178 default: 179 return "", 0, UnknownNetworkError(network) 180 } 181 return network, 0, nil 182 }

Of course: As soon as you specify a port, you also need to specify the protocol! However, the error is not really clear on whats happening.

Since we are using port 161 by default for snmp, it is clear that the aformentiones schemes will not work. And in fact, they don't really make any sense to me... I really don't know why go-snmp offers them in the first place.
For the sockets as connection-schemes:
A socket does not need a port. Unfortunately, specifying something like unix://test:123 will also result in an UnknownNetworkError. I think this can be quite confusing for the user.

Maybe, after all, it does make sense to limit the connection-schemes to udp{4,6} and tcp{4,6}. Or we have to implement checks in snmp.go to ensure correct formatting of the schemes.
Maybe this is the reason why the connection-schemes were limited in the first place.
Otherwise, mis-configuration may result in confusing error-messages for the user.

Penny for your thoughts :)

Okay, you convinced me 😆 Fine for me to keep it as is.

I added some documentation, so the next guy is spared of this discovery-process :)
Thanks for your input. Greatly appreciated!

Hipska · 2021-02-04T22:23:59Z

plugins/inputs/snmp/snmp.go

+	if err := gs.SetAgent(agent); err != nil {
+		return nil, fmt.Errorf("setting gosnmpwrapper agent: %w", err)


I think the idea was to behave it similar like the lines above, so it should have been err = gs.SetAgent(agent)

Suggested change

if err := gs.SetAgent(agent); err != nil {

return nil, fmt.Errorf("setting gosnmpwrapper agent: %w", err)

err = gs.SetAgent(agent)

if err != nil {

return nil, err

Hipska

Do you run go test before you commit? Anyway, last commit is not well formatted.

reimda

Thanks for your work!

reimda · 2021-02-10T00:11:17Z

plugins/inputs/snmp/README.md

  ##   example: agents = ["udp://127.0.0.1:161"]
  ##            agents = ["tcp://127.0.0.1:161"]
+  ##            agents = ["udpv4://v4only-snmp-agent"]


Is there an extra "v" in the scheme in this example?

Suggested change

## agents = ["udpv4://v4only-snmp-agent"]

## agents = ["udp4://v4only-snmp-agent"]

Wow, good catch!

reimda · 2021-02-10T00:11:35Z

plugins/inputs/snmp/snmp.go

  ##   example: agents = ["udp://127.0.0.1:161"]
  ##            agents = ["tcp://127.0.0.1:161"]
+  ##            agents = ["udpv4://v4only-snmp-agent"]


extra v here too

Uhhh.
How many Typos can there be in a few-line-change!
Thanks for your input! :)

That's what reviews are for!

Hey I noticed you said you're just getting started with go. If you're looking for more opportunities to write go code, feel free to grab issues with the "enhancement" or "good first issue" label. The influx and community devs and I are happy to give you feedback on PRs.

Thanks again for this PR!

(cherry picked from commit 198bcc8)

…a#8804)

Expose v4/v6-only connection-schemes through GosnmpWrapper

7331092

telegraf-tiger bot requested changes Feb 4, 2021

View reviewed changes

jaydizz mentioned this pull request Feb 4, 2021

SNMP input - force IPv4/IPv6 #7879

Closed

telegraf-tiger bot requested changes Feb 4, 2021

View reviewed changes

telegraf-tiger bot approved these changes Feb 4, 2021

View reviewed changes

Hipska suggested changes Feb 4, 2021

View reviewed changes

Hipska added area/snmp feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Feb 4, 2021

Review by Hipska

0d55e69

Hipska approved these changes Feb 4, 2021

View reviewed changes

Hipska reviewed Feb 4, 2021

View reviewed changes

Hipska suggested changes Feb 4, 2021

View reviewed changes

jaydizz added 2 commits February 4, 2021 18:20

Added Error-Handling for wrong connection-scheme

9dc2dc1

Line caused test to fail

0482f17

Hipska reviewed Feb 4, 2021

View reviewed changes

Reordered connection-schemes to be more logical

cc1bc50

Hipska approved these changes Feb 4, 2021

View reviewed changes

Hipska self-assigned this Feb 4, 2021

Simplyfied the switch-case

f60dfdc

Hipska reviewed Feb 4, 2021

View reviewed changes

Streamlined errorhandling when setting scheme in gosnmpwrapper

40ffa63

Hipska suggested changes Feb 5, 2021

View reviewed changes

Indentation fixed

3e15db8

Hipska approved these changes Feb 5, 2021

View reviewed changes

Hipska added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Feb 5, 2021

Hipska assigned Hipska and unassigned Hipska Feb 5, 2021

Added documentation for why the available schemes are limited

bdc99ef

Hipska approved these changes Feb 5, 2021

View reviewed changes

reimda suggested changes Feb 10, 2021

View reviewed changes

Typo in Documentation

4953f2d

Hipska approved these changes Feb 10, 2021

View reviewed changes

Hipska requested a review from reimda February 10, 2021 14:46

reimda approved these changes Feb 10, 2021

View reviewed changes

reimda merged commit 198bcc8 into influxdata:master Feb 10, 2021

ssoroka pushed a commit that referenced this pull request Feb 17, 2021

Expose v4/v6-only connection-schemes through GosnmpWrapper (#8804)

8e39a05

(cherry picked from commit 198bcc8)

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

Expose v4/v6-only connection-schemes through GosnmpWrapper (influxdat…

e4ea443

…a#8804)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose v4/v6-only connection-schemes through GosnmpWrapper #8804

Expose v4/v6-only connection-schemes through GosnmpWrapper #8804

jaydizz commented Feb 4, 2021 •

edited

telegraf-tiger bot left a comment

jaydizz commented Feb 4, 2021

telegraf-tiger bot left a comment

jaydizz commented Feb 4, 2021

telegraf-tiger bot left a comment

Hipska Feb 4, 2021

jaydizz Feb 4, 2021

Hipska Feb 4, 2021

jaydizz Feb 4, 2021

Hipska commented Feb 4, 2021

Hipska Feb 4, 2021

jaydizz Feb 4, 2021

Hipska Feb 4, 2021

jaydizz Feb 4, 2021 •

edited

Hipska Feb 4, 2021

jaydizz Feb 4, 2021

Hipska Feb 4, 2021

Hipska Feb 4, 2021 •

edited

jaydizz Feb 4, 2021

Hipska Feb 5, 2021

jaydizz Feb 5, 2021

Hipska Feb 5, 2021

jaydizz Feb 5, 2021

Hipska Feb 5, 2021

jaydizz Feb 5, 2021

Hipska Feb 4, 2021 •

edited

Hipska left a comment

reimda left a comment

reimda Feb 10, 2021

Hipska Feb 10, 2021

reimda Feb 10, 2021

jaydizz Feb 10, 2021

reimda Feb 10, 2021

		## example: agents = ["udp://127.0.0.1:161"]
		## agents = ["tcp://127.0.0.1:161"]

	if !strings.Contains(agent, "://") {
	agent = "udp://" + agent
	}

		if err := gs.SetAgent(agent); err != nil {
		return nil, fmt.Errorf("setting gosnmpwrapper agent: %w", err)

-	if err := gs.SetAgent(agent); err != nil {
-		return nil, fmt.Errorf("setting gosnmpwrapper agent: %w", err)
+	err = gs.SetAgent(agent)
+ 	if err != nil {
+ 		return nil, err

	## agents = ["udpv4://v4only-snmp-agent"]
	## agents = ["udp4://v4only-snmp-agent"]

Expose v4/v6-only connection-schemes through GosnmpWrapper #8804

Expose v4/v6-only connection-schemes through GosnmpWrapper #8804

Conversation

jaydizz commented Feb 4, 2021 • edited

Required for all PRs:

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

jaydizz commented Feb 4, 2021

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

jaydizz commented Feb 4, 2021

telegraf-tiger bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hipska commented Feb 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaydizz Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hipska Feb 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hipska Feb 4, 2021 • edited

Choose a reason for hiding this comment

Hipska left a comment

Choose a reason for hiding this comment

reimda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaydizz commented Feb 4, 2021 •

edited

jaydizz Feb 4, 2021 •

edited

Hipska Feb 4, 2021 •

edited

Hipska Feb 4, 2021 •

edited