feat(execution): remove spinnaker accounts from execution context (#4656

) * feat(execution): Add a method to build AuthenticationDetails without Spinnaker accounts * feat(execution): Add the execution.includeAllowedAccounts feature flag When true, includes accounts in the pipeline execution. When false, excludes them * feat(execution): Add feature flag when building a PIPELINE ExecutionType to include or exclude the list of allowed accounts from the execution context * test(execution): Define the behavior when building a PIPELINE ExecutionType --------- Co-authored-by: Daniel Zheng <[email protected]>
spinnaker · Feb 23, 2024 · f5fb537 · f5fb537
1 parent d3ede81
commit f5fb537
Show file tree

Hide file tree

Showing 8 changed files with 226 additions and 7 deletions.
diff --git a/...ore/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java b/...ore/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java
@@ -42,6 +42,9 @@ public class ExecutionConfigurationProperties {
    */
   private Set<OrchestrationExecution> allowedOrchestrationExecutions = Set.of();
 
+  /** flag to include/exclude the set of allowed accounts in an execution context */
+  private boolean includeAllowedAccounts = true;
+
   /**
    * helper method that returns an {@link Optional<OrchestrationExecution>} object if the
    * orchestration execution provided as an input is in the allowed orchestration executions set.

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncher.java
@@ -230,6 +230,7 @@ private PipelineExecution parsePipeline(Map<String, Object> config) {
                     config.get("source"), PipelineExecution.PipelineSource.class))
         .withSpelEvaluator(getString(config, "spelEvaluator"))
         .withTemplateVariables(getMap(config, "templateVariables"))
+        .withIncludeAllowedAccounts(executionConfigurationProperties.isIncludeAllowedAccounts())
         .build();
   }
 
@@ -269,8 +270,14 @@ private PipelineExecution parseOrchestration(Map<String, Object> config) {
     PipelineExecution.AuthenticationDetails auth = new PipelineExecution.AuthenticationDetails();
     auth.setUser(getString(config, "user"));
 
-    orchestration.setAuthentication(
-        PipelineExecutionImpl.AuthenticationHelper.build().orElse(auth));
+    if (executionConfigurationProperties.isIncludeAllowedAccounts()) {
+      orchestration.setAuthentication(
+          PipelineExecutionImpl.AuthenticationHelper.build().orElse(auth));
+    } else {
+      orchestration.setAuthentication(
+          PipelineExecutionImpl.AuthenticationHelper.buildWithoutAccounts().orElse(auth));
+    }
+
     orchestration.setOrigin((String) config.getOrDefault("origin", "unknown"));
     orchestration.setStartTimeExpiry((Long) config.get("startTimeExpiry"));
     orchestration.setSpelEvaluator(getString(config, "spelEvaluator"));

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilder.java
@@ -26,10 +26,17 @@
 
 @Deprecated
 public class PipelineBuilder {
+  private boolean includeAllowedAccounts;
+
   public PipelineBuilder(String application) {
     pipeline = PipelineExecutionImpl.newPipeline(application);
   }
 
+  public PipelineBuilder withIncludeAllowedAccounts(boolean includeAllowedAccounts) {
+    this.includeAllowedAccounts = includeAllowedAccounts;
+    return this;
+  }
+
   public PipelineBuilder withId(String id) {
     if (!Strings.isNullOrEmpty(id)) {
       pipeline.setId(id);
@@ -113,9 +120,15 @@ public PipelineBuilder withStages(List<Map<String, Object>> stages) {
 
   public PipelineExecution build() {
     pipeline.setBuildTime(System.currentTimeMillis());
-    pipeline.setAuthentication(
-        PipelineExecutionImpl.AuthenticationHelper.build()
-            .orElse(new PipelineExecution.AuthenticationDetails()));
+    if (this.includeAllowedAccounts) {
+      pipeline.setAuthentication(
+          PipelineExecutionImpl.AuthenticationHelper.build()
+              .orElse(new PipelineExecution.AuthenticationDetails()));
+    } else {
+      pipeline.setAuthentication(
+          PipelineExecutionImpl.AuthenticationHelper.buildWithoutAccounts()
+              .orElse(new PipelineExecution.AuthenticationDetails()));
+    }
 
     return pipeline;
   }

diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImpl.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImpl.java
@@ -444,5 +444,13 @@ public static Optional<AuthenticationDetails> build() {
 
       return Optional.empty();
     }
+
+    public static Optional<AuthenticationDetails> buildWithoutAccounts() {
+      Optional<String> spinnakerUserOptional = AuthenticatedRequest.getSpinnakerUser();
+      if (spinnakerUserOptional.isPresent()) {
+        return Optional.of(new AuthenticationDetails(spinnakerUserOptional.orElse(null)));
+      }
+      return Optional.empty();
+    }
   }
 }
diff --git a/...rc/test/groovy/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImplSpec.groovy b/...rc/test/groovy/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImplSpec.groovy
@@ -57,6 +57,19 @@ class PipelineExecutionImplSpec extends Specification {
     authenticationDetails.allowedAccounts == ["Account1", "Account2"] as Set
   }
 
+  def "should build AuthenticationDetails without accounts"() {
+    given:
+    MDC.put(Header.USER.header, "SpinnakerUser")
+    MDC.put(Header.ACCOUNTS.header, "Account1,Account2")
+
+    when:
+    def authenticationDetails = PipelineExecutionImpl.AuthenticationHelper.buildWithoutAccounts().get()
+
+    then:
+    authenticationDetails.user == "SpinnakerUser"
+    authenticationDetails.allowedAccounts == [] as Set
+  }
+
   def "should calculate context from outputs of all stages"() {
     given:
     def pipeline = pipeline {

diff --git a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncherTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncherTest.java
@@ -16,13 +16,14 @@
 
 package com.netflix.spinnaker.orca.pipeline;
 
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.Mockito.never;
-import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.*;
 
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spectator.api.Registry;
+import com.netflix.spinnaker.kork.common.Header;
 import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionType;
 import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import com.netflix.spinnaker.orca.config.ExecutionConfigurationProperties;
@@ -31,12 +32,14 @@
 import java.time.Clock;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import org.slf4j.MDC;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.context.properties.EnableConfigurationProperties;
 import org.springframework.boot.test.context.SpringBootTest;
@@ -73,6 +76,7 @@ public void setup() {
     clock = Clock.systemUTC();
     pipelineValidator = Optional.empty();
     registry = Optional.empty();
+    MDC.clear();
 
     executionLauncher =
         new ExecutionLauncher(
@@ -199,6 +203,135 @@ public void testOrchestrationExecutionWhenUserIsNotInAllowList() throws Exceptio
                 + " disabled for user: [email protected]");
   }
 
+  @DisplayName(
+      "when includeAllowedAccounts: true, then the orchestration should contain Spinnaker accounts")
+  @Test
+  public void testIncludeSpinnakerAccountsInOrchestration() throws Exception {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // override properties to allow orchestration executions
+    ExecutionConfigurationProperties executionConfigurationProperties =
+        new ExecutionConfigurationProperties();
+    executionConfigurationProperties.setBlockOrchestrationExecutions(false);
+    executionLauncher =
+        new ExecutionLauncher(
+            objectMapper,
+            executionRepository,
+            executionRunner,
+            clock,
+            applicationEventPublisher,
+            pipelineValidator,
+            registry,
+            executionConfigurationProperties);
+
+    // when
+    PipelineExecution pipelineExecution =
+        executionLauncher.start(
+            ExecutionType.ORCHESTRATION, getConfigJson("ad-hoc/deploy-manifest.json"));
+
+    // then
+    // verify that the execution runner attempted to start the execution as expected
+    verify(executionRunner).start(pipelineExecution);
+    // verify that accounts are set in the pipeline execution
+    assertThat(pipelineExecution.getAuthentication().getAllowedAccounts())
+        .isEqualTo(Set.of("Account1", "Account2"));
+  }
+
+  @DisplayName(
+      "when includeAllowedAccounts: false, then the orchestration should not contain Spinnaker accounts")
+  @Test
+  public void testExcludeSpinnakerAccountsFromOrchestration() throws Exception {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // override properties to 1. allow orchestration executions and 2. set includeAllowedAccounts to
+    // false
+    ExecutionConfigurationProperties executionConfigurationProperties =
+        new ExecutionConfigurationProperties();
+    executionConfigurationProperties.setBlockOrchestrationExecutions(false);
+    executionConfigurationProperties.setIncludeAllowedAccounts(false);
+    executionLauncher =
+        new ExecutionLauncher(
+            objectMapper,
+            executionRepository,
+            executionRunner,
+            clock,
+            applicationEventPublisher,
+            pipelineValidator,
+            registry,
+            executionConfigurationProperties);
+
+    // when
+    PipelineExecution pipelineExecution =
+        executionLauncher.start(
+            ExecutionType.ORCHESTRATION, getConfigJson("ad-hoc/deploy-manifest.json"));
+
+    // then
+    // verify that the execution runner attempted to start the execution as expected
+    verify(executionRunner).start(pipelineExecution);
+    // verify that accounts are not set in the pipeline execution
+    assertThat(pipelineExecution.getAuthentication().getAllowedAccounts()).isEqualTo(Set.of());
+  }
+
+  @DisplayName(
+      "when includeAllowedAccounts: true, then the pipeline should contain Spinnaker accounts")
+  @Test
+  public void testIncludeSpinnakerAccountsInPipeline() throws Exception {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // when
+    PipelineExecution pipelineExecution =
+        executionLauncher.start(
+            ExecutionType.PIPELINE, getConfigJson("ad-hoc/deploy-manifest.json"));
+
+    // then
+    // verify that the execution runner attempted to start the execution as expected
+    verify(executionRunner).start(pipelineExecution);
+    // verify that accounts are set in the pipeline execution
+    assertThat(pipelineExecution.getAuthentication().getAllowedAccounts())
+        .isEqualTo(Set.of("Account1", "Account2"));
+  }
+
+  @DisplayName(
+      "when includeAllowedAccounts: false, then the pipeline should not contain Spinnaker accounts")
+  @Test
+  public void testExcludeSpinnakerAccountsFromPipeline() throws Exception {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // override properties to set includeAllowedAccounts to false
+    ExecutionConfigurationProperties executionConfigurationProperties =
+        new ExecutionConfigurationProperties();
+    executionConfigurationProperties.setIncludeAllowedAccounts(false);
+    executionLauncher =
+        new ExecutionLauncher(
+            objectMapper,
+            executionRepository,
+            executionRunner,
+            clock,
+            applicationEventPublisher,
+            pipelineValidator,
+            registry,
+            executionConfigurationProperties);
+
+    // when
+    PipelineExecution pipelineExecution =
+        executionLauncher.start(
+            ExecutionType.PIPELINE, getConfigJson("ad-hoc/deploy-manifest.json"));
+
+    // then
+    // verify that the execution runner attempted to start the execution as expected
+    verify(executionRunner).start(pipelineExecution);
+    // verify that accounts are not set in the pipeline execution
+    assertThat(pipelineExecution.getAuthentication().getAllowedAccounts()).isEqualTo(Set.of());
+  }
+
   private Map<String, Object> getConfigJson(String resource) throws Exception {
     return objectMapper.readValue(
         ExecutionLauncherTest.class.getResourceAsStream(resource), Map.class);

diff --git a/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java
@@ -16,14 +16,24 @@
 
 package com.netflix.spinnaker.orca.pipeline.model;
 
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 
+import com.netflix.spinnaker.kork.common.Header;
+import com.netflix.spinnaker.orca.api.pipeline.models.PipelineExecution;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.slf4j.MDC;
 
 class PipelineBuilderTest {
+  @BeforeEach
+  public void setup() {
+    MDC.clear();
+  }
 
   @Test
   void withStagesChecksForNull() {
@@ -40,4 +50,35 @@ void withStagesChecksForType() {
         IllegalArgumentException.class,
         () -> pipelineBuilder.withStages(List.of(stageWithoutType)));
   }
+
+  @Test
+  void buildIncludesAllowedAccountsWhenTrue() {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // when
+    PipelineBuilder pipelineBuilder =
+        new PipelineBuilder("my-application").withIncludeAllowedAccounts(true);
+    PipelineExecution execution = pipelineBuilder.build();
+
+    // then
+    assertThat(execution.getAuthentication().getAllowedAccounts())
+        .isEqualTo(Set.of("Account1", "Account2"));
+  }
+
+  @Test
+  void buildExcludesAllowedAccountsWhenFalse() {
+    // given
+    MDC.put(Header.USER.getHeader(), "SpinnakerUser");
+    MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2");
+
+    // when
+    PipelineBuilder pipelineBuilder =
+        new PipelineBuilder("my-application").withIncludeAllowedAccounts(false);
+    PipelineExecution execution = pipelineBuilder.build();
+
+    // then
+    assertThat(execution.getAuthentication().getAllowedAccounts()).isEqualTo(Set.of());
+  }
 }
diff --git a/orca-core/src/test/resources/execution-launcher-properties.yml b/orca-core/src/test/resources/execution-launcher-properties.yml
@@ -7,3 +7,4 @@ executions:
     - name: updateApplication # eg. config to show how to set the name and allowAllUsers properties for an execution
       allowAllUsers: true
     - name: saveApplication # eg. config to show how to set just the name of an execution. It defaults to allowAllUsers: true
+  includeAllowedAccounts: true