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 } }