Skip to content
This repository has been archived by the owner on May 3, 2022. It is now read-only.

Commit

Permalink
Fixed out-of-range index dereferencing error in release controller
Browse files Browse the repository at this point in the history 
This commit fixes a problem where a corresponding release strategy was
resolved twice: once for an actual execution and second time for
reporting. These resolutions were happening in distinct places: the 1st
one in strategy executor, the latter in release controller. As a result,
the release controller one was causing a panic as it was not taking into
account the updated logic of strategy resolution where an incumbent is
supposed to look ahead and use it's successor's strategy, and the index
validity was only happening in strategy executor, which was calculating
the desired strategy correctly. This commit is also moving things
around: strategy executor is being initialized with a specific strategy
and a pointer to the target step and Execute() step takes the sequence
of executable releases as arguments.

Signed-off-by: Oleg Sidorov <[email protected]>
  • Loading branch information
Oleg Sidorov committed Feb 13, 2020
1 parent fa72fb9 commit c6a2a1c
Show file tree
Hide file tree
Showing 3 changed files with 257 additions and 195 deletions.
167 changes: 97 additions & 70 deletions pkg/controller/release/release_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,15 +273,13 @@ func (c *Controller) syncOneReleaseHandler(key string) error {
}

var condition *shipper.ReleaseCondition
var strategyPatches []StrategyPatch
var trans []ReleaseStrategyStateTransition
var relinfo *releaseInfo
var stepAchieved bool
var patches []StrategyPatch
var execRel *shipper.Release

// we keep baseRel as a comparison baseline in order to figure out if
// we even have to send an update
baseRel := rel.DeepCopy()
patches := make([]StrategyPatch, 0)

diff := diffutil.NewMultiDiff()
defer func() {
Expand Down Expand Up @@ -357,7 +355,7 @@ func (c *Controller) syncOneReleaseHandler(key string) error {
)
diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition))

stepAchieved, strategyPatches, trans, err = c.executeReleaseStrategy(relinfo)
execRel, patches, err = c.executeReleaseStrategy(relinfo, diff)
if err != nil {
releaseStrategyExecutedCond := releaseutil.NewReleaseCondition(
shipper.ReleaseConditionTypeStrategyExecuted,
Expand All @@ -369,59 +367,7 @@ func (c *Controller) syncOneReleaseHandler(key string) error {

goto ApplyChanges
}
patches = append(patches, strategyPatches...)

condition = releaseutil.NewReleaseCondition(
shipper.ReleaseConditionTypeStrategyExecuted,
corev1.ConditionTrue,
"",
"",
)
diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition))

if stepAchieved {
strategy := rel.Spec.Environment.Strategy
targetStep := rel.Spec.TargetStep
isLastStep := int(targetStep) == len(strategy.Steps)-1
previouslyAchievedStep := rel.Status.AchievedStep

if previouslyAchievedStep == nil || targetStep != previouslyAchievedStep.Step {
targetStepName := strategy.Steps[targetStep].Name
rel.Status.AchievedStep = &shipper.AchievedStep{
Step: targetStep,
Name: targetStepName,
}
c.recorder.Eventf(
rel,
corev1.EventTypeNormal,
"StrategyApplied",
"step [%d] finished",
targetStep,
)
}

if isLastStep {
condition := releaseutil.NewReleaseCondition(
shipper.ReleaseConditionTypeComplete,
corev1.ConditionTrue,
"",
"",
)
diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition))
}
}

for _, t := range trans {
c.recorder.Eventf(
rel,
corev1.EventTypeNormal,
"ReleaseStateTransitioned",
"Release %q had its state %q transitioned to %q",
shippercontroller.MetaKey(rel),
t.State,
t.New,
)
}
rel = execRel

ApplyChanges:

Expand Down Expand Up @@ -454,41 +400,122 @@ func (c *Controller) applicationReleases(rel *shipper.Release) ([]*shipper.Relea
return releases, nil
}

func (c *Controller) executeReleaseStrategy(relinfo *releaseInfo) (bool, []StrategyPatch, []ReleaseStrategyStateTransition, error) {
releases, err := c.applicationReleases(relinfo.release)
func (c *Controller) executeReleaseStrategy(relinfo *releaseInfo, diff *diffutil.MultiDiff) (*shipper.Release, []StrategyPatch, error) {
rel := relinfo.release.DeepCopy()

releases, err := c.applicationReleases(rel)
if err != nil {
return false, nil, nil, err
return nil, nil, err
}
prev, succ, err := releaseutil.GetSiblingReleases(relinfo.release, releases)
prev, succ, err := releaseutil.GetSiblingReleases(rel, releases)
if err != nil {
return false, nil, nil, err
return nil, nil, err
}

var relinfoPrev, relinfoSucc *releaseInfo
if prev != nil {
relinfoPrev, err = c.buildReleaseInfo(prev)
if err != nil {
return false, nil, nil, err
return nil, nil, err
}
}
if succ != nil {
relinfoSucc, err = c.buildReleaseInfo(succ)
if err != nil {
return false, nil, nil, err
return nil, nil, err
}
}

executor := NewStrategyExecutor(relinfoPrev, relinfo, relinfoSucc)
isHead := succ == nil
var strategy *shipper.RolloutStrategy
var targetStep int32
// A head release uses it's local spec-defined strategy, any other release
// follows it's successor state, therefore looking into the forecoming spec.
if isHead {
strategy = rel.Spec.Environment.Strategy
targetStep = rel.Spec.TargetStep
} else {
strategy = succ.Spec.Environment.Strategy
targetStep = succ.Spec.TargetStep
}

// Looks like a malformed input. Informing about a problem and bailing out.
if targetStep >= int32(len(strategy.Steps)) {
err := fmt.Errorf("no step %d in strategy for Release %q",
targetStep, controller.MetaKey(rel))
return nil, nil, shippererrors.NewUnrecoverableError(err)
}

executor := NewStrategyExecutor(strategy, targetStep)

complete, patches, trans, err := executor.Execute()
complete, patches, trans := executor.Execute(relinfoPrev, relinfo, relinfoSucc)

if len(patches) == 0 {
klog.V(4).Infof("Strategy verified for release %q, nothing to patch", controller.MetaKey(relinfo.release))
klog.V(4).Infof("Strategy verified for release %q, nothing to patch", controller.MetaKey(rel))
} else {
klog.V(4).Infof("Strategy has been executed for release %q, applying patches", controller.MetaKey(relinfo.release))
klog.V(4).Infof("Strategy has been executed for release %q, applying patches", controller.MetaKey(rel))
}

condition := releaseutil.NewReleaseCondition(
shipper.ReleaseConditionTypeStrategyExecuted,
corev1.ConditionTrue,
"",
"",
)
diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition))

isLastStep := int(targetStep) == len(strategy.Steps)-1
prevStep := rel.Status.AchievedStep

if complete {
var achievedStep int32
var achievedStepName string
if isHead {
achievedStep = targetStep
achievedStepName = strategy.Steps[achievedStep].Name
} else {
achievedStep = int32(len(rel.Spec.Environment.Strategy.Steps)) - 1
achievedStepName = rel.Spec.Environment.Strategy.Steps[achievedStep].Name

}
if prevStep == nil || achievedStep != prevStep.Step {
rel.Status.AchievedStep = &shipper.AchievedStep{
Step: achievedStep,
Name: achievedStepName,
}
c.recorder.Eventf(
rel,
corev1.EventTypeNormal,
"StrategyApplied",
"step [%d] finished",
achievedStep,
)
}

if isLastStep {
condition := releaseutil.NewReleaseCondition(
shipper.ReleaseConditionTypeComplete,
corev1.ConditionTrue,
"",
"",
)
diff.Append(releaseutil.SetReleaseCondition(&rel.Status, *condition))
}
}

for _, t := range trans {
c.recorder.Eventf(
rel,
corev1.EventTypeNormal,
"ReleaseStateTransitioned",
"Release %q had its state %q transitioned to %q",
shippercontroller.MetaKey(rel),
t.State,
t.New,
)
}

return complete, patches, trans, err
return rel, patches, nil
}

func (c *Controller) applyPatch(namespace string, patch StrategyPatch) error {
Expand Down
73 changes: 66 additions & 7 deletions pkg/controller/release/release_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,16 @@ var vanguard = shipper.RolloutStrategy{
},
}

var fullon = shipper.RolloutStrategy{
Steps: []shipper.RolloutStrategyStep{
{
Name: "full on",
Capacity: shipper.RolloutStrategyStepValue{Incumbent: 0, Contender: 100},
Traffic: shipper.RolloutStrategyStepValue{Incumbent: 0, Contender: 100},
},
},
}

type role int

const (
Expand Down Expand Up @@ -726,7 +736,7 @@ func (f *fixture) expectReleaseWaitingForCommand(rel *shipper.Release, step int3
fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForCommand" transitioned to "True"`, relKey),
fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForInstallation" transitioned to "False"`, relKey),
fmt.Sprintf(`Normal ReleaseStateTransitioned Release "%s" had its state "WaitingForTraffic" transitioned to "False"`, relKey),
fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`),
`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`,
}
}

Expand Down Expand Up @@ -2338,8 +2348,6 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T
contender := f.buildContender(namespace, contenderName, totalReplicaCount)
incumbent := f.buildIncumbent(namespace, incumbentName, totalReplicaCount)

missingStepMsg := fmt.Sprintf("failed to execute strategy: \"no step 2 in strategy for Release \\\"%s/%s\\\"\"", namespace, contenderName)

// We define 2 steps and will intentionally set target step index out of this bound
strategyStaging := shipper.RolloutStrategy{
Steps: []shipper.RolloutStrategyStep{
Expand Down Expand Up @@ -2379,7 +2387,9 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T
},
{
Type: shipper.ReleaseConditionTypeStrategyExecuted,
Status: corev1.ConditionTrue,
Status: corev1.ConditionFalse,
Reason: conditions.StrategyExecutionFailed,
Message: fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, incumbentName),
},
}

Expand All @@ -2397,7 +2407,7 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T
Type: shipper.ReleaseConditionTypeStrategyExecuted,
Status: corev1.ConditionFalse,
Reason: conditions.StrategyExecutionFailed,
Message: missingStepMsg,
Message: fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, contenderName),
},
}

Expand Down Expand Up @@ -2434,8 +2444,11 @@ func TestApplicationExposesStrategyFailureSuccessorIndexOutOfBounds(t *testing.T
[]string{"releases"},
})
f.expectedEvents = append(f.expectedEvents,
`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted True]`,
fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`, missingStepMsg))
fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`,
fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, incumbentName)),
fmt.Sprintf(`Normal ReleaseConditionChanged [] -> [Scheduled True], [] -> [StrategyExecuted False StrategyExecutionFailed %s]`,
fmt.Sprintf(`failed to execute strategy: "no step 2 in strategy for Release \"%s/%s\""`, namespace, contenderName)),
)

f.run()
}
Expand Down Expand Up @@ -2770,3 +2783,49 @@ func TestControllerChooseClustersSkipsUnschedulable(t *testing.T) {

f.run()
}

func TestIncumbentOutOfRangeTargetStep(t *testing.T) {
namespace := "test-namespace"
incumbentName, contenderName := "test-incumbent", "test-contender"
app := buildApplication(namespace, "test-app")
cluster := buildCluster("minikube")

f := newFixture(t, app.DeepCopy(), cluster.DeepCopy())
f.cycles = 2

totalReplicaCount := int32(10)
contender := f.buildContender(namespace, contenderName, totalReplicaCount)
incumbent := f.buildIncumbent(namespace, incumbentName, totalReplicaCount)

// Incumbent spec contains only 1 strategy step but we intentionally
// specify an out-of-range index in order to test if it carefully
// handles indices.
incumbent.release.Spec.TargetStep = 2
incumbent.release.Spec.Environment.Strategy = &fullon
incumbent.release.Status.AchievedStep.Step = 0
incumbent.trafficTarget.Spec.Clusters[0].Weight = 50
incumbent.capacityTarget.Spec.Clusters[0].Percent = 50
incumbent.capacityTarget.Spec.Clusters[0].TotalReplicaCount = totalReplicaCount

step := int32(1)
contender.release.Spec.TargetStep = step
contender.capacityTarget.Spec.Clusters[0].Percent = 50
contender.capacityTarget.Spec.Clusters[0].TotalReplicaCount = totalReplicaCount
contender.trafficTarget.Spec.Clusters[0].Weight = 50

f.addObjects(
contender.release.DeepCopy(),
contender.installationTarget.DeepCopy(),
contender.capacityTarget.DeepCopy(),
contender.trafficTarget.DeepCopy(),

incumbent.release.DeepCopy(),
incumbent.installationTarget.DeepCopy(),
incumbent.capacityTarget.DeepCopy(),
incumbent.trafficTarget.DeepCopy(),
)

f.expectReleaseWaitingForCommand(contender.release, step)

f.run()
}
Loading

0 comments on commit c6a2a1c

Please sign in to comment.