Skip to content

Commit

Permalink
Merge pull request #1707 from vrothberg/RUN-1934
Browse files Browse the repository at this point in the history
containers.conf: appendable string arrays, Part 1
  • Loading branch information
openshift-ci[bot] authored Oct 24, 2023
2 parents 8d0bd25 + 0917cca commit 7977328
Show file tree
Hide file tree
Showing 11 changed files with 147 additions and 113 deletions.
73 changes: 0 additions & 73 deletions internal/attributed-string-slice/attributed_string_slice.go

This file was deleted.

92 changes: 92 additions & 0 deletions internal/attributedstring/slice.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
package attributedstring

import (
"bytes"
"fmt"

"github.com/BurntSushi/toml"
)

// Slice allows for extending a TOML string array with custom
// attributes that control how the array is marshaled into a Go string.
//
// Specifically, an Slice can be configured to avoid it being
// overridden by a subsequent unmarshal sequence. When the `append` attribute
// is specified, the array will be appended instead (e.g., `array=["9",
// {append=true}]`).
type Slice struct { // A "mixed-type array" in TOML.
// Note that the fields below _must_ be exported. Otherwise the TOML
// encoder would fail during type reflection.
Values []string
Attributes struct { // Using a struct allows for adding more attributes in the future.
Append *bool // Nil if not set by the user
}
}

// Get returns the Slice values or an empty string slice.
func (a *Slice) Get() []string {
if a.Values == nil {
return []string{}
}
return a.Values
}

// UnmarshalTOML is the custom unmarshal method for Slice.
func (a *Slice) 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)
}
a.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 a.Attributes.Append != nil && *a.Attributes.Append { // If _explicitly_ configured, append the loaded slice.
a.Values = append(a.Values, loadedStrings...)
} else { // Default: override the existing Slice.
a.Values = loadedStrings
}
return nil
}

// MarshalTOML is the custom marshal method for Slice.
func (a *Slice) MarshalTOML() ([]byte, error) {
iFaceSlice := make([]interface{}, 0, len(a.Values))

for _, x := range a.Values {
iFaceSlice = append(iFaceSlice, x)
}

if a.Attributes.Append != nil {
Attributes := make(map[string]any)
Attributes["append"] = *a.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
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package attributedstringslice
package attributedstring

import (
"bytes"
Expand All @@ -9,7 +9,7 @@ import (
)

type testConfig struct {
Array attributedStringSlice `toml:"array,omitempty"`
Array Slice `toml:"array,omitempty"`
}

const (
Expand All @@ -35,10 +35,10 @@ func loadConfigs(configs []string) (*testConfig, error) {
return &config, nil
}

func TestAttributedStringSliceLoading(t *testing.T) {
func TestSliceLoading(t *testing.T) {
for _, test := range []struct {
configs []string
expectedSlice []string
expectedValues []string
expectedAppend *bool
expectedErrorSubstring string
}{
Expand Down Expand Up @@ -71,16 +71,16 @@ func TestAttributedStringSliceLoading(t *testing.T) {
}
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)
require.Equal(t, result.Array.Values, test.expectedValues, "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) {
func TestSliceEncoding(t *testing.T) {
for _, test := range []struct {
configs []string
marshalledData string
expectedSlice []string
expectedValues []string
expectedAppend *bool
}{
{
Expand All @@ -106,8 +106,8 @@ func TestAttributedStringSliceEncoding(t *testing.T) {
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)
require.Equal(t, result.Array.Values, test.expectedValues, "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)
Expand All @@ -121,7 +121,7 @@ func TestAttributedStringSliceEncoding(t *testing.T) {
_, 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)
require.Equal(t, reloadedConfig.Array.Values, test.expectedValues, "slices do not match: %v", test)
require.Equal(t, reloadedConfig.Array.Attributes.Append, test.expectedAppend, "append field does not match: %v", test)
}
}
9 changes: 5 additions & 4 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"

"github.com/BurntSushi/toml"
"github.com/containers/common/internal/attributedstring"
"github.com/containers/common/libnetwork/types"
"github.com/containers/common/pkg/capabilities"
"github.com/containers/common/pkg/util"
Expand Down Expand Up @@ -71,7 +72,7 @@ type ContainersConfig struct {
Devices []string `toml:"devices,omitempty"`

// Volumes to add to all containers
Volumes []string `toml:"volumes,omitempty"`
Volumes attributedstring.Slice `toml:"volumes,omitempty"`

// ApparmorProfile is the apparmor profile name which is used as the
// default for the runtime.
Expand Down Expand Up @@ -133,7 +134,7 @@ type ContainersConfig struct {
EnableLabeledUsers bool `toml:"label_users,omitempty"`

// Env is the environment variable list for container process.
Env []string `toml:"env,omitempty"`
Env attributedstring.Slice `toml:"env,omitempty"`

// EnvHost Pass all host environment variables into the container.
EnvHost bool `toml:"env_host,omitempty"`
Expand Down Expand Up @@ -171,7 +172,7 @@ type ContainersConfig struct {
LogTag string `toml:"log_tag,omitempty"`

// Mount to add to all containers
Mounts []string `toml:"mounts,omitempty"`
Mounts attributedstring.Slice `toml:"mounts,omitempty"`

// NetNS indicates how to create a network namespace for the container
NetNS string `toml:"netns,omitempty"`
Expand Down Expand Up @@ -907,7 +908,7 @@ func (c *Config) GetDefaultEnvEx(envHost, httpProxy bool) []string {
}
}
}
return append(env, c.Containers.Env...)
return append(env, c.Containers.Env.Get()...)
}

// Capabilities returns the capabilities parses the Add and Drop capability
Expand Down
26 changes: 19 additions & 7 deletions pkg/config/config_local_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"runtime"
"strings"

"github.com/containers/common/internal/attributedstring"
"github.com/containers/common/libnetwork/types"
. "github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"
Expand Down Expand Up @@ -342,6 +343,14 @@ var _ = Describe("Config Local", func() {
tmpfile := "containers.conf.test"
oldContainersConf, envSet := os.LookupEnv("CONTAINERS_CONF")
os.Setenv("CONTAINERS_CONF", tmpfile)
defer func() {
if envSet {
os.Setenv("CONTAINERS_CONF", oldContainersConf)
} else {
os.Unsetenv("CONTAINERS_CONF")
}
}()

config, err := ReadCustomConfig()
gomega.Expect(err).To(gomega.BeNil())
config.Containers.Devices = []string{
Expand All @@ -350,22 +359,25 @@ var _ = Describe("Config Local", func() {
"/dev/sdc:/dev/xvdc",
"/dev/sdc:rm",
}
boolTrue := true
config.Containers.Env = attributedstring.Slice{Values: []string{"A", "B", "C"}}
config.Containers.Env.Attributes.Append = &boolTrue

err = config.Write()
// Undo that
if envSet {
os.Setenv("CONTAINERS_CONF", oldContainersConf)
} else {
os.Unsetenv("CONTAINERS_CONF")
}
// Then
gomega.Expect(err).To(gomega.BeNil())

fi, err := os.Stat(tmpfile)
gomega.Expect(err).To(gomega.BeNil())
perm := int(fi.Mode().Perm())
// 436 decimal = 644 octal
gomega.Expect(perm).To(gomega.Equal(420))
defer os.Remove(tmpfile)

writtenConfig, err := ReadCustomConfig()
gomega.Expect(err).To(gomega.BeNil())
gomega.Expect(writtenConfig.Containers.Devices).To(gomega.BeEquivalentTo(config.Containers.Devices))
gomega.Expect(writtenConfig.Containers.Env).To(gomega.BeEquivalentTo(config.Containers.Env))
gomega.Expect(writtenConfig.Containers.Env.Attributes.Append).To(gomega.BeEquivalentTo(&boolTrue))
})
It("Default Umask", func() {
// Given
Expand Down
10 changes: 5 additions & 5 deletions pkg/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -285,8 +285,8 @@ image_copy_tmp_dir="storage"`
// Then
gomega.Expect(err).To(gomega.BeNil())
gomega.Expect(defaultConfig.Engine.CgroupManager).To(gomega.Equal("systemd"))
gomega.Expect(defaultConfig.Containers.Env).To(gomega.BeEquivalentTo(envs))
gomega.Expect(defaultConfig.Containers.Mounts).To(gomega.BeEquivalentTo(mounts))
gomega.Expect(defaultConfig.Containers.Env.Values).To(gomega.BeEquivalentTo(envs))
gomega.Expect(defaultConfig.Containers.Mounts.Values).To(gomega.BeEquivalentTo(mounts))
gomega.Expect(defaultConfig.Containers.PidsLimit).To(gomega.BeEquivalentTo(2048))
gomega.Expect(defaultConfig.Network.CNIPluginDirs).To(gomega.Equal(pluginDirs))
gomega.Expect(defaultConfig.Network.NetavarkPluginDirs).To(gomega.Equal([]string{"/usr/netavark"}))
Expand Down Expand Up @@ -427,7 +427,7 @@ image_copy_tmp_dir="storage"`
gomega.Expect(err).To(gomega.BeNil())
gomega.Expect(config.Containers.ApparmorProfile).To(gomega.Equal(apparmor.Profile))
gomega.Expect(config.Containers.PidsLimit).To(gomega.BeEquivalentTo(2048))
gomega.Expect(config.Containers.Env).To(gomega.BeEquivalentTo(envs))
gomega.Expect(config.Containers.Env.Values).To(gomega.BeEquivalentTo(envs))
gomega.Expect(config.Containers.UserNS).To(gomega.BeEquivalentTo(""))
gomega.Expect(config.Network.CNIPluginDirs).To(gomega.Equal(DefaultCNIPluginDirs))
gomega.Expect(config.Network.NetavarkPluginDirs).To(gomega.Equal(DefaultNetavarkPluginDirs))
Expand Down Expand Up @@ -866,8 +866,8 @@ env=["foo=bar"]`

expectOldEnv := []string{"PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}
expectNewEnv := []string{"foo=bar"}
gomega.Expect(cfg.Containers.Env).To(gomega.Equal(expectOldEnv))
gomega.Expect(newCfg.Containers.Env).To(gomega.Equal(expectNewEnv))
gomega.Expect(cfg.Containers.Env.Values).To(gomega.Equal(expectOldEnv))
gomega.Expect(newCfg.Containers.Env.Values).To(gomega.Equal(expectNewEnv))
// Reload change back to default global configuration
_, err = Reload()
gomega.Expect(err).To(gomega.BeNil())
Expand Down
Loading

0 comments on commit 7977328

Please sign in to comment.