Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down Expand Up @@ -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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -73,6 +76,7 @@ public void setup() {
clock = Clock.systemUTC();
pipelineValidator = Optional.empty();
registry = Optional.empty();
MDC.clear();

executionLauncher =
new ExecutionLauncher(
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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