Do not rely on Jenkins url for resource identification

plugin/src/main/java/jenkins/plugins/openstack/compute/JCloudsSlave.java

@@ -57,7 +57,8 @@ public class JCloudsSlave extends AbstractCloudSlave implements TrackedItem {
 
     /** metadata fields that aren't worth showing to the user. */
     private static final List<String> HIDDEN_METADATA_VALUES = Arrays.asList(
-            Openstack.FINGERPRINT_KEY,
+            Openstack.FINGERPRINT_KEY_FINGERPRINT,
+            Openstack.FINGERPRINT_KEY_URL,
             JCloudsSlaveTemplate.OPENSTACK_CLOUD_NAME_KEY,
             JCloudsSlaveTemplate.OPENSTACK_TEMPLATE_NAME_KEY,
             ServerScope.METADATA_KEY

plugin/src/main/java/jenkins/plugins/openstack/compute/internal/FipScope.java

@@ -29,16 +29,35 @@
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import java.util.Objects;
 
+/**
+ * Scope of FIP for deletion.
+ *
+ * This uses URL+ServerID, do not need to use instance identity as the ServerID is unique.
+ */
 @Restricted(NoExternalUse.class)
 public class FipScope {
+    /*package*/ static final int MAX_DESCRIPTION_LENGTH = 250;
+
+    public static @Nonnull String getDescription(
+            @Nonnull String url, @Nonnull String identity, @Nonnull Server server
+    ) {
+        String description = "{ '" + Openstack.FINGERPRINT_KEY_URL + "': '" + url + "', '"
+                + Openstack.FINGERPRINT_KEY_FINGERPRINT + "': '" + identity
+                + "', 'jenkins-scope': 'server:" + server.getId() + "' }"
+        ;
+
+        if (description.length() < MAX_DESCRIPTION_LENGTH) return description;
 
-    public static @Nonnull String getDescription(@Nonnull String instanceFingerprint, @Nonnull Server server) {
-        return "{ '" + Openstack.FINGERPRINT_KEY + "': '" + instanceFingerprint + "', 'jenkins-scope': 'server:" + server.getId() + "' }";
+        // Avoid URL that is used only for human consumption anyway
+        return "{ '" + Openstack.FINGERPRINT_KEY_FINGERPRINT + "': '" + identity
+                + "', 'jenkins-scope': 'server:" + server.getId() + "' }"
+        ;
     }
 
-    public static @CheckForNull String getServerId(@Nonnull String instanceFingerprint, @CheckForNull String description) {
-        String scope = getScopeString(instanceFingerprint, description);
+    public static @CheckForNull String getServerId(@Nonnull String url, @Nonnull String identity, @CheckForNull String description) {
+        String scope = getScopeString(url, identity, description);
         if (scope == null) return null;
 
         if (!scope.startsWith("server:")) {
@@ -47,12 +66,16 @@ public class FipScope {
         return scope.substring(7);
     }
 
-    private static @CheckForNull String getScopeString(@Nonnull String instanceFingerprint, @CheckForNull String description) {
+    private static @CheckForNull String getScopeString(@Nonnull String url, @Nonnull String identity, @CheckForNull String description) {
         try {
             JSONObject jsonObject = JSONObject.fromObject(description);
 
-            boolean isOurs = instanceFingerprint.equals(jsonObject.getString(Openstack.FINGERPRINT_KEY));
-            if (!isOurs) return null;
+            // Not ours
+            String attachedIdentity = jsonObject.optString(Openstack.FINGERPRINT_KEY_FINGERPRINT, null);
+            String attachedUrl = jsonObject.optString(Openstack.FINGERPRINT_KEY_URL, null);
+            if (attachedIdentity == null && attachedUrl == null) return null;
+            if (attachedIdentity != null && !Objects.equals(attachedIdentity, identity)) return null;
+            if (attachedUrl != null && !Objects.equals(attachedUrl, url)) return null;
 
             return jsonObject.getString("jenkins-scope");
         } catch (net.sf.json.JSONException ex) {

plugin/src/main/java/jenkins/plugins/openstack/compute/internal/Openstack.java

@@ -35,6 +35,10 @@
 import hudson.util.FormValidation;
 import jenkins.model.Jenkins;
 import jenkins.plugins.openstack.compute.auth.OpenstackCredential;
+import org.apache.commons.codec.Charsets;
+import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.codec.digest.DigestUtils;
+import org.jenkinsci.main.modules.instance_identity.InstanceIdentity;
 import org.kohsuke.accmod.Restricted;
 import org.kohsuke.accmod.restrictions.NoExternalUse;
 import org.openstack4j.api.Builders;
@@ -108,7 +112,10 @@
 public class Openstack {
 
     private static final Logger LOGGER = Logger.getLogger(Openstack.class.getName());
-    public static final String FINGERPRINT_KEY = "jenkins-instance";
+    public static final String FINGERPRINT_KEY_URL = "jenkins-instance";
+    public static final String FINGERPRINT_KEY_FINGERPRINT = "jenkins-identity";
+
+    private static String INSTANCE_FINGERPRINT;
 
     private static final Comparator<Date> ACCEPT_NULLS = Comparator.nullsLast(Comparator.naturalOrder());
     private static final Comparator<Flavor> FLAVOR_COMPARATOR = Comparator.nullsLast(Comparator.comparing(Flavor::getName));
@@ -359,7 +366,7 @@ public Openstack(@Nonnull final OSClient<?> client) {
         for (NetFloatingIP ip : clientProvider.get().networking().floatingip().list()) {
             if (ip.getFixedIpAddress() != null) continue; // Used
 
-            String serverId = FipScope.getServerId(instanceFingerprint(), ip.getDescription());
+            String serverId = FipScope.getServerId(instanceUrl(), instanceFingerprint(), ip.getDescription());
             if (serverId == null) continue; // Not ours
 
             freeIps.add(ip.getId());
@@ -481,20 +488,41 @@ public static boolean isOccupied(@Nonnull Server server) {
     }
 
     private boolean isOurs(@Nonnull Server server) {
-        return instanceFingerprint().equals(server.getMetadata().get(FINGERPRINT_KEY));
+        Map<String, String> metadata = server.getMetadata();
+        String serverFingerprint = metadata.get(FINGERPRINT_KEY_FINGERPRINT);
+        return serverFingerprint == null
+                ? Objects.equals(instanceUrl(), metadata.get(FINGERPRINT_KEY_URL)) // Earlier versions ware only using URL, collect severs provisioned by those
+                : Objects.equals(instanceUrl(), metadata.get(FINGERPRINT_KEY_URL)) && Objects.equals(instanceFingerprint(), serverFingerprint)
+        ;
     }
 
     /**
      * Identification for instances launched by this instance via this plugin.
      *
      * @return Identifier to filter instances we control.
      */
-    private @Nonnull String instanceFingerprint() {
+    @VisibleForTesting
+    public @Nonnull String instanceUrl() {
         String rootUrl = Jenkins.get().getRootUrl();
         if (rootUrl == null) throw new IllegalStateException("Jenkins instance URL is not configured");
         return rootUrl;
     }
 
+    /**
+     * Binary fingerprint to be unique worldwide.
+     */
+    @VisibleForTesting
+    public @Nonnull String instanceFingerprint() {
+        if (INSTANCE_FINGERPRINT == null) {
+            // Use salted hash not to disclose the public key. The key is used to authenticate agent connections.
+            INSTANCE_FINGERPRINT = DigestUtils.sha1Hex(
+                    "openstack-cloud-plugin-identity-fingerprint:"
+                    + new String(Base64.encodeBase64(InstanceIdentity.get().getPublic().getEncoded()), Charsets.UTF_8)
+            );
+        }
+        return INSTANCE_FINGERPRINT;
+    }
+
     public @Nonnull Server getServerById(@Nonnull String id) throws NoSuchElementException {
         Server server = clientProvider.get().compute().servers().get(id);
         if (server == null) throw new NoSuchElementException("No such server running: " + id);
@@ -518,6 +546,9 @@ private boolean isOurs(@Nonnull Server server) {
      */
     public @Nonnull Server bootAndWaitActive(@Nonnull ServerCreateBuilder request, @Nonnegative int timeout) throws ActionFailed {
         debug("Booting machine");
+
+        // Mark the server as ours
+        attachFingerprint(request);
         try {
             Server server = _bootAndWaitActive(request, timeout);
             if (server == null) {
@@ -550,9 +581,14 @@ private boolean isOurs(@Nonnull Server server) {
         }
     }
 
+    @VisibleForTesting // mocking
+    public void attachFingerprint(@Nonnull ServerCreateBuilder request) {
+        request.addMetadataItem(FINGERPRINT_KEY_URL, instanceUrl());
+        request.addMetadataItem(FINGERPRINT_KEY_FINGERPRINT, instanceFingerprint());
+    }
+
     @Restricted(NoExternalUse.class) // Test hook
     public Server _bootAndWaitActive(@Nonnull ServerCreateBuilder request, @Nonnegative int timeout) {
-        request.addMetadataItem(FINGERPRINT_KEY, instanceFingerprint());
         return clientProvider.get().compute().servers().bootAndWaitActive(request.build(), timeout);
     }
 
@@ -618,7 +654,7 @@ public void destroyServer(@Nonnull Server server) throws ActionFailed {
         debug("Allocating floating IP for {0} in {1}", server.getName(), server.getName());
         NetworkingService networking = clientProvider.get().networking();
 
-        String desc = FipScope.getDescription(instanceFingerprint(), server);
+        String desc = FipScope.getDescription(instanceUrl(), instanceFingerprint(), server);
 
         Port port = getServerPorts(server).get(0);
         Network network = networking.network().list(Collections.singletonMap("name", poolName)).get(0);

plugin/src/test/java/jenkins/plugins/openstack/PluginTestRule.java

@@ -91,6 +91,7 @@
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.RETURNS_SMART_NULLS;
 import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.doCallRealMethod;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static org.mockito.Mockito.withSettings;
@@ -374,7 +375,11 @@ public JCloudsCloud configureSlaveProvisioning(JCloudsCloud cloud, Collection<Ne
             Map<String, Network> nets = (Map<String, Network>) invocation.getArguments()[0];
             return nets.values().stream().collect(Collectors.toMap(n -> n, b -> 100));
         });
-        when(os.bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenAnswer((Answer<Server>) invocation -> {
+        when(os.bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenCallRealMethod();
+        doCallRealMethod().when(os).attachFingerprint(any(ServerCreateBuilder.class));
+        doCallRealMethod().when(os).instanceUrl();
+        doCallRealMethod().when(os).instanceFingerprint();
+        when(os._bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenAnswer((Answer<Server>) invocation -> {
             ServerCreateBuilder builder = (ServerCreateBuilder) invocation.getArguments()[0];
 
             ServerCreate create = builder.build();
@@ -508,7 +513,6 @@ public MockServerBuilder() {
             when(server.getStatus()).thenReturn(Server.Status.ACTIVE);
             when(server.getMetadata()).thenReturn(metadata);
             when(server.getOsExtendedVolumesAttached()).thenReturn(Collections.singletonList(UUID.randomUUID().toString()));
-            metadata.put("jenkins-instance", jenkins.getRootUrl()); // Mark the slave as ours
         }
 
         public MockServerBuilder name(String name) {

plugin/src/test/java/jenkins/plugins/openstack/compute/ProvisioningTest.java

@@ -7,7 +7,6 @@
 import hudson.model.FreeStyleProject;
 import hudson.model.Label;
 import hudson.model.Node;
-import hudson.model.Slave;
 import hudson.model.TaskListener;
 import hudson.node_monitors.DiskSpaceMonitorDescriptor;
 import hudson.plugins.sshslaves.SSHLauncher;
@@ -43,13 +42,16 @@
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 
+import static jenkins.plugins.openstack.compute.internal.Openstack.FINGERPRINT_KEY_FINGERPRINT;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.empty;
 import static org.hamcrest.Matchers.emptyIterable;
+import static org.hamcrest.Matchers.emptyString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.iterableWithSize;
+import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -299,10 +301,13 @@ public void correctMetadataSet() throws Exception {
         JCloudsSlaveTemplate template = j.dummySlaveTemplate("label");
         final JCloudsCloud cloud = j.configureSlaveProvisioningWithFloatingIP(j.dummyCloud(template));
 
+        assertThat(cloud.getOpenstack().instanceUrl(), not(emptyString()));
+        assertThat(cloud.getOpenstack().instanceFingerprint(), not(emptyString()));
+System.out.println(cloud.getOpenstack().instanceFingerprint());
         Server server = template.provisionServer(null, null);
         Map<String, String> m = server.getMetadata();
-
-        assertEquals(j.getURL().toExternalForm(), m.get(Openstack.FINGERPRINT_KEY));
+        assertEquals(cloud.getOpenstack().instanceUrl(), m.get(Openstack.FINGERPRINT_KEY_URL));
+        assertEquals(cloud.getOpenstack().instanceFingerprint(), m.get(FINGERPRINT_KEY_FINGERPRINT));
         assertEquals(cloud.name, m.get(JCloudsSlaveTemplate.OPENSTACK_CLOUD_NAME_KEY));
         assertEquals(template.getName(), m.get(JCloudsSlaveTemplate.OPENSTACK_TEMPLATE_NAME_KEY));
         assertEquals(new ServerScope.Node(server.getName()).getValue(), m.get(ServerScope.METADATA_KEY));

plugin/src/test/java/jenkins/plugins/openstack/compute/internal/FipScopeTest.java

@@ -34,14 +34,16 @@
 
 public class FipScopeTest {
 
-    private static final String FNGRPRNT = "https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins";
-    public static final String REALISTIC_EXAMPLE
-            = "{ 'jenkins-instance': 'https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }";
+    private static final String URL = "https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins";
+    private static final String FINGERPRINT = "3919bce9a2f5fc4f730bd6462e23454ecb1fb089";
+    public static final String LEGACY_DESCRIPTION = "{ 'jenkins-instance': 'https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }";
+    public static final String EXPECTED_DESCRIPTION = "{ 'jenkins-instance': 'https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins', 'jenkins-identity': '3919bce9a2f5fc4f730bd6462e23454ecb1fb089', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }";
+    public static final String ABBREVIATED_DESCRIPTION = "{ 'jenkins-identity': '3919bce9a2f5fc4f730bd6462e23454ecb1fb089', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }";
 
     @Test
     public void getDescription() {
-        assertEquals(REALISTIC_EXAMPLE, FipScope.getDescription(FNGRPRNT, server("d8eca2df-7795-4069-b2ef-1e2412491345")));
-        assertThat("Possible length is larger than maximal size of FIP description", REALISTIC_EXAMPLE.length(), lessThanOrEqualTo(250));
+        assertEquals(EXPECTED_DESCRIPTION, FipScope.getDescription(URL, FINGERPRINT, server("d8eca2df-7795-4069-b2ef-1e2412491345")));
+        assertThat("Possible length is larger than maximal size of FIP description", EXPECTED_DESCRIPTION.length(), lessThanOrEqualTo(FipScope.MAX_DESCRIPTION_LENGTH));
     }
 
     private Server server(String id) {
@@ -52,13 +54,16 @@ private Server server(String id) {
 
     @Test
     public void getServerId() {
-        assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(FNGRPRNT, REALISTIC_EXAMPLE));
+//        assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, EXPECTED_DESCRIPTION));
+        assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, LEGACY_DESCRIPTION));
+        assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, ABBREVIATED_DESCRIPTION));
 
-        assertNull(FipScope.getServerId(FNGRPRNT, "{ 'jenkins-instance': 'https://some.other.jenkins.io', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
-        assertNull(FipScope.getServerId(FNGRPRNT, "{ [ 'not', 'the', 'json', 'you', 'expect' ] }"));
-        assertNull(FipScope.getServerId(FNGRPRNT, "[ 'not', 'the', 'json', 'you', 'expect' ]"));
-        assertNull(FipScope.getServerId(FNGRPRNT, "Human description"));
-        assertNull(FipScope.getServerId(FNGRPRNT, ""));
-        assertNull(FipScope.getServerId(FNGRPRNT, null));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ 'jenkins-instance': 'https://some.other.jenkins.io', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ 'jenkins-identity': 'different-than-expected', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ foo: [ 'not', 'the', 'json', 'you', 'expect' ] }"));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, "[ 'not', 'the', 'json', 'you', 'expect' ]"));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, "Human description"));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, ""));
+        assertNull(FipScope.getServerId(URL, FINGERPRINT, null));
     }
 }

plugin/src/test/java/jenkins/plugins/openstack/compute/internal/OpenstackTest.java

@@ -379,6 +379,7 @@ public void deleteAfterFailedBoot() {
 
         doReturn(server).when(os)._bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class));
         doThrow(new Openstack.ActionFailed("Fake deletion failure")).when(os).destroyServer(server);
+        doNothing().when(os).attachFingerprint(any(ServerCreateBuilder.class));
 
         try {
             os.bootAndWaitActive(mock(ServerCreateBuilder.class), 1);