From 0610d41acc2afc2d54c3064828f42607e231513c Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Thu, 22 Jun 2023 15:21:05 -0700 Subject: [PATCH 1/4] feat(execution): Add a method to build AuthenticationDetails without Spinnaker accounts --- .../orca/pipeline/model/PipelineExecutionImpl.java | 8 ++++++++ .../pipeline/model/PipelineExecutionImplSpec.groovy | 13 +++++++++++++ 2 files changed, 21 insertions(+) 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 index f03fc6e43c..34430668f3 100644 --- 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 build() { return Optional.empty(); } + + public static Optional buildWithoutAccounts() { + Optional spinnakerUserOptional = AuthenticatedRequest.getSpinnakerUser(); + if (spinnakerUserOptional.isPresent()) { + return Optional.of(new AuthenticationDetails(spinnakerUserOptional.orElse(null))); + } + return Optional.empty(); + } } } diff --git a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImplSpec.groovy b/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImplSpec.groovy index c005bd2b19..9bda671abc 100644 --- a/orca-core/src/test/groovy/com/netflix/spinnaker/orca/pipeline/model/PipelineExecutionImplSpec.groovy +++ b/orca-core/src/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 { From a7a1f5d7d8ca1323f1310f16aa55fbcce9b4a085 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Thu, 22 Jun 2023 15:22:56 -0700 Subject: [PATCH 2/4] feat(execution): Add the execution.includeAllowedAccounts feature flag When true, includes accounts in the pipeline execution. When false, excludes them --- .../ExecutionConfigurationProperties.java | 3 + .../orca/pipeline/ExecutionLauncher.java | 10 ++- .../orca/pipeline/ExecutionLauncherTest.java | 81 ++++++++++++++++++- .../execution-launcher-properties.yml | 1 + 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/orca-core/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java b/orca-core/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java index 66de4b760d..72e4f9d0fa 100644 --- a/orca-core/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java +++ b/orca-core/src/main/java/com/netflix/spinnaker/orca/config/ExecutionConfigurationProperties.java @@ -42,6 +42,9 @@ public class ExecutionConfigurationProperties { */ private Set 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} 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 index 3d5019edc1..2fd5b60948 100644 --- 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 @@ -269,8 +269,14 @@ private PipelineExecution parseOrchestration(Map 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/test/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncherTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/ExecutionLauncherTest.java index f9400e1e1b..d3a3c79fab 100644 --- 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,79 @@ public void testOrchestrationExecutionWhenUserIsNotInAllowList() throws Exceptio + " disabled for user: not-explicitly-permitted-user@abc.com"); } + @DisplayName( + "when includeAllowedAccounts: true, then the orchestration should contain Spinnaker accounts") + @Test + public void testIncludeSpinnakerAccounts() 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 testExcludeSpinnakerAccounts() 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()); + } + private Map getConfigJson(String resource) throws Exception { return objectMapper.readValue( ExecutionLauncherTest.class.getResourceAsStream(resource), Map.class); diff --git a/orca-core/src/test/resources/execution-launcher-properties.yml b/orca-core/src/test/resources/execution-launcher-properties.yml index b82c89a763..3fb639d87b 100644 --- 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 From 0d6ad7effebd13d0faf75fe47405fceb7936b505 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Thu, 22 Jun 2023 20:58:26 -0700 Subject: [PATCH 3/4] feat(execution): Add feature flag when building a PIPELINE ExecutionType to include or exclude the list of allowed accounts from the execution context --- .../orca/pipeline/ExecutionLauncher.java | 1 + .../orca/pipeline/model/PipelineBuilder.java | 19 +++++++-- .../pipeline/model/PipelineBuilderTest.java | 41 +++++++++++++++++++ 3 files changed, 58 insertions(+), 3 deletions(-) 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 index 2fd5b60948..6255292179 100644 --- 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 config) { config.get("source"), PipelineExecution.PipelineSource.class)) .withSpelEvaluator(getString(config, "spelEvaluator")) .withTemplateVariables(getMap(config, "templateVariables")) + .withIncludeAllowedAccounts(executionConfigurationProperties.isIncludeAllowedAccounts()) .build(); } 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 index f618b47b26..c3c56c2a73 100644 --- 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> 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/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java b/orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/model/PipelineBuilderTest.java index e678d43636..c2028aad33 100644 --- 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()); + } } From ccd04ded03a99364acd000e91318607e84c65188 Mon Sep 17 00:00:00 2001 From: Daniel Zheng Date: Fri, 23 Jun 2023 09:59:28 -0700 Subject: [PATCH 4/4] test(execution): Define the behavior when building a PIPELINE ExecutionType --- .../orca/pipeline/ExecutionLauncherTest.java | 60 ++++++++++++++++++- 1 file changed, 58 insertions(+), 2 deletions(-) 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 index d3a3c79fab..30c8437236 100644 --- 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 @@ -206,7 +206,7 @@ public void testOrchestrationExecutionWhenUserIsNotInAllowList() throws Exceptio @DisplayName( "when includeAllowedAccounts: true, then the orchestration should contain Spinnaker accounts") @Test - public void testIncludeSpinnakerAccounts() throws Exception { + public void testIncludeSpinnakerAccountsInOrchestration() throws Exception { // given MDC.put(Header.USER.getHeader(), "SpinnakerUser"); MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2"); @@ -242,7 +242,7 @@ public void testIncludeSpinnakerAccounts() throws Exception { @DisplayName( "when includeAllowedAccounts: false, then the orchestration should not contain Spinnaker accounts") @Test - public void testExcludeSpinnakerAccounts() throws Exception { + public void testExcludeSpinnakerAccountsFromOrchestration() throws Exception { // given MDC.put(Header.USER.getHeader(), "SpinnakerUser"); MDC.put(Header.ACCOUNTS.getHeader(), "Account1,Account2"); @@ -276,6 +276,62 @@ public void testExcludeSpinnakerAccounts() throws Exception { 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 getConfigJson(String resource) throws Exception { return objectMapper.readValue( ExecutionLauncherTest.class.getResourceAsStream(resource), Map.class);