Fix DeviceOp DTCL

The did not account for the possiblity of null host keys, hence
it incurred NPEs when it was attempting to update its internal
cache.

This also refactor the code so that we share the code path between
DeviceOp and DeviceConfig.

JIRA: NETCONF-615
Change-Id: I1e3329604e1686ee4a79baeea96dc354c49c9e04
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit be977cc2f87dc153beb8542c2924c340848a453a)

diff --git a/netconf/callhome-provider/src/main/java/org/opendaylight/netconf/callhome/mount/CallHomeAuthProviderImpl.java b/netconf/callhome-provider/src/main/java/org/opendaylight/netconf/callhome/mount/CallHomeAuthProviderImpl.java
index a115bec6c897fedefb04685ddf00cb89cf13c47d..f6af5e41f8145b661ceb6c5335630effbff3658c 100644 (file)
--- a/netconf/callhome-provider/src/main/java/org/opendaylight/netconf/callhome/mount/CallHomeAuthProviderImpl.java
+++ b/netconf/callhome-provider/src/main/java/org/opendaylight/netconf/callhome/mount/CallHomeAuthProviderImpl.java
@@ -7,7 +7,6 @@
  */
 package org.opendaylight.netconf.callhome.mount;
 
-import com.google.common.base.Objects;
 import com.google.common.net.InetAddresses;
 import java.io.IOException;
 import java.net.InetSocketAddress;
@@ -20,6 +19,7 @@ import java.util.concurrent.ConcurrentMap;
 import javax.annotation.Nonnull;
 import org.opendaylight.mdsal.binding.api.DataBroker;
 import org.opendaylight.mdsal.binding.api.DataObjectModification;
+import org.opendaylight.mdsal.binding.api.DataObjectModification.ModificationType;
 import org.opendaylight.mdsal.binding.api.DataTreeChangeListener;
 import org.opendaylight.mdsal.binding.api.DataTreeIdentifier;
 import org.opendaylight.mdsal.binding.api.DataTreeModification;
@@ -126,119 +126,112 @@ public class CallHomeAuthProviderImpl implements CallHomeAuthorizationProvider,
         return remoteAddress.toString();
     }
 
-    private static class DeviceConfig implements DataTreeChangeListener<Device> {
-
-        private final AuthorizedKeysDecoder keyDecoder = new AuthorizedKeysDecoder();
-
-        private final ConcurrentMap<PublicKey, Device> byPublicKey = new ConcurrentHashMap<>();
+    private abstract static class AbstractDeviceListener implements DataTreeChangeListener<Device> {
 
         @Override
-        public void onDataTreeChanged(@Nonnull final Collection<DataTreeModification<Device>> mods) {
+        public final void onDataTreeChanged(final Collection<DataTreeModification<Device>> mods) {
             for (DataTreeModification<Device> dataTreeModification : mods) {
-                DataObjectModification<Device> rootNode = dataTreeModification.getRootNode();
-                process(rootNode);
-            }
-        }
-
-        private void process(final DataObjectModification<Device> deviceMod) {
-            Device before = deviceMod.getDataBefore();
-            Device after = deviceMod.getDataAfter();
-
-            if (before == null) {
-                putDevice(after);
-            } else if (after == null) {
-                // Delete
-                removeDevice(before);
-            } else {
-                if (!Objects.equal(before.getSshHostKey(), after.getSshHostKey())) {
-                    // key changed // we should remove previous key.
-                    removeDevice(before);
+                final DataObjectModification<Device> deviceMod = dataTreeModification.getRootNode();
+                final ModificationType modType = deviceMod.getModificationType();
+                switch (modType) {
+                    case DELETE:
+                        deleteDevice(deviceMod.getDataBefore());
+                        break;
+                    case SUBTREE_MODIFIED:
+                    case WRITE:
+                        deleteDevice(deviceMod.getDataBefore());
+                        writeDevice(deviceMod.getDataAfter());
+                        break;
+                    default:
+                        throw new IllegalStateException("Unhandled modification type " + modType);
                 }
-                putDevice(after);
             }
         }
 
-        private void putDevice(final Device device) {
-            PublicKey key = publicKey(device);
-            if (key == null) {
-                return;
+        private void deleteDevice(final Device dataBefore) {
+            if (dataBefore != null) {
+                final String publicKey = dataBefore.getSshHostKey();
+                if (publicKey != null) {
+                    LOG.debug("Removing device {}", dataBefore.getUniqueId());
+                    removeDevice(publicKey, dataBefore);
+                } else {
+                    LOG.debug("Ignoring removal of device {}, no host key present", dataBefore.getUniqueId());
+                }
             }
-            byPublicKey.put(key, device);
         }
 
-        private void removeDevice(final Device device) {
-            PublicKey key = publicKey(device);
-            if (key == null) {
-                return;
+        private void writeDevice(final Device dataAfter) {
+            final String publicKey = dataAfter.getSshHostKey();
+            if (publicKey != null) {
+                LOG.debug("Adding device {}", dataAfter.getUniqueId());
+                addDevice(publicKey, dataAfter);
+            } else {
+                LOG.debug("Ignoring addition of device {}, no host key present", dataAfter.getUniqueId());
             }
-            byPublicKey.remove(key);
         }
 
-        private PublicKey publicKey(final Device device) {
-            String hostKey = device.getSshHostKey();
-            try {
-                return keyDecoder.decodePublicKey(hostKey);
-            } catch (GeneralSecurityException e) {
-                LOG.error("Unable to decode SSH key for {}. Ignoring update for this device", device.getUniqueId(), e);
-                return null;
-            }
-        }
+        abstract void addDevice(String publicKey, Device device);
 
-        private Device get(final PublicKey key) {
-            return byPublicKey.get(key);
-        }
+        abstract void removeDevice(String publicKey, Device device);
     }
 
-    private static class DeviceOp implements DataTreeChangeListener<Device> {
+    private static class DeviceConfig extends AbstractDeviceListener {
+        private final ConcurrentMap<PublicKey, Device> byPublicKey = new ConcurrentHashMap<>();
+        private final AuthorizedKeysDecoder keyDecoder = new AuthorizedKeysDecoder();
 
-        private final ConcurrentMap<String, Device> byPublicKey = new ConcurrentHashMap<>();
+        Device get(final PublicKey key) {
+            return byPublicKey.get(key);
+        }
 
         @Override
-        public void onDataTreeChanged(@Nonnull final Collection<DataTreeModification<Device>> mods) {
-            for (DataTreeModification<Device> dataTreeModification : mods) {
-                DataObjectModification<Device> rootNode = dataTreeModification.getRootNode();
-                process(rootNode);
+        void addDevice(final String publicKey, final Device device) {
+            final PublicKey key = publicKey(publicKey, device);
+            if (key != null) {
+                byPublicKey.put(key, device);
             }
         }
 
-        private void process(final DataObjectModification<Device> deviceMod) {
-            Device before = deviceMod.getDataBefore();
-            Device after = deviceMod.getDataAfter();
-
-            if (before == null) {
-                putDevice(after);
-            } else if (after == null) {
-                // Delete
-                removeDevice(before);
-            } else {
-                if (!Objects.equal(before.getSshHostKey(), after.getSshHostKey())) {
-                    // key changed // we should remove previous key.
-                    removeDevice(before);
-                }
-                putDevice(after);
+        @Override
+        void removeDevice(final String publicKey, final Device device) {
+            final PublicKey key = publicKey(publicKey, device);
+            if (key != null) {
+                byPublicKey.remove(key);
             }
         }
 
-        private void putDevice(final Device device) {
-            String key = device.getSshHostKey();
-            byPublicKey.put(key, device);
+        private PublicKey publicKey(final String hostKey, final Device device) {
+            try {
+                return keyDecoder.decodePublicKey(hostKey);
+            } catch (GeneralSecurityException e) {
+                LOG.error("Unable to decode SSH key for {}. Ignoring update for this device", device.getUniqueId(), e);
+                return null;
+            }
         }
+    }
 
-        private void removeDevice(final Device device) {
-            String key = device.getSshHostKey();
-            byPublicKey.remove(key);
-        }
+    private static class DeviceOp extends AbstractDeviceListener {
+        private final ConcurrentMap<String, Device> byPublicKey = new ConcurrentHashMap<>();
 
         Device get(final PublicKey serverKey) {
-            String skey = "";
-
+            final String skey;
             try {
                 skey = AuthorizedKeysDecoder.encodePublicKey(serverKey);
-                return byPublicKey.get(skey);
             } catch (IOException | IllegalArgumentException e) {
-                LOG.error("Unable to encode server key: {}", skey, e);
+                LOG.error("Unable to encode server key: {}", serverKey, e);
                 return null;
             }
+
+            return byPublicKey.get(skey);
+        }
+
+        @Override
+        void removeDevice(final String publicKey, final Device device) {
+            byPublicKey.remove(publicKey);
+        }
+
+        @Override
+        void addDevice(final String publicKey, final Device device) {
+            byPublicKey.put(publicKey, device);
         }
     }