Skip to content

Commit

Permalink
lib/resourcemerge/core: Fix panic on container removal
Browse files Browse the repository at this point in the history
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, openshift#178).

This commit cherry-picks e8e80c2 (lib/resourcemerge/core: Fix panic
on container/port removal, 2019-12-16, openshift#282) back to the 4.2 branch.
But since f268b6d (Add support to EnsureServicePorts, 2019-11-25, openshift#272)
and its EnsureServicePorts was never backported to the 4.2 branch,
I've only kept the container portion of e8e80c2 in this commit.
  • Loading branch information
wking committed Feb 6, 2020
1 parent 9f4f04e commit 1563b4a
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
3 changes: 2 additions & 1 deletion lib/resourcemerge/core.go
Expand Up @@ -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 {
Expand Down
21 changes: 21 additions & 0 deletions lib/resourcemerge/core_test.go
Expand Up @@ -246,6 +246,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 {
Expand Down

0 comments on commit 1563b4a

Please sign in to comment.