Simplify config parsing

We can assume that the config is a flat map of keys and values and manually merge them into the `Config` struct. This allows to avoid manually parsing the YAML. Signed-off-by: Sascha Grunert <[email protected]>
kubernetes-sigs · Oct 24, 2024 · 6ff82ef · 6ff82ef
1 parent 4c7bab8
commit 6ff82ef
Show file tree

Hide file tree

Showing 3 changed files with 118 additions and 146 deletions.
diff --git a/go.mod b/go.mod
@@ -24,7 +24,6 @@ require (
 	golang.org/x/text v0.19.0
 	google.golang.org/grpc v1.67.1
 	google.golang.org/protobuf v1.35.1
-	gopkg.in/yaml.v3 v3.0.1
 	k8s.io/api v0.31.2
 	k8s.io/apimachinery v0.31.2
 	k8s.io/client-go v0.31.2
@@ -92,6 +91,7 @@ require (
 	google.golang.org/genproto/googleapis/rpc v0.0.0-20241007155032-5fefd90f89a9 // indirect
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
+	gopkg.in/yaml.v3 v3.0.1 // indirect
 	k8s.io/cli-runtime v0.31.1 // indirect
 	k8s.io/component-base v0.31.2 // indirect
 	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 // indirect

diff --git a/pkg/common/file.go b/pkg/common/file.go
@@ -19,10 +19,12 @@ package common
 import (
 	"fmt"
 	"os"
-	gofilepath "path/filepath"
-	"strconv"
+	"path/filepath"
+	"reflect"
+	"slices"
 
-	"gopkg.in/yaml.v3"
+	"github.com/sirupsen/logrus"
+	"sigs.k8s.io/yaml"
 )
 
 // Config is the internal representation of the yaml that defines
@@ -34,177 +36,147 @@ type Config struct {
 	Debug             bool
 	PullImageOnCreate bool
 	DisablePullOnRun  bool
-	yamlData          *yaml.Node // YAML representation of config
 }
 
+const (
+	runtimeEndpointKey   = "runtime-endpoint"
+	imageEndpointKey     = "image-endpoint"
+	timeoutKey           = "timeout"
+	debugKey             = "debug"
+	pullImageOnCreateKey = "pull-image-on-create"
+	disablePullOnRunKey  = "disable-pull-on-run"
+)
+
 // ReadConfig reads from a file with the given name and returns a config or
 // an error if the file was unable to be parsed.
-func ReadConfig(filepath string) (*Config, error) {
-	data, err := os.ReadFile(filepath)
+func ReadConfig(path string) (*Config, error) {
+	data, err := os.ReadFile(path)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("read config file path: %w", err)
+	}
+
+	m := map[string]any{}
+	if err := yaml.Unmarshal(data, &m); err != nil {
+		return nil, fmt.Errorf("unmarshal YAML config: %w", err)
+	}
+	logrus.Debugf("Unmarshaled config map: %v", m)
+
+	for k := range m {
+		if !slices.Contains([]string{
+			runtimeEndpointKey,
+			imageEndpointKey,
+			timeoutKey,
+			debugKey,
+			pullImageOnCreateKey,
+			disablePullOnRunKey,
+		}, k) {
+			return nil, fmt.Errorf("invalid config option: %s", k)
+		}
 	}
-	yamlConfig := &yaml.Node{}
-	err = yaml.Unmarshal(data, yamlConfig)
+
+	c := &Config{}
+
+	runtimeEndpoint, err := mapKeyValue[string](m, runtimeEndpointKey)
 	if err != nil {
 		return nil, err
 	}
-	config, err := getConfigOptions(yamlConfig)
+	c.RuntimeEndpoint = runtimeEndpoint
+
+	imageEndpoint, err := mapKeyValue[string](m, imageEndpointKey)
 	if err != nil {
 		return nil, err
 	}
-	return config, err
-}
+	c.ImageEndpoint = imageEndpoint
 
-// WriteConfig writes config to file
-// an error if the file was unable to be written to.
-func WriteConfig(c *Config, filepath string) error {
-	if c == nil {
-		c = &Config{}
-	}
-	if c.yamlData == nil {
-		c.yamlData = &yaml.Node{}
+	timeout, err := mapKeyValue[int](m, timeoutKey)
+	if err != nil {
+		return nil, err
 	}
+	c.Timeout = timeout
 
-	setConfigOptions(c)
+	debug, err := mapKeyValue[bool](m, debugKey)
+	if err != nil {
+		return nil, err
+	}
+	c.Debug = debug
 
-	data, err := yaml.Marshal(c.yamlData)
+	pullImageOnCreate, err := mapKeyValue[bool](m, pullImageOnCreateKey)
 	if err != nil {
-		return err
+		return nil, err
 	}
+	c.PullImageOnCreate = pullImageOnCreate
 
-	if err := os.MkdirAll(gofilepath.Dir(filepath), 0o755); err != nil {
-		return err
+	disablePullOnRun, err := mapKeyValue[bool](m, disablePullOnRunKey)
+	if err != nil {
+		return nil, err
 	}
-	return os.WriteFile(filepath, data, 0o644)
+	c.DisablePullOnRun = disablePullOnRun
+
+	return c, nil
 }
 
-// Extracts config options from the yaml data which is loaded from file.
-func getConfigOptions(yamlData *yaml.Node) (*Config, error) {
-	config := &Config{yamlData: yamlData}
-
-	if len(yamlData.Content) == 0 ||
-		yamlData.Content[0].Content == nil {
-		return config, nil
-	}
-	contentLen := len(yamlData.Content[0].Content)
-
-	// YAML representation contains 2 yaml ScalarNodes per config option.
-	// One is config option name and other is the value of the option
-	// These ScalarNodes help preserve comments associated with
-	// the YAML entry
-	for indx := 0; indx < contentLen-1; {
-		configOption := yamlData.Content[0].Content[indx]
-		name := configOption.Value
-		value := yamlData.Content[0].Content[indx+1].Value
-		var err error
-		switch name {
-		case "runtime-endpoint":
-			config.RuntimeEndpoint = value
-		case "image-endpoint":
-			config.ImageEndpoint = value
-		case "timeout":
-			config.Timeout, err = strconv.Atoi(value)
-			if err != nil {
-				return nil, fmt.Errorf("parsing config option '%s': %w", name, err)
-			}
-		case "debug":
-			config.Debug, err = strconv.ParseBool(value)
-			if err != nil {
-				return nil, fmt.Errorf("parsing config option '%s': %w", name, err)
-			}
-		case "pull-image-on-create":
-			config.PullImageOnCreate, err = strconv.ParseBool(value)
-			if err != nil {
-				return nil, fmt.Errorf("parsing config option '%s': %w", name, err)
-			}
-		case "disable-pull-on-run":
-			config.DisablePullOnRun, err = strconv.ParseBool(value)
-			if err != nil {
-				return nil, fmt.Errorf("parsing config option '%s': %w", name, err)
-			}
-		default:
-			return nil, fmt.Errorf("Config option '%s' is not valid", name)
+func mapKeyValue[T any](m map[string]any, key string) (ret T, err error) {
+	if value, ok := m[key]; ok {
+		// Even Integer values will be interpreted as float
+		if reflect.TypeOf(value).Kind() == reflect.Float64 {
+			//nolint:forcetypeassert // type assertion done before
+			value = int(value.(float64))
 		}
-		indx += 2
-	}
 
-	return config, nil
+		if typedValue, ok := value.(T); ok {
+			return typedValue, nil
+		} else {
+			return ret, fmt.Errorf("invalid value \"%T\" for key %q", value, key)
+		}
+	}
+	return ret, nil
 }
 
-// Set config options on yaml data for persistece to file.
-func setConfigOptions(config *Config) {
-	setConfigOption("runtime-endpoint", config.RuntimeEndpoint, config.yamlData)
-	setConfigOption("image-endpoint", config.ImageEndpoint, config.yamlData)
-	setConfigOption("timeout", strconv.Itoa(config.Timeout), config.yamlData)
-	setConfigOption("debug", strconv.FormatBool(config.Debug), config.yamlData)
-	setConfigOption("pull-image-on-create", strconv.FormatBool(config.PullImageOnCreate), config.yamlData)
-	setConfigOption("disable-pull-on-run", strconv.FormatBool(config.DisablePullOnRun), config.yamlData)
-}
+// WriteConfig writes config to file and return
+// an error if the file was unable to be written.
+func WriteConfig(c *Config, path string) error {
+	if c == nil {
+		c = &Config{}
+	}
+	m := map[string]any{}
 
-// Set config option on yaml.
-func setConfigOption(configName, configValue string, yamlData *yaml.Node) {
-	if len(yamlData.Content) == 0 {
-		yamlData.Kind = yaml.DocumentNode
-		yamlData.Content = make([]*yaml.Node, 1)
-		yamlData.Content[0] = &yaml.Node{
-			Kind: yaml.MappingNode,
-			Tag:  "!!map",
-		}
+	if c.RuntimeEndpoint != "" {
+		m[runtimeEndpointKey] = c.RuntimeEndpoint
 	}
-	contentLen := 0
-	foundOption := false
-	if yamlData.Content[0].Content != nil {
-		contentLen = len(yamlData.Content[0].Content)
+
+	if c.ImageEndpoint != "" {
+		m[imageEndpointKey] = c.ImageEndpoint
 	}
 
-	// Set value on existing config option
-	for indx := 0; indx < contentLen-1; {
-		name := yamlData.Content[0].Content[indx].Value
-		if name == configName {
-			yamlData.Content[0].Content[indx+1].Value = configValue
-			foundOption = true
-			break
-		}
-		indx += 2
-	}
-
-	// New config option to set
-	// YAML representation contains 2 yaml ScalarNodes per config option.
-	// One is config option name and other is the value of the option
-	// These ScalarNodes help preserve comments associated with
-	// the YAML entry
-	if !foundOption {
-		const (
-			tagPrefix = "!!"
-			tagStr    = tagPrefix + "str"
-			tagBool   = tagPrefix + "bool"
-			tagInt    = tagPrefix + "int"
-		)
-		name := &yaml.Node{
-			Kind:  yaml.ScalarNode,
-			Value: configName,
-			Tag:   tagStr,
-		}
-		var tagType string
-		switch configName {
-		case "timeout":
-			tagType = tagInt
-		case "debug":
-			tagType = tagBool
-		case "pull-image-on-create":
-			tagType = tagBool
-		case "disable-pull-on-run":
-			tagType = tagBool
-		default:
-			tagType = tagStr
-		}
+	if c.Timeout != 0 {
+		m[timeoutKey] = c.Timeout
+	}
 
-		value := &yaml.Node{
-			Kind:  yaml.ScalarNode,
-			Value: configValue,
-			Tag:   tagType,
-		}
-		yamlData.Content[0].Content = append(yamlData.Content[0].Content, name, value)
+	if c.Debug {
+		m[debugKey] = c.Debug
 	}
+
+	if c.PullImageOnCreate {
+		m[pullImageOnCreateKey] = c.PullImageOnCreate
+	}
+
+	if c.DisablePullOnRun {
+		m[disablePullOnRunKey] = c.DisablePullOnRun
+	}
+
+	logrus.Debugf("Marshalling config map: %v", m)
+	data, err := yaml.Marshal(m)
+	if err != nil {
+		return fmt.Errorf("marshal YAML config: %w", err)
+	}
+
+	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+		return fmt.Errorf("ensure config path dir: %w", err)
+	}
+
+	if err := os.WriteFile(path, data, 0o644); err != nil {
+		return fmt.Errorf("write config file: %w", err)
+	}
+
+	return nil
 }
diff --git a/pkg/framework/util.go b/pkg/framework/util.go
@@ -29,10 +29,10 @@ import (
 	"github.com/google/uuid"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	"gopkg.in/yaml.v3"
 	internalapi "k8s.io/cri-api/pkg/apis"
 	runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1"
 	remote "k8s.io/cri-client/pkg"
+	"sigs.k8s.io/yaml"
 
 	"sigs.k8s.io/cri-tools/pkg/common"
 )