From b5fa0ffb7a5d6e1ec463835f23bdba3c9f7b6cbc Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Mon, 3 Aug 2026 15:50:48 +1000 Subject: [PATCH] fix: stop viperblockd discarding the configured S3 region (#677) The viperblock start command applies --s3-host, --s3-bucket and --s3-region over spinifex.toml whenever viper reports a non-empty value. s3-bucket and s3-region carried non-empty defaults, so the override fired on every start even though the service unit passes neither flag nor environment variable, and the region read from the config was replaced with ap-southeast-2. Any cluster deployed to another region could not launch an instance: viperblockd signed its S3 requests with ap-southeast-2, predastore verified them against its own region and rejected them as AuthorizationHeaderMalformed, the volume mount failed and RunInstances ended in launch_failed. The bare-metal e2e cell, which installs with --region bare-metal, failed every instance-backed test this way. Default all three to empty, matching s3-host, so an unset flag leaves the configured value alone while an explicit flag or SPINIFEX_VIPERBLOCK_* still wins. The predastore.toml template also pinned the main bucket to a literal ap-southeast-2 while the top-level key and the northstar bucket both rendered the configured region; it now renders it too. --- cmd/spinifex/cmd/service.go | 7 ++- .../cmd/service_bind_collision_test.go | 11 +++-- cmd/spinifex/cmd/templates/predastore.toml | 2 +- cmd/spinifex/cmd/viperblock_region_test.go | 48 +++++++++++++++++++ 4 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 cmd/spinifex/cmd/viperblock_region_test.go diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index 18879d2ee..a053cf9c9 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -1301,9 +1301,12 @@ func init() { serviceCmd.AddCommand(viperblockCmd) + // These override spinifex.toml only when set, and the start command tests them + // for emptiness to decide that. A non-empty default would make the override + // unconditional, discarding the configured value on every start. viperblockCmd.PersistentFlags().String("s3-host", "", "Predastore (S3) host URI") - viperblockCmd.PersistentFlags().String("s3-bucket", "predastore", "Predastore (S3) bucket") - viperblockCmd.PersistentFlags().String("s3-region", "ap-southeast-2", "Predastore (S3) region") + viperblockCmd.PersistentFlags().String("s3-bucket", "", "Predastore (S3) bucket") + viperblockCmd.PersistentFlags().String("s3-region", "", "Predastore (S3) region") viperblockCmd.PersistentFlags().String("plugin-path", "/opt/spinifex/lib/nbdkit-viperblock-plugin.so", "Pathname to the nbdkit viperblockplugin") bindViperblockEnv() diff --git a/cmd/spinifex/cmd/service_bind_collision_test.go b/cmd/spinifex/cmd/service_bind_collision_test.go index ffb1eae74..890790b05 100644 --- a/cmd/spinifex/cmd/service_bind_collision_test.go +++ b/cmd/spinifex/cmd/service_bind_collision_test.go @@ -129,16 +129,19 @@ func TestViperblockFlagsLookUpTheirOwnCommand(t *testing.T) { } // TestViperblockFlagDefaultsReachViper drives the real bindViperblockEnv, so -// a lookup pointed at the wrong command fails here: an unbound flag leaves -// its default invisible and s3-bucket resolves "" rather than "predastore". +// a lookup pointed at the wrong command fails here: an unbound flag leaves its +// default invisible and plugin-path resolves "" rather than the install path. +// +// The s3-* flags override spinifex.toml and so must default empty, which leaves +// plugin-path as the only default that can prove the binding. func TestViperblockFlagDefaultsReachViper(t *testing.T) { t.Cleanup(func() { viper.Reset() }) viper.Reset() bindViperblockEnv() - assert.Equal(t, "predastore", viper.GetString("s3-bucket")) - assert.Equal(t, "ap-southeast-2", viper.GetString("s3-region")) assert.Equal(t, "/opt/spinifex/lib/nbdkit-viperblock-plugin.so", viper.GetString("plugin-path")) + assert.Empty(t, viper.GetString("s3-bucket")) + assert.Empty(t, viper.GetString("s3-region")) } // TestViperblockEnvBeatsUnchangedFlagDefault pins viper's precedence: wiring diff --git a/cmd/spinifex/cmd/templates/predastore.toml b/cmd/spinifex/cmd/templates/predastore.toml index dec28e5ac..3adee8288 100644 --- a/cmd/spinifex/cmd/templates/predastore.toml +++ b/cmd/spinifex/cmd/templates/predastore.toml @@ -66,7 +66,7 @@ role = "state-replica" [[buckets]] name = "predastore" -region = "ap-southeast-2" +region = "{{.Region}}" type = "distributed" public = false encryption = "" diff --git a/cmd/spinifex/cmd/viperblock_region_test.go b/cmd/spinifex/cmd/viperblock_region_test.go new file mode 100644 index 000000000..d9648424f --- /dev/null +++ b/cmd/spinifex/cmd/viperblock_region_test.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "testing" + + toml "github.com/pelletier/go-toml/v2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// The viperblock start command applies each of these over spinifex.toml whenever +// viper reports a non-empty value, so a non-empty flag default makes the override +// unconditional and discards the configured value on every start. +func TestViperblockConfigOverrideFlagsDefaultEmpty(t *testing.T) { + t.Parallel() + + for _, name := range []string{"s3-host", "s3-bucket", "s3-region"} { + flag := viperblockCmd.PersistentFlags().Lookup(name) + require.NotNil(t, flag, "flag %s must exist", name) + assert.Empty(t, flag.DefValue, "flag %s must default empty so an unset flag cannot clobber spinifex.toml", name) + } +} + +// predastore verifies each request's SigV4 credential scope against its own +// configured region, so a bucket pinned to a literal region breaks every client +// on a cluster deployed anywhere else. +func TestPredastoreTemplateBucketsUseConfiguredRegion(t *testing.T) { + t.Parallel() + + settings := northstarSettings() + settings.Region = "bare-metal" + content := renderTemplate(t, predastoreTomlTemplate, settings) + + var parsed struct { + Region string `toml:"region"` + Buckets []struct { + Name string `toml:"name"` + Region string `toml:"region"` + } `toml:"buckets"` + } + require.NoError(t, toml.Unmarshal([]byte(content), &parsed)) + + assert.Equal(t, "bare-metal", parsed.Region) + require.NotEmpty(t, parsed.Buckets) + for _, bucket := range parsed.Buckets { + assert.Equal(t, "bare-metal", bucket.Region, "bucket %s", bucket.Name) + } +}