|
| 1 | +From 73dd46eed7a77f5f5b454eaeb1c227fb5efab320 Mon Sep 17 00:00:00 2001 |
| 2 | +From: David Son < [email protected]> |
| 3 | +Date: Fri, 18 Jul 2025 19:42:35 +0000 |
| 4 | +Subject: [PATCH] Hard-fail on config parsing errors |
| 5 | + |
| 6 | +Signed-off-by: David Son < [email protected]> |
| 7 | +--- |
| 8 | + config/config.go | 21 ++++++++++++----- |
| 9 | + config/config_test.go | 12 ++++++++++ |
| 10 | + integration/startup_test.go | 46 +++++++++++++++++++++++++++++++++++++ |
| 11 | + integration/util_test.go | 6 +++++ |
| 12 | + 4 files changed, 79 insertions(+), 6 deletions(-) |
| 13 | + |
| 14 | +diff --git a/config/config.go b/config/config.go |
| 15 | +index 25251c0f..6deb4818 100644 |
| 16 | +--- a/config/config.go |
| 17 | ++++ b/config/config.go |
| 18 | +@@ -39,6 +39,7 @@ |
| 19 | + package config |
| 20 | + |
| 21 | + import ( |
| 22 | ++ "errors" |
| 23 | + "fmt" |
| 24 | + "os" |
| 25 | + |
| 26 | +@@ -84,8 +85,8 @@ func NewConfig() *Config { |
| 27 | + |
| 28 | + // Set any defaults which do not align with Go zero values. |
| 29 | + var initParsers = []configParser{defaultPullModes, defaultDirectoryCacheConfig} |
| 30 | +- for _, p := range append(initParsers, parsers...) { |
| 31 | +- p(cfg) |
| 32 | ++ if err := parseConfig(cfg, append(initParsers, parsers...)); err != nil { |
| 33 | ++ return nil |
| 34 | + } |
| 35 | + |
| 36 | + return cfg |
| 37 | +@@ -102,18 +103,26 @@ func NewConfigFromToml(cfgPath string) (*Config, error) { |
| 38 | + defer f.Close() |
| 39 | + |
| 40 | + cfg := NewConfig() |
| 41 | ++ if cfg == nil { |
| 42 | ++ return nil, errors.New("error creating default config") |
| 43 | ++ } |
| 44 | + // Get configuration from specified file |
| 45 | + if err = toml.NewDecoder(f).Decode(cfg); err != nil { |
| 46 | + return nil, fmt.Errorf("failed to load config file %q: %w", cfgPath, err) |
| 47 | + } |
| 48 | +- parseConfig(cfg) |
| 49 | ++ if err := parseConfig(cfg, parsers); err != nil { |
| 50 | ++ return nil, fmt.Errorf("config file at %q: %w", cfgPath, err) |
| 51 | ++ } |
| 52 | + return cfg, nil |
| 53 | + } |
| 54 | + |
| 55 | +-func parseConfig(cfg *Config) { |
| 56 | +- for _, p := range parsers { |
| 57 | +- p(cfg) |
| 58 | ++func parseConfig(cfg *Config, cfgParsers []configParser) error { |
| 59 | ++ for _, p := range cfgParsers { |
| 60 | ++ if err := p(cfg); err != nil { |
| 61 | ++ return fmt.Errorf("failed to parse config: %v", err) |
| 62 | ++ } |
| 63 | + } |
| 64 | ++ return nil |
| 65 | + } |
| 66 | + |
| 67 | + func parseRootConfig(cfg *Config) error { |
| 68 | +diff --git a/config/config_test.go b/config/config_test.go |
| 69 | +index c1b73720..4facc5e3 100644 |
| 70 | +--- a/config/config_test.go |
| 71 | ++++ b/config/config_test.go |
| 72 | +@@ -265,6 +265,18 @@ concurrent_download_chunk_size = "1MB" |
| 73 | + } |
| 74 | + }, |
| 75 | + }, |
| 76 | ++ { |
| 77 | ++ name: "IncorrectChunkConfig", |
| 78 | ++ config: []byte(` |
| 79 | ++[pull_modes.parallel_pull_unpack] |
| 80 | ++concurrent_download_chunk_size = "badchunksize" |
| 81 | ++`), |
| 82 | ++ assert: func(t *testing.T, actual *Config, err error) { |
| 83 | ++ if err == nil { |
| 84 | ++ t.Error("Expected error, got none") |
| 85 | ++ } |
| 86 | ++ }, |
| 87 | ++ }, |
| 88 | + } |
| 89 | + |
| 90 | + for _, test := range tests { |
| 91 | +diff --git a/integration/startup_test.go b/integration/startup_test.go |
| 92 | +index f69488bb..e1774205 100644 |
| 93 | +--- a/integration/startup_test.go |
| 94 | ++++ b/integration/startup_test.go |
| 95 | +@@ -21,9 +21,11 @@ import ( |
| 96 | + "regexp" |
| 97 | + "strings" |
| 98 | + "testing" |
| 99 | ++ "time" |
| 100 | + |
| 101 | + "github.com/awslabs/soci-snapshotter/config" |
| 102 | + shell "github.com/awslabs/soci-snapshotter/util/dockershell" |
| 103 | ++ "github.com/awslabs/soci-snapshotter/util/testutil" |
| 104 | + "github.com/rs/xid" |
| 105 | + ) |
| 106 | + |
| 107 | +@@ -137,3 +139,47 @@ func TestSnapshotterSystemdStartup(t *testing.T) { |
| 108 | + }) |
| 109 | + } |
| 110 | + } |
| 111 | ++ |
| 112 | ++// TestSnapshotterStartupWithBadConfig ensures snapshotter does not start if snapshotter values are incorrect. |
| 113 | ++// Note that incorrect fields are ignored by TOML and thus are expected to work |
| 114 | ++func TestSnapshotterStartupWithBadConfig(t *testing.T) { |
| 115 | ++ tests := []struct { |
| 116 | ++ name string |
| 117 | ++ opts []snapshotterConfigOpt |
| 118 | ++ expectedErrStr string |
| 119 | ++ }{ |
| 120 | ++ { |
| 121 | ++ name: "Bad parallel pull size", |
| 122 | ++ opts: []snapshotterConfigOpt{withParallelPullMode(), withConcurrentDownloadChunkSizeStr("badstring")}, |
| 123 | ++ expectedErrStr: "invalid size format", |
| 124 | ++ }, |
| 125 | ++ } |
| 126 | ++ |
| 127 | ++ for _, tc := range tests { |
| 128 | ++ t.Run(tc.name, func(tt *testing.T) { |
| 129 | ++ sh, cleanup := newSnapshotterBaseShell(t) |
| 130 | ++ defer cleanup() |
| 131 | ++ // rebootContainerd would be ideal here, but that always uses a config, |
| 132 | ++ // so we just manually start SOCI with/without a config file. |
| 133 | ++ err := testutil.KillMatchingProcess(sh, "soci-snapshotter-grpc") |
| 134 | ++ if err != nil { |
| 135 | ++ sh.Fatal("failed to kill soci: %v", err) |
| 136 | ++ } |
| 137 | ++ configToml := getSnapshotterConfigToml(t, tc.opts...) |
| 138 | ++ snRunCmd := []string{"/usr/local/bin/soci-snapshotter-grpc", "--log-level", sociLogLevel} |
| 139 | ++ snRunCmd = addConfig(t, sh, configToml, snRunCmd...) |
| 140 | ++ |
| 141 | ++ errCh := make(chan error, 1) |
| 142 | ++ go func() { |
| 143 | ++ _, err := sh.OLog(snRunCmd...) |
| 144 | ++ errCh <- err |
| 145 | ++ }() |
| 146 | ++ |
| 147 | ++ select { |
| 148 | ++ case <-errCh: |
| 149 | ++ case <-time.After(2 * time.Second): |
| 150 | ++ t.Fatalf("expected err %s but snapshotter did not fail within 2 seconds", tc.expectedErrStr) |
| 151 | ++ } |
| 152 | ++ }) |
| 153 | ++ } |
| 154 | ++} |
| 155 | +diff --git a/integration/util_test.go b/integration/util_test.go |
| 156 | +index 332a95ef..dda17f5f 100644 |
| 157 | +--- a/integration/util_test.go |
| 158 | ++++ b/integration/util_test.go |
| 159 | +@@ -383,6 +383,12 @@ func withConcurrentDownloadChunkSize(chunkSize int64) snapshotterConfigOpt { |
| 160 | + } |
| 161 | + } |
| 162 | + |
| 163 | ++func withConcurrentDownloadChunkSizeStr(chunkSizeStr string) snapshotterConfigOpt { |
| 164 | ++ return func(c *config.Config) { |
| 165 | ++ c.PullModes.Parallel.ConcurrentDownloadChunkSizeStr = chunkSizeStr |
| 166 | ++ } |
| 167 | ++} |
| 168 | ++ |
| 169 | + func withConcurrentUnpacks(n int64) snapshotterConfigOpt { |
| 170 | + return func(c *config.Config) { |
| 171 | + c.PullModes.Parallel.MaxConcurrentUnpacks = n |
| 172 | +-- |
| 173 | +2.47.1 |
| 174 | + |
0 commit comments