fix: return error instead of panicking on empty rollback stages

Author: armistcxy

pkg/app/pipedv1/plugin/ecs/deployment/pipeline.go

@@ -15,9 +15,15 @@
 package deployment
 
 import (
+	"errors"
+
 	sdk "github.com/pipe-cd/piped-plugin-sdk-go"
 )
 
+// ErrRollbackRequiresStages is returned when a rollback is requested but no
+// stage was provided in the request.
+var ErrRollbackRequiresStages = errors.New("rollback requires at least one stage")
+
 const (
 	// StageECSSync represents the ECS sync stage.
 	StageECSSync = "ECS_SYNC"
@@ -76,7 +82,7 @@ func buildQuickSyncPipeline(autoRollback bool) []sdk.QuickSyncStage {
 	return out
 }
 
-func buildPipelineStages(input *sdk.BuildPipelineSyncStagesInput) []sdk.PipelineStage {
+func buildPipelineStages(input *sdk.BuildPipelineSyncStagesInput) ([]sdk.PipelineStage, error) {
 	stages := input.Request.Stages
 
 	out := make([]sdk.PipelineStage, 0, len(stages)+1)
@@ -90,6 +96,11 @@ func buildPipelineStages(input *sdk.BuildPipelineSyncStagesInput) []sdk.Pipeline
 	}
 
 	if input.Request.Rollback {
+		// Guard against a rollback request without any stage, which would
+		// panic on stages[0] below.
+		if len(stages) == 0 {
+			return nil, ErrRollbackRequiresStages
+		}
 		// The rollback stage must reuse one of the requested indexes and
 		// piped runs rollback stages as a trail sorted by index.
 		// Use the smallest requested index so our rollback runs first among all plugins' rollbacks.
@@ -106,5 +117,5 @@ func buildPipelineStages(input *sdk.BuildPipelineSyncStagesInput) []sdk.Pipeline
 		})
 	}
 
-	return out
+	return out, nil
 }

pkg/app/pipedv1/plugin/ecs/deployment/pipeline_test.go

@@ -81,7 +81,8 @@ func TestBuildPipelineStages(t *testing.T) {
 					Rollback: tc.rollback,
 				},
 			}
-			got := buildPipelineStages(input)
+			got, err := buildPipelineStages(input)
+			require.NoError(t, err)
 
 			require.Len(t, got, len(tc.wantNames))
 			for i, s := range got {
@@ -93,6 +94,21 @@ func TestBuildPipelineStages(t *testing.T) {
 	}
 }
 
+// TestBuildPipelineStagesRollbackWithoutStages ensures a rollback request with
+// no stages returns an error instead of panicking on stages[0].
+func TestBuildPipelineStagesRollbackWithoutStages(t *testing.T) {
+	t.Parallel()
+
+	got, err := buildPipelineStages(&sdk.BuildPipelineSyncStagesInput{
+		Request: sdk.BuildPipelineSyncStagesRequest{
+			Stages:   []sdk.StageConfig{},
+			Rollback: true,
+		},
+	})
+	assert.Nil(t, got)
+	assert.ErrorIs(t, err, ErrRollbackRequiresStages)
+}
+
 // replica of piped's controller.validateStageIndexes (unexported there), kept in sync manually.
 func validateStageIndexes(req []sdk.StageConfig, res []sdk.PipelineStage) error {
 	reqIndexes := make(map[int]struct{})
@@ -117,14 +133,15 @@ func TestBuildPipelineStagesRollbackIndexContract(t *testing.T) {
 		{Name: StageECSPrimaryRollout, Index: 0},
 		{Name: StageECSCanaryRollout, Index: 1},
 	}
-	got := buildPipelineStages(&sdk.BuildPipelineSyncStagesInput{
+	got, err := buildPipelineStages(&sdk.BuildPipelineSyncStagesInput{
 		Request: sdk.BuildPipelineSyncStagesRequest{
 			Stages:   reqStages,
 			Rollback: true,
 		},
 	})
+	require.NoError(t, err)
 
-	err := validateStageIndexes(reqStages, got)
+	err = validateStageIndexes(reqStages, got)
 	assert.NoError(t, err, "rollback stage index must be one of the requested indexes")
 }
 

pkg/app/pipedv1/plugin/ecs/deployment/plugin.go

@@ -55,8 +55,12 @@ func (p *ECSPlugin) BuildPipelineSyncStages(
 	_ *ecsconfig.ECSPluginConfig,
 	input *sdk.BuildPipelineSyncStagesInput,
 ) (*sdk.BuildPipelineSyncStagesResponse, error) {
+	stages, err := buildPipelineStages(input)
+	if err != nil {
+		return nil, err
+	}
 	return &sdk.BuildPipelineSyncStagesResponse{
-		Stages: buildPipelineStages(input),
+		Stages: stages,
 	}, nil
 }