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

fix multithreading problem when using multiple snmp inputs with multiple v3 credentials #80

Merged

Conversation

colinsurprenant
Copy link
Contributor

@colinsurprenant colinsurprenant commented Aug 13, 2020

When using multiple snmp inputs within a single pipeline or in multiple pipelines could result in decoding problems and in particular when using SNMPv3 the following error could occur when using different v3 credentials across the snmp inputs:

org.snmp4j.MessageException: Message processing model 3 returned error: Unknown security name

This PR fixes the problem by using singleton factories to produce a single udp or tcp Snmp object that is listened on and for v3 clients, a single USM for all threads.

I was first able to reproduce the problem locally using a local snmpd server using 2 different SNMPv3 users and configuring 2 pipelines polling the same server but with a different user.

I successfully tested the this fix in the following scenarios:

  • single pipeline, single snmp input, multiple udp hosts
  • single pipeline, single snmp input, mix of udp and tcp hosts
  • single pipeline, multiple snmp inputs, mix of udp and tcp hosts
  • multiple pipelines, multiple snmp inputs, mix of udp and tcp hosts

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

I am confident now that this enables multiple pipelines to have smnp inputs, and for those inputs to share an org.snmp4j.security.SecurityModels, so in that sense it is a step forward and I am okay with it being merged, as long as we call a few things out.

Because share security models, two independent inputs that share a security_name but otherwise have separate settings (e.g., targeting different hosts in separate realms) will end up in a last-to-start-wins situation. We also end up in a state where we cannot tear down listeners when pipelines shut down. If we were to merge this as-is, we would need to document these limitations.

I believe that we can have independent security models (and therefore unshared Snmp instances) by initializing our @snmp with org.slf4j.Snpp(MessageDispatcher, TransportMapping), because it is only when Snmp is initialized without a MessageDispatcher that it initializes itself with one using an org.slf4j.mp.MPv3 that falls through to the singleton org.snmp4j.security.SecurityModels#getInstance().

@colinsurprenant
Copy link
Contributor Author

@yaauie thanks for the review. A few comments:

Because share security models, two independent inputs that share a security_name but otherwise have separate settings (e.g., targeting different hosts in separate realms) will end up in a last-to-start-wins situation

Good point. I have not specifically tested for this condition and you are certainly right about that. If you are ok I will create an issue for this and look for this specific condition as a followup.

We also end up in a state where we cannot tear down listeners when pipelines shut down

Another good point. Note that this should not be a regression because the plugin does not currently have any closing logic to start with (should definitely be added, will open an issue for that too) - IMO the only condition where this might have an effect is if configuration(s) are edited and pipeline(s) reloaded in a way that there is no more snmp input plugin running (or more precisely if either a udp or tcp listener is not required anymore), the Snmp#close method will never be called and resources will not be freed.

I think this is definitely a potential problem we should look after but OTOH I think the risk for this resulting in a logstash usage problem is quite low.

I believe that we can have independent security models (and therefore unshared Snmp instances) by initializing our @snmp with org.slf4j.Snpp(MessageDispatcher, TransportMapping), because it is only when Snmp is initialized without a MessageDispatcher that it initializes itself with one using an org.slf4j.mp.MPv3 that falls through to the singleton org.snmp4j.security.SecurityModels#getInstance()

Great insight, will investigate.

@colinsurprenant
Copy link
Contributor Author

created #81 and #82 followups.

@colinsurprenant
Copy link
Contributor Author

@yaauie FYI I created #84 which is the actual proper fix and gets rid of the singleton objects by using Snmp(MessageDispatcher, TransportMapping) as discussed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants