Unifying s3 client and auth across all plugins

indix · May 12, 2017 · 0796662 · 0796662
1 parent 38beb97
commit 0796662
Show file tree

Hide file tree

Showing 7 changed files with 72 additions and 86 deletions.
diff --git a/fetch/src/main/java/com/indix/gocd/s3fetch/FetchExecutor.java b/fetch/src/main/java/com/indix/gocd/s3fetch/FetchExecutor.java
@@ -43,8 +43,8 @@ public TaskExecutionResult execute(Config config, final Context context) {
         }
     }
 
-    public S3ArtifactStore getS3ArtifactStore(GoEnvironment env, String bucket) {
-        return new S3ArtifactStore(s3Client(env), bucket);
+    protected S3ArtifactStore getS3ArtifactStore(GoEnvironment env, String bucket) {
+        return new S3ArtifactStore(env, bucket);
     }
 
     private void setupDestinationDirectory(String destination) {
@@ -58,18 +58,7 @@ private void setupDestinationDirectory(String destination) {
         }
     }
 
-    public AmazonS3Client s3Client(GoEnvironment env) {
-        AmazonS3Client client;
-        if (env.hasAWSUseIamRole()) {
-            client = new AmazonS3Client(new InstanceProfileCredentialsProvider());
-        } else {
-            client = new AmazonS3Client(new BasicAWSCredentials(env.get(AWS_ACCESS_KEY_ID), env.get(AWS_SECRET_ACCESS_KEY)));
-        }
-        return client;
-    }
-
-
-    public String getArtifactsLocationTemplate(Config config, GoEnvironment env) {
+    private String getArtifactsLocationTemplate(Config config, GoEnvironment env) {
         String repoName = config.getRepo();
         String packageName = config.getPkg();
         logger.debug(String.format("S3 fetch config uses repoName=%s and packageName=%s", repoName, packageName));

diff --git a/fetch/src/test/java/com/indix/gocd/s3fetch/FetchExecutorTest.java b/fetch/src/test/java/com/indix/gocd/s3fetch/FetchExecutorTest.java
@@ -27,7 +27,6 @@
 
 
 public class FetchExecutorTest {
-    private final String destination = "artifacts";
     private final String bucket = "gocd";
     Maps.MapBuilder<String, String> mockEnvironmentVariables;
     private FetchExecutor fetchExecutor;
@@ -82,8 +81,9 @@ public void shouldBeFailureIfFetchConfigNotValid() {
     public void shouldBeFailureIfUnableToFetchArtifacts() {
         Map<String, String> mockVariables = mockEnvironmentVariables.build();
         AmazonS3Client mockClient = mockClient();
+        S3ArtifactStore store = new S3ArtifactStore(mockClient, bucket);
         doThrow(new AmazonClientException("Exception message")).when(mockClient).listObjects(any(ListObjectsRequest.class));
-        doReturn(mockClient).when(fetchExecutor).s3Client(any(GoEnvironment.class));
+        doReturn(store).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(bucket));
 
         TaskExecutionResult result = fetchExecutor.execute(config, mockContext(mockVariables));
 
@@ -95,7 +95,8 @@ public void shouldBeFailureIfUnableToFetchArtifacts() {
     public void shouldBeSuccessResultOnSuccessfulFetch() {
         Map<String, String> mockVariables = mockEnvironmentVariables.build();
         AmazonS3Client mockClient = mockClient();
-        doReturn(mockClient).when(fetchExecutor).s3Client(any(GoEnvironment.class));
+        S3ArtifactStore store = new S3ArtifactStore(mockClient, bucket);
+        doReturn(store).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(bucket));
         S3ArtifactStore mockStore = mockStore();
 
         doReturn(mockStore).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), any(String.class));
@@ -116,11 +117,9 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithDashesInTheName() {
                 .with("GO_PACKAGE_REPO_WITH_DASH_PACKAGE_WITH_DASH_STAGE_NAME", "defaultStage")
                 .with("GO_PACKAGE_REPO_WITH_DASH_PACKAGE_WITH_DASH_JOB_NAME", "defaultJob")
                 .build();
-        AmazonS3Client mockClient = mockClient();
-        doReturn(mockClient).when(fetchExecutor).s3Client(any(GoEnvironment.class));
-        S3ArtifactStore mockStore = mockStore();
+        S3ArtifactStore store = mockStore();
+        doReturn(store).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(bucket));
 
-        doReturn(mockStore).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), any(String.class));
         config = new Config(Maps.builder()
                 .with(Constants.REPO, Maps.builder().with("value", "repo-with-dash").build())
                 .with(Constants.PACKAGE, Maps.builder().with("value", "package-with-dash").build())
@@ -130,7 +129,7 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithDashesInTheName() {
 
         assertTrue(result.isSuccessful());
         assertThat(result.message(), is("Fetched all artifacts"));
-        verify(mockStore, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
+        verify(store, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
     }
 
     @Test
@@ -142,11 +141,9 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithPeriodsInTheName() {
                 .with("GO_PACKAGE_REPO_WITH_PERIOD_PACKAGE_WITH_PERIOD_STAGE_NAME", "defaultStage")
                 .with("GO_PACKAGE_REPO_WITH_PERIOD_PACKAGE_WITH_PERIOD_JOB_NAME", "defaultJob")
                 .build();
-        AmazonS3Client mockClient = mockClient();
-        doReturn(mockClient).when(fetchExecutor).s3Client(any(GoEnvironment.class));
-        S3ArtifactStore mockStore = mockStore();
+        S3ArtifactStore store = mockStore();
+        doReturn(store).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(bucket));
 
-        doReturn(mockStore).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), any(String.class));
         config = new Config(Maps.builder()
                 .with(Constants.REPO, Maps.builder().with("value", "repo-with.period").build())
                 .with(Constants.PACKAGE, Maps.builder().with("value", "package-with.period").build())
@@ -156,7 +153,7 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithPeriodsInTheName() {
 
         assertTrue(result.isSuccessful());
         assertThat(result.message(), is("Fetched all artifacts"));
-        verify(mockStore, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
+        verify(store, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
     }
 
     @Test
@@ -168,11 +165,9 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithSpecialCharacters() {
                 .with("GO_PACKAGE_REPO_WITH________________________________PACKAGE_WITH________________________________STAGE_NAME", "defaultStage")
                 .with("GO_PACKAGE_REPO_WITH________________________________PACKAGE_WITH________________________________JOB_NAME", "defaultJob")
                 .build();
-        AmazonS3Client mockClient = mockClient();
-        doReturn(mockClient).when(fetchExecutor).s3Client(any(GoEnvironment.class));
-        S3ArtifactStore mockStore = mockStore();
+        S3ArtifactStore store = mockStore();
+        doReturn(store).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(bucket));
 
-        doReturn(mockStore).when(fetchExecutor).getS3ArtifactStore(any(GoEnvironment.class), any(String.class));
         config = new Config(Maps.builder()
                 .with(Constants.REPO, Maps.builder().with("value", "repo-with`~!@#$%^&*()-+=[{]}\\|;:'\",<.>/?").build())
                 .with(Constants.PACKAGE, Maps.builder().with("value", "package-with`~!@#$%^&*()-+=[{]}\\|;:'\",<.>/?").build())
@@ -182,7 +177,7 @@ public void shouldBeAbleToHandleTaskConfigEntriesWithSpecialCharacters() {
 
         assertTrue(result.isSuccessful());
         assertThat(result.message(), is("Fetched all artifacts"));
-        verify(mockStore, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
+        verify(store, times(1)).getPrefix("TestPublish/defaultStage/defaultJob/20.1", "here/artifacts");
     }
 
     private Context mockContext(final Map<String, String> environmentMap) {

diff --git a/material/src/main/java/com/indix/gocd/s3material/plugin/S3PackageMaterialPoller.java b/material/src/main/java/com/indix/gocd/s3material/plugin/S3PackageMaterialPoller.java
@@ -75,7 +75,7 @@ private GoPluginApiResponse handleLatestRevisionSince(GoPluginApiRequest goPlugi
         String s3Bucket = repositoryKeyValuePairs.get(S3_BUCKET);
         S3ArtifactStore artifactStore = s3ArtifactStore(s3Bucket);
         try {
-            RevisionStatus revision = artifactStore.getLatest(s3Client(), artifact(packageKeyValuePairs));
+            RevisionStatus revision = artifactStore.getLatest(artifact(packageKeyValuePairs));
             if(new Revision(revision.revision.getRevision()).compareTo(new Revision(previousRevision)) > 0) {
                 return createResponse(DefaultGoPluginApiResponse.SUCCESS_RESPONSE_CODE, revision.toMap());
             }
@@ -93,7 +93,7 @@ private GoPluginApiResponse handleGetLatestRevision(GoPluginApiRequest goPluginA
         String s3Bucket = repositoryKeyValuePairs.get(S3_BUCKET);
         S3ArtifactStore artifactStore = s3ArtifactStore(s3Bucket);
         try {
-            RevisionStatus revision = artifactStore.getLatest(s3Client(), artifact(packageKeyValuePairs));
+            RevisionStatus revision = artifactStore.getLatest(artifact(packageKeyValuePairs));
             return createResponse(DefaultGoPluginApiResponse.SUCCESS_RESPONSE_CODE, revision.toMap());
         } catch (Exception e) {
             logger.error(e.getMessage(), e);
@@ -222,17 +222,7 @@ private Map<String, String> keyValuePairs(GoPluginApiRequest goPluginApiRequest,
     }
 
     public S3ArtifactStore s3ArtifactStore(String s3Bucket) {
-        return new S3ArtifactStore(s3Client(), s3Bucket);
-    }
-
-    private static AmazonS3Client s3Client() {
-        // The s3 client has a nice way to pick up the creds.
-        // It first checks the env to see if it contains the required key related variables/values
-        // If not, it checks the java system properties to see if it's set there(ideally via -D args)
-        // If not, it falls back to check ~/.aws/credentials file
-        // If not, finally, very insecure way, it tries to fetch from the internal metadata service that each
-        // instance comes with(if its exposed).
-        return new AmazonS3Client();
+        return new S3ArtifactStore(s3Bucket);
     }
 
     private Artifact artifact(Map<String, String> packageConfig) {

diff --git a/material/src/test/scala/com/indix/gocd/s3material/plugin/S3PackageMaterialPollerSpec.scala b/material/src/test/scala/com/indix/gocd/s3material/plugin/S3PackageMaterialPollerSpec.scala
@@ -41,7 +41,7 @@ class S3PackageMaterialPollerSpec extends FlatSpec with MockitoSugar with org.sc
 
   it should "return null result if no new revision since previous revision" in {
     val status = new RevisionStatus(new Revision("1.1"), new Date(), "", "", "")
-    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[AmazonS3Client], Matchers.any[Artifact])
+    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[Artifact])
     val result = sut.handle(getRequest(S3PackageMaterialPoller.REQUEST_LATEST_REVISION_SINCE, """
                   |{
                   |    "repository-configuration": {
@@ -73,7 +73,7 @@ class S3PackageMaterialPollerSpec extends FlatSpec with MockitoSugar with org.sc
 
   it should "get more latest revision since previous revision" in {
     val status = new RevisionStatus(new Revision("1.2"), new Date(), "", "", "")
-    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[AmazonS3Client], Matchers.any[Artifact])
+    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[Artifact])
     val result = sut.handle(getRequest(S3PackageMaterialPoller.REQUEST_LATEST_REVISION_SINCE, """
                   |{
                   |    "repository-configuration": {
@@ -105,7 +105,7 @@ class S3PackageMaterialPollerSpec extends FlatSpec with MockitoSugar with org.sc
 
   it should "get latest revision" in {
     val status = new RevisionStatus(new Revision("1.1"), new Date(), "", "", "")
-    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[AmazonS3Client], Matchers.any[Artifact])
+    doReturn(status).when(mockS3ArtifactStore).getLatest(Matchers.any[Artifact])
     val result = sut.handle(getRequest(S3PackageMaterialPoller.REQUEST_LATEST_REVISION, """
                   |{
                   |    "repository-configuration": {

diff --git a/publish/src/main/java/com/indix/gocd/s3publish/PublishExecutor.java b/publish/src/main/java/com/indix/gocd/s3publish/PublishExecutor.java
@@ -1,10 +1,8 @@
 package com.indix.gocd.s3publish;
 
-import com.amazonaws.ClientConfiguration;
-import com.amazonaws.auth.BasicAWSCredentials;
 import com.amazonaws.auth.InstanceProfileCredentialsProvider;
-import com.amazonaws.services.s3.AmazonS3Client;
-import com.amazonaws.services.s3.S3ClientOptions;
+import com.amazonaws.services.s3.AmazonS3;
+import com.amazonaws.services.s3.AmazonS3ClientBuilder;
 import com.amazonaws.services.s3.model.ObjectMetadata;
 import com.amazonaws.services.s3.model.PutObjectRequest;
 import com.google.gson.JsonSyntaxException;
@@ -44,7 +42,7 @@ public TaskExecutionResult execute(Config config, final Context context) {
         if (env.isAbsent(GO_SERVER_DASHBOARD_URL)) return envNotFound(GO_SERVER_DASHBOARD_URL);
 
         final String bucket = env.get(GO_ARTIFACTS_S3_BUCKET);
-        final S3ArtifactStore store = new S3ArtifactStore(s3Client(env), bucket);
+        final S3ArtifactStore store = getS3ArtifactStore(env, bucket);
         store.setStorageClass(env.getOrElse(AWS_STORAGE_CLASS, STORAGE_CLASS_STANDARD));
 
         final String destinationPrefix = getDestinationPrefix(config, env);
@@ -87,31 +85,19 @@ public void execute(String includedFile) {
         return new TaskExecutionResult(true,"Published all artifacts to S3");
     }
 
-    /*
-        Made public only for tests
-     */
-    public String[] parseSourcePath(String source, String workingDir) {
+    protected S3ArtifactStore getS3ArtifactStore(GoEnvironment env, String bucket) {
+        return new S3ArtifactStore(env, bucket);
+    }
+
+    protected String[] parseSourcePath(String source, String workingDir) {
         DirectoryScanner directoryScanner = new DirectoryScanner();
         directoryScanner.setBasedir(workingDir);
         directoryScanner.setIncludes(new String[]{source});
         directoryScanner.scan();
         return ArrayUtils.addAll(directoryScanner.getIncludedFiles(), directoryScanner.getIncludedDirectories());
     }
 
-    public AmazonS3Client s3Client(GoEnvironment env) {
-        AmazonS3Client client = null;
-        if (env.hasAWSUseIamRole()) {
-            client = new AmazonS3Client(new InstanceProfileCredentialsProvider());
-        } else {
-            client = new AmazonS3Client(new BasicAWSCredentials(env.get(AWS_ACCESS_KEY_ID), env.get(AWS_SECRET_ACCESS_KEY)));
-        }
-        return client;
-    }
-
-    /*
-        Made public for tests
-     */
-    public boolean fileExists(File localFileToUpload) {
+    protected boolean fileExists(File localFileToUpload) {
         return localFileToUpload.exists();
     }
 

diff --git a/publish/src/test/java/com/indix/gocd/s3publish/PublishExecutorTest.java b/publish/src/test/java/com/indix/gocd/s3publish/PublishExecutorTest.java
@@ -7,6 +7,7 @@
 import com.indix.gocd.utils.GoEnvironment;
 import com.indix.gocd.utils.TaskExecutionResult;
 import com.indix.gocd.utils.mocks.MockContext;
+import com.indix.gocd.utils.store.S3ArtifactStore;
 import com.indix.gocd.utils.utils.Maps;
 import org.junit.Before;
 import org.junit.Test;
@@ -29,13 +30,16 @@ public class PublishExecutorTest {
     Maps.MapBuilder<String, String> mockEnvironmentVariables;
     private PublishExecutor publishExecutor;
     private Config config;
+    private S3ArtifactStore store;
+    private String testS3Bucket = "testS3Bucket";
+
 
     @Before
     public void setUp() throws Exception {
         mockEnvironmentVariables = Maps.<String, String>builder()
                 .with(AWS_SECRET_ACCESS_KEY, "secretKey")
                 .with(AWS_ACCESS_KEY_ID, "accessId")
-                .with(GO_ARTIFACTS_S3_BUCKET, "testS3Bucket")
+                .with(GO_ARTIFACTS_S3_BUCKET, testS3Bucket)
                 .with(GO_SERVER_DASHBOARD_URL, "http://go.server:8153")
                 .with("GO_PIPELINE_NAME", "pipeline")
                 .with("GO_STAGE_NAME", "stage")
@@ -298,7 +302,6 @@ public void shouldUploadALocalFileToS3WithSlashDestinationPrefix() {
         assertThat(jarPutRequest.getKey(), is("s3publish-0.1.31.jar"));
         assertNull(jarPutRequest.getMetadata());
     }
-
     private TaskExecutionResult executeMockPublish(final AmazonS3Client mockClient, Config config, String[] files) {
         return executeMockPublish(mockClient, config, files, mockEnvironmentVariables);
     }
@@ -307,7 +310,9 @@ private TaskExecutionResult executeMockPublish(final AmazonS3Client mockClient,
                                                    Maps.MapBuilder<String, String> mockVariablesBuilder) {
         Map<String, String> mockVariables = mockVariablesBuilder.build();
 
-        doReturn(mockClient).when(publishExecutor).s3Client(any(GoEnvironment.class));
+        store = new S3ArtifactStore(mockClient, testS3Bucket);
+
+        doReturn(store).when(publishExecutor).getS3ArtifactStore(any(GoEnvironment.class), eq(testS3Bucket));
         doReturn(files).when(publishExecutor).parseSourcePath(anyString(), anyString());
 
         return publishExecutor.execute(config, mockContext(mockVariables));