Skip to content

Commit

Permalink
Fix API Server panic when volume component has no ephemeral field s…
Browse files Browse the repository at this point in the history
…et (#7080)

* Add unit test highlighting the issue

* Safely dereference volume component 'ephemeral' field
  • Loading branch information
rm3l authored Sep 8, 2023
1 parent bfffaa8 commit 0deb3de
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 2 deletions.
5 changes: 3 additions & 2 deletions pkg/apiserver-impl/devstate/content.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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,
})
}
Expand Down
66 changes: 66 additions & 0 deletions pkg/apiserver-impl/devstate/content_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
})
}
}

0 comments on commit 0deb3de

Please sign in to comment.