From e8e80c2c8f932616f14742415ffd611e66fe5f6d Mon Sep 17 00:00:00 2001
From: "W. Trevor King" <wking@tremily.us>
Date: Mon, 16 Dec 2019 16:16:38 -0800
Subject: [PATCH] lib/resourcemerge/core: Fix panic on container/port removal

Avoid:

  $ go test ./lib/resourcemerge/
  panic: runtime error: index out of range [recovered]
  			 panic: runtime error: index out of range

  goroutine 38 [running]:
  testing.tRunner.func1(0xc0001ab000)
    .../sdk/go1.12.9/src/testing/testing.go:830 +0x392
  panic(0xccb520, 0x163f880)
    .../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5
  github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1)
    .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840
  github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...)
    .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6
  github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000)
    .../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7
  testing.tRunner(0xc0001ab000, 0xc0001d8770)
    .../sdk/go1.12.9/src/testing/testing.go:865 +0xc0
  created by testing.(*T).Run
    .../sdk/go1.12.9/src/testing/testing.go:916 +0x35a
  FAIL		github.com/openshift/cluster-version-operator/lib/resourcemerge	0.010s

(with the core_test.go but the old core.go) when removing an entry
mutated the existing slice without re-entering the:

  for i, whatever := range *existing

With this commit, we iterate from the back of the existing slice, so
any removals affect indexes that we've already covered.  For both
containers and service ports, any appends happen later in the
function, so we don't need to worry about slice expansion at this
point.

The buggy logic was originally from 3d1ad762b3 (Remove containers if
requested in Update, 2019-04-26, #178).
---
 lib/resourcemerge/core.go      |  6 ++++--
 lib/resourcemerge/core_test.go | 21 +++++++++++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go
index 667a64716..2b5dca14c 100644
--- a/lib/resourcemerge/core.go
+++ b/lib/resourcemerge/core.go
@@ -62,7 +62,8 @@ func ensurePodSpec(modified *bool, existing *corev1.PodSpec, required corev1.Pod
 }
 
 func ensureContainers(modified *bool, existing *[]corev1.Container, required []corev1.Container) {
-	for i, existingContainer := range *existing {
+	for i := len(*existing) - 1; i >= 0; i-- {
+		existingContainer := &(*existing)[i]
 		var existingCurr *corev1.Container
 		for _, requiredContainer := range required {
 			if existingContainer.Name == requiredContainer.Name {
@@ -203,7 +204,8 @@ func ensureContainerPort(modified *bool, existing *corev1.ContainerPort, require
 }
 
 func EnsureServicePorts(modified *bool, existing *[]corev1.ServicePort, required []corev1.ServicePort) {
-	for i, existingServicePort := range *existing {
+	for i := len(*existing) - 1; i >= 0; i-- {
+		existingServicePort := &(*existing)[i]
 		var existingCurr *corev1.ServicePort
 		for _, requiredServicePort := range required {
 			if existingServicePort.Name == requiredServicePort.Name {
diff --git a/lib/resourcemerge/core_test.go b/lib/resourcemerge/core_test.go
index c2afc9b42..1afc22c64 100644
--- a/lib/resourcemerge/core_test.go
+++ b/lib/resourcemerge/core_test.go
@@ -247,6 +247,27 @@ func TestEnsurePodSpec(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "remove a container",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					corev1.Container{Name: "test-A"},
+					corev1.Container{Name: "test-B"},
+				},
+			},
+			input: corev1.PodSpec{
+				Containers: []corev1.Container{
+					corev1.Container{Name: "test-B"},
+				},
+			},
+
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				Containers: []corev1.Container{
+					corev1.Container{Name: "test-B"},
+				},
+			},
+		},
 	}
 
 	for _, test := range tests {