diff --git a/internal/config/alt/update.go b/internal/config/alt/update.go index c427efdd..6f38fd3f 100644 --- a/internal/config/alt/update.go +++ b/internal/config/alt/update.go @@ -69,11 +69,18 @@ func Update(ctx context.Context, cnf *config.Config, debugLog func(fmt string, i return nil } if newCnfStruct.Metadata.Version != "" { - cmp, err := version.Compare(cnf.Metadata.Version, newCnfStruct.Metadata.Version) - if err != nil { + // An unusable local version cannot say whether the new config is newer, so + // treat the config as out of date. Returning an error would be permanent: it + // happens before the file is rewritten, so the version stays unusable. + if !version.Validate(cnf.Metadata.Version) { + debugLog("Ignoring unusable local config version %q", cnf.Metadata.Version) + } else if cmp, err := version.Compare( + cnf.Metadata.Version, newCnfStruct.Metadata.Version, + ); err != nil { + // Only the new version can fail to parse here, and FetchConfig validates + // it first. Do not apply a config whose version cannot be compared. return fmt.Errorf("could not compare config versions: %w", err) - } - if cmp >= 0 { + } else if cmp >= 0 { debugLog("Config is already up to date (version %s)", cnf.Metadata.Version) return nil } diff --git a/internal/config/alt/update_test.go b/internal/config/alt/update_test.go index ff810da8..30008b6b 100644 --- a/internal/config/alt/update_test.go +++ b/internal/config/alt/update_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strconv" "testing" "time" @@ -30,8 +31,7 @@ func TestUpdate(t *testing.T) { require.NoError(t, err) // Set up state so that it stays in a temporary directory. - err = os.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) - require.NoError(t, err) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) // Set up the config to be updated via a test HTTP server. remoteConfig := testConfig @@ -86,9 +86,12 @@ func TestUpdate(t *testing.T) { resetTimes() remoteConfig = append(remoteConfig, []byte("\nmetadata: {version: 1.0.1}")...) + // A local version that cannot be parsed says nothing about whether the new + // config is newer, so the update proceeds rather than failing. cnf.Metadata.Version = "invalid" err = alt.Update(ctx, cnf, logger) - assert.ErrorContains(t, err, "could not compare config versions") + assert.NoError(t, err) + assert.Contains(t, lastLogged, "Automatically updated config file") resetTimes() cnf.Metadata.Version = "1.0.1" err = alt.Update(ctx, cnf, logger) @@ -130,3 +133,85 @@ func TestShouldUpdate(t *testing.T) { cnf.Metadata.URL = "" assert.False(t, alt.ShouldUpdate(cnf)) } + +// testConfigWithMetadata copies the test config and appends a metadata section. +// It copies because testConfig is shared by every test in the package. +func testConfigWithMetadata(metadata string) []byte { + cnf := make([]byte, 0, len(testConfig)+len("\nmetadata: ")+len(metadata)) + cnf = append(cnf, testConfig...) + cnf = append(cnf, "\nmetadata: "...) + return append(cnf, metadata...) +} + +// TestUpdateWithUnusableLocalVersion checks that a config with a URL but no +// usable version is still updated, which is what clears the bad version. +func TestUpdateWithUnusableLocalVersion(t *testing.T) { + for _, localVersion := range []string{"", "invalid", "1.2.3.4"} { + t.Run("local version "+strconv.Quote(localVersion), func(t *testing.T) { + tempDir := t.TempDir() + testConfigFilename := filepath.Join(tempDir, "config.yaml") + require.NoError(t, os.WriteFile(testConfigFilename, testConfig, 0o600)) + hourAgo := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo)) + + cnf, err := config.FromYAML(testConfig) + require.NoError(t, err) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) + + remoteConfig := testConfigWithMetadata("{version: 1.0.1}") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(remoteConfig) + })) + defer server.Close() + + cnf.SourceFile = testConfigFilename + cnf.Metadata.URL = server.URL + "/config.yaml" + cnf.Metadata.Version = localVersion + + var lastLogged string + err = alt.Update(config.ToContext(context.Background(), cnf), cnf, + func(msg string, args ...any) { lastLogged = fmt.Sprintf(msg, args...) }) + assert.NoError(t, err) + assert.Contains(t, lastLogged, "Automatically updated config file") + + // The new version is now on disk, so the next run compares cleanly. + b, err := os.ReadFile(testConfigFilename) + require.NoError(t, err) + updated, err := config.FromYAML(b) + require.NoError(t, err) + assert.Equal(t, "1.0.1", updated.Metadata.Version) + }) + } +} + +// TestUpdateWithUnusableNewVersion checks that a served config whose version +// will not parse is rejected while fetching and never applied. +func TestUpdateWithUnusableNewVersion(t *testing.T) { + tempDir := t.TempDir() + testConfigFilename := filepath.Join(tempDir, "config.yaml") + localConfig := testConfigWithMetadata("{version: 1.0.0}") + require.NoError(t, os.WriteFile(testConfigFilename, localConfig, 0o600)) + hourAgo := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(testConfigFilename, hourAgo, hourAgo)) + + cnf, err := config.FromYAML(localConfig) + require.NoError(t, err) + require.Equal(t, "1.0.0", cnf.Metadata.Version) + t.Setenv(cnf.Application.EnvPrefix+"HOME", tempDir) + + remoteConfig := testConfigWithMetadata("{version: not-a-version}") + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(remoteConfig) + })) + defer server.Close() + + cnf.SourceFile = testConfigFilename + cnf.Metadata.URL = server.URL + "/config.yaml" + + err = alt.Update(config.ToContext(context.Background(), cnf), cnf, func(string, ...any) {}) + assert.ErrorContains(t, err, "invalid config") + + b, err := os.ReadFile(testConfigFilename) + require.NoError(t, err) + assert.Equal(t, string(localConfig), string(b), "the local config must not be modified") +}