From 25aae45da8dc4d01a490950c178f4745993b6b1b Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Wed, 18 Oct 2023 22:30:31 +0200 Subject: [PATCH] Do not start devices with invalid configuration We have an unfortunate modeling accident, where netconf-node's 'host' and 'port' leaves cannot be made mandatory unless an incompatible change is made. This means invalid configuration may hit the configuration datastore (rather than being rejected) and we need to deal with that. Redefine ensureNode() to issue a deleteNode() on error and restructure internals to capture required state before we make a decision whether to start a node. This rids us of a source of NPE when the NetconfNode is completely missing. JIRA: NETCONF-1114 Change-Id: I2f0fd8a65d1c508cb439c1994cf6c446d051412d Signed-off-by: Robert Varga Signed-off-by: Oleksandr Zharov --- .../impl/NetconfTopologyImplTest.java | 1 + .../topology/spi/AbstractNetconfTopology.java | 47 ++++++++++--------- 2 files changed, 27 insertions(+), 21 deletions(-) diff --git a/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java b/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java index 6616968d03..d3f561b49a 100644 --- a/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java +++ b/apps/netconf-topology-impl/src/test/java/org/opendaylight/netconf/topology/impl/NetconfTopologyImplTest.java @@ -113,6 +113,7 @@ class NetconfTopologyImplTest { final var node = new NodeBuilder() .withKey(key) .addAugmentation(new NetconfNodeBuilder() + .setLockDatastore(true) .setHost(new Host(new IpAddress(new Ipv4Address("127.0.0.1")))) .setPort(new PortNumber(Uint16.valueOf(9999))) .setReconnectOnChangedSchema(true) diff --git a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java index 00e6a58ea8..73798ff01e 100644 --- a/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java +++ b/apps/netconf-topology/src/main/java/org/opendaylight/netconf/topology/spi/AbstractNetconfTopology.java @@ -12,10 +12,10 @@ import static java.util.Objects.requireNonNull; import com.google.common.annotations.VisibleForTesting; import io.netty.util.concurrent.EventExecutor; import java.util.HashMap; +import java.util.NoSuchElementException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; import java.util.concurrent.ScheduledExecutorService; -import org.checkerframework.checker.lock.qual.Holding; import org.opendaylight.controller.config.threadpool.ScheduledThreadPool; import org.opendaylight.controller.config.threadpool.ThreadPool; import org.opendaylight.mdsal.binding.api.DataBroker; @@ -103,9 +103,33 @@ public abstract class AbstractNetconfTopology { LOG.info("RemoteDevice{{}} was already configured, disconnecting", nodeId); prev.close(); } + final var netconfNode = node.augmentation(NetconfNode.class); + if (netconfNode == null) { + LOG.warn("RemoteDevice{{}} is missing NETCONF node configuration, not connecting it", nodeId); + return; + } + final RemoteDeviceId deviceId; + try { + deviceId = NetconfNodeUtils.toRemoteDeviceId(nodeId, netconfNode); + } catch (NoSuchElementException e) { + LOG.warn("RemoteDevice{{}} has invalid configuration, not connecting it", nodeId, e); + return; + } LOG.info("Connecting RemoteDevice{{}}, with config {}", nodeId, hideCredentials(node)); - setupConnection(nodeId, node); + + // Instantiate the handler ... + final var nodeOptional = node.augmentation(NetconfNodeAugmentedOptional.class); + final var deviceSalFacade = createSalFacade(deviceId, netconfNode.requireLockDatastore()); + final var nodeHandler = new NetconfNodeHandler(clientDispatcher, eventExecutor, keepaliveExecutor, + baseSchemas, schemaManager, processingExecutor, builderFactory, deviceActionFactory, deviceSalFacade, + deviceId, nodeId, netconfNode, nodeOptional); + + // ... record it ... + activeConnectors.put(nodeId, nodeHandler); + + // ... and start it + nodeHandler.connect(); } // Non-final for testing @@ -128,25 +152,6 @@ public abstract class AbstractNetconfTopology { activeConnectors.clear(); } - @Holding("this") - protected final void setupConnection(final NodeId nodeId, final Node configNode) { - final var netconfNode = configNode.augmentation(NetconfNode.class); - final var nodeOptional = configNode.augmentation(NetconfNodeAugmentedOptional.class); - - // Instantiate the handler ... - final var deviceId = NetconfNodeUtils.toRemoteDeviceId(nodeId, netconfNode); - final var deviceSalFacade = createSalFacade(deviceId, netconfNode.requireLockDatastore()); - final var nodeHandler = new NetconfNodeHandler(clientDispatcher, eventExecutor, keepaliveExecutor, - baseSchemas, schemaManager, processingExecutor, builderFactory, deviceActionFactory, deviceSalFacade, - deviceId, nodeId, netconfNode, nodeOptional); - - // ... record it ... - activeConnectors.put(nodeId, nodeHandler); - - // ... and start it - nodeHandler.connect(); - } - protected RemoteDeviceHandler createSalFacade(final RemoteDeviceId deviceId, final boolean lockDatastore) { return new NetconfTopologyDeviceSalFacade(deviceId, mountPointService, lockDatastore, dataBroker); } -- 2.36.6