From 71589b7f7e69b85820c9deb75fc6d0b6c5f00eb1 Mon Sep 17 00:00:00 2001 From: Fabricio Voznika Date: Tue, 1 Sep 2020 11:10:15 -0700 Subject: [PATCH] Let flags be overriden from OCI annotations This allows runsc flags to be set per sandbox instance. For example, K8s pod annotations can be used to enable --debug for a single pod, making troubleshoot much easier. Similarly, features like --vfs2 can be enabled for experimentation without affecting other pods in the node. Closes #3494 PiperOrigin-RevId: 329542815 --- runsc/cmd/boot.go | 2 +- runsc/cmd/checkpoint.go | 2 +- runsc/cmd/create.go | 2 +- runsc/cmd/gofer.go | 6 +-- runsc/cmd/restore.go | 2 +- runsc/cmd/run.go | 2 +- runsc/config/config.go | 2 + runsc/config/config_test.go | 87 ++++++++++++++++++++++++++++++++++++ runsc/config/flags.go | 36 +++++++++++++++ runsc/specutils/BUILD | 1 + runsc/specutils/specutils.go | 21 +++++++-- 11 files changed, 152 insertions(+), 11 deletions(-) diff --git a/runsc/cmd/boot.go b/runsc/cmd/boot.go index 357f46517..cd419e1aa 100644 --- a/runsc/cmd/boot.go +++ b/runsc/cmd/boot.go @@ -168,7 +168,7 @@ func (b *Boot) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) // Get the spec from the specFD. specFile := os.NewFile(uintptr(b.specFD), "spec file") defer specFile.Close() - spec, err := specutils.ReadSpecFromFile(b.bundleDir, specFile) + spec, err := specutils.ReadSpecFromFile(b.bundleDir, specFile, conf) if err != nil { Fatalf("reading spec: %v", err) } diff --git a/runsc/cmd/checkpoint.go b/runsc/cmd/checkpoint.go index db46d509f..8fe0c427a 100644 --- a/runsc/cmd/checkpoint.go +++ b/runsc/cmd/checkpoint.go @@ -118,7 +118,7 @@ func (c *Checkpoint) Execute(_ context.Context, f *flag.FlagSet, args ...interfa Fatalf("setting bundleDir") } - spec, err := specutils.ReadSpec(bundleDir) + spec, err := specutils.ReadSpec(bundleDir, conf) if err != nil { Fatalf("reading spec: %v", err) } diff --git a/runsc/cmd/create.go b/runsc/cmd/create.go index 4d9085244..e76f7ba1d 100644 --- a/runsc/cmd/create.go +++ b/runsc/cmd/create.go @@ -91,7 +91,7 @@ func (c *Create) Execute(_ context.Context, f *flag.FlagSet, args ...interface{} if bundleDir == "" { bundleDir = getwdOrDie() } - spec, err := specutils.ReadSpec(bundleDir) + spec, err := specutils.ReadSpec(bundleDir, conf) if err != nil { return Errorf("reading spec: %v", err) } diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index 7da02c3af..bba00d551 100644 --- a/runsc/cmd/gofer.go +++ b/runsc/cmd/gofer.go @@ -100,15 +100,15 @@ func (g *Gofer) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) return subcommands.ExitUsageError } + conf := args[0].(*config.Config) + specFile := os.NewFile(uintptr(g.specFD), "spec file") defer specFile.Close() - spec, err := specutils.ReadSpecFromFile(g.bundleDir, specFile) + spec, err := specutils.ReadSpecFromFile(g.bundleDir, specFile, conf) if err != nil { Fatalf("reading spec: %v", err) } - conf := args[0].(*config.Config) - if g.setUpRoot { if err := setupRootFS(spec, conf); err != nil { Fatalf("Error setting up root FS: %v", err) diff --git a/runsc/cmd/restore.go b/runsc/cmd/restore.go index b16975804..096ec814c 100644 --- a/runsc/cmd/restore.go +++ b/runsc/cmd/restore.go @@ -88,7 +88,7 @@ func (r *Restore) Execute(_ context.Context, f *flag.FlagSet, args ...interface{ if bundleDir == "" { bundleDir = getwdOrDie() } - spec, err := specutils.ReadSpec(bundleDir) + spec, err := specutils.ReadSpec(bundleDir, conf) if err != nil { return Errorf("reading spec: %v", err) } diff --git a/runsc/cmd/run.go b/runsc/cmd/run.go index 1161de67a..c48cbe4cd 100644 --- a/runsc/cmd/run.go +++ b/runsc/cmd/run.go @@ -75,7 +75,7 @@ func (r *Run) Execute(_ context.Context, f *flag.FlagSet, args ...interface{}) s if bundleDir == "" { bundleDir = getwdOrDie() } - spec, err := specutils.ReadSpec(bundleDir) + spec, err := specutils.ReadSpec(bundleDir, conf) if err != nil { return Errorf("reading spec: %v", err) } diff --git a/runsc/config/config.go b/runsc/config/config.go index bca27ebf1..df134bb2f 100644 --- a/runsc/config/config.go +++ b/runsc/config/config.go @@ -157,6 +157,8 @@ type Config struct { // Enables FUSE usage. FUSE bool `flag:"fuse"` + AllowFlagOverride bool `flag:"allow-flag-override"` + // TestOnlyAllowRunAsCurrentUserWithoutChroot should only be used in // tests. It allows runsc to start the sandbox process as the current // user, and without chrooting the sandbox process. This can be diff --git a/runsc/config/config_test.go b/runsc/config/config_test.go index af7867a2a..fb162b7eb 100644 --- a/runsc/config/config_test.go +++ b/runsc/config/config_test.go @@ -183,3 +183,90 @@ func TestValidationFail(t *testing.T) { }) } } + +func TestOverride(t *testing.T) { + c, err := NewFromFlags() + if err != nil { + t.Fatal(err) + } + c.AllowFlagOverride = true + + t.Run("string", func(t *testing.T) { + c.RootDir = "foobar" + if err := c.Override("root", "bar"); err != nil { + t.Fatalf("Override(root, bar) failed: %v", err) + } + defer setDefault("root") + if c.RootDir != "bar" { + t.Errorf("Override(root, bar) didn't work: %+v", c) + } + }) + + t.Run("bool", func(t *testing.T) { + c.Debug = true + if err := c.Override("debug", "false"); err != nil { + t.Fatalf("Override(debug, false) failed: %v", err) + } + defer setDefault("debug") + if c.Debug { + t.Errorf("Override(debug, false) didn't work: %+v", c) + } + }) + + t.Run("enum", func(t *testing.T) { + c.FileAccess = FileAccessShared + if err := c.Override("file-access", "exclusive"); err != nil { + t.Fatalf("Override(file-access, exclusive) failed: %v", err) + } + defer setDefault("file-access") + if c.FileAccess != FileAccessExclusive { + t.Errorf("Override(file-access, exclusive) didn't work: %+v", c) + } + }) +} + +func TestOverrideDisabled(t *testing.T) { + c, err := NewFromFlags() + if err != nil { + t.Fatal(err) + } + const errMsg = "flag override disabled" + if err := c.Override("root", "path"); err == nil || !strings.Contains(err.Error(), errMsg) { + t.Errorf("Override() wrong error: %v", err) + } +} + +func TestOverrideError(t *testing.T) { + c, err := NewFromFlags() + if err != nil { + t.Fatal(err) + } + c.AllowFlagOverride = true + for _, tc := range []struct { + name string + value string + error string + }{ + { + name: "invalid", + value: "valid", + error: `flag "invalid" not found`, + }, + { + name: "debug", + value: "invalid", + error: "error setting flag debug", + }, + { + name: "file-access", + value: "invalid", + error: "invalid file access type", + }, + } { + t.Run(tc.name, func(t *testing.T) { + if err := c.Override(tc.name, tc.value); err == nil || !strings.Contains(err.Error(), tc.error) { + t.Errorf("Override(%q, %q) wrong error: %v", tc.name, tc.value, err) + } + }) + } +} diff --git a/runsc/config/flags.go b/runsc/config/flags.go index 488a4b9fb..eff46e938 100644 --- a/runsc/config/flags.go +++ b/runsc/config/flags.go @@ -48,6 +48,7 @@ func RegisterFlags() { flag.Bool("log-packets", false, "enable network packet logging.") flag.String("debug-log-format", "text", "log format: text (default), json, or json-k8s.") flag.Bool("alsologtostderr", false, "send log messages to stderr.") + flag.Bool("allow-flag-override", false, "allow OCI annotations (dev.gvisor.flag.) to override flags for debugging.") // Debugging flags: strace related flag.Bool("strace", false, "enable strace.") @@ -149,6 +150,41 @@ func (c *Config) ToFlags() []string { return rv } +// Override writes a new value to a flag. +func (c *Config) Override(name string, value string) error { + if !c.AllowFlagOverride { + return fmt.Errorf("flag override disabled, use --allow-flag-override to enable it") + } + + obj := reflect.ValueOf(c).Elem() + st := obj.Type() + for i := 0; i < st.NumField(); i++ { + f := st.Field(i) + fieldName, ok := f.Tag.Lookup("flag") + if !ok || fieldName != name { + // Not a flag field, or flag name doesn't match. + continue + } + fl := flag.CommandLine.Lookup(name) + if fl == nil { + // Flag must exist if there is a field match above. + panic(fmt.Sprintf("Flag %q not found", name)) + } + + // Use flag to convert the string value to the underlying flag type, using + // the same rules as the command-line for consistency. + if err := fl.Value.Set(value); err != nil { + return fmt.Errorf("error setting flag %s=%q: %w", name, value, err) + } + x := reflect.ValueOf(flag.Get(fl.Value)) + obj.Field(i).Set(x) + + // Validates the config again to ensure it's left in a consistent state. + return c.validate() + } + return fmt.Errorf("flag %q not found. Cannot set it to %q", name, value) +} + func getVal(field reflect.Value) string { if str, ok := field.Addr().Interface().(fmt.Stringer); ok { return str.String() diff --git a/runsc/specutils/BUILD b/runsc/specutils/BUILD index 43851a22f..679d8bc8e 100644 --- a/runsc/specutils/BUILD +++ b/runsc/specutils/BUILD @@ -16,6 +16,7 @@ go_library( "//pkg/bits", "//pkg/log", "//pkg/sentry/kernel/auth", + "//runsc/config", "@com_github_cenkalti_backoff//:go_default_library", "@com_github_mohae_deepcopy//:go_default_library", "@com_github_opencontainers_runtime_spec//specs-go:go_default_library", diff --git a/runsc/specutils/specutils.go b/runsc/specutils/specutils.go index 5015c3a84..a2275398a 100644 --- a/runsc/specutils/specutils.go +++ b/runsc/specutils/specutils.go @@ -35,6 +35,7 @@ import ( "gvisor.dev/gvisor/pkg/bits" "gvisor.dev/gvisor/pkg/log" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" + "gvisor.dev/gvisor/runsc/config" ) // ExePath must point to runsc binary, which is normally the same binary. It's @@ -161,18 +162,18 @@ func OpenSpec(bundleDir string) (*os.File, error) { // ReadSpec reads an OCI runtime spec from the given bundle directory. // ReadSpec also normalizes all potential relative paths into absolute // path, e.g. spec.Root.Path, mount.Source. -func ReadSpec(bundleDir string) (*specs.Spec, error) { +func ReadSpec(bundleDir string, conf *config.Config) (*specs.Spec, error) { specFile, err := OpenSpec(bundleDir) if err != nil { return nil, fmt.Errorf("error opening spec file %q: %v", filepath.Join(bundleDir, "config.json"), err) } defer specFile.Close() - return ReadSpecFromFile(bundleDir, specFile) + return ReadSpecFromFile(bundleDir, specFile, conf) } // ReadSpecFromFile reads an OCI runtime spec from the given File, and // normalizes all relative paths into absolute by prepending the bundle dir. -func ReadSpecFromFile(bundleDir string, specFile *os.File) (*specs.Spec, error) { +func ReadSpecFromFile(bundleDir string, specFile *os.File, conf *config.Config) (*specs.Spec, error) { if _, err := specFile.Seek(0, os.SEEK_SET); err != nil { return nil, fmt.Errorf("error seeking to beginning of file %q: %v", specFile.Name(), err) } @@ -195,6 +196,20 @@ func ReadSpecFromFile(bundleDir string, specFile *os.File) (*specs.Spec, error) m.Source = absPath(bundleDir, m.Source) } } + + // Override flags using annotation to allow customization per sandbox + // instance. + for annotation, val := range spec.Annotations { + const flagPrefix = "dev.gvisor.flag." + if strings.HasPrefix(annotation, flagPrefix) { + name := annotation[len(flagPrefix):] + log.Infof("Overriding flag: %s=%q", name, val) + if err := conf.Override(name, val); err != nil { + return nil, err + } + } + } + return &spec, nil }