Skip to content

Commit e8e80c2

Browse files
committed
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 3d1ad76 (Remove containers if requested in Update, 2019-04-26, #178).
1 parent 2671e35 commit e8e80c2

File tree

2 files changed

+25
-2
lines changed

2 files changed

+25
-2
lines changed

lib/resourcemerge/core.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,8 @@ func ensurePodSpec(modified *bool, existing *corev1.PodSpec, required corev1.Pod
6262
}
6363

6464
func ensureContainers(modified *bool, existing *[]corev1.Container, required []corev1.Container) {
65-
for i, existingContainer := range *existing {
65+
for i := len(*existing) - 1; i >= 0; i-- {
66+
existingContainer := &(*existing)[i]
6667
var existingCurr *corev1.Container
6768
for _, requiredContainer := range required {
6869
if existingContainer.Name == requiredContainer.Name {
@@ -203,7 +204,8 @@ func ensureContainerPort(modified *bool, existing *corev1.ContainerPort, require
203204
}
204205

205206
func EnsureServicePorts(modified *bool, existing *[]corev1.ServicePort, required []corev1.ServicePort) {
206-
for i, existingServicePort := range *existing {
207+
for i := len(*existing) - 1; i >= 0; i-- {
208+
existingServicePort := &(*existing)[i]
207209
var existingCurr *corev1.ServicePort
208210
for _, requiredServicePort := range required {
209211
if existingServicePort.Name == requiredServicePort.Name {

lib/resourcemerge/core_test.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,27 @@ func TestEnsurePodSpec(t *testing.T) {
247247
},
248248
},
249249
},
250+
{
251+
name: "remove a container",
252+
existing: corev1.PodSpec{
253+
Containers: []corev1.Container{
254+
corev1.Container{Name: "test-A"},
255+
corev1.Container{Name: "test-B"},
256+
},
257+
},
258+
input: corev1.PodSpec{
259+
Containers: []corev1.Container{
260+
corev1.Container{Name: "test-B"},
261+
},
262+
},
263+
264+
expectedModified: true,
265+
expected: corev1.PodSpec{
266+
Containers: []corev1.Container{
267+
corev1.Container{Name: "test-B"},
268+
},
269+
},
270+
},
250271
}
251272

252273
for _, test := range tests {

0 commit comments

Comments
 (0)