Repository: knative/client
Pull request #1533 (2021-11-30 09:27:52)

Fixed panic when traffic split to unavailable revision
## Description
Panic during updating traffic to a revision that is not ready

<!-- Please add a short summary about what the pull request is going to bring on the table -->
Before:

```
$ kn service update hello-example-3 --env TARGET="pqr" --traffic abcd=50,hello-example-3-00002=50

E1130 00:13:13.786017   76154 runtime.go:78] Observed a panic: runtime.boundsError{x:0, y:0, signed:true, code:0x0} (runtime error: index out of range [0] with length 0)
goroutine 1 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x246a6a0, 0xc0006ac570})
	/home/prow/go/src/knative.dev/client/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:74 +0x85
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x5a0})
	/home/prow/go/src/knative.dev/client/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:48 +0x75
panic({0x246a6a0, 0xc0006ac570})
	/root/.gvm/gos/go1.17.2/src/runtime/panic.go:1038 +0x215
knative.dev/client/pkg/serving.ContainerStatus(...)
	/home/prow/go/src/knative.dev/client/pkg/serving/revision_template.go:71
knative.dev/client/pkg/serving.PinImageToDigest(0xc00011a118, 0xc000192700)
	/home/prow/go/src/knative.dev/client/pkg/serving/config_changes.go:147 +0x293
knative.dev/client/pkg/kn/commands/service.(*ConfigurationEditFlags).Apply(0xc00012e780, 0xc00011a000, 0x23376a0, 0xc00069cb00)
	/home/prow/go/src/knative.dev/client/pkg/kn/commands/service/configuration_edit_flags.go:242 +0x252
knative.dev/client/pkg/kn/commands/service.NewServiceUpdateCommand.func1.1(0xc00011a000)
	/home/prow/go/src/knative.dev/client/pkg/kn/commands/service/update.go:100 +0x1d9
```
Now:
```
$ ./kn service update hello-example-3 --env TARGET="pqr" --traffic abcd=50,hello-example-3-00002=50

Updating Service 'hello-example-3' in namespace 'default':

  0.033s Revision "hello-example-3-00002" failed to become ready.
  0.093s ...
Error: RevisionMissing: Revision "hello-example-3-00002" failed to become ready.
```
## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* Added nil pointed check
* Added array length check


## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #

<!--
Please add an entry to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1533#" title="Author self-approved">vyasgun</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dprotaso** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @dprotaso` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/knative/client/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dprotaso"]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#1533](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (a202a58) into [main](https://codecov.io/gh/knative/client/commit/8fd19e6bea269753cd4a50f035259d079e0c81c3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (8fd19e6) will **not change** coverage.
> The diff coverage is `50.00%`.

> :exclamation: Current head a202a58 differs from pull request most recent head 76117e6. Consider uploading reports for the commit 76117e6 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/knative/client/pull/1533/graphs/tree.svg?width=650&height=150&src=pr&token=bkTp6BzJiV&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@           Coverage Diff           @@
##             main    #1533   +/-   ##
=======================================
  Coverage   79.52%   79.52%           
=======================================
  Files         162      162           
  Lines        8553     8553           
=======================================
  Hits         6802     6802           
  Misses       1073     1073           
  Partials      678      678           
```


| [Impacted Files](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [pkg/serving/revision\_template.go](https://codecov.io/gh/knative/client/pull/1533/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3NlcnZpbmcvcmV2aXNpb25fdGVtcGxhdGUuZ28=) | `90.90% <0.00%> (ø)` | |
| [pkg/serving/config\_changes.go](https://codecov.io/gh/knative/client/pull/1533/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3NlcnZpbmcvY29uZmlnX2NoYW5nZXMuZ28=) | `89.85% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [8fd19e6...76117e6](https://codecov.io/gh/knative/client/pull/1533?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-client-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/serving/config_changes.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1533/pull-knative-client-go-coverage/1465689957468213248/artifacts/line-cov.html#file0) | 94.5% | 95.9% | 1.4
[pkg/serving/revision_template.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_client/1533/pull-knative-client-go-coverage/1465689957468213248/artifacts/line-cov.html#file1) | 95.3% | 97.7% | 2.3

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/client/pull/1533#pullrequestreview-819146895" title="LGTM">dsimansk</a>*, *<a href="https://github.com/knative/client/pull/1533#" title="Author self-approved">vyasgun</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fclient).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/client/blob/main/OWNERS)~~ [dsimansk]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/CHANGELOG.adoc b/CHANGELOG.adoc
index 5c5e8844..d5129002 100644
--- a/CHANGELOG.adoc
+++ b/CHANGELOG.adoc
@@ -21,6 +21,10 @@
 | Fixed panic in kn service describe
 | https://github.com/knative/client/pull/1529[#1529]
 
+| 🐛
+| Fixed panic in kn service update
+| https://github.com/knative/client/pull/1533[#1533]
+
 |===
 
 ## v1.0.0 (2021-11-02)
diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go
index 771b5881..5c5cc57e 100644
--- a/pkg/serving/config_changes.go
+++ b/pkg/serving/config_changes.go
@@ -145,7 +145,7 @@ func PinImageToDigest(currentRevisionTemplate *servingv1.RevisionTemplateSpec, b
 	}
 
 	containerStatus := ContainerStatus(baseRevision)
-	if containerStatus.ImageDigest != "" {
+	if containerStatus != nil && containerStatus.ImageDigest != "" {
 		return flags.UpdateImage(&currentRevisionTemplate.Spec.PodSpec, containerStatus.ImageDigest)
 	}
 	return nil
diff --git a/pkg/serving/revision_template.go b/pkg/serving/revision_template.go
index 0339a22f..efb6907a 100644
--- a/pkg/serving/revision_template.go
+++ b/pkg/serving/revision_template.go
@@ -65,7 +65,7 @@ func ContainerIndexOfRevisionSpec(revisionSpec *servingv1.RevisionSpec) int {
 // such status could be found
 func ContainerStatus(r *servingv1.Revision) *servingv1.ContainerStatus {
 	idx := ContainerIndexOfRevisionSpec(&r.Spec)
-	if idx == -1 {
+	if idx < 0 || idx >= len(r.Status.ContainerStatuses) {
 		return nil
 	}
 	return &r.Status.ContainerStatuses[idx]


Test patch:
diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go
index b8dc7d21..cb4e5117 100644
--- a/pkg/serving/config_changes_test.go
+++ b/pkg/serving/config_changes_test.go
@@ -141,6 +141,16 @@ func TestPinImageToDigestInvalidImages(t *testing.T) {
 	assert.ErrorContains(t, err, "unexpected image")
 }
 
+func TestPinImageToDigestNilContainerStatus(t *testing.T) {
+	template, _ := getRevisionTemplate()
+	revision := &servingv1.Revision{}
+	revision.Spec = template.Spec
+	revision.ObjectMeta = template.ObjectMeta
+	revision.Status.ContainerStatuses = nil
+	err := PinImageToDigest(template, revision)
+	assert.NilError(t, err)
+}
+
 func TestUpdateTimestampAnnotation(t *testing.T) {
 	template, _ := getRevisionTemplate()
 	UpdateTimestampAnnotation(template)
diff --git a/pkg/serving/revision_template_test.go b/pkg/serving/revision_template_test.go
index 69a4ad68..a152719b 100644
--- a/pkg/serving/revision_template_test.go
+++ b/pkg/serving/revision_template_test.go
@@ -151,3 +151,87 @@ func TestPort(t *testing.T) {
 	revisionSpec.PodSpec.Containers[0].Ports = []corev1.ContainerPort{port}
 	assert.Equal(t, (int32)(42), *Port(revisionSpec))
 }
+
+func TestContainerStatus(t *testing.T) {
+	tests := []struct {
+		name string
+		rev  *servingv1.Revision
+		want *servingv1.ContainerStatus
+	}{
+		{
+			"no container",
+			&servingv1.Revision{
+				Spec: servingv1.RevisionSpec{},
+				Status: servingv1.RevisionStatus{ContainerStatuses: []servingv1.ContainerStatus{
+					{
+						Name: "user-container",
+					},
+				}},
+			},
+			nil,
+		},
+		{
+			"1 container",
+			&servingv1.Revision{
+				Spec: servingv1.RevisionSpec{
+					PodSpec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name:  "user-container",
+								Ports: []corev1.ContainerPort{{ContainerPort: 80}},
+							},
+						},
+					},
+				},
+				Status: servingv1.RevisionStatus{ContainerStatuses: []servingv1.ContainerStatus{
+					{
+						Name: "user-container",
+					},
+				}},
+			},
+			&servingv1.ContainerStatus{
+				Name: "user-container",
+			},
+		},
+		{
+			"3 containers",
+			&servingv1.Revision{
+				Spec: servingv1.RevisionSpec{
+					PodSpec: corev1.PodSpec{
+						Containers: []corev1.Container{
+							{
+								Name: "sidecar-container-1",
+							},
+							{
+								Name:  "user-container",
+								Ports: []corev1.ContainerPort{{ContainerPort: 80}},
+							},
+							{
+								Name: "sidecar-container-2",
+							},
+						}}},
+				Status: servingv1.RevisionStatus{
+					ContainerStatuses: []servingv1.ContainerStatus{
+						{
+							Name: "sidecar-container-1",
+						},
+						{
+							Name: "user-container",
+						},
+						{
+							Name: "sidecar-container-2",
+						},
+					},
+				}},
+			&servingv1.ContainerStatus{
+				Name: "user-container",
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ContainerStatus(tt.rev)
+			assert.DeepEqual(t, got, tt.want)
+		})
+	}
+}