You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2021/10/04 18:46:45 UTC
[GitHub] [nifi] tpalfy commented on a change in pull request #5088: NIFI-3320: SendTrapSNMP and ListenTrapSNMP processors added.
tpalfy commented on a change in pull request #5088:
URL: https://github.com/apache/nifi/pull/5088#discussion_r721583555
##########
File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/factory/core/SNMPManagerFactory.java
##########
@@ -14,27 +14,29 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.nifi.snmp.factory;
+package org.apache.nifi.snmp.factory.core;
+import org.apache.nifi.processor.exception.ProcessException;
import org.apache.nifi.snmp.configuration.SNMPConfiguration;
import org.snmp4j.Snmp;
-import org.snmp4j.Target;
-import org.snmp4j.mp.SnmpConstants;
+import org.snmp4j.smi.UdpAddress;
+import org.snmp4j.transport.DefaultUdpTransportMapping;
-public class V2cSNMPFactory extends AbstractSNMPFactory implements SNMPFactory {
+import java.io.IOException;
- @Override
- public boolean supports(final int version) {
- return SnmpConstants.version2c == version;
- }
+public class SNMPManagerFactory {
- @Override
- public Snmp createSnmpManagerInstance(final SNMPConfiguration configuration) {
- return createSnmpClient();
- }
+ private static final String LOCALHOST = "127.0.0.1";
- @Override
- public Target createTargetInstance(final SNMPConfiguration configuration) {
- return createCommunityTarget(configuration);
+ public Snmp createSnmpManagerInstance(final SNMPConfiguration configuration) {
Review comment:
Not sure why this my previous comment was marked as resolved.
Were you able to confirm that simply defining the port (without the address - considering we want to open a server port) is not enough?
##########
File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/operations/SNMPRequestTest.java
##########
@@ -16,45 +16,66 @@
*/
package org.apache.nifi.snmp.operations;
+import org.apache.nifi.snmp.configuration.SNMPConfiguration;
import org.apache.nifi.snmp.dto.SNMPSingleResponse;
import org.apache.nifi.snmp.dto.SNMPTreeResponse;
-import org.apache.nifi.snmp.helper.SNMPTestUtils;
+import org.apache.nifi.snmp.dto.SNMPValue;
+import org.apache.nifi.snmp.exception.RequestTimeoutException;
+import org.apache.nifi.snmp.factory.core.SNMPFactoryProvider;
+import org.apache.nifi.snmp.helper.configurations.SNMPConfigurationFactory;
+import org.apache.nifi.snmp.helper.configurations.SNMPV1V2cConfigurationFactory;
+import org.apache.nifi.snmp.helper.configurations.SNMPV3ConfigurationFactory;
import org.apache.nifi.snmp.testagents.TestAgent;
-import org.apache.nifi.util.MockFlowFile;
+import org.apache.nifi.snmp.testagents.TestSNMPV1Agent;
+import org.apache.nifi.snmp.testagents.TestSNMPV2cAgent;
+import org.apache.nifi.snmp.testagents.TestSNMPV3Agent;
import org.junit.After;
import org.junit.Before;
-import org.snmp4j.CommunityTarget;
-import org.snmp4j.Snmp;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
import org.snmp4j.agent.mo.DefaultMOFactory;
import org.snmp4j.agent.mo.MOAccessImpl;
+import org.snmp4j.mp.SnmpConstants;
import org.snmp4j.smi.OID;
import org.snmp4j.smi.OctetString;
import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
+import java.util.HashSet;
import java.util.Map;
+import java.util.Set;
import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.fail;
-public abstract class SNMPRequestTest {
-
- protected static final String LOCALHOST = "127.0.0.1";
- protected static final String INVALID_HOST = "127.0.0.2";
- protected static final String READ_ONLY_OID_1 = "1.3.6.1.4.1.32437.1.5.1.4.2.0";
- protected static final String READ_ONLY_OID_2 = "1.3.6.1.4.1.32437.1.5.1.4.3.0";
- protected static final String WRITE_ONLY_OID = "1.3.6.1.4.1.32437.1.5.1.4.4.0";
- protected static final String READ_ONLY_OID_VALUE_1 = "TestOID1";
- protected static final String READ_ONLY_OID_VALUE_2 = "TestOID2";
- protected static final String WRITE_ONLY_OID_VALUE = "writeOnlyOID";
- protected static final String SNMP_PROP_DELIMITER = "$";
- protected static final String SNMP_PROP_PREFIX = "snmp" + SNMP_PROP_DELIMITER;
- protected static final String NOT_WRITABLE = "Not writable";
- protected static final String NO_ACCESS = "No access";
- protected static final String SUCCESS = "Success";
- protected static final String EXPECTED_OID_VALUE = "testValue";
+@RunWith(Parameterized.class)
+public class SNMPRequestTest {
Review comment:
In almost all tests there's an
```java
snmpResourceHandler.close();
```
at the end.
But what if a test fails? Via assertion failure for example.
It should be in an `@After` method (probably after a null check as the `snmpResourceHandler` would need to be extracted to a field.)
##########
File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/helper/configurations/SNMPV1V2cConfigurationFactory.java
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.snmp.helper.configurations;
+
+import org.apache.nifi.snmp.configuration.SNMPConfiguration;
+
+public class SNMPV1V2cConfigurationFactory implements SNMPConfigurationFactory {
+
+ private int snmpVersion;
+
+ public SNMPV1V2cConfigurationFactory(int snmpVersion) {
+ this.snmpVersion = snmpVersion;
+ }
+
+ @Override
+ public SNMPConfiguration createSnmpGetSetConfiguration(final int agentPort) {
+ return SNMPConfiguration.builder()
+ .setTargetHost(DEFAULT_HOST)
+ .setTargetPort(String.valueOf(agentPort))
+ .setCommunityString(COMMUNITY_STRING)
+ .setVersion(snmpVersion)
+ .build();
+ }
+
+ @Override
+ public SNMPConfiguration createSnmpGetSetConfigWithCustomHost(final String host, final int agentPort) {
+ return SNMPConfiguration.builder()
+ .setTargetHost(host)
+ .setTargetPort(String.valueOf(agentPort))
+ .setCommunityString(COMMUNITY_STRING)
+ .setVersion(snmpVersion)
+ .setSecurityLevel(SECURITY_LEVEL)
+ .setSecurityName(SECURITY_NAME)
+ .setAuthProtocol(AUTH_PROTOCOL)
+ .setAuthPassphrase(AUTH_PASSPHRASE)
+ .setPrivacyProtocol(PRIV_PROTOCOL)
+ .setPrivacyPassphrase(PRIV_PASSPHRASE)
+ .build();
+ }
Review comment:
```suggestion
@Override
public SNMPConfiguration createSnmpGetSetConfigWithCustomHost(final String host, final int agentPort) {
return SNMPConfiguration.builder()
.setTargetHost(host)
.setTargetPort(String.valueOf(agentPort))
.setCommunityString(COMMUNITY_STRING)
.setVersion(snmpVersion)
.build();
}
```
##########
File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/helper/configurations/SNMPConfigurationFactory.java
##########
@@ -14,17 +14,27 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.nifi.snmp.factory;
+package org.apache.nifi.snmp.helper.configurations;
import org.apache.nifi.snmp.configuration.SNMPConfiguration;
-import org.snmp4j.Snmp;
-import org.snmp4j.Target;
-public interface SNMPFactory {
+public interface SNMPConfigurationFactory {
- boolean supports(final int version);
+ String DEFAULT_HOST = "127.0.0.1";
+ String COMMUNITY_STRING = "public";
- Snmp createSnmpManagerInstance(final SNMPConfiguration configuration);
+ // V3 security (users are set in test agents)
+ String SECURITY_LEVEL = "authPriv";
+ String SECURITY_NAME = "SHAAES128";
+ String AUTH_PROTOCOL = "SHA";
+ String AUTH_PASSPHRASE = "SHAAES128AuthPassphrase";
+ String PRIV_PROTOCOL = "AES128";
+ String PRIV_PASSPHRASE = "SHAAES128PrivPassphrase";
Review comment:
The proper place for these would be in `SNMPV3ConfigurationFactory`.
##########
File path: nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/properties/BasicProperties.java
##########
@@ -0,0 +1,74 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.snmp.processors.properties;
+
+import org.apache.nifi.components.AllowableValue;
+import org.apache.nifi.components.PropertyDescriptor;
+import org.apache.nifi.processor.util.StandardValidators;
+import org.snmp4j.mp.SnmpConstants;
+
+public class BasicProperties {
+
+ public static final AllowableValue SNMP_V1 = new AllowableValue(String.valueOf(SnmpConstants.version1), "v1", "SNMP version 1");
+ public static final AllowableValue SNMP_V2C = new AllowableValue(String.valueOf(SnmpConstants.version2c), "v2c", "SNMP version 2c");
+ public static final AllowableValue SNMP_V3 = new AllowableValue(String.valueOf(SnmpConstants.version3), "v3", "SNMP version 3 with improved security");
+
+
+ public static final PropertyDescriptor SNMP_VERSION = new PropertyDescriptor.Builder()
+ .name("snmp-version")
+ .displayName("SNMP Version")
+ .description("Three significant versions of SNMP have been developed and deployed. " +
+ "SNMPv1 is the original version of the protocol. More recent versions, " +
+ "SNMPv2c and SNMPv3, feature improvements in performance, flexibility and security.")
+ .required(true)
+ .allowableValues(SNMP_V1, SNMP_V2C, SNMP_V3)
+ .defaultValue(SNMP_V1.getValue())
+ .build();
+
+ public static final PropertyDescriptor SNMP_COMMUNITY = new PropertyDescriptor.Builder()
+ .name("snmp-community")
+ .displayName("SNMP Community")
+ .description("SNMPv1 and SNMPv2 use communities to establish trust between managers and agents." +
+ " Most agents support three community names, one each for read-only, read-write and trap." +
+ " These three community strings control different types of activities. The read-only community" +
+ " applies to get requests. The read-write community string applies to set requests. The trap" +
+ " community string applies to receipt of traps.")
+ .required(true)
+ .sensitive(true)
+ .defaultValue("public")
+ .addValidator(StandardValidators.NON_EMPTY_VALIDATOR)
+ .dependsOn(SNMP_VERSION, SNMP_V1, SNMP_V2C)
+ .build();
+
+ public static final PropertyDescriptor SNMP_RETRIES = new PropertyDescriptor.Builder()
+ .name("snmp-retries")
+ .displayName("Number of Retries")
+ .description("Set the number of retries when requesting the SNMP Agent.")
+ .required(false)
+ .defaultValue("0")
+ .addValidator(StandardValidators.NON_NEGATIVE_INTEGER_VALIDATOR)
+ .build();
+
+ public static final PropertyDescriptor SNMP_TIMEOUT = new PropertyDescriptor.Builder()
+ .name("snmp-timeout")
+ .displayName("Timeout (ms)")
+ .description("Set the timeout (in milliseconds) when requesting the SNMP Agent.")
+ .required(false)
+ .defaultValue("5000 ms")
+ .addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
+ .build();
Review comment:
There's a reason I suggested omitting the `(ms)` and `(in milliseconds)` part.
The _user doesn't need_ to define the timeout in milliseconds. That's the point of the time period properties. They can confirm the time dimension with any kind of unit.
When we _read_ the value we can extract it in milliseconds regardless.
```suggestion
public static final PropertyDescriptor SNMP_TIMEOUT = new PropertyDescriptor.Builder()
.name("snmp-timeout")
.displayName("Timeout")
.description("Set the timeout when requesting the SNMP Agent.")
.required(false)
.defaultValue("5000 ms")
.addValidator(StandardValidators.TIME_PERIOD_VALIDATOR)
.build();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org