fix(s3): Refactor S3 config to support separate plugin binaries bucket (

#715)

front50-core/src/main/groovy/com/netflix/spinnaker/front50/plugins/CachingPluginBinaryStorageService.java

@@ -98,6 +98,7 @@ private synchronized void storeCache(String key, byte[] binary) {
     Path binaryPath = CACHE_PATH.resolve(key);
     if (!binaryPath.toFile().exists()) {
       try {
+        Files.createDirectories(binaryPath.getParent());
         Files.write(binaryPath, binary, StandardOpenOption.CREATE_NEW);
       } catch (IOException e) {
         log.error("Failed to write plugin binary to local filesystem cache: {}", key, e);

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3ClientFactory.java

@@ -0,0 +1,72 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.front50.config;
+
+import com.amazonaws.ClientConfiguration;
+import com.amazonaws.Protocol;
+import com.amazonaws.auth.AWSCredentialsProvider;
+import com.amazonaws.regions.Region;
+import com.amazonaws.regions.Regions;
+import com.amazonaws.services.s3.AmazonS3;
+import com.amazonaws.services.s3.AmazonS3Client;
+import com.amazonaws.services.s3.S3ClientOptions;
+import java.util.Optional;
+import org.apache.commons.lang3.StringUtils;
+
+/**
+ * Creates an S3 client.
+ *
+ * <p>Since there are multiple implementations of {@link S3Properties} and we create different
+ * clients based on those properties, the actual factory code needed to be split out.
+ */
+public class S3ClientFactory {
+
+  public static AmazonS3 create(
+      AWSCredentialsProvider awsCredentialsProvider, S3Properties s3Properties) {
+    ClientConfiguration clientConfiguration = new ClientConfiguration();
+    if (s3Properties.getProxyProtocol() != null) {
+      if (s3Properties.getProxyProtocol().equalsIgnoreCase("HTTPS")) {
+        clientConfiguration.setProtocol(Protocol.HTTPS);
+      } else {
+        clientConfiguration.setProtocol(Protocol.HTTP);
+      }
+      Optional.ofNullable(s3Properties.getProxyHost()).ifPresent(clientConfiguration::setProxyHost);
+      Optional.ofNullable(s3Properties.getProxyPort())
+          .map(Integer::parseInt)
+          .ifPresent(clientConfiguration::setProxyPort);
+    }
+
+    AmazonS3Client client = new AmazonS3Client(awsCredentialsProvider, clientConfiguration);
+
+    if (!StringUtils.isEmpty(s3Properties.getEndpoint())) {
+      client.setEndpoint(s3Properties.getEndpoint());
+
+      if (!StringUtils.isEmpty(s3Properties.getRegionOverride())) {
+        client.setSignerRegionOverride(s3Properties.getRegionOverride());
+      }
+
+      client.setS3ClientOptions(
+          S3ClientOptions.builder().setPathStyleAccess(s3Properties.getPathStyleAccess()).build());
+    } else {
+      Optional.ofNullable(s3Properties.getRegion())
+          .map(Regions::fromName)
+          .map(Region::getRegion)
+          .ifPresent(client::setRegion);
+    }
+
+    return client;
+  }
+}

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3Config.java

@@ -1,19 +1,8 @@
 package com.netflix.spinnaker.front50.config;
 
-import com.amazonaws.ClientConfiguration;
-import com.amazonaws.Protocol;
-import com.amazonaws.auth.AWSCredentialsProvider;
-import com.amazonaws.regions.Region;
-import com.amazonaws.regions.Regions;
-import com.amazonaws.services.s3.AmazonS3;
-import com.amazonaws.services.s3.AmazonS3Client;
-import com.amazonaws.services.s3.S3ClientOptions;
 import com.netflix.spinnaker.kork.aws.bastion.BastionConfig;
-import java.util.Optional;
-import org.apache.commons.lang3.StringUtils;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
-import org.springframework.context.annotation.Bean;
 import org.springframework.context.annotation.Configuration;
 import org.springframework.context.annotation.Import;
 
@@ -22,45 +11,7 @@
 @Import({
   BastionConfig.class,
   S3StorageServiceConfiguration.class,
-  S3StorageServiceConfiguration.class
+  S3PluginStorageConfiguration.class
 })
-@EnableConfigurationProperties(S3Properties.class)
-public class S3Config extends CommonStorageServiceDAOConfig {
-
-  @Bean
-  public AmazonS3 awsS3Client(
-      AWSCredentialsProvider awsCredentialsProvider, S3Properties s3Properties) {
-    ClientConfiguration clientConfiguration = new ClientConfiguration();
-    if (s3Properties.getProxyProtocol() != null) {
-      if (s3Properties.getProxyProtocol().equalsIgnoreCase("HTTPS")) {
-        clientConfiguration.setProtocol(Protocol.HTTPS);
-      } else {
-        clientConfiguration.setProtocol(Protocol.HTTP);
-      }
-      Optional.ofNullable(s3Properties.getProxyHost()).ifPresent(clientConfiguration::setProxyHost);
-      Optional.ofNullable(s3Properties.getProxyPort())
-          .map(Integer::parseInt)
-          .ifPresent(clientConfiguration::setProxyPort);
-    }
-
-    AmazonS3Client client = new AmazonS3Client(awsCredentialsProvider, clientConfiguration);
-
-    if (!StringUtils.isEmpty(s3Properties.getEndpoint())) {
-      client.setEndpoint(s3Properties.getEndpoint());
-
-      if (!StringUtils.isEmpty(s3Properties.getRegionOverride())) {
-        client.setSignerRegionOverride(s3Properties.getRegionOverride());
-      }
-
-      client.setS3ClientOptions(
-          S3ClientOptions.builder().setPathStyleAccess(s3Properties.getPathStyleAccess()).build());
-    } else {
-      Optional.ofNullable(s3Properties.getRegion())
-          .map(Regions::fromName)
-          .map(Region::getRegion)
-          .ifPresent(client::setRegion);
-    }
-
-    return client;
-  }
-}
+@EnableConfigurationProperties({S3MetadataStorageProperties.class, S3PluginStorageProperties.class})
+public class S3Config extends CommonStorageServiceDAOConfig {}

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3EventingConfiguration.java

@@ -42,7 +42,7 @@ public class S3EventingConfiguration {
 
   @Bean
   public AmazonSQS awsSQSClient(
-      AWSCredentialsProvider awsCredentialsProvider, S3Properties s3Properties) {
+      AWSCredentialsProvider awsCredentialsProvider, S3MetadataStorageProperties s3Properties) {
     return AmazonSQSClientBuilder.standard()
         .withCredentials(awsCredentialsProvider)
         .withClientConfiguration(new ClientConfiguration())
@@ -52,7 +52,7 @@ public AmazonSQS awsSQSClient(
 
   @Bean
   public AmazonSNS awsSNSClient(
-      AWSCredentialsProvider awsCredentialsProvider, S3Properties s3Properties) {
+      AWSCredentialsProvider awsCredentialsProvider, S3MetadataStorageProperties s3Properties) {
     return AmazonSNSClientBuilder.standard()
         .withCredentials(awsCredentialsProvider)
         .withClientConfiguration(new ClientConfiguration())
@@ -65,7 +65,7 @@ public TemporarySQSQueue temporaryQueueSupport(
       Optional<ApplicationInfoManager> applicationInfoManager,
       AmazonSQS amazonSQS,
       AmazonSNS amazonSNS,
-      S3Properties s3Properties) {
+      S3MetadataStorageProperties s3Properties) {
     return new TemporarySQSQueue(
         amazonSQS,
         amazonSNS,
@@ -76,7 +76,7 @@ public TemporarySQSQueue temporaryQueueSupport(
   @Bean
   public ObjectKeyLoader eventingS3ObjectKeyLoader(
       ObjectMapper objectMapper,
-      S3Properties s3Properties,
+      S3MetadataStorageProperties s3Properties,
       StorageService storageService,
       TemporarySQSQueue temporaryQueueSupport,
       Registry registry) {

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3MetadataStorageProperties.java

@@ -0,0 +1,21 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.front50.config;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+@ConfigurationProperties("spinnaker.s3")
+public class S3MetadataStorageProperties extends S3Properties {}

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3PluginBinaryServiceConfiguration.java → front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3PluginStorageConfiguration.java

@@ -15,6 +15,7 @@
  */
 package com.netflix.spinnaker.front50.config;
 
+import com.amazonaws.auth.AWSCredentialsProvider;
 import com.amazonaws.services.s3.AmazonS3;
 import com.netflix.spinnaker.front50.plugins.PluginBinaryStorageService;
 import com.netflix.spinnaker.front50.plugins.S3PluginBinaryStorageService;
@@ -23,12 +24,18 @@
 import org.springframework.context.annotation.Configuration;
 
 @Configuration
-@ConditionalOnProperty("spinnaker.s3.plugin-binary-storage.enabled")
-public class S3PluginBinaryServiceConfiguration {
+@ConditionalOnProperty("spinnaker.s3.plugin-storage.enabled")
+public class S3PluginStorageConfiguration {
+
+  @Bean
+  public AmazonS3 awsS3PluginClient(
+      AWSCredentialsProvider awsCredentialsProvider, S3PluginStorageProperties s3Properties) {
+    return S3ClientFactory.create(awsCredentialsProvider, s3Properties);
+  }
 
   @Bean
   PluginBinaryStorageService pluginBinaryStorageService(
-      AmazonS3 amazonS3, S3Properties properties) {
-    return new S3PluginBinaryStorageService(amazonS3, properties);
+      AmazonS3 awsS3PluginClient, S3PluginStorageProperties properties) {
+    return new S3PluginBinaryStorageService(awsS3PluginClient, properties);
   }
 }

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3PluginStorageProperties.java

@@ -0,0 +1,21 @@
+/*
+ * Copyright 2020 Netflix, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.netflix.spinnaker.front50.config;
+
+import org.springframework.boot.context.properties.ConfigurationProperties;
+
+@ConfigurationProperties("spinnaker.s3.plugin-storage")
+public class S3PluginStorageProperties extends S3Properties {}

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3Properties.java

@@ -16,11 +16,9 @@
 
 package com.netflix.spinnaker.front50.config;
 
-import org.springframework.boot.context.properties.ConfigurationProperties;
 import org.springframework.boot.context.properties.NestedConfigurationProperty;
 
-@ConfigurationProperties("spinnaker.s3")
-public class S3Properties extends S3BucketProperties {
+public abstract class S3Properties extends S3BucketProperties {
   String rootFolder;
 
   @NestedConfigurationProperty S3FailoverProperties failover = new S3FailoverProperties();

front50-s3/src/main/java/com/netflix/spinnaker/front50/config/S3StorageServiceConfiguration.java

@@ -15,6 +15,7 @@
  */
 package com.netflix.spinnaker.front50.config;
 
+import com.amazonaws.auth.AWSCredentialsProvider;
 import com.amazonaws.services.s3.AmazonS3;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spinnaker.front50.model.S3StorageService;
@@ -27,13 +28,20 @@
 public class S3StorageServiceConfiguration {
 
   @Bean
-  public S3StorageService s3StorageService(AmazonS3 amazonS3, S3Properties s3Properties) {
+  public AmazonS3 awsS3MetadataClient(
+      AWSCredentialsProvider awsCredentialsProvider, S3MetadataStorageProperties s3Properties) {
+    return S3ClientFactory.create(awsCredentialsProvider, s3Properties);
+  }
+
+  @Bean
+  public S3StorageService s3StorageService(
+      AmazonS3 awsS3MetadataClient, S3MetadataStorageProperties s3Properties) {
     ObjectMapper awsObjectMapper = new ObjectMapper();
 
     S3StorageService service =
         new S3StorageService(
             awsObjectMapper,
-            amazonS3,
+            awsS3MetadataClient,
             s3Properties.getBucket(),
             s3Properties.getRootFolder(),
             s3Properties.isFailoverEnabled(),

front50-s3/src/main/java/com/netflix/spinnaker/front50/model/EventingS3ObjectKeyLoader.java

@@ -27,7 +27,7 @@
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListenableFutureTask;
 import com.netflix.spectator.api.Registry;
-import com.netflix.spinnaker.front50.config.S3Properties;
+import com.netflix.spinnaker.front50.config.S3MetadataStorageProperties;
 import com.netflix.spinnaker.front50.model.events.S3Event;
 import com.netflix.spinnaker.front50.model.events.S3EventWrapper;
 import java.io.IOException;
@@ -78,7 +78,7 @@ public class EventingS3ObjectKeyLoader implements ObjectKeyLoader, Runnable {
   public EventingS3ObjectKeyLoader(
       ExecutorService executionService,
       ObjectMapper objectMapper,
-      S3Properties s3Properties,
+      S3MetadataStorageProperties s3Properties,
       TemporarySQSQueue temporarySQSQueue,
       StorageService storageService,
       Registry registry,

front50-s3/src/main/java/com/netflix/spinnaker/front50/plugins/S3PluginBinaryStorageService.java

@@ -21,7 +21,7 @@
 import com.amazonaws.services.s3.model.*;
 import com.google.common.hash.Hashing;
 import com.google.common.io.ByteStreams;
-import com.netflix.spinnaker.front50.config.S3Properties;
+import com.netflix.spinnaker.front50.config.S3PluginStorageProperties;
 import com.netflix.spinnaker.kork.exceptions.SystemException;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
@@ -34,9 +34,9 @@
 public class S3PluginBinaryStorageService implements PluginBinaryStorageService {
 
   private final AmazonS3 amazonS3;
-  private final S3Properties properties;
+  private final S3PluginStorageProperties properties;
 
-  public S3PluginBinaryStorageService(AmazonS3 amazonS3, S3Properties properties) {
+  public S3PluginBinaryStorageService(AmazonS3 amazonS3, S3PluginStorageProperties properties) {
     this.amazonS3 = amazonS3;
     this.properties = properties;
   }
@@ -101,7 +101,7 @@ public byte[] load(@Nonnull String key) {
   }
 
   private String buildFolder() {
-    return properties.getRootFolder() + "/pluginBinaries";
+    return (properties.getRootFolder() + "/plugins").replaceAll("//", "/");
   }
 
   private String buildObjectKey(String key) {

front50-s3/src/test/groovy/com/netflix/spinnaker/front50/model/EventingS3ObjectKeyLoaderSpec.groovy

@@ -18,6 +18,7 @@ package com.netflix.spinnaker.front50.model
 
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.spectator.api.Registry
+import com.netflix.spinnaker.front50.config.S3MetadataStorageProperties
 import com.netflix.spinnaker.front50.config.S3Properties
 import com.netflix.spinnaker.front50.model.events.S3Event
 import spock.lang.Specification
@@ -30,7 +31,7 @@ import java.util.concurrent.ExecutorService
 class EventingS3ObjectKeyLoaderSpec extends Specification {
   def taskScheduler = Mock(ExecutorService)
   def objectMapper = new ObjectMapper()
-  def s3Properties = new S3Properties(
+  def s3Properties = new S3MetadataStorageProperties(
     rootFolder: "root"
   )
   def temporarySQSQueue = Mock(TemporarySQSQueue)