Skip to content

Commit

Permalink
fix: do not panic in merge.Merge if map value is nil
Browse files Browse the repository at this point in the history
Checking for `zeroValue` is not enough when accessing `map[string]any`.

Closes #8005

Signed-off-by: Dmitriy Matrenichev <dmitry.matrenichev@siderolabs.com>
(cherry picked from commit 6329222)
  • Loading branch information
DmitriyMV authored and smira committed Dec 8, 2023
1 parent c2259bf commit ecee92c
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 13 deletions.
22 changes: 19 additions & 3 deletions pkg/machinery/config/merge/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ import (
// - if it is a struct, merge is performed for each field of the struct.
// - if the type implements 'merger' interface, Merge function is called to handle the merge process.
// - merger interface should be implemented on the pointer to the type.
func Merge(left, right interface{}) error {
func Merge(left, right any) error {
return merge(reflect.ValueOf(left), reflect.ValueOf(right), false)
}

type merger interface {
Merge(other interface{}) error
Merge(other any) error
}

var (
Expand Down Expand Up @@ -105,7 +105,7 @@ func merge(vl, vr reflect.Value, replace bool) error {
}

for _, k := range vr.MapKeys() {
if vl.MapIndex(k) != zeroValue {
if !isNilOrZero(vl.MapIndex(k)) {
valueType := tl.Elem()

var v, rightV reflect.Value
Expand Down Expand Up @@ -193,3 +193,19 @@ func merge(vl, vr reflect.Value, replace bool) error {

return nil
}

// isNilOrZero returns true if the [reflect.Value] is zero [reflect.Value] or something that is nil.
// We need it because if map contains a key with `nil` value, simply comparing that result to the `zeroValue`
// is not enough.
func isNilOrZero(idx reflect.Value) bool {
if idx == zeroValue {
return true
}

switch idx.Kind() { //nolint:exhaustive
case reflect.Interface, reflect.Pointer, reflect.Slice, reflect.Map, reflect.Chan, reflect.Func:
return idx.IsNil()
default:
return false
}
}
50 changes: 40 additions & 10 deletions pkg/machinery/config/merge/merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type Struct struct {

type CustomSlice []string

func (s *CustomSlice) Merge(other interface{}) error {
func (s *CustomSlice) Merge(other any) error {
otherSlice, ok := other.(CustomSlice)
if !ok {
return fmt.Errorf("other is not CustomSlice: %v", other)
Expand All @@ -45,13 +45,13 @@ func (s *CustomSlice) Merge(other interface{}) error {
return nil
}

type Unstructured map[string]interface{}
type Unstructured map[string]any

func TestMerge(t *testing.T) {
for _, tt := range []struct {
name string
left, right interface{}
expected interface{}
left, right any
expected any
}{
{
name: "zero",
Expand Down Expand Up @@ -249,17 +249,17 @@ func TestMerge(t *testing.T) {
name: "unstructured",
left: &Unstructured{
"a": "aa",
"map": map[string]interface{}{
"slice": []interface{}{
"map": map[string]any{
"slice": []any{
"s1",
},
"some": "value",
},
},
right: &Unstructured{
"b": "bb",
"map": map[string]interface{}{
"slice": []interface{}{
"map": map[string]any{
"slice": []any{
"s2",
},
"other": "thing",
Expand All @@ -268,8 +268,8 @@ func TestMerge(t *testing.T) {
expected: &Unstructured{
"a": "aa",
"b": "bb",
"map": map[string]interface{}{
"slice": []interface{}{
"map": map[string]any{
"slice": []any{
"s1",
"s2",
},
Expand All @@ -278,6 +278,36 @@ func TestMerge(t *testing.T) {
},
},
},
{
name: "unstructed with nil value",
left: Unstructured{
"a": nil,
"b": []any{
"c",
"d",
},
},
right: Unstructured{
"a": Unstructured{
"b": []any{
"c",
"d",
},
},
},
expected: Unstructured{
"a": Unstructured{
"b": []any{
"c",
"d",
},
},
"b": []any{
"c",
"d",
},
},
},
} {
t.Run(tt.name, func(t *testing.T) {
err := merge.Merge(tt.left, tt.right)
Expand Down

0 comments on commit ecee92c

Please sign in to comment.