fix(userconfig): empty config marshal panics (#492)

CHANGELOG.md

@@ -3,6 +3,7 @@
 ## [MAJOR.MINOR.PATCH] - YYYY-MM-DD
 
 - Make `projectVpcId` and `projectVPCRef` mutable
+- Fix panic on `nil` user config conversion
 
 ## v0.13.0 - 2023-08-18
 

controllers/basic_controller.go

@@ -3,14 +3,12 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"reflect"
 	"strings"
 	"time"
 
 	"github.com/aiven/aiven-go-client"
 	"github.com/go-logr/logr"
 	"github.com/hashicorp/go-multierror"
-	"github.com/liip/sheriff"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -451,95 +449,6 @@ func setupLogger(log logr.Logger, o client.Object) logr.Logger {
 	return log.WithValues("kind", kind, "name", name, "annotations", a)
 }
 
-// UserConfigurationToAPI converts UserConfiguration options structure
-// to Aiven API compatible map[string]interface{}
-func UserConfigurationToAPI(c interface{}) interface{} {
-	result := make(map[string]interface{})
-
-	v := reflect.ValueOf(c)
-
-	// if its a pointer, resolve its value
-	if v.Kind() == reflect.Ptr {
-		v = reflect.Indirect(v)
-	}
-
-	if v.Kind() != reflect.Struct {
-		switch v.Kind() {
-		case reflect.Int64:
-			return *c.(*int64)
-		case reflect.Bool:
-			return *c.(*bool)
-		default:
-			return c
-		}
-	}
-
-	structType := v.Type()
-
-	// convert UserConfig structure to a map
-	for i := 0; i < structType.NumField(); i++ {
-		name := strings.ReplaceAll(structType.Field(i).Tag.Get("json"), ",omitempty", "")
-
-		if structType.Kind() == reflect.Struct {
-			result[name] = UserConfigurationToAPI(v.Field(i).Interface())
-		} else {
-			result[name] = v.Elem().Field(i).Interface()
-		}
-	}
-
-	// remove all the nil and empty map data
-	for key, val := range result {
-		if val == nil || isNil(val) || val == "" {
-			delete(result, key)
-		}
-
-		if reflect.TypeOf(val).Kind() == reflect.Map {
-			if len(val.(map[string]interface{})) == 0 {
-				delete(result, key)
-			}
-		}
-	}
-
-	return result
-}
-
-// UserConfigurationToAPIV2 same as UserConfigurationToAPI but uses sheriff.Marshal
-// which can subset fields from create or update operation
-func UserConfigurationToAPIV2(userConfig interface{}, groups []string) (map[string]interface{}, error) {
-	if userConfig == nil {
-		return nil, nil
-	}
-
-	o := &sheriff.Options{
-		Groups: groups,
-	}
-
-	i, err := sheriff.Marshal(o, userConfig)
-	if err != nil {
-		return nil, err
-	}
-
-	m, ok := i.(map[string]interface{})
-	if !ok {
-		// It is an empty pointer
-		// sheriff just returned the very same object
-		return nil, nil
-	}
-
-	return m, nil
-}
-
-func isNil(i interface{}) bool {
-	if i == nil {
-		return true
-	}
-	switch reflect.TypeOf(i).Kind() {
-	case reflect.Ptr, reflect.Map, reflect.Array, reflect.Chan, reflect.Slice:
-		return reflect.ValueOf(i).IsNil()
-	}
-	return false
-}
-
 func toOptionalStringPointer(s string) *string {
 	if s == "" {
 		return nil

controllers/common.go

@@ -4,9 +4,11 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"reflect"
 	"strconv"
 	"strings"
 
+	"github.com/liip/sheriff"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -167,3 +169,36 @@ func getSecretPrefix(o objWithSecret) string {
 	kind := o.GetObjectKind()
 	return strings.ToUpper(kind.GroupVersionKind().Kind) + "_"
 }
+
+// userConfigurationToAPI converts user config into a map
+func userConfigurationToAPI(c any, groups ...string) (map[string]any, error) {
+	if c == nil || (reflect.ValueOf(c).Kind() == reflect.Ptr && reflect.ValueOf(c).IsNil()) {
+		return nil, nil
+	}
+
+	o := &sheriff.Options{
+		Groups: groups,
+	}
+
+	i, err := sheriff.Marshal(o, c)
+	if err != nil {
+		return nil, err
+	}
+
+	m, ok := i.(map[string]interface{})
+	if !ok {
+		// It is an empty pointer
+		// sheriff just returned the very same object
+		return nil, nil
+	}
+
+	return m, nil
+}
+
+func CreateUserConfiguration(userConfig any) (map[string]any, error) {
+	return userConfigurationToAPI(userConfig, "create", "update")
+}
+
+func UpdateUserConfiguration(userConfig any) (map[string]any, error) {
+	return userConfigurationToAPI(userConfig, "update")
+}

controllers/common_test.go

@@ -0,0 +1,17 @@
+package controllers
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	kafkaconnectuserconfig "github.com/aiven/aiven-operator/api/v1alpha1/userconfig/integration/kafka_connect"
+)
+
+// TestCreateEmptyUserConfiguration shouldn't panic
+func TestCreateEmptyUserConfiguration(t *testing.T) {
+	var uc *kafkaconnectuserconfig.KafkaConnectUserConfig
+	m, err := CreateUserConfiguration(uc)
+	assert.Nil(t, m)
+	assert.NoError(t, err)
+}

controllers/generic_service_handler.go

@@ -51,7 +51,7 @@ func (h *genericServiceHandler) createOrUpdate(a *aiven.Client, object client.Ob
 	var reason string
 	if !exists {
 		reason = "Created"
-		userConfig, err := UserConfigurationToAPIV2(o.getUserConfig(), []string{"create", "update"})
+		userConfig, err := CreateUserConfiguration(o.getUserConfig())
 		if err != nil {
 			return err
 		}
@@ -83,7 +83,7 @@ func (h *genericServiceHandler) createOrUpdate(a *aiven.Client, object client.Ob
 		}
 	} else {
 		reason = "Updated"
-		userConfig, err := UserConfigurationToAPIV2(o.getUserConfig(), []string{"update"})
+		userConfig, err := UpdateUserConfiguration(o.getUserConfig())
 		if err != nil {
 			return err
 		}

controllers/serviceintegration_controller.go

@@ -52,7 +52,7 @@ func (h ServiceIntegrationHandler) createOrUpdate(avn *aiven.Client, i client.Ob
 	var reason string
 	var integration *aiven.ServiceIntegration
 	if si.Status.ID == "" {
-		userConfigMap, err := UserConfigurationToAPIV2(userConfig, []string{"create", "update"})
+		userConfigMap, err := CreateUserConfiguration(userConfig)
 		if err != nil {
 			return err
 		}
@@ -76,7 +76,7 @@ func (h ServiceIntegrationHandler) createOrUpdate(avn *aiven.Client, i client.Ob
 
 		reason = "Created"
 	} else {
-		userConfigMap, err := UserConfigurationToAPIV2(userConfig, []string{"update"})
+		userConfigMap, err := UpdateUserConfiguration(userConfig)
 		if err != nil {
 			return err
 		}