From 0deb3de9316f3bd2b905fcf363b6169a6cdfd940 Mon Sep 17 00:00:00 2001
From: Armel Soro <asoro@redhat.com>
Date: Sat, 9 Sep 2023 00:18:05 +0200
Subject: [PATCH] Fix API Server panic when volume component has no `ephemeral`
 field set (#7080)

* Add unit test highlighting the issue

* Safely dereference volume component 'ephemeral' field
---
 pkg/apiserver-impl/devstate/content.go      |  5 +-
 pkg/apiserver-impl/devstate/content_test.go | 66 +++++++++++++++++++++
 2 files changed, 69 insertions(+), 2 deletions(-)

diff --git a/pkg/apiserver-impl/devstate/content.go b/pkg/apiserver-impl/devstate/content.go
index 51536f974..aef7788fc 100644
--- a/pkg/apiserver-impl/devstate/content.go
+++ b/pkg/apiserver-impl/devstate/content.go
@@ -8,9 +8,10 @@ import (
 	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
 	"github.com/devfile/api/v2/pkg/devfile"
 	"github.com/devfile/library/v2/pkg/devfile/parser/data/v2/common"
+	"k8s.io/utils/pointer"
+
 	. "github.com/redhat-developer/odo/pkg/apiserver-gen/go"
 	"github.com/redhat-developer/odo/pkg/libdevfile"
-	"k8s.io/utils/pointer"
 )
 
 const (
@@ -319,7 +320,7 @@ func (o *DevfileState) getVolumes() ([]Volume, error) {
 	for _, volume := range volumes {
 		result = append(result, Volume{
 			Name:      volume.Name,
-			Ephemeral: *volume.Volume.Ephemeral,
+			Ephemeral: pointer.BoolDeref(volume.Volume.Ephemeral, false),
 			Size:      volume.Volume.Size,
 		})
 	}
diff --git a/pkg/apiserver-impl/devstate/content_test.go b/pkg/apiserver-impl/devstate/content_test.go
index 511c1af3a..ad022240e 100644
--- a/pkg/apiserver-impl/devstate/content_test.go
+++ b/pkg/apiserver-impl/devstate/content_test.go
@@ -3,8 +3,13 @@ package devstate
 import (
 	"testing"
 
+	"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
+	"github.com/devfile/library/v2/pkg/devfile/parser/data"
 	"github.com/google/go-cmp/cmp"
+	"k8s.io/utils/pointer"
+
 	. "github.com/redhat-developer/odo/pkg/apiserver-gen/go"
+	"github.com/redhat-developer/odo/pkg/testingutil"
 )
 
 func TestDevfileState_GetContent(t *testing.T) {
@@ -44,3 +49,64 @@ func TestDevfileState_GetContent(t *testing.T) {
 		})
 	}
 }
+
+func TestDevfileState_getVolumes(t *testing.T) {
+	var (
+		volWithNoEphemeral = testingutil.GetFakeVolumeComponent("vol-ephemeral-not-set", "1Gi")
+		volEphemeral       = testingutil.GetFakeVolumeComponent("vol-ephemeral-true", "2Gi")
+		volEphemeralFalse  = testingutil.GetFakeVolumeComponent("vol-ephemeral-false", "3Gi")
+	)
+	volWithNoEphemeral.Volume.Ephemeral = nil
+	volEphemeral.Volume.Ephemeral = pointer.Bool(true)
+	volEphemeralFalse.Volume.Ephemeral = pointer.Bool(false)
+
+	tests := []struct {
+		name    string
+		state   func() (DevfileState, error)
+		want    []Volume
+		wantErr bool
+	}{
+		{
+			name: "should not panic if 'ephemeral' is not set on the Devfile volume component",
+			state: func() (DevfileState, error) {
+				devfileData, err := data.NewDevfileData(string(data.APISchemaVersion220))
+				if err != nil {
+					return DevfileState{}, err
+				}
+				err = devfileData.AddComponents([]v1alpha2.Component{
+					volEphemeral,
+					volWithNoEphemeral,
+					volEphemeralFalse,
+				})
+				if err != nil {
+					return DevfileState{}, err
+				}
+				s := NewDevfileState()
+				s.Devfile.Data = devfileData
+				return s, nil
+			},
+			want: []Volume{
+				{Name: "vol-ephemeral-true", Ephemeral: true, Size: "2Gi"},
+				{Name: "vol-ephemeral-not-set", Ephemeral: false, Size: "1Gi"},
+				{Name: "vol-ephemeral-false", Ephemeral: false, Size: "3Gi"},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			o, err := tt.state()
+			if err != nil {
+				t.Fatalf("DevfileState.getVolumes() error preparing Devfile state: %v", err)
+				return
+			}
+			got, err := o.getVolumes()
+			if (err != nil) != tt.wantErr {
+				t.Errorf("DevfileState.getVolumes() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if diff := cmp.Diff(tt.want, got); diff != "" {
+				t.Errorf("DevfileState.getVolumes() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}