From c6a2a1c06275904e8e3333ed0480f3c69d134ee1 Mon Sep 17 00:00:00 2001 From: Oleg Sidorov Date: Tue, 11 Feb 2020 19:51:49 +0100 Subject: [PATCH] Fixed out-of-range index dereferencing error in release controller 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 --- pkg/controller/release/release_controller.go | 167 ++++++++------ .../release/release_controller_test.go | 73 +++++- pkg/controller/release/strategy_executor.go | 212 ++++++++---------- 3 files changed, 257 insertions(+), 195 deletions(-) diff --git a/pkg/controller/release/release_controller.go b/pkg/controller/release/release_controller.go index 653f83d07..8928ad602 100644 --- a/pkg/controller/release/release_controller.go +++ b/pkg/controller/release/release_controller.go @@ -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() { @@ -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, @@ -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: @@ -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 { diff --git a/pkg/controller/release/release_controller_test.go b/pkg/controller/release/release_controller_test.go index c7554582a..91a2ee957 100644 --- a/pkg/controller/release/release_controller_test.go +++ b/pkg/controller/release/release_controller_test.go @@ -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 ( @@ -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]`, } } @@ -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{ @@ -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), }, } @@ -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), }, } @@ -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() } @@ -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() +} diff --git a/pkg/controller/release/strategy_executor.go b/pkg/controller/release/strategy_executor.go index d4a315650..848232bb8 100644 --- a/pkg/controller/release/strategy_executor.go +++ b/pkg/controller/release/strategy_executor.go @@ -8,23 +8,10 @@ import ( shipper "github.com/bookingcom/shipper/pkg/apis/shipper/v1alpha1" "github.com/bookingcom/shipper/pkg/controller" - shippererrors "github.com/bookingcom/shipper/pkg/errors" "github.com/bookingcom/shipper/pkg/util/conditions" releaseutil "github.com/bookingcom/shipper/pkg/util/release" ) -type StrategyExecutor struct { - curr, prev, succ *releaseInfo -} - -func NewStrategyExecutor(prev, curr, succ *releaseInfo) *StrategyExecutor { - return &StrategyExecutor{ - prev: prev, - curr: curr, - succ: succ, - } -} - type PipelineContinuation bool const ( @@ -49,19 +36,106 @@ type Extra struct { HasIncumbent bool } -func (p *Pipeline) Process(strategy *shipper.RolloutStrategy, targetStep int32, extra Extra, cond conditions.StrategyConditionsMap) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { - var res []StrategyPatch +func (p *Pipeline) Process(strategy *shipper.RolloutStrategy, step int32, extra Extra, cond conditions.StrategyConditionsMap) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { + var patches []StrategyPatch var trans []ReleaseStrategyStateTransition - for _, step := range *p { - cont, stepres, steptrans := step(strategy, targetStep, extra, cond) - res = append(res, stepres...) + for _, stage := range *p { + cont, steppatches, steptrans := stage(strategy, step, extra, cond) + patches = append(patches, steppatches...) trans = append(trans, steptrans...) if cont == PipelineBreak { - return false, res, trans + return false, patches, trans } } - return true, res, trans + return true, patches, trans +} + +type StrategyExecutor struct { + strategy *shipper.RolloutStrategy + step int32 +} + +func NewStrategyExecutor(strategy *shipper.RolloutStrategy, step int32) *StrategyExecutor { + return &StrategyExecutor{ + strategy: strategy, + step: step, + } +} + +/* + For each release object: + 0. Ensure release scheduled. + 0.1. Choose clusters. + 0.2. Ensure target objects exist. + 0.2.1. Compare chosen clusters and if different, update the spec. + 1. Find it's ancestor. + 2. For the head release, ensure installation. + 2.1. Simply check installation targets. + 3. For the head release, ensure capacity. + 3.1. Ensure the capacity corresponds to the strategy contender. + 4. For the head release, ensure traffic. + 4.1. Ensure the traffic corresponds to the strategy contender. + 5. For a tail release, ensure traffic. + 5.1. Look at the leader and check it's target traffic. + 5.2. Look at the strategy and figure out the target traffic. + 6. For a tail release, ensure capacity. + 6.1. Look at the leader and check it's target capacity. + 6.2 Look at the strategy and figure out the target capacity. + 7. Make necessary adjustments to the release object. +*/ + +func (e *StrategyExecutor) Execute(prev, curr, succ *releaseInfo) (bool, []StrategyPatch, []ReleaseStrategyStateTransition) { + isHead, hasTail := succ == nil, prev != nil + hasIncumbent := prev != nil || succ != nil + + // There is no really a point in making any changes until the successor + // has completed it's transition, therefore we're hoilding off and aborting + // the pipeline execution. An alternative to this approach could be to make + // an autonomous move purely based on the picture of the world. But due to + // the limited visilibility of what's happening to the successor (as it + // might be following it's successor) it could be that a preliminary action + // would create more noise than help really. + if !isHead { + if !releaseutil.ReleaseAchievedTargetStep(succ.release) { + return false, nil, nil + } + } + + var releaseStrategyConditions []shipper.ReleaseStrategyCondition + if curr.release.Status.Strategy != nil { + releaseStrategyConditions = curr.release.Status.Strategy.Conditions + } + cond := conditions.NewStrategyConditions(releaseStrategyConditions...) + + // the last step is slightly special from others: at this moment shipper + // no longer waits for a command but marks a release as complete. + isLastStep := int(e.step) == len(e.strategy.Steps)-1 + // The reason because isHead is not included in the extra set is mainly + // because the pipeline is picking up 2 distinct tuples of releases + // (curr+succ) and (prev+curr), therefore isHead is supposed to be + // calculated by enforcers. + extra := Extra{ + HasIncumbent: hasIncumbent, + IsLastStep: isLastStep, + } + + pipeline := NewPipeline() + if isHead { + pipeline.Enqueue(genInstallationEnforcer(curr, nil)) + } + pipeline.Enqueue(genCapacityEnforcer(curr, succ)) + pipeline.Enqueue(genTrafficEnforcer(curr, succ)) + + if isHead { + if hasTail { + pipeline.Enqueue(genTrafficEnforcer(prev, curr)) + pipeline.Enqueue(genCapacityEnforcer(prev, curr)) + } + pipeline.Enqueue(genReleaseStrategyStateEnforcer(curr, nil)) + } + + return pipeline.Process(e.strategy, e.step, extra, cond) } func genInstallationEnforcer(curr, succ *releaseInfo) PipelineStep { @@ -274,104 +348,6 @@ func genReleaseStrategyStateEnforcer(curr, succ *releaseInfo) PipelineStep { } } -/* - For each release object: - 0. Ensure release scheduled. - 0.1. Choose clusters. - 0.2. Ensure target objects exist. - 0.2.1. Compare chosen clusters and if different, update the spec. - 1. Find it's ancestor. - 2. For the head release, ensure installation. - 2.1. Simply check installation targets. - 3. For the head release, ensure capacity. - 3.1. Ensure the capacity corresponds to the strategy contender. - 4. For the head release, ensure traffic. - 4.1. Ensure the traffic corresponds to the strategy contender. - 5. For a tail release, ensure traffic. - 5.1. Look at the leader and check it's target traffic. - 5.2. Look at the strategy and figure out the target traffic. - 6. For a tail release, ensure capacity. - 6.1. Look at the leader and check it's target capacity. - 6.2 Look at the strategy and figure out the target capacity. - 7. Make necessary adjustments to the release object. -*/ - -func (e *StrategyExecutor) Execute() (bool, []StrategyPatch, []ReleaseStrategyStateTransition, error) { - isHead, hasTail := e.succ == nil, e.prev != nil - hasIncumbent := e.prev != nil || e.succ != nil - - // There is no really a point in making any changes until the successor - // has completed it's transition, therefore we're hoilding off and aborting - // the pipeline execution. An alternative to this approach could be to make - // an autonomous move purely based on the picture of the world. But due to - // the limited visilibility of what's happening to the successor (as it - // might be following it's successor) it could be that a preliminary action - // would create more noise than help really. - if !isHead { - if !releaseutil.ReleaseAchievedTargetStep(e.succ.release) { - return false, nil, nil, nil - //shippererrors.NewContenderStrategyIncompleteError( - // controller.MetaKey(e.succ.release)) - } - } - - var releaseStrategyConditions []shipper.ReleaseStrategyCondition - if e.curr.release.Status.Strategy != nil { - releaseStrategyConditions = e.curr.release.Status.Strategy.Conditions - } - cond := conditions.NewStrategyConditions(releaseStrategyConditions...) - - 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 = e.curr.release.Spec.Environment.Strategy - targetStep = e.curr.release.Spec.TargetStep - } else { - strategy = e.succ.release.Spec.Environment.Strategy - targetStep = e.succ.release.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(e.curr.release)) - return false, nil, nil, shippererrors.NewUnrecoverableError(err) - } - - // the last step is slightly special from others: at this moment shipper - // no longer waits for a command but marks a release as complete. - isLastStep := int(targetStep) == len(strategy.Steps)-1 - // The reason because isHead is not included in the extra set is mainly - // because the is picking up 2 distinct tuples of releases (curr+succ) and - // (prev+curr), therefore isHead is supposed to be calculated by enforcers. - extra := Extra{ - HasIncumbent: hasIncumbent, - IsLastStep: isLastStep, - } - - pipeline := NewPipeline() - if isHead { - pipeline.Enqueue(genInstallationEnforcer(e.curr, nil)) - } - pipeline.Enqueue(genCapacityEnforcer(e.curr, e.succ)) - pipeline.Enqueue(genTrafficEnforcer(e.curr, e.succ)) - - if isHead { - if hasTail { - pipeline.Enqueue(genTrafficEnforcer(e.prev, e.curr)) - pipeline.Enqueue(genCapacityEnforcer(e.prev, e.curr)) - } - pipeline.Enqueue(genReleaseStrategyStateEnforcer(e.curr, nil)) - } - - complete, patches, trans := pipeline.Process(strategy, targetStep, extra, cond) - - return complete, patches, trans, nil -} - func buildContenderStrategyConditionsPatch( name string, cond conditions.StrategyConditionsMap,