From 79780fe1e3a77fff43dbef9e7b901d8236bba2b9 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Thu, 16 May 2019 17:32:36 +0200 Subject: [PATCH] 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 (cherry picked from commit be977cc2f87dc153beb8542c2924c340848a453a) --- .../mount/CallHomeAuthProviderImpl.java | 153 +++++++++--------- 1 file changed, 73 insertions(+), 80 deletions(-) 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 a115bec6c8..f6af5e41f8 100644 --- 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 { - - private final AuthorizedKeysDecoder keyDecoder = new AuthorizedKeysDecoder(); - - private final ConcurrentMap byPublicKey = new ConcurrentHashMap<>(); + private abstract static class AbstractDeviceListener implements DataTreeChangeListener { @Override - public void onDataTreeChanged(@Nonnull final Collection> mods) { + public final void onDataTreeChanged(final Collection> mods) { for (DataTreeModification dataTreeModification : mods) { - DataObjectModification rootNode = dataTreeModification.getRootNode(); - process(rootNode); - } - } - - private void process(final DataObjectModification 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 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 { + private static class DeviceConfig extends AbstractDeviceListener { + private final ConcurrentMap byPublicKey = new ConcurrentHashMap<>(); + private final AuthorizedKeysDecoder keyDecoder = new AuthorizedKeysDecoder(); - private final ConcurrentMap byPublicKey = new ConcurrentHashMap<>(); + Device get(final PublicKey key) { + return byPublicKey.get(key); + } @Override - public void onDataTreeChanged(@Nonnull final Collection> mods) { - for (DataTreeModification dataTreeModification : mods) { - DataObjectModification 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 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 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); } } -- 2.36.6