From f02f959c16880c75ce1f89a7a53b69cd27d60bdc Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Tue, 14 Feb 2023 18:04:07 -0800 Subject: [PATCH] `runsc`: Introduce the concept of "config bundles". Configuration bundles are named sets of flag name-value pairs. They represent a higher-level configuration intent than flags, whose mapping to actual flag values may change over the course of `runsc` releases. For example, the `experimental-high-performance` bundle given as example in this change represents the intent that the user places is OK with using more experimental features, so long as they tend to yield higher performance than more stable non-experimental features. The set of which specific flags and features this bundle maps to will and should change as features mature and become defaults. Another useful example might be a "troubleshooting" bundle which simultaneously enables debug logging, syscall tracing, and profile tracing. This could be useful when asking users to provide debug information when troubleshooting issues on their behalf. Bundles are applied by using pod annotations of the format `"dev.gvisor.bundle.$BUNDLE_NAME" = "true"`. A pod may specify multiple bundles, as long as they do not map to conflicting flag values. The user may still specify flags specified in bundles on the command-line, and whatever value is specified on the command-line (even if it matches the flag default) will take precedence over the value specified by the bundle. PiperOrigin-RevId: 509689110 --- runsc/config/BUILD | 7 +- runsc/config/config.go | 11 ++ runsc/config/config_bundles.go | 29 ++++ runsc/config/config_test.go | 289 +++++++++++++++++++++++++++++++-- runsc/config/flags.go | 111 ++++++++++++- runsc/flag/flag.go | 3 + runsc/specutils/specutils.go | 27 ++- 7 files changed, 458 insertions(+), 19 deletions(-) create mode 100644 runsc/config/config_bundles.go diff --git a/runsc/config/BUILD b/runsc/config/BUILD index ab1905823..0a4b1867a 100644 --- a/runsc/config/BUILD +++ b/runsc/config/BUILD @@ -6,10 +6,12 @@ go_library( name = "config", srcs = [ "config.go", + "config_bundles.go", "flags.go", ], visibility = ["//:sandbox"], deps = [ + "//pkg/log", "//pkg/refs", "//pkg/sentry/watchdog", "//runsc/flag", @@ -23,5 +25,8 @@ go_test( "config_test.go", ], library = ":config", - deps = ["//runsc/flag"], + deps = [ + "//runsc/flag", + "@com_github_google_go_cmp//cmp:go_default_library", + ], ) diff --git a/runsc/config/config.go b/runsc/config/config.go index 5d272b5cd..d28ecba7d 100644 --- a/runsc/config/config.go +++ b/runsc/config/config.go @@ -298,6 +298,10 @@ type Config struct { // multiple tests are run in parallel, since there is no way to pass // parameters to the runtime from docker. TestOnlyTestNameEnv string `flag:"TESTONLY-test-name-env"` + + // explicitlySet contains whether a flag was explicitly set on the command-line from which this + // Config was constructed. Nil when the Config was not initialized from a FlagSet. + explicitlySet map[string]struct{} } func (c *Config) validate() error { @@ -359,6 +363,13 @@ func (c *Config) GetOverlay2() Overlay2 { return c.Overlay2 } +// Bundle is a set of flag name-value pairs. +type Bundle map[string]string + +// BundleName is a human-friendly name for a Bundle. +// It is used as part of an annotation to specify that the user wants to apply a Bundle. +type BundleName string + // MetricMetadata returns key-value pairs that are useful to include in metrics // exported about the sandbox this config represents. func (c *Config) MetricMetadata() map[string]string { diff --git a/runsc/config/config_bundles.go b/runsc/config/config_bundles.go new file mode 100644 index 000000000..25e1e9c8d --- /dev/null +++ b/runsc/config/config_bundles.go @@ -0,0 +1,29 @@ +// Copyright 2023 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//go:build go1.1 +// +build go1.1 + +package config + +// Bundles is the set of each Bundle. +// Each bundle is a named set of flag names and flag values. +// Bundles may be turned on using pod annotations. +// Bundles have lower precedence than flag pod annotation and command-line flags. +// Bundles are mutually exclusive iff their flag values overlap and differ. +var Bundles = map[BundleName]Bundle{ + "experimental-high-performance": { + "overlay2": "root:self", + }, +} diff --git a/runsc/config/config_test.go b/runsc/config/config_test.go index ed64b079d..bfdc12ae0 100644 --- a/runsc/config/config_test.go +++ b/runsc/config/config_test.go @@ -15,9 +15,11 @@ package config import ( + "reflect" "strings" "testing" + "github.com/google/go-cmp/cmp" "gvisor.dev/gvisor/runsc/flag" ) @@ -73,21 +75,83 @@ func TestFromFlags(t *testing.T) { } } -func TestToFlags(t *testing.T) { +func TestToFlagsFromFlags(t *testing.T) { testFlags := flag.NewFlagSet("test", flag.ContinueOnError) RegisterFlags(testFlags) + testFlags.Set("root", "some-path") + testFlags.Set("debug", "true") + testFlags.Set("profile", "false") // Matches default value. + testFlags.Set("num-network-channels", "123") + testFlags.Set("network", "none") c, err := NewFromFlags(testFlags) if err != nil { t.Fatal(err) } - c.RootDir = "some-path" - c.Debug = true - c.NumNetworkChannels = 123 - c.Network = NetworkNone + + flags := c.ToFlags() + if len(flags) != 5 { + t.Errorf("wrong number of flags set, want: 5, got: %d: %s", len(flags), flags) + } + t.Logf("Flags: %s", flags) + fm := map[string]string{} + for _, f := range flags { + kv := strings.Split(f, "=") + fm[kv[0]] = kv[1] + } + for name, want := range map[string]string{ + "--root": "some-path", + "--debug": "true", + "--profile": "false", + "--num-network-channels": "123", + "--network": "none", + } { + if got, ok := fm[name]; ok { + if got != want { + t.Errorf("flag %q, want: %q, got: %q", name, want, got) + } + } else { + t.Errorf("flag %q not set", name) + } + } +} + +func TestToFlagsFromManual(t *testing.T) { + c := &Config{ + RootDir: "some-path", + Debug: true, + ProfileEnable: false, // Matches default flag value. + NumNetworkChannels: 123, + Network: NetworkNone, + } + + // Create a second config with flag-default values that we'll copy from. + testFlags := flag.NewFlagSet("test", flag.ContinueOnError) + RegisterFlags(testFlags) + cfgDefault, err := NewFromFlags(testFlags) + if err != nil { + t.Fatal(err) + } + + // Set all the unset fields of c to their flag-default value from cfgDefault. + cfgReflect := reflect.ValueOf(c).Elem() + cfgDefaultReflect := reflect.ValueOf(cfgDefault).Elem() + cfgType := cfgReflect.Type() + for i := 0; i < cfgType.NumField(); i++ { + f := cfgType.Field(i) + name, ok := f.Tag.Lookup("flag") + if !ok { + // No flag set for this field. + continue + } + if name == "root" || name == "debug" || name == "profile" || name == "num-network-channels" || name == "network" { + continue + } + cfgReflect.Field(i).Set(cfgDefaultReflect.Field(i)) + } flags := c.ToFlags() if len(flags) != 4 { - t.Errorf("wrong number of flags set, want: 5, got: %d: %s", len(flags), flags) + t.Errorf("wrong number of flags set, want: 4, got: %d: %s", len(flags), flags) } t.Logf("Flags: %s", flags) fm := map[string]string{} @@ -109,6 +173,9 @@ func TestToFlags(t *testing.T) { t.Errorf("flag %q not set", name) } } + if _, hasProfile := fm["--profile"]; hasProfile { + t.Error("--profile flag unexpectedly set") + } } // TestInvalidFlags checks that enum flags fail when value is not in enum set. @@ -262,7 +329,7 @@ func TestOverride(t *testing.T) { t.Run("string", func(t *testing.T) { c.RootDir = "foobar" - if err := c.Override(testFlags, "root", "bar"); err != nil { + if err := c.Override(testFlags, "root", "bar", false); err != nil { t.Fatalf("Override(root, bar) failed: %v", err) } if c.RootDir != "bar" { @@ -272,7 +339,7 @@ func TestOverride(t *testing.T) { t.Run("bool", func(t *testing.T) { c.Debug = true - if err := c.Override(testFlags, "debug", "false"); err != nil { + if err := c.Override(testFlags, "debug", "false", false); err != nil { t.Fatalf("Override(debug, false) failed: %v", err) } if c.Debug { @@ -282,7 +349,7 @@ func TestOverride(t *testing.T) { t.Run("enum", func(t *testing.T) { c.FileAccess = FileAccessShared - if err := c.Override(testFlags, "file-access", "exclusive"); err != nil { + if err := c.Override(testFlags, "file-access", "exclusive", false); err != nil { t.Fatalf("Override(file-access, exclusive) failed: %v", err) } if c.FileAccess != FileAccessExclusive { @@ -299,7 +366,7 @@ func TestOverrideDisabled(t *testing.T) { t.Fatal(err) } const errMsg = "flag override disabled" - if err := c.Override(testFlags, "root", "path"); err == nil || !strings.Contains(err.Error(), errMsg) { + if err := c.Override(testFlags, "root", "path", false); err == nil || !strings.Contains(err.Error(), errMsg) { t.Errorf("Override() wrong error: %v", err) } } @@ -334,7 +401,7 @@ func TestOverrideError(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if err := c.Override(testFlags, tc.name, tc.value); err == nil || !strings.Contains(err.Error(), tc.error) { + if err := c.Override(testFlags, tc.name, tc.value, false); err == nil || !strings.Contains(err.Error(), tc.error) { t.Errorf("Override(%q, %q) wrong error: %v", tc.name, tc.value, err) } }) @@ -351,6 +418,7 @@ func TestOverrideAllowlist(t *testing.T) { for _, tc := range []struct { flag string value string + force bool error string }{ { @@ -381,6 +449,11 @@ func TestOverrideAllowlist(t *testing.T) { value: "true", error: "flag override disabled", }, + { + flag: "profile", + value: "true", + force: true, + }, { flag: "profile", value: "123", @@ -388,7 +461,7 @@ func TestOverrideAllowlist(t *testing.T) { }, } { t.Run(tc.flag, func(t *testing.T) { - err := c.Override(testFlags, tc.flag, tc.value) + err := c.Override(testFlags, tc.flag, tc.value, tc.force) if len(tc.error) == 0 { if err != nil { t.Errorf("Unexpected error: %v", err) @@ -399,3 +472,195 @@ func TestOverrideAllowlist(t *testing.T) { }) } } + +func TestBundles(t *testing.T) { + noChange := func(t *testing.T, old, new *Config) { + t.Helper() + if diff := cmp.Diff(old, new, cmp.AllowUnexported(Config{})); diff != "" { + t.Errorf("different configs:\n%+v\nvs\n%+v\nDiff:\n%s", old, new, diff) + } + } + for _, test := range []struct { + // Name of the test. + Name string + + // List of bundles that exist for the purpose of this test. + BundleConfig map[BundleName]Bundle + + // Command-line arguments passed as explicit flags. + CommandLine []string + + // Names of the bundles to apply. + Bundles []BundleName + + // Whether we expect applying bundles to fail. + WantErr bool + + // If bundles were successfully applied, this function is called to compare + // pre-bundle-application and post-bundle-application configs. + Verify func(t *testing.T, old, new *Config) + }{ + { + Name: "empty bundle", + BundleConfig: map[BundleName]Bundle{"empty": {}}, + Bundles: []BundleName{"empty"}, + Verify: noChange, + }, + { + Name: "no-op bundle", + BundleConfig: map[BundleName]Bundle{ + "no-debug": { + "debug": "false", + }, + }, + Bundles: []BundleName{"no-debug"}, + Verify: noChange, + }, + { + Name: "invalid flag", + BundleConfig: map[BundleName]Bundle{ + "invalid-flag": { + "not-a-real-flag": "nope.avi", + }, + }, + Bundles: []BundleName{"invalid-flag"}, + WantErr: true, + }, + { + Name: "duplicate no-op bundles", + BundleConfig: map[BundleName]Bundle{ + "empty": {}, + "no-debug": { + "debug": "false", + }, + }, + Bundles: []BundleName{"no-debug", "no-debug"}, + Verify: noChange, + }, + { + Name: "simple bundle", + BundleConfig: map[BundleName]Bundle{ + "empty": {}, + "debug": { + "debug": "true", + }, + "no-debug": { + "debug": "false", + }, + }, + Bundles: []BundleName{"debug"}, + Verify: func(t *testing.T, old, new *Config) { + t.Helper() + if old.Debug { + t.Error("debug was previously set to true") + } + if !new.Debug { + t.Error("debug was not set to true") + } + }, + }, + { + Name: "incompatible bundles", + BundleConfig: map[BundleName]Bundle{ + "debug": { + "debug": "true", + }, + "no-debug": { + "debug": "false", + }, + }, + Bundles: []BundleName{"debug", "no-debug"}, + WantErr: true, + }, + { + Name: "compatible bundles", + BundleConfig: map[BundleName]Bundle{ + "debug": { + "debug": "true", + }, + "debug-and-profile": { + "debug": "true", + "profile": "true", + }, + }, + Bundles: []BundleName{"debug", "debug-and-profile"}, + Verify: func(t *testing.T, old, new *Config) { + t.Helper() + if old.Debug || old.ProfileEnable { + t.Error("debug/profiling was previously set to true") + } + if !new.Debug { + t.Error("debug was not set to true") + } + if !new.ProfileEnable { + t.Error("profiling was not set to true") + } + }, + }, + { + Name: "command line takes precedence to non-default value", + BundleConfig: map[BundleName]Bundle{ + "no-debug": { + "debug": "false", + }, + }, + CommandLine: []string{"-debug=true"}, + Bundles: []BundleName{"no-debug"}, + Verify: func(t *testing.T, old, new *Config) { + t.Helper() + noChange(t, old, new) + if !new.Debug { + t.Error("debug was not set to true") + } + }, + }, + { + Name: "command line takes precedence to default value", + BundleConfig: map[BundleName]Bundle{ + "debug": { + "debug": "true", + }, + }, + CommandLine: []string{"-debug=false"}, + Bundles: []BundleName{"debug"}, + Verify: func(t *testing.T, old, new *Config) { + t.Helper() + noChange(t, old, new) + if new.Debug { + t.Error("debug was set to true") + } + }, + }, + } { + t.Run(test.Name, func(t *testing.T) { + oldBundles := Bundles + defer func() { + Bundles = oldBundles + }() + Bundles = test.BundleConfig + flagSet := flag.NewFlagSet(test.Name, flag.ContinueOnError) + RegisterFlags(flagSet) + if err := flagSet.Parse(test.CommandLine); err != nil { + t.Fatalf("cannot parse command line %q: %v", test.CommandLine, err) + } + cfg, err := NewFromFlags(flagSet) + if err != nil { + t.Fatalf("cannot generate config from flags: %v", err) + } + oldCfg := *cfg + err = cfg.ApplyBundles(flagSet, test.Bundles...) + if test.WantErr && err == nil { + t.Error("got no error, but expected one") + } + if !test.WantErr && err != nil { + t.Errorf("got unexpected error: %v", err) + } + if err != nil && test.Verify != nil && !t.Failed() { + t.Error("cannot specify Verify function for erroring tests") + } + if err == nil && test.Verify != nil { + test.Verify(t, &oldCfg, cfg) + } + }) + } +} diff --git a/runsc/config/flags.go b/runsc/config/flags.go index 7fb338972..875c22607 100644 --- a/runsc/config/flags.go +++ b/runsc/config/flags.go @@ -20,7 +20,9 @@ import ( "path/filepath" "reflect" "strconv" + "strings" + "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sentry/watchdog" "gvisor.dev/gvisor/runsc/flag" @@ -144,9 +146,22 @@ func checkOciSeccomp(name string, value string) error { return nil } +// isFlagExplicitlySet returns whether the given flag name is explicitly set. +// Doesn't check for flag existence; returns `false` for flags that don't exist. +func isFlagExplicitlySet(flagSet *flag.FlagSet, name string) bool { + explicit := false + + // The FlagSet.Visit function only visits flags that are explicitly set, as opposed to VisitAll. + flagSet.Visit(func(fl *flag.Flag) { + explicit = explicit || fl.Name == name + }) + + return explicit +} + // NewFromFlags creates a new Config with values coming from command line flags. func NewFromFlags(flagSet *flag.FlagSet) (*Config, error) { - conf := &Config{} + conf := &Config{explicitlySet: map[string]struct{}{}} obj := reflect.ValueOf(conf).Elem() st := obj.Type() @@ -163,6 +178,9 @@ func NewFromFlags(flagSet *flag.FlagSet) (*Config, error) { } x := reflect.ValueOf(flag.Get(fl.Value)) obj.Field(i).Set(x) + if isFlagExplicitlySet(flagSet, name) { + conf.explicitlySet[name] = struct{}{} + } } if len(conf.RootDir) == 0 { @@ -204,7 +222,15 @@ func (c *Config) ToFlags() []string { panic(fmt.Sprintf("Flag %q not found", name)) } if val == flag.DefValue { - continue + // If this config wasn't populated from a FlagSet, don't plumb through default flags. + if c.explicitlySet == nil { + continue + } + // If this config was populated from a FlagSet, plumb through only default flags which were + // explicitly specified. + if _, explicit := c.explicitlySet[name]; !explicit { + continue + } } rv = append(rv, fmt.Sprintf("--%s=%s", flag.Name, val)) } @@ -212,7 +238,7 @@ func (c *Config) ToFlags() []string { } // Override writes a new value to a flag. -func (c *Config) Override(flagSet *flag.FlagSet, name string, value string) error { +func (c *Config) Override(flagSet *flag.FlagSet, name string, value string, force bool) error { obj := reflect.ValueOf(c).Elem() st := obj.Type() for i := 0; i < st.NumField(); i++ { @@ -227,8 +253,10 @@ func (c *Config) Override(flagSet *flag.FlagSet, name string, value string) erro // Flag must exist if there is a field match above. panic(fmt.Sprintf("Flag %q not found", name)) } - if err := c.isOverrideAllowed(name, value); err != nil { - return fmt.Errorf("error setting flag %s=%q: %w", name, value, err) + if !force { + if err := c.isOverrideAllowed(name, value); err != nil { + return fmt.Errorf("error setting flag %s=%q: %w", name, value, err) + } } // Use flag to convert the string value to the underlying flag type, using @@ -262,6 +290,79 @@ func (c *Config) isOverrideAllowed(name string, value string) error { return fmt.Errorf("flag override disabled, use --allow-flag-override to enable it") } +// ApplyBundles applies the given bundles by name. +// It returns an error if a bundle doesn't exist, or if the given +// bundles have conflicting flag values. +// Config values which are already specified prior to calling ApplyBundles do not change. +func (c *Config) ApplyBundles(flagSet *flag.FlagSet, bundleNames ...BundleName) error { + // Populate a map from flag name to flag value to bundle name. + flagToValueToBundleName := make(map[string]map[string]BundleName) + for _, bundleName := range bundleNames { + b := Bundles[bundleName] + if b == nil { + return fmt.Errorf("no such bundle: %q", bundleName) + } + for flagName, val := range b { + valueToBundleName := flagToValueToBundleName[flagName] + if valueToBundleName == nil { + valueToBundleName = make(map[string]BundleName) + flagToValueToBundleName[flagName] = valueToBundleName + } + valueToBundleName[val] = bundleName + } + } + + // Check for conflicting flag values between the bundles. + for flagName, valueToBundleName := range flagToValueToBundleName { + if len(valueToBundleName) == 1 { + continue + } + bundleNameToValue := make(map[string]string) + for val, bundleName := range valueToBundleName { + bundleNameToValue[string(bundleName)] = val + } + var sb strings.Builder + first := true + for _, bundleName := range bundleNames { + if val, ok := bundleNameToValue[string(bundleName)]; ok { + if !first { + sb.WriteString(", ") + } + sb.WriteString(fmt.Sprintf("bundle %q sets --%s=%q", bundleName, flagName, val)) + first = false + } + } + return fmt.Errorf("flag --%s is specified by multiple bundles: %s", flagName, sb.String()) + } + + // Actually apply flag values. + for flagName, valueToBundleName := range flagToValueToBundleName { + fl := flagSet.Lookup(flagName) + if fl == nil { + return fmt.Errorf("flag --%s not found", flagName) + } + prevValue := fl.Value.String() + // Note: We verified earlier that valueToBundleName has length 1, + // so this loop executes exactly once per flag. + for val, bundleName := range valueToBundleName { + if isFlagExplicitlySet(flagSet, flagName) { + if prevValue != val { + log.Infof("Bundle %s is supposed to have the effect of setting flag --%s to %q, but this flag was also explicitly set to --%s=%q on the command-line; the command-line value --%s=%q takes precedence.", bundleName, flagName, val, flagName, prevValue, flagName, prevValue) + } + continue + } + if err := c.Override(flagSet, flagName, val /* force= */, true); err != nil { + return err + } + if prevValue != val { + log.Infof("Applying bundle %s: flag --%s has been updated from --%s=%q to --%s=%q", bundleName, flagName, flagName, prevValue, flagName, val) + } + } + } + + return c.validate() +} + func getVal(field reflect.Value) string { if str, ok := field.Addr().Interface().(fmt.Stringer); ok { return str.String() diff --git a/runsc/flag/flag.go b/runsc/flag/flag.go index 37c638c1d..772d832be 100644 --- a/runsc/flag/flag.go +++ b/runsc/flag/flag.go @@ -25,6 +25,9 @@ import ( // FlagSet is an alias for flag.FlagSet. type FlagSet = flag.FlagSet +// Flag is an alias for flag.Flag. +type Flag = flag.Flag + // Aliases for flag functions. var ( Bool = flag.Bool diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go index a531adf9c..c9bb6ecf3 100644 --- a/runsc/specutils/specutils.go +++ b/runsc/specutils/specutils.go @@ -195,6 +195,31 @@ func ReadSpecFromFile(bundleDir string, specFile *os.File, conf *config.Config) m.Source = absPath(bundleDir, m.Source) } } + // Look for config bundle annotations and verify that they exist. + const configBundlePrefix = "dev.gvisor.bundle." + var bundles []config.BundleName + for annotation, val := range spec.Annotations { + if !strings.HasPrefix(annotation, configBundlePrefix) { + continue + } + if val != "true" { + return nil, fmt.Errorf("invalid value %q for annotation %q (must be set to 'true' or removed entirely)", val, annotation) + } + bundleName := config.BundleName(annotation[len(configBundlePrefix):]) + if _, exists := config.Bundles[bundleName]; !exists { + log.Warningf("Bundle name %q (from annotation %q=%q) does not exist; this bundle may have been deprecated. Skipping.", bundleName, annotation, val) + continue + } + bundles = append(bundles, bundleName) + } + + // Apply config bundles, if any. + if len(bundles) > 0 { + log.Infof("Applying config bundles: %v", bundles) + if err := conf.ApplyBundles(flag.CommandLine, bundles...); err != nil { + return nil, err + } + } // Override flags using annotation to allow customization per sandbox // instance. @@ -203,7 +228,7 @@ func ReadSpecFromFile(bundleDir string, specFile *os.File, conf *config.Config) if strings.HasPrefix(annotation, flagPrefix) { name := annotation[len(flagPrefix):] log.Infof("Overriding flag: %s=%q", name, val) - if err := conf.Override(flag.CommandLine, name, val); err != nil { + if err := conf.Override(flag.CommandLine, name, val /* force= */, false); err != nil { return nil, err } }