Issue 876:It is possible to write ComputeServiceAdapter in a way that leads to orphaned credentials

compute/src/main/java/org/jclouds/compute/domain/NodeMetadata.java

@@ -33,16 +33,15 @@ import com.google.inject.ImplementedBy;
 @ImplementedBy(NodeMetadataImpl.class)
 public interface NodeMetadata extends ComputeMetadata {
    /**
-    * <h4>note</h4> hostname is something that is set in the operating system
+    * <h4>note</h4> hostname is something that is set in the operating system image, so this value,
-    * image, so this value, if present, cannot be guaranteed on images not
+    * if present, cannot be guaranteed on images not directly controlled by the cloud provider.
-    * directly controlled by the cloud provider.
     * 
     * @return hostname of the node, or null if unknown
     * 
     */
    @Nullable
    String getHostname();
-   
+
    /**
     * Tag used for all resources that belong to the same logical group. run, destroy commands are
     * scoped to group.
@@ -101,8 +100,11 @@ public interface NodeMetadata extends ComputeMetadata {
 
    /**
     * If possible, these are returned upon all detail requests. However, it is often the case that
-    * credentials are only available at "run" time.
+    * credentials are only available when a node is initially created.
+    * 
+    * @see ComputeServiceContext#credentialStore
     */
+   @Nullable
    LoginCredentials getCredentials();
 
    /**

compute/src/main/java/org/jclouds/compute/predicates/RefreshAndDoubleCheckOnFailUnlessStateInvalid.java

@@ -24,6 +24,7 @@ import java.util.concurrent.atomic.AtomicReference;
 import javax.annotation.Resource;
 import javax.inject.Singleton;
 
+import org.jclouds.compute.ComputeService;
 import org.jclouds.compute.domain.NodeMetadata;
 import org.jclouds.compute.domain.NodeState;
 import org.jclouds.compute.strategy.GetNodeMetadataStrategy;
@@ -35,7 +36,9 @@ import com.google.inject.Inject;
 
 /**
  * 
- * Tests to see if a node is active.
+ * The point of RefreshAndDoubleCheckOnFailUnlessStateInvalid is to keep an atomic reference to a
+ * node, so as to eliminate a redundant {@link ComputeService#getNodeMetadata} call after the
+ * predicate passes.
  * 
  * @author Adrian Cole
  */
@@ -53,7 +56,8 @@ public class RefreshAndDoubleCheckOnFailUnlessStateInvalid implements Predicate<
       this(intended, ImmutableSet.of(NodeState.ERROR), client);
    }
 
-   public RefreshAndDoubleCheckOnFailUnlessStateInvalid(NodeState intended, Set<NodeState> invalids, GetNodeMetadataStrategy client) {
+   public RefreshAndDoubleCheckOnFailUnlessStateInvalid(NodeState intended, Set<NodeState> invalids,
+            GetNodeMetadataStrategy client) {
       this.intended = intended;
       this.client = client;
       this.invalids = invalids;
@@ -74,7 +78,7 @@ public class RefreshAndDoubleCheckOnFailUnlessStateInvalid implements Predicate<
       logger.trace("%s: looking for node state %s: currently: %s", node.getId(), intended, node.getState());
       if (invalids.contains(node.getState()))
          throw new IllegalStateException("node " + node.getId() + " in location " + node.getLocation()
-                  + " is in invalid state "+node.getState());
+                  + " is in invalid state " + node.getState());
       return node.getState() == intended;
    }
 

compute/src/main/java/org/jclouds/compute/strategy/impl/AdaptingComputeServiceStrategies.java

@@ -60,8 +60,8 @@ import com.google.common.collect.Iterables;
  */
 @Singleton
 public class AdaptingComputeServiceStrategies<N, H, I, L> implements CreateNodeWithGroupEncodedIntoName,
-      DestroyNodeStrategy, GetNodeMetadataStrategy, ListNodesStrategy, RebootNodeStrategy, ResumeNodeStrategy,
+         DestroyNodeStrategy, GetNodeMetadataStrategy, ListNodesStrategy, RebootNodeStrategy, ResumeNodeStrategy,
-      SuspendNodeStrategy {
+         SuspendNodeStrategy {
    @Resource
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
@@ -73,14 +73,14 @@ public class AdaptingComputeServiceStrategies<N, H, I, L> implements CreateNodeW
 
    @Inject
    public AdaptingComputeServiceStrategies(Map<String, Credentials> credentialStore,
-         PrioritizeCredentialsFromTemplate prioritizeCredentialsFromTemplate, ComputeServiceAdapter<N, H, I, L> client,
+            PrioritizeCredentialsFromTemplate prioritizeCredentialsFromTemplate,
-         Function<N, NodeMetadata> nodeMetadataAdapter) {
+            ComputeServiceAdapter<N, H, I, L> client, Function<N, NodeMetadata> nodeMetadataAdapter) {
       this.credentialStore = checkNotNull(credentialStore, "credentialStore");
       this.prioritizeCredentialsFromTemplate = checkNotNull(prioritizeCredentialsFromTemplate,
-            "prioritizeCredentialsFromTemplate");
+               "prioritizeCredentialsFromTemplate");
       this.client = checkNotNull(client, "client");
-      this.nodeMetadataAdapter = Functions.compose(addLoginCredentials,
+      this.nodeMetadataAdapter = Functions.compose(addLoginCredentials, checkNotNull(nodeMetadataAdapter,
-            checkNotNull(nodeMetadataAdapter, "nodeMetadataAdapter"));
+               "nodeMetadataAdapter"));
    }
 
    private final Function<NodeMetadata, NodeMetadata> addLoginCredentials = new Function<NodeMetadata, NodeMetadata>() {
@@ -88,8 +88,8 @@ public class AdaptingComputeServiceStrategies<N, H, I, L> implements CreateNodeW
       @Override
       public NodeMetadata apply(NodeMetadata arg0) {
          return credentialStore.containsKey("node#" + arg0.getId()) ? NodeMetadataBuilder.fromNodeMetadata(arg0)
-               .credentials(LoginCredentials.fromCredentials(credentialStore.get("node#" + arg0.getId()))).build()
+                  .credentials(LoginCredentials.fromCredentials(credentialStore.get("node#" + arg0.getId()))).build()
-               : arg0;
+                  : arg0;
       }
 
       @Override
@@ -116,7 +116,7 @@ public class AdaptingComputeServiceStrategies<N, H, I, L> implements CreateNodeW
       return nodeMetadataAdapter.apply(node);
    }
 
-   //TODO: make reboot/resume/suspend return the node they affected
+   // TODO: make reboot/resume/suspend return the node they affected
    @Override
    public NodeMetadata rebootNode(String id) {
       NodeMetadata node = getNode(checkNotNull(id, "id"));
@@ -166,13 +166,28 @@ public class AdaptingComputeServiceStrategies<N, H, I, L> implements CreateNodeW
       checkState(name != null && name.indexOf(group) != -1, "name should have %s encoded into it", group);
       checkState(template != null, "template was null");
       checkState(template.getOptions() != null, "template options was null");
-      
+
       NodeAndInitialCredentials<N> from = client.createNodeWithGroupEncodedIntoName(group, name, template);
       LoginCredentials fromNode = from.getCredentials();
       LoginCredentials creds = prioritizeCredentialsFromTemplate.apply(template, fromNode);
-      if (creds != null)
+      String credsKey = "node#" + from.getNodeId();
-         credentialStore.put("node#" + from.getNodeId(), creds);
+      if (creds != null) {
+         credentialStore.put(credsKey, creds);
+      } else {
+         logger.trace("node(%s) creation did not return login credentials", from.getNodeId());
+      }
       NodeMetadata node = nodeMetadataAdapter.apply(from.getNode());
+      //TODO: test case that proves this
+      checkState(node.getId().equals(from.getNodeId()),
+               "nodeAndInitialCredentials.getNodeId() returned %s, while parsed nodemetadata.getId() returned %s", from
+                        .getNodeId(), node.getId());
+      if (creds != null) {
+         Credentials credsFromCache = credentialStore.get(credsKey);
+         //TODO: test case that proves this
+         checkState(node.getCredentials().equals(credsFromCache),
+                  "credentialStore.get(%s): %s does not match node(%s).getCredentials(): %s", credsKey, credsFromCache,
+                  node.getId(), node.getCredentials());
+      }
       return node;
    }
 

compute/src/test/java/org/jclouds/compute/predicates/AtomicNodePredicatesTest.java

@@ -29,6 +29,7 @@ import org.jclouds.compute.domain.NodeMetadata;
 import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.NodeState;
 import org.jclouds.compute.strategy.GetNodeMetadataStrategy;
+import org.jclouds.domain.LoginCredentials;
 import org.testng.Assert;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
@@ -77,6 +78,30 @@ public class AtomicNodePredicatesTest {
       verify(computeService);
 
    }
+
+   @Test
+   public void testRefreshUpdatesAtomicReferenceOnRecheckPendingAcceptsNewCredentials() {
+      LoginCredentials creds = LoginCredentials.builder().user("user").password("password").build();
+      NodeMetadata newNode = new NodeMetadataBuilder().id("myid").state(NodeState.UNRECOGNIZED).credentials(creds).build();
+
+      LoginCredentials creds2 = LoginCredentials.builder().user("user").password("password2").build();
+
+      NodeMetadata pending = new NodeMetadataBuilder().id("myid").state(NodeState.PENDING).credentials(creds2).build();
+      
+      GetNodeMetadataStrategy computeService = createMock(GetNodeMetadataStrategy.class);
+
+      expect(computeService.getNode("myid")).andReturn(pending);
+
+      replay(computeService);
+
+      AtomicNodeRunning nodeRunning = new AtomicNodeRunning(computeService);
+      AtomicReference<NodeMetadata> reference = new AtomicReference<NodeMetadata>(newNode);
+      Assert.assertFalse(nodeRunning.apply(reference));
+      Assert.assertEquals(reference.get(), pending);
+
+      verify(computeService);
+   }
+   
    @Test
    public void testRefreshUpdatesAtomicReferenceOnRecheckRunning() {
       NodeMetadata running = new NodeMetadataBuilder().id("myid").state(NodeState.RUNNING).build();