Skip to content

Commit

Permalink
fix(rollback): Guard against entity tags not being enabled (#2054)
Browse files Browse the repository at this point in the history 
If entity tags are not enabled, the expectation is that the rollback
will fall back to looking at previous server groups and picking the
newest.
  • Loading branch information
@ajordens
ajordens committed Mar 14, 2018
1 parent 9dd0d69 commit 9f3f4a0
Show file tree
Hide file tree
Showing 12 changed files with 90 additions and 26 deletions.
9 changes: 8 additions & 1 deletion orca-clouddriver/src/main/groovy/com/netflix/spinnaker/orca/clouddriver/FeaturesService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,18 @@

@Component
public class FeaturesService {
private final FeaturesRestService featuresRestService;

@Autowired
private FeaturesRestService featuresRestService;
public FeaturesService(FeaturesRestService featuresRestService) {
this.featuresRestService = featuresRestService;
}

public boolean isStageAvailable(String stageType) {
return featuresRestService.getStages().stream().anyMatch(s -> s.enabled && stageType.equals(s.name));
}

public boolean areEntityTagsAvailable() {
return isStageAvailable("upsertEntityTags");
}
}
2 changes: 1 addition & 1 deletion ...nnaker/orca/clouddriver/pipeline/providers/aws/ApplySourceServerGroupCapacityStage.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class ApplySourceServerGroupCapacityStage implements StageDefinitionBuilder {
@Override
List<Stage> afterStages(@Nonnull Stage stage) {
try {
def taggingEnabled = featuresService.isStageAvailable("upsertEntityTags")
def taggingEnabled = featuresService.areEntityTagsAvailable()
if (!taggingEnabled) {
return []
}
Expand Down
2 changes: 1 addition & 1 deletion .../com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CloneServerGroupStage.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class CloneServerGroupStage extends AbstractDeployStrategyStage {

@Override
protected List<TaskNode.TaskDefinition> basicTasks(Stage stage) {
def taggingEnabled = featuresService.isStageAvailable("upsertEntityTags")
def taggingEnabled = featuresService.areEntityTagsAvailable()

def tasks = [
new TaskNode.TaskDefinition("cloneServerGroup", CloneServerGroupTask),
Expand Down
2 changes: 1 addition & 1 deletion ...com/netflix/spinnaker/orca/clouddriver/pipeline/servergroup/CreateServerGroupStage.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class CreateServerGroupStage extends AbstractDeployStrategyStage {

@Override
protected List<TaskNode.TaskDefinition> basicTasks(Stage stage) {
def taggingEnabled = featuresService.isStageAvailable("upsertEntityTags")
def taggingEnabled = featuresService.areEntityTagsAvailable()

def tasks = [
TaskNode.task("createServerGroup", CreateServerGroupTask),
Expand Down
7 changes: 6 additions & 1 deletion ...lix/spinnaker/orca/clouddriver/pipeline/servergroup/rollback/PreviousImageRollback.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore
import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.frigga.Names
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.orca.clouddriver.FeaturesService
import com.netflix.spinnaker.orca.clouddriver.OortService
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.CloneServerGroupStage
import com.netflix.spinnaker.orca.pipeline.model.Stage
Expand All @@ -45,13 +46,17 @@ class PreviousImageRollback implements Rollback {
@JsonIgnore
OortService oortService

@Autowired
@JsonIgnore
FeaturesService featuresService

@Autowired
@JsonIgnore
RetrySupport retrySupport

@Override
List<Stage> buildStages(Stage parentStage) {
def previousImageRollbackSupport = new PreviousImageRollbackSupport(objectMapper, oortService, retrySupport)
def previousImageRollbackSupport = new PreviousImageRollbackSupport(objectMapper, oortService, featuresService, retrySupport)
def stages = []

def parentStageContext = parentStage.context
Expand Down
35 changes: 28 additions & 7 deletions ...pinnaker/orca/clouddriver/pipeline/servergroup/rollback/PreviousImageRollbackSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,38 +19,59 @@
import com.fasterxml.jackson.databind.DeserializationFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.netflix.spinnaker.kork.core.RetrySupport;
import com.netflix.spinnaker.orca.clouddriver.FeaturesService;
import com.netflix.spinnaker.orca.clouddriver.OortService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Objects;

public class PreviousImageRollbackSupport {
private final Logger log = LoggerFactory.getLogger(this.getClass());

private final ObjectMapper objectMapper;
private final OortService oortService;
private final FeaturesService featuresService;
private final RetrySupport retrySupport;

public PreviousImageRollbackSupport(ObjectMapper objectMapper,
OortService oortService,
FeaturesService featuresService,
RetrySupport retrySupport) {
this.objectMapper = objectMapper.configure(
DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false
);
this.oortService = oortService;
this.featuresService = featuresService;
this.retrySupport = retrySupport;
}

public ImageDetails getImageDetailsFromEntityTags(String cloudProvider,
String credentials,
String region,
String serverGroupName) {
List<Map> entityTags = retrySupport.retry(() -> oortService.getEntityTags(
cloudProvider,
"serverGroup",
serverGroupName,
credentials,
region
), 15, 2000, false);
List<Map> entityTags = null;

try {
entityTags = retrySupport.retry(() -> {
if (!featuresService.areEntityTagsAvailable()) {
return Collections.emptyList();
}

return oortService.getEntityTags(
cloudProvider,
"serverGroup",
serverGroupName,
credentials,
region
);
}, 15, 2000, false);
} catch (Exception e) {
log.warn("Unable to fetch entity tags, reason: {}", e.getMessage());
}

if (entityTags != null && entityTags.size() > 1) {
// this should _not_ happen
Expand Down
10 changes: 8 additions & 2 deletions ...com/netflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.netflix.spinnaker.orca.ExecutionStatus;
import com.netflix.spinnaker.orca.RetryableTask;
import com.netflix.spinnaker.orca.TaskResult;
import com.netflix.spinnaker.orca.clouddriver.FeaturesService;
import com.netflix.spinnaker.orca.clouddriver.OortService;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.RollbackServerGroupStage;
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback.PreviousImageRollbackSupport;
Expand Down Expand Up @@ -71,17 +72,22 @@ public class DetermineRollbackCandidatesTask extends AbstractCloudProviderAwareT
private final ObjectMapper objectMapper;
private final RetrySupport retrySupport;
private final OortService oortService;
private final FeaturesService featuresService;
private final PreviousImageRollbackSupport previousImageRollbackSupport;

@Autowired
public DetermineRollbackCandidatesTask(ObjectMapper objectMapper,
RetrySupport retrySupport,
OortService oortService) {
OortService oortService,
FeaturesService featuresService) {
this.objectMapper = objectMapper;
this.retrySupport = retrySupport;
this.oortService = oortService;
this.featuresService = featuresService;

this.previousImageRollbackSupport = new PreviousImageRollbackSupport(objectMapper, oortService, retrySupport);
this.previousImageRollbackSupport = new PreviousImageRollbackSupport(
objectMapper, oortService, featuresService, retrySupport
);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...louddriver/src/main/groovy/com/netflix/spinnaker/orca/kato/pipeline/RollingPushStage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public class RollingPushStage implements StageDefinitionBuilder {

@Override
public void taskGraph(Stage stage, TaskNode.Builder builder) {
boolean taggingEnabled = featuresService.isStageAvailable("upsertEntityTags");
boolean taggingEnabled = featuresService.areEntityTagsAvailable();
builder
.withTask("captureParentInterestingHealthProviderNames", CaptureParentInterestingHealthProviderNamesTask.class)
.withTask("determineTerminationCandidates", DetermineTerminationCandidatesTask.class)
Expand Down
8 changes: 4 additions & 4 deletions ...er/orca/clouddriver/pipeline/providers/aws/ApplySourceServerGroupCapacityStageSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class ApplySourceServerGroupCapacityStageSpec extends Specification {
def afterStages = stageBuilder.afterStages(stage)

then:
1 * featuresService.isStageAvailable("upsertEntityTags") >> { return false }
1 * featuresService.areEntityTagsAvailable() >> { return false }
0 * oortService.getEntityTags(_)

afterStages.isEmpty()
Expand All @@ -64,7 +64,7 @@ class ApplySourceServerGroupCapacityStageSpec extends Specification {
def afterStages = stageBuilder.afterStages(stage)

then:
1 * featuresService.isStageAvailable("upsertEntityTags") >> { return true }
1 * featuresService.areEntityTagsAvailable() >> { return true }
1 * oortService.getEntityTags(_) >> { return [] }

afterStages.isEmpty()
Expand All @@ -77,7 +77,7 @@ class ApplySourceServerGroupCapacityStageSpec extends Specification {
then:
notThrown(RuntimeException)

1 * featuresService.isStageAvailable("upsertEntityTags") >> { throw new RuntimeException("An Exception!") }
1 * featuresService.areEntityTagsAvailable() >> { throw new RuntimeException("An Exception!") }
0 * oortService.getEntityTags(_)

afterStages.isEmpty()
Expand All @@ -88,7 +88,7 @@ class ApplySourceServerGroupCapacityStageSpec extends Specification {
def afterStages = stageBuilder.afterStages(stage)

then:
1 * featuresService.isStageAvailable("upsertEntityTags") >> { return true }
1 * featuresService.areEntityTagsAvailable() >> { return true }
1 * oortService.getEntityTags([
"tag:spinnaker:pinned_capacity": "*",
"entityId" : "app-stack-details-v001",
Expand Down
7 changes: 6 additions & 1 deletion ...spinnaker/orca/clouddriver/pipeline/servergroup/rollback/PreviousImageRollbackSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.orca.clouddriver.FeaturesService
import com.netflix.spinnaker.orca.clouddriver.OortService
import com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.CloneServerGroupStage
import com.netflix.spinnaker.orca.pipeline.model.SyntheticStageOwner
Expand All @@ -31,6 +32,9 @@ class PreviousImageRollbackSpec extends Specification {
def objectMapper = new ObjectMapper()
def cloneServerGroupStage = new CloneServerGroupStage()
def oortService = Mock(OortService)
def featuresService = Mock(FeaturesService) {
_ * areEntityTagsAvailable() >> { return true }
}
def retrySupport = Spy(RetrySupport) {
_ * sleep(_) >> { /* do nothing */ }
}
Expand All @@ -41,6 +45,7 @@ class PreviousImageRollbackSpec extends Specification {

cloneServerGroupStage: cloneServerGroupStage,
oortService: oortService,
featuresService: featuresService,
retrySupport: retrySupport
)

Expand All @@ -66,7 +71,7 @@ class PreviousImageRollbackSpec extends Specification {
def afterStages = allStages.findAll { it.syntheticStageOwner == SyntheticStageOwner.STAGE_AFTER }

then:
(imageName ? 0 : 1) * oortService.getEntityTags(_, _, _, _, _) >> {
(imageName ? 0 : 1) * oortService.getEntityTags(*_) >> {
// should only call `getEntityTags()` if an image was not explicitly provided to the rollback
return [[
tags: [
Expand Down
16 changes: 15 additions & 1 deletion ...er/orca/clouddriver/pipeline/servergroup/rollback/PreviousImageRollbackSupportSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,28 @@ package com.netflix.spinnaker.orca.clouddriver.pipeline.servergroup.rollback

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.orca.clouddriver.FeaturesService
import com.netflix.spinnaker.orca.clouddriver.OortService
import spock.lang.Specification
import spock.lang.Subject;

class PreviousImageRollbackSupportSpec extends Specification {
def objectMapper = new ObjectMapper()
def oortService = Mock(OortService)
def featuresService = Mock(FeaturesService)
def retrySupport = Spy(RetrySupport) {
_ * sleep(_) >> { /* do nothing */ }
}

@Subject
def rollbackSupport = new PreviousImageRollbackSupport(objectMapper, oortService, retrySupport)
def rollbackSupport = new PreviousImageRollbackSupport(objectMapper, oortService, featuresService, retrySupport)

def "should raise exception if multiple entity tags found"() {
when:
rollbackSupport.getImageDetailsFromEntityTags("aws", "test", "us-west-2", "application-v002")

then:
1 * featuresService.areEntityTagsAvailable() >> { return true }
1 * oortService.getEntityTags(*_) >> {
return [
[id: "1"],
Expand All @@ -47,4 +50,15 @@ class PreviousImageRollbackSupportSpec extends Specification {
def e = thrown(IllegalStateException)
e.message == "More than one set of entity tags found for aws:serverGroup:application-v002:test:us-west-2"
}

def "should not attempt to fetch entity tags when not enabled"() {
when:
def imageDetails = rollbackSupport.getImageDetailsFromEntityTags("aws", "test", "us-west-2", "application-v002")

then:
1 * featuresService.areEntityTagsAvailable() >> { return false }
0 * oortService.getEntityTags(*_)

imageDetails == null
}
}
16 changes: 11 additions & 5 deletions ...tflix/spinnaker/orca/clouddriver/tasks/cluster/DetermineRollbackCandidatesTaskSpec.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package com.netflix.spinnaker.orca.clouddriver.tasks.cluster

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.kork.core.RetrySupport
import com.netflix.spinnaker.orca.clouddriver.FeaturesService
import com.netflix.spinnaker.orca.clouddriver.OortService
import retrofit.client.Response
import retrofit.mime.TypedByteArray
Expand All @@ -32,12 +33,14 @@ import static com.netflix.spinnaker.orca.test.model.ExecutionBuilder.stage;
class DetermineRollbackCandidatesTaskSpec extends Specification {
def objectMapper = new ObjectMapper()
def oortService = Mock(OortService)
def featuresService = Mock(FeaturesService)

@Subject
def task = new DetermineRollbackCandidatesTask(
objectMapper,
new RetrySupport(),
oortService
oortService,
featuresService
)

def stage = stage {
Expand Down Expand Up @@ -77,6 +80,8 @@ class DetermineRollbackCandidatesTaskSpec extends Specification {
]
])
}
1 * featuresService.areEntityTagsAvailable() >> { return areEntityTagsEnabled }
(shouldFetchEntityTags ? 1 : 0) * oortService.getEntityTags(*_) >> { return [] }

result.context == [
imagesToRestore: [
Expand All @@ -97,9 +102,9 @@ class DetermineRollbackCandidatesTaskSpec extends Specification {
]

where:
additionalStageContext || shouldFetchServerGroup
[:] || false // stage context includes moniker, no need to fetch server group
[moniker: null, serverGroup: "servergroup-v001"] || true
additionalStageContext | areEntityTagsEnabled || shouldFetchServerGroup || shouldFetchEntityTags
[:] | true || false || true // stage context includes moniker, no need to fetch server group
[moniker: null, serverGroup: "servergroup-v001"] | false || true || false
}

def "should build PREVIOUS_IMAGE rollback context when there are _only_ entity tags"() {
Expand All @@ -114,7 +119,8 @@ class DetermineRollbackCandidatesTaskSpec extends Specification {
]
])
}
1 * oortService.getEntityTags(_, _, _, _, _) >> {
1 * featuresService.areEntityTagsAvailable() >> { return true }
1 * oortService.getEntityTags(*_) >> {
return [buildSpinnakerMetadata("my_image-0", "ami-xxxxx0", "5")]
}

Expand Down

0 comments on commit 9f3f4a0

Please sign in to comment.