Merge pull request #12698 from johngmyers/fix-panic

Fix out of bounds error when instance detach fails

pkg/instancegroups/instancegroups.go

@@ -151,13 +151,16 @@ func (c *RollingUpdateCluster) rollingUpdateInstanceGroup(group *cloudinstances.
 	if maxSurge > 0 && !c.CloudOnly {
 		skippedNodes := 0
 		for numSurge := 1; numSurge <= maxSurge; numSurge++ {
-			u := update[len(update)-numSurge+skippedNodes]
+			u := update[len(update)-numSurge-skippedNodes]
 			if u.Status != cloudinstances.CloudInstanceStatusDetached {
 				if err := c.detachInstance(u); err != nil {
 					// If detaching a node fails, we simply proceed to the next one instead of
 					// bubbling up the error.
 					skippedNodes++
 					numSurge--
+					if maxSurge > len(update)-skippedNodes {
+						maxSurge = len(update) - skippedNodes
+					}
 				}
 
 				// If noneReady, wait until after one node is detached and its replacement validates

pkg/instancegroups/rollingupdate_test.go

@@ -19,6 +19,7 @@ package instancegroups
 import (
 	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"sync"
 	"testing"
@@ -1351,6 +1352,34 @@ func TestRollingUpdateMaxSurgeGreaterThanNeedUpdate(t *testing.T) {
 	assert.Equal(t, 2, countDetach.Count)
 }
 
+type failDetachAutoscaling struct {
+	autoscalingiface.AutoScalingAPI
+}
+
+func (m *failDetachAutoscaling) DetachInstances(input *autoscaling.DetachInstancesInput) (*autoscaling.DetachInstancesOutput, error) {
+	return nil, fmt.Errorf("testing error")
+}
+
+func TestRollingUpdateDetachFails(t *testing.T) {
+
+	c, cloud := getTestSetup()
+
+	cloud.MockAutoscaling = &failDetachAutoscaling{AutoScalingAPI: cloud.MockAutoscaling}
+	cloud.MockEC2 = &ec2IgnoreTags{EC2API: cloud.MockEC2}
+
+	ten := intstr.FromInt(10)
+	c.Cluster.Spec.RollingUpdate = &kopsapi.RollingUpdate{
+		MaxSurge: &ten,
+	}
+
+	groups := make(map[string]*cloudinstances.CloudInstanceGroup)
+	makeGroup(groups, c.K8sClient, cloud, "node-1", kopsapi.InstanceGroupRoleNode, 3, 2)
+	err := c.RollingUpdate(groups, &kopsapi.InstanceGroupList{})
+	assert.NoError(t, err, "rolling update")
+
+	assertGroupInstanceCount(t, cloud, "node-1", 1)
+}
+
 // Request validate (1)            -->
 //                                 <-- validated
 // Detach instance                 -->