tpalfy commented on code in PR #6034:
URL: https://github.com/apache/nifi/pull/6034#discussion_r909616877
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/resources/docs/org.apache.nifi.snmp.processors.ListenTrapSNMP/additionalDetails.html:
##########
@@ -24,10 +24,11 @@
<h2>Summary</h2>
<p>
This processor listens to SNMP traps and creates a flowfile from the trap
PDU.
- The versions SNMPv1, SNMPv2c and SNMPv3 are supproted. The component is
based on <a href="http://www.snmp4j.org/";>SNMP4J</a>.
+ The versions SNMPv1, SNMPv2c and SNMPv3 are supported. The component is
based on <a href="http://www.snmp4j.org/";>SNMP4J</a>.
</p>
<p>
- In case of SNMPv3, users can be specified in a json file. E.g.:
+ SNMPv3 has user-based security. There are three ways to specify the USM
user database and these can be selected using the USM Users Source property.
Review Comment:
```suggestion
SNMPv3 has user-based security. There are three ways to specify the USM
user database and can be selected using the USM Users Source property.
```
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/utils/UsmReader.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.utils;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import org.apache.nifi.processor.exception.ProcessException;
+import org.snmp4j.security.UsmUser;
+import org.snmp4j.smi.OID;
+import org.snmp4j.smi.OctetString;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Scanner;
+import java.util.stream.Collectors;
+
+public interface UsmReader {
Review Comment:
This factory interface pattern (for a lack of better words) is imaginative
but quite unusual and has some drawbacks.
The
```
List<UsmUser> readUsm(String usmUsers);
```
method has parameter that can mean different things and based on that the
implementation logic is different.
I'd rather have that "different thing" passed on to regular implementations
via constructor.
The solution overall would be cleaner and more conventional.
Also writing tests to those implementations would be straightforward - the
current implementations lack proper testing.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/operations/SNMPTrapReceiverHandler.java:
##########
@@ -38,27 +33,28 @@
import org.snmp4j.smi.Integer32;
import org.snmp4j.smi.OctetString;
-import java.io.File;
-import java.io.FileNotFoundException;
import java.io.IOException;
import java.util.List;
-import java.util.Scanner;
public class SNMPTrapReceiverHandler {
private static final Logger logger =
LoggerFactory.getLogger(SNMPTrapReceiverHandler.class);
private final SNMPConfiguration configuration;
- private final String usmUsersFilePath;
+ private final List<UsmUser> usmUsers;
private Snmp snmpManager;
private boolean isStarted;
- public SNMPTrapReceiverHandler(final SNMPConfiguration configuration,
final String usmUsersFilePath) {
+ public SNMPTrapReceiverHandler(final SNMPConfiguration configuration,
final List<UsmUser> usmUsers) {
this.configuration = configuration;
- this.usmUsersFilePath = usmUsersFilePath;
+ this.usmUsers = usmUsers;
snmpManager = new
SNMPManagerFactory().createSnmpManagerInstance(configuration);
}
+ public SNMPTrapReceiverHandler(final SNMPConfiguration configuration) {
Review Comment:
This constructor is not used, can be removed.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/test/java/org/apache/nifi/snmp/operations/SNMPTrapReceiverHandlerTest.java:
##########
@@ -95,22 +88,17 @@ public void testCloseTrapReceiverCleansUpResources() throws
IOException {
}
@Test
- public void testAddUsmUsers() throws JsonProcessingException,
FileNotFoundException {
+ public void testAddUsmUsers() {
Review Comment:
This test could be substantially simplified:
```java
@Test
public void testAddUsmUsers() {
final List<UsmUser> usmUsers =
UsmReader.jsonFileUsmReader().readUsm(USERS_JSON);
final SNMPConfiguration snmpConfiguration =
SNMPConfiguration.builder()
.setManagerPort(NetworkUtils.getAvailableUdpPort())
.setVersion(SnmpConstants.version3)
.build();
final Snmp mockSnmpManager = mock(Snmp.class, RETURNS_DEEP_STUBS);
final ArgumentCaptor<UsmUser> usmUserCaptor =
ArgumentCaptor.forClass(UsmUser.class);
final SNMPTrapReceiverHandler trapReceiverHandler = new
SNMPTrapReceiverHandler(snmpConfiguration, usmUsers);
trapReceiverHandler.setSnmpManager(mockSnmpManager);
trapReceiverHandler.createTrapReceiver(null, null);
verify(mockSnmpManager.getUSM(),
times(2)).addUser(usmUserCaptor.capture());
verify(mockSnmpManager).addCommandResponder(any(SNMPTrapReceiver.class));
assertTrue(trapReceiverHandler.isStarted());
assertEquals(usmUsers, usmUserCaptor.getAllValues());
}
```
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/processors/ListenTrapSNMP.java:
##########
@@ -99,29 +147,73 @@ public class ListenTrapSNMP extends
AbstractSessionFactoryProcessor {
)));
private volatile SNMPTrapReceiverHandler snmpTrapReceiverHandler;
+ private volatile List<UsmUser> usmUsers;
+
+
+ @Override
+ protected Collection<ValidationResult> customValidate(ValidationContext
validationContext) {
Review Comment:
Setting the USM users requires an O(n) operation that may hit the disk.
We could argue that n (number of users) will never be exceedingly large and
reading one small file from the disk should not take long.
Still, in general validation - depending on the flow - may occur 1000s of
times every 5 seconds.
I would move this logic to the user-initiated verification and into the
@OnScheduled method.
##########
nifi-nar-bundles/nifi-snmp-bundle/nifi-snmp-processors/src/main/java/org/apache/nifi/snmp/utils/UsmReader.java:
##########
@@ -0,0 +1,141 @@
+/*
+ * 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.utils;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
+import com.fasterxml.jackson.databind.module.SimpleModule;
+import org.apache.nifi.processor.exception.ProcessException;
+import org.snmp4j.security.UsmUser;
+import org.snmp4j.smi.OID;
+import org.snmp4j.smi.OctetString;
+
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Scanner;
+import java.util.stream.Collectors;
+
+public interface UsmReader {
+
+ List<UsmUser> readUsm(String usmUsers);
+
+ static UsmReader jsonFileUsmReader() {
+ return usmUsersFilePath -> {
+ final List<UsmUser> userDetails;
+ try (Scanner scanner = new Scanner(new File(usmUsersFilePath))) {
+ final String content = scanner.useDelimiter("\\Z").next();
+ ObjectMapper mapper = new ObjectMapper();
+ SimpleModule module = new SimpleModule();
+ module.addDeserializer(UsmUser.class, new
UsmUserDeserializer());
+ mapper.registerModule(module);
+ userDetails = mapper.readValue(content, new
TypeReference<List<UsmUser>>() {
Review Comment:
This is a duplication of 64-72.
--
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-unsubscr...@nifi.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
