Skip to content

Commit 5eb347e

Browse files
olivergondzaolivergondza
authored
Merge pull request #326 from olivergondza/node-identification-by-identity
Do not rely on Jenkins url for resource identification
2 parents a6334fe + ce174dc commit 5eb347e

7 files changed

Lines changed: 106 additions & 31 deletions

File tree

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,8 @@ public class JCloudsSlave extends AbstractCloudSlave implements TrackedItem {
5757

5858
/** metadata fields that aren't worth showing to the user. */
5959
private static final List<String> HIDDEN_METADATA_VALUES = Arrays.asList(
60-
Openstack.FINGERPRINT_KEY,
60+
Openstack.FINGERPRINT_KEY_FINGERPRINT,
61+
Openstack.FINGERPRINT_KEY_URL,
6162
JCloudsSlaveTemplate.OPENSTACK_CLOUD_NAME_KEY,
6263
JCloudsSlaveTemplate.OPENSTACK_TEMPLATE_NAME_KEY,
6364
ServerScope.METADATA_KEY

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

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,35 @@
2929

3030
import javax.annotation.CheckForNull;
3131
import javax.annotation.Nonnull;
32+
import java.util.Objects;
3233

34+
/**
35+
* Scope of FIP for deletion.
36+
*
37+
* This uses URL+ServerID, do not need to use instance identity as the ServerID is unique.
38+
*/
3339
@Restricted(NoExternalUse.class)
3440
public class FipScope {
41+
/*package*/ static final int MAX_DESCRIPTION_LENGTH = 250;
42+
43+
public static @Nonnull String getDescription(
44+
@Nonnull String url, @Nonnull String identity, @Nonnull Server server
45+
) {
46+
String description = "{ '" + Openstack.FINGERPRINT_KEY_URL + "': '" + url + "', '"
47+
+ Openstack.FINGERPRINT_KEY_FINGERPRINT + "': '" + identity
48+
+ "', 'jenkins-scope': 'server:" + server.getId() + "' }"
49+
;
50+
51+
if (description.length() < MAX_DESCRIPTION_LENGTH) return description;
3552

36-
public static @Nonnull String getDescription(@Nonnull String instanceFingerprint, @Nonnull Server server) {
37-
return "{ '" + Openstack.FINGERPRINT_KEY + "': '" + instanceFingerprint + "', 'jenkins-scope': 'server:" + server.getId() + "' }";
53+
// Avoid URL that is used only for human consumption anyway
54+
return "{ '" + Openstack.FINGERPRINT_KEY_FINGERPRINT + "': '" + identity
55+
+ "', 'jenkins-scope': 'server:" + server.getId() + "' }"
56+
;
3857
}
3958

40-
public static @CheckForNull String getServerId(@Nonnull String instanceFingerprint, @CheckForNull String description) {
41-
String scope = getScopeString(instanceFingerprint, description);
59+
public static @CheckForNull String getServerId(@Nonnull String url, @Nonnull String identity, @CheckForNull String description) {
60+
String scope = getScopeString(url, identity, description);
4261
if (scope == null) return null;
4362

4463
if (!scope.startsWith("server:")) {
@@ -47,12 +66,16 @@ public class FipScope {
4766
return scope.substring(7);
4867
}
4968

50-
private static @CheckForNull String getScopeString(@Nonnull String instanceFingerprint, @CheckForNull String description) {
69+
private static @CheckForNull String getScopeString(@Nonnull String url, @Nonnull String identity, @CheckForNull String description) {
5170
try {
5271
JSONObject jsonObject = JSONObject.fromObject(description);
5372

54-
boolean isOurs = instanceFingerprint.equals(jsonObject.getString(Openstack.FINGERPRINT_KEY));
55-
if (!isOurs) return null;
73+
// Not ours
74+
String attachedIdentity = jsonObject.optString(Openstack.FINGERPRINT_KEY_FINGERPRINT, null);
75+
String attachedUrl = jsonObject.optString(Openstack.FINGERPRINT_KEY_URL, null);
76+
if (attachedIdentity == null && attachedUrl == null) return null;
77+
if (attachedIdentity != null && !Objects.equals(attachedIdentity, identity)) return null;
78+
if (attachedUrl != null && !Objects.equals(attachedUrl, url)) return null;
5679

5780
return jsonObject.getString("jenkins-scope");
5881
} catch (net.sf.json.JSONException ex) {

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

Lines changed: 42 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@
3535
import hudson.util.FormValidation;
3636
import jenkins.model.Jenkins;
3737
import jenkins.plugins.openstack.compute.auth.OpenstackCredential;
38+
import org.apache.commons.codec.Charsets;
39+
import org.apache.commons.codec.binary.Base64;
40+
import org.apache.commons.codec.digest.DigestUtils;
41+
import org.jenkinsci.main.modules.instance_identity.InstanceIdentity;
3842
import org.kohsuke.accmod.Restricted;
3943
import org.kohsuke.accmod.restrictions.NoExternalUse;
4044
import org.openstack4j.api.Builders;
@@ -108,7 +112,10 @@
108112
public class Openstack {
109113

110114
private static final Logger LOGGER = Logger.getLogger(Openstack.class.getName());
111-
public static final String FINGERPRINT_KEY = "jenkins-instance";
115+
public static final String FINGERPRINT_KEY_URL = "jenkins-instance";
116+
public static final String FINGERPRINT_KEY_FINGERPRINT = "jenkins-identity";
117+
118+
private static String INSTANCE_FINGERPRINT;
112119

113120
private static final Comparator<Date> ACCEPT_NULLS = Comparator.nullsLast(Comparator.naturalOrder());
114121
private static final Comparator<Flavor> FLAVOR_COMPARATOR = Comparator.nullsLast(Comparator.comparing(Flavor::getName));
@@ -359,7 +366,7 @@ public Openstack(@Nonnull final OSClient<?> client) {
359366
for (NetFloatingIP ip : clientProvider.get().networking().floatingip().list()) {
360367
if (ip.getFixedIpAddress() != null) continue; // Used
361368

362-
String serverId = FipScope.getServerId(instanceFingerprint(), ip.getDescription());
369+
String serverId = FipScope.getServerId(instanceUrl(), instanceFingerprint(), ip.getDescription());
363370
if (serverId == null) continue; // Not ours
364371

365372
freeIps.add(ip.getId());
@@ -481,20 +488,41 @@ public static boolean isOccupied(@Nonnull Server server) {
481488
}
482489

483490
private boolean isOurs(@Nonnull Server server) {
484-
return instanceFingerprint().equals(server.getMetadata().get(FINGERPRINT_KEY));
491+
Map<String, String> metadata = server.getMetadata();
492+
String serverFingerprint = metadata.get(FINGERPRINT_KEY_FINGERPRINT);
493+
return serverFingerprint == null
494+
? Objects.equals(instanceUrl(), metadata.get(FINGERPRINT_KEY_URL)) // Earlier versions ware only using URL, collect severs provisioned by those
495+
: Objects.equals(instanceUrl(), metadata.get(FINGERPRINT_KEY_URL)) && Objects.equals(instanceFingerprint(), serverFingerprint)
496+
;
485497
}
486498

487499
/**
488500
* Identification for instances launched by this instance via this plugin.
489501
*
490502
* @return Identifier to filter instances we control.
491503
*/
492-
private @Nonnull String instanceFingerprint() {
504+
@VisibleForTesting
505+
public @Nonnull String instanceUrl() {
493506
String rootUrl = Jenkins.get().getRootUrl();
494507
if (rootUrl == null) throw new IllegalStateException("Jenkins instance URL is not configured");
495508
return rootUrl;
496509
}
497510

511+
/**
512+
* Binary fingerprint to be unique worldwide.
513+
*/
514+
@VisibleForTesting
515+
public @Nonnull String instanceFingerprint() {
516+
if (INSTANCE_FINGERPRINT == null) {
517+
// Use salted hash not to disclose the public key. The key is used to authenticate agent connections.
518+
INSTANCE_FINGERPRINT = DigestUtils.sha1Hex(
519+
"openstack-cloud-plugin-identity-fingerprint:"
520+
+ new String(Base64.encodeBase64(InstanceIdentity.get().getPublic().getEncoded()), Charsets.UTF_8)
521+
);
522+
}
523+
return INSTANCE_FINGERPRINT;
524+
}
525+
498526
public @Nonnull Server getServerById(@Nonnull String id) throws NoSuchElementException {
499527
Server server = clientProvider.get().compute().servers().get(id);
500528
if (server == null) throw new NoSuchElementException("No such server running: " + id);
@@ -518,6 +546,9 @@ private boolean isOurs(@Nonnull Server server) {
518546
*/
519547
public @Nonnull Server bootAndWaitActive(@Nonnull ServerCreateBuilder request, @Nonnegative int timeout) throws ActionFailed {
520548
debug("Booting machine");
549+
550+
// Mark the server as ours
551+
attachFingerprint(request);
521552
try {
522553
Server server = _bootAndWaitActive(request, timeout);
523554
if (server == null) {
@@ -550,9 +581,14 @@ private boolean isOurs(@Nonnull Server server) {
550581
}
551582
}
552583

584+
@VisibleForTesting // mocking
585+
public void attachFingerprint(@Nonnull ServerCreateBuilder request) {
586+
request.addMetadataItem(FINGERPRINT_KEY_URL, instanceUrl());
587+
request.addMetadataItem(FINGERPRINT_KEY_FINGERPRINT, instanceFingerprint());
588+
}
589+
553590
@Restricted(NoExternalUse.class) // Test hook
554591
public Server _bootAndWaitActive(@Nonnull ServerCreateBuilder request, @Nonnegative int timeout) {
555-
request.addMetadataItem(FINGERPRINT_KEY, instanceFingerprint());
556592
return clientProvider.get().compute().servers().bootAndWaitActive(request.build(), timeout);
557593
}
558594

@@ -618,7 +654,7 @@ public void destroyServer(@Nonnull Server server) throws ActionFailed {
618654
debug("Allocating floating IP for {0} in {1}", server.getName(), server.getName());
619655
NetworkingService networking = clientProvider.get().networking();
620656

621-
String desc = FipScope.getDescription(instanceFingerprint(), server);
657+
String desc = FipScope.getDescription(instanceUrl(), instanceFingerprint(), server);
622658

623659
Port port = getServerPorts(server).get(0);
624660
Network network = networking.network().list(Collections.singletonMap("name", poolName)).get(0);

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,7 @@
9191
import static org.mockito.Matchers.any;
9292
import static org.mockito.Mockito.RETURNS_SMART_NULLS;
9393
import static org.mockito.Mockito.doAnswer;
94+
import static org.mockito.Mockito.doCallRealMethod;
9495
import static org.mockito.Mockito.mock;
9596
import static org.mockito.Mockito.when;
9697
import static org.mockito.Mockito.withSettings;
@@ -374,7 +375,11 @@ public JCloudsCloud configureSlaveProvisioning(JCloudsCloud cloud, Collection<Ne
374375
Map<String, Network> nets = (Map<String, Network>) invocation.getArguments()[0];
375376
return nets.values().stream().collect(Collectors.toMap(n -> n, b -> 100));
376377
});
377-
when(os.bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenAnswer((Answer<Server>) invocation -> {
378+
when(os.bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenCallRealMethod();
379+
doCallRealMethod().when(os).attachFingerprint(any(ServerCreateBuilder.class));
380+
doCallRealMethod().when(os).instanceUrl();
381+
doCallRealMethod().when(os).instanceFingerprint();
382+
when(os._bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class))).thenAnswer((Answer<Server>) invocation -> {
378383
ServerCreateBuilder builder = (ServerCreateBuilder) invocation.getArguments()[0];
379384

380385
ServerCreate create = builder.build();
@@ -508,7 +513,6 @@ public MockServerBuilder() {
508513
when(server.getStatus()).thenReturn(Server.Status.ACTIVE);
509514
when(server.getMetadata()).thenReturn(metadata);
510515
when(server.getOsExtendedVolumesAttached()).thenReturn(Collections.singletonList(UUID.randomUUID().toString()));
511-
metadata.put("jenkins-instance", jenkins.getRootUrl()); // Mark the slave as ours
512516
}
513517

514518
public MockServerBuilder name(String name) {

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import hudson.model.FreeStyleProject;
88
import hudson.model.Label;
99
import hudson.model.Node;
10-
import hudson.model.Slave;
1110
import hudson.model.TaskListener;
1211
import hudson.node_monitors.DiskSpaceMonitorDescriptor;
1312
import hudson.plugins.sshslaves.SSHLauncher;
@@ -43,13 +42,16 @@
4342
import java.util.concurrent.ExecutionException;
4443
import java.util.concurrent.Future;
4544

45+
import static jenkins.plugins.openstack.compute.internal.Openstack.FINGERPRINT_KEY_FINGERPRINT;
4646
import static org.hamcrest.MatcherAssert.assertThat;
4747
import static org.hamcrest.Matchers.containsString;
4848
import static org.hamcrest.Matchers.empty;
4949
import static org.hamcrest.Matchers.emptyIterable;
50+
import static org.hamcrest.Matchers.emptyString;
5051
import static org.hamcrest.Matchers.equalTo;
5152
import static org.hamcrest.Matchers.instanceOf;
5253
import static org.hamcrest.Matchers.iterableWithSize;
54+
import static org.hamcrest.Matchers.not;
5355
import static org.hamcrest.Matchers.startsWith;
5456
import static org.junit.Assert.assertEquals;
5557
import static org.junit.Assert.assertFalse;
@@ -299,10 +301,13 @@ public void correctMetadataSet() throws Exception {
299301
JCloudsSlaveTemplate template = j.dummySlaveTemplate("label");
300302
final JCloudsCloud cloud = j.configureSlaveProvisioningWithFloatingIP(j.dummyCloud(template));
301303

304+
assertThat(cloud.getOpenstack().instanceUrl(), not(emptyString()));
305+
assertThat(cloud.getOpenstack().instanceFingerprint(), not(emptyString()));
306+
System.out.println(cloud.getOpenstack().instanceFingerprint());
302307
Server server = template.provisionServer(null, null);
303308
Map<String, String> m = server.getMetadata();
304-
305-
assertEquals(j.getURL().toExternalForm(), m.get(Openstack.FINGERPRINT_KEY));
309+
assertEquals(cloud.getOpenstack().instanceUrl(), m.get(Openstack.FINGERPRINT_KEY_URL));
310+
assertEquals(cloud.getOpenstack().instanceFingerprint(), m.get(FINGERPRINT_KEY_FINGERPRINT));
306311
assertEquals(cloud.name, m.get(JCloudsSlaveTemplate.OPENSTACK_CLOUD_NAME_KEY));
307312
assertEquals(template.getName(), m.get(JCloudsSlaveTemplate.OPENSTACK_TEMPLATE_NAME_KEY));
308313
assertEquals(new ServerScope.Node(server.getName()).getValue(), m.get(ServerScope.METADATA_KEY));

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

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,14 +34,16 @@
3434

3535
public class FipScopeTest {
3636

37-
private static final String FNGRPRNT = "https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins";
38-
public static final String REALISTIC_EXAMPLE
39-
= "{ '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' }";
37+
private static final String URL = "https://some-quite-long-jenkins-url-to-make-sure-it-fits.acme.com:8080/jenkins";
38+
private static final String FINGERPRINT = "3919bce9a2f5fc4f730bd6462e23454ecb1fb089";
39+
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' }";
40+
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' }";
41+
public static final String ABBREVIATED_DESCRIPTION = "{ 'jenkins-identity': '3919bce9a2f5fc4f730bd6462e23454ecb1fb089', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }";
4042

4143
@Test
4244
public void getDescription() {
43-
assertEquals(REALISTIC_EXAMPLE, FipScope.getDescription(FNGRPRNT, server("d8eca2df-7795-4069-b2ef-1e2412491345")));
44-
assertThat("Possible length is larger than maximal size of FIP description", REALISTIC_EXAMPLE.length(), lessThanOrEqualTo(250));
45+
assertEquals(EXPECTED_DESCRIPTION, FipScope.getDescription(URL, FINGERPRINT, server("d8eca2df-7795-4069-b2ef-1e2412491345")));
46+
assertThat("Possible length is larger than maximal size of FIP description", EXPECTED_DESCRIPTION.length(), lessThanOrEqualTo(FipScope.MAX_DESCRIPTION_LENGTH));
4547
}
4648

4749
private Server server(String id) {
@@ -52,13 +54,16 @@ private Server server(String id) {
5254

5355
@Test
5456
public void getServerId() {
55-
assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(FNGRPRNT, REALISTIC_EXAMPLE));
57+
// assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, EXPECTED_DESCRIPTION));
58+
assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, LEGACY_DESCRIPTION));
59+
assertEquals("d8eca2df-7795-4069-b2ef-1e2412491345", FipScope.getServerId(URL, FINGERPRINT, ABBREVIATED_DESCRIPTION));
5660

57-
assertNull(FipScope.getServerId(FNGRPRNT, "{ 'jenkins-instance': 'https://some.other.jenkins.io', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
58-
assertNull(FipScope.getServerId(FNGRPRNT, "{ [ 'not', 'the', 'json', 'you', 'expect' ] }"));
59-
assertNull(FipScope.getServerId(FNGRPRNT, "[ 'not', 'the', 'json', 'you', 'expect' ]"));
60-
assertNull(FipScope.getServerId(FNGRPRNT, "Human description"));
61-
assertNull(FipScope.getServerId(FNGRPRNT, ""));
62-
assertNull(FipScope.getServerId(FNGRPRNT, null));
61+
assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ 'jenkins-instance': 'https://some.other.jenkins.io', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
62+
assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ 'jenkins-identity': 'different-than-expected', 'jenkins-scope': 'server:d8eca2df-7795-4069-b2ef-1e2412491345' }"));
63+
assertNull(FipScope.getServerId(URL, FINGERPRINT, "{ foo: [ 'not', 'the', 'json', 'you', 'expect' ] }"));
64+
assertNull(FipScope.getServerId(URL, FINGERPRINT, "[ 'not', 'the', 'json', 'you', 'expect' ]"));
65+
assertNull(FipScope.getServerId(URL, FINGERPRINT, "Human description"));
66+
assertNull(FipScope.getServerId(URL, FINGERPRINT, ""));
67+
assertNull(FipScope.getServerId(URL, FINGERPRINT, null));
6368
}
6469
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@ public void deleteAfterFailedBoot() {
379379

380380
doReturn(server).when(os)._bootAndWaitActive(any(ServerCreateBuilder.class), any(Integer.class));
381381
doThrow(new Openstack.ActionFailed("Fake deletion failure")).when(os).destroyServer(server);
382+
doNothing().when(os).attachFingerprint(any(ServerCreateBuilder.class));
382383

383384
try {
384385
os.bootAndWaitActive(mock(ServerCreateBuilder.class), 1);

0 commit comments

Comments
 (0)