From 6506f4f02493aedd3e3c0a35145b9b9b5f200d7f Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 28 Sep 2023 16:07:03 +0200 Subject: [PATCH] POC: appendable string arrays in containers.conf As discussed in https://github.com/containers/podman/issues/20000, we need an opt-in mechanism to _append_ string arrays during loading sequence of containers.conf files. At the moment, existing fields/data will be overriden with each loaded config that sets the specified field/option. The TOML (toml.io) config format does not allow for attributing fields and structs are implicitly represented as "tables". I wanted to extend a string array with a simple boolean field, for instance: ```TOML env=["FOO=bar"] env.append=true ``` TOML doesn't suppor tthe upper idea as it's not a properly formatted table. So I looked for alternatives and found that TOML supports so-called "mixed-type arrays". As the same suggests, such arrays allow for including more than one type and that seemed like a reasonable candidate as it allows for _extending_ the existing syntax without introducing new fields or even yet-another way of loading conf files. The new format can be seen in the tests. Please note that this is just a _tested_ POC. Integrating the POC in containers.conf may turn into a bigger journey as Podman is directly (ab)using many of the fields. Since they have to be changed to the new type (see POC), Podman will not compile without changes. Signed-off-by: Valentin Rothberg --- .../attributed_string_slice.go | 73 ++++++++++ .../attributed_string_slice_test.go | 127 ++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 internal/attributed-string-slice/attributed_string_slice.go create mode 100644 internal/attributed-string-slice/attributed_string_slice_test.go diff --git a/internal/attributed-string-slice/attributed_string_slice.go b/internal/attributed-string-slice/attributed_string_slice.go new file mode 100644 index 000000000..cc9c565aa --- /dev/null +++ b/internal/attributed-string-slice/attributed_string_slice.go @@ -0,0 +1,73 @@ +package attributedstringslice + +import ( + "bytes" + "fmt" + + "github.com/BurntSushi/toml" +) + +type attributedStringSlice struct { // A "mixed-type array" in TOML. + slice []string + attributes struct { // Using a struct allows for adding more attributes in the feature. + append *bool // Nil if not set by the user + } +} + +func (ts *attributedStringSlice) UnmarshalTOML(data interface{}) error { + iFaceSlice, ok := data.([]interface{}) + if !ok { + return fmt.Errorf("unable to cast to interface array: %v", data) + } + + var loadedStrings []string + for _, x := range iFaceSlice { // Iterate over each item in the slice. + switch val := x.(type) { + case string: // Strings are directly appended to the slice. + loadedStrings = append(loadedStrings, val) + case map[string]interface{}: // The attribute struct is represented as a map. + for k, v := range val { // Iterate over all _supported_ keys. + switch k { + case "append": + boolVal, ok := v.(bool) + if !ok { + return fmt.Errorf("unable to cast append to bool: %v", k) + } + ts.attributes.append = &boolVal + default: // Unsupported map key. + return fmt.Errorf("unsupported key %q in map: %v", k, val) + } + } + default: // Unsupported item. + return fmt.Errorf("unsupported item in attributed string slice: %v", x) + } + } + + if ts.attributes.append != nil && *ts.attributes.append { // If _explicitly_ configured, append the loaded slice. + ts.slice = append(ts.slice, loadedStrings...) + } else { // Default: override the existing slice. + ts.slice = loadedStrings + } + return nil +} + +func (ts *attributedStringSlice) MarshalTOML() ([]byte, error) { + iFaceSlice := make([]interface{}, 0, len(ts.slice)) + + for _, x := range ts.slice { + iFaceSlice = append(iFaceSlice, x) + } + + if ts.attributes.append != nil { + attributes := make(map[string]any) + attributes["append"] = *ts.attributes.append + iFaceSlice = append(iFaceSlice, attributes) + } + + buf := new(bytes.Buffer) + enc := toml.NewEncoder(buf) + if err := enc.Encode(iFaceSlice); err != nil { + return nil, err + } + return buf.Bytes(), nil +} diff --git a/internal/attributed-string-slice/attributed_string_slice_test.go b/internal/attributed-string-slice/attributed_string_slice_test.go new file mode 100644 index 000000000..d583af30e --- /dev/null +++ b/internal/attributed-string-slice/attributed_string_slice_test.go @@ -0,0 +1,127 @@ +package attributedstringslice + +import ( + "bytes" + "testing" + + "github.com/BurntSushi/toml" + "github.com/stretchr/testify/require" +) + +type testConfig struct { + Array attributedStringSlice `toml:"array,omitempty"` +} + +const ( + confingDefault = `array=["1", "2", "3"]` + configAppendFront = `array=[{append=true},"4", "5", "6"]` + configAppendMid = `array=["7", {append=true}, "8"]` + configAppendBack = `array=["9", {append=true}]` + configAppendFalse = `array=["10", {append=false}]` +) + +var ( + bTrue = true + bFalse = false +) + +func loadConfigs(configs []string) (*testConfig, error) { + var config testConfig + for _, c := range configs { + if _, err := toml.Decode(c, &config); err != nil { + return nil, err + } + } + return &config, nil +} + +func TestAttributedStringSliceLoading(t *testing.T) { + for _, test := range []struct { + configs []string + expectedSlice []string + expectedAppend *bool + expectedErrorSubstring string + }{ + // Load single configs + {[]string{confingDefault}, []string{"1", "2", "3"}, nil, ""}, + {[]string{configAppendFront}, []string{"4", "5", "6"}, &bTrue, ""}, + {[]string{configAppendMid}, []string{"7", "8"}, &bTrue, ""}, + {[]string{configAppendBack}, []string{"9"}, &bTrue, ""}, + {[]string{configAppendFalse}, []string{"10"}, &bFalse, ""}, + // Append=true + {[]string{confingDefault, configAppendFront}, []string{"1", "2", "3", "4", "5", "6"}, &bTrue, ""}, + {[]string{configAppendFront, confingDefault}, []string{"4", "5", "6", "1", "2", "3"}, &bTrue, ""}, // The attribute is sticky unless explicitly being turned off in a later config + {[]string{configAppendFront, confingDefault, configAppendBack}, []string{"4", "5", "6", "1", "2", "3", "9"}, &bTrue, ""}, + // Append=false + {[]string{confingDefault, configAppendFalse}, []string{"10"}, &bFalse, ""}, + {[]string{confingDefault, configAppendMid, configAppendFalse}, []string{"10"}, &bFalse, ""}, + {[]string{confingDefault, configAppendFalse, configAppendMid}, []string{"10", "7", "8"}, &bTrue, ""}, // Append can be re-enabled by a later config + + // Error checks + {[]string{`array=["1", false]`}, nil, nil, `unsupported item in attributed string slice: false`}, + {[]string{`array=["1", 42]`}, nil, nil, `unsupported item in attributed string slice: 42`}, // Stop a `int` such that it passes on 32bit as well + {[]string{`array=["1", {foo=true}]`}, nil, nil, `unsupported key "foo" in map: `}, + {[]string{`array=["1", {append="false"}]`}, nil, nil, `unable to cast append to bool: `}, + } { + result, err := loadConfigs(test.configs) + if test.expectedErrorSubstring != "" { + require.Error(t, err, "test is expected to fail: %v", test) + require.ErrorContains(t, err, test.expectedErrorSubstring, "error does not match: %v", test) + continue + } + require.NoError(t, err, "test is expected to succeed: %v", test) + require.NotNil(t, result, "loaded config must not be nil: %v", test) + require.Equal(t, result.Array.slice, test.expectedSlice, "slices do not match: %v", test) + require.Equal(t, result.Array.attributes.append, test.expectedAppend, "append field does not match: %v", test) + } +} + +func TestAttributedStringSliceEncoding(t *testing.T) { + for _, test := range []struct { + configs []string + marshalledData string + expectedSlice []string + expectedAppend *bool + }{ + { + []string{confingDefault}, + "array = [\"1\", \"2\", \"3\"]\n", + []string{"1", "2", "3"}, + nil, + }, + { + []string{configAppendFront}, + "array = [\"4\", \"5\", \"6\", {append = true}]\n", + []string{"4", "5", "6"}, + &bTrue, + }, + { + []string{configAppendFront, configAppendFalse}, + "array = [\"10\", {append = false}]\n", + []string{"10"}, + &bFalse, + }, + } { + // 1) Load the configs + result, err := loadConfigs(test.configs) + require.NoError(t, err, "loading config must succeed") + require.NotNil(t, result, "loaded config must not be nil") + require.Equal(t, result.Array.slice, test.expectedSlice, "slices do not match: %v", test) + require.Equal(t, result.Array.attributes.append, test.expectedAppend, "append field does not match: %v", test) + + // 2) Marshal the config to emulate writing it to disk + buf := new(bytes.Buffer) + enc := toml.NewEncoder(buf) + encErr := enc.Encode(result) + require.NoError(t, encErr, "encoding config must work") + require.Equal(t, buf.String(), test.marshalledData) + + // 3) Reload the marshaled config to make sure that data is preserved + var reloadedConfig testConfig + _, decErr := toml.Decode(buf.String(), &reloadedConfig) + require.NoError(t, decErr, "loading config must succeed") + require.NotNil(t, reloadedConfig, "re-loaded config must not be nil") + require.Equal(t, reloadedConfig.Array.slice, test.expectedSlice, "slices do not match: %v", test) + require.Equal(t, reloadedConfig.Array.attributes.append, test.expectedAppend, "append field does not match: %v", test) + } +}