perf(artifacts): compile appliationsRegex once instead of on each use (…

…#1161) * refactor(artifacts/test): remove cast when mocking headObject call * test(artifacts): demonstrate behavior of applicationsRegex * perf(artifacts): compile appliationsRegex once instead of on each use
spinnaker · Mar 3, 2024 · 6372617 · 6372617
1 parent 690ec5d
commit 6372617
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 5 deletions.
diff --git a/...ain/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java b/...ain/java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorer.java
@@ -48,7 +48,7 @@ public class S3ArtifactStoreStorer implements ArtifactStoreStorer {
   private final S3Client s3Client;
   private final String bucket;
   private final ArtifactStoreURIBuilder uriBuilder;
-  private final String applicationsRegex;
+  private final Pattern applicationsPattern;
 
   public S3ArtifactStoreStorer(
       S3Client s3Client,
@@ -58,7 +58,8 @@ public S3ArtifactStoreStorer(
     this.s3Client = s3Client;
     this.bucket = bucket;
     this.uriBuilder = uriBuilder;
-    this.applicationsRegex = applicationsRegex;
+    this.applicationsPattern =
+        (applicationsRegex != null) ? Pattern.compile(applicationsRegex) : null;
   }
 
   /**
@@ -76,7 +77,7 @@ public Artifact store(Artifact artifact) {
       return artifact;
     }
 
-    if (applicationsRegex != null && !Pattern.matches(applicationsRegex, application)) {
+    if (applicationsPattern != null && !applicationsPattern.matcher(application).matches()) {
       return artifact;
     }
 

diff --git a/...java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorerTest.java b/...java/com/netflix/spinnaker/kork/artifacts/artifactstore/s3/S3ArtifactStoreStorerTest.java
@@ -17,7 +17,11 @@
 package com.netflix.spinnaker.kork.artifacts.artifactstore.s3;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 import com.netflix.spinnaker.kork.artifacts.ArtifactTypes;
@@ -26,9 +30,13 @@
 import com.netflix.spinnaker.kork.common.Header;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
+import java.util.List;
+import java.util.stream.Stream;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
-import org.mockito.Mockito;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import software.amazon.awssdk.services.s3.S3Client;
 import software.amazon.awssdk.services.s3.model.HeadObjectRequest;
 import software.amazon.awssdk.services.s3.model.S3Exception;
@@ -37,7 +45,7 @@ public class S3ArtifactStoreStorerTest {
   @Test
   public void testExceptionPathOfObjectExists() {
     S3Client client = mock(S3Client.class);
-    when(client.headObject((HeadObjectRequest) Mockito.any()))
+    when(client.headObject(any(HeadObjectRequest.class)))
         .thenThrow(S3Exception.builder().statusCode(400).build());
     AuthenticatedRequest.set(Header.APPLICATION, "my-application");
     S3ArtifactStoreStorer artifactStoreStorer =
@@ -55,4 +63,45 @@ public void testExceptionPathOfObjectExists() {
             });
     assertEquals(expectedExceptionMessage, e.getMessage());
   }
+
+  @ParameterizedTest(
+      name = "testApplicationsRegex application = {0}, applicationsRegex = {1}, enabled = {2}")
+  @MethodSource("applicationsRegexArgs")
+  void testApplicationsRegex(String application, String applicationsRegex, boolean enabled) {
+    S3Client client = mock(S3Client.class);
+    AuthenticatedRequest.set(Header.APPLICATION, application);
+    S3ArtifactStoreStorer artifactStoreStorer =
+        new S3ArtifactStoreStorer(
+            client, "my-bucket", new ArtifactStoreURISHA256Builder(), applicationsRegex);
+    artifactStoreStorer.store(
+        Artifact.builder()
+            .type(ArtifactTypes.EMBEDDED_BASE64.getMimeType())
+            .reference("aGVsbG8gd29ybGQK")
+            .build());
+    // If enabled, expect a call to check if the object exists in the artifact store
+    verify(client, times(enabled ? 1 : 0)).headObject(any(HeadObjectRequest.class));
+    verifyNoMoreInteractions(client);
+  }
+
+  private static Stream<Arguments> applicationsRegexArgs() {
+    List<String> apps = List.of("app-one", "app-two", "app-three");
+
+    // The artifact store is enabled only for exact matches (e.g. not partial
+    // matches) of applications in the list.
+    String allowRegex = "^(" + String.join("|", apps) + ")$";
+
+    // To test applicationsRegex as a "deny list", meaning that the artifact
+    // store is enabled for all apps except those in the list, use a negative
+    // lookahead (?!).  See https://stackoverflow.com/a/406408/9572.
+    String denyRegex = "^((?!(" + String.join("|", apps) + ")).)*$";
+
+    return Stream.of(
+        Arguments.of("app-one", allowRegex, true),
+        Arguments.of("app-four", allowRegex, false),
+        Arguments.of("one", allowRegex, false),
+        Arguments.of("app-one", denyRegex, false),
+        Arguments.of("app-four", denyRegex, true),
+        Arguments.of("one", denyRegex, true),
+        Arguments.of("any", null, true));
+  }
 }