Skip to content

Commit 368c0ec

Browse files
leehinmanCopilot
andauthored
make creation of meta.json file atomic (#51791)
* make creation of meta.json file atomic * improve changelog summary Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent 670afa9 commit 368c0ec

3 files changed

Lines changed: 151 additions & 15 deletions

File tree

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
# REQUIRED
2+
# Kind can be one of:
3+
# - breaking-change: a change to previously-documented behavior
4+
# - deprecation: functionality that is being removed in a later release
5+
# - bug-fix: fixes a problem in a previous version
6+
# - enhancement: extends functionality but does not break or fix existing behavior
7+
# - feature: new functionality
8+
# - known-issue: problems that we are aware of in a given version
9+
# - security: impacts on the security of a product or a user’s deployment.
10+
# - upgrade: important information for someone upgrading from a prior version
11+
# - other: does not fit into any of the other categories
12+
kind: bug-fix
13+
14+
# REQUIRED for all kinds
15+
# Change summary; a 80ish characters long description of the change.
16+
summary: Prevent Filebeat startup failure when meta.json is left empty after migration
17+
18+
# REQUIRED for breaking-change, deprecation, known-issue
19+
# Long description; in case the summary is not enough to describe the change
20+
# this field accommodate a description without length limits.
21+
# description:
22+
23+
# REQUIRED for breaking-change, deprecation, known-issue
24+
# impact:
25+
26+
# REQUIRED for breaking-change, deprecation, known-issue
27+
# action:
28+
29+
# REQUIRED for all kinds
30+
# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
31+
component: filebeat
32+
33+
# AUTOMATED
34+
# OPTIONAL to manually add other PR URLs
35+
# PR URL: A link the PR that added the changeset.
36+
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
37+
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
38+
# Please provide it if you are adding a fragment for a different PR.
39+
pr: https://github.com/elastic/beats/pull/51791
40+
41+
# AUTOMATED
42+
# OPTIONAL to manually add other issue URLs
43+
# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
44+
# If not present is automatically filled by the tooling with the issue linked to the PR number.
45+
# issue: https://github.com/owner/repo/1234

filebeat/registrar/migrate.go

Lines changed: 24 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ func (m *Migrator) updateToVersion1(regHome string) error {
241241
return fmt.Errorf("migration complete but failed to remove original data file: %v: %w", origDataFile, err)
242242
}
243243

244-
if err := os.WriteFile(filepath.Join(regHome, "meta.json"), []byte(`{"version": "1"}`), m.permissions); err != nil {
244+
if err := safeWriteFile(filepath.Join(regHome, "meta.json"), []byte(`{"version": "1"}`), m.permissions); err != nil {
245245
return fmt.Errorf("failed to update the meta.json file: %w", err)
246246
}
247247

@@ -290,29 +290,38 @@ func isFile(path string, log *logp.Logger) bool {
290290
}
291291

292292
func safeWriteFile(path string, data []byte, perm os.FileMode) error {
293-
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm)
293+
f, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".tmp-*")
294294
if err != nil {
295-
return err
295+
return fmt.Errorf("error creating tmp file: %w", err)
296+
}
297+
tmpName := f.Name()
298+
// Always close and remove the tmp file on error path, errors will be ignored
299+
// in the success case
300+
defer func() {
301+
_ = f.Close()
302+
_ = os.Remove(tmpName)
303+
}()
304+
305+
if err = f.Chmod(perm); err != nil {
306+
return fmt.Errorf("error setting tmp file permissions: %w", err)
296307
}
297308

298-
for len(data) > 0 {
299-
var n int
300-
n, err = f.Write(data)
301-
if err != nil {
302-
break
303-
}
309+
if _, err = f.Write(data); err != nil {
310+
return fmt.Errorf("error writing to tmp file: %w", err)
311+
}
304312

305-
data = data[n:]
313+
if err = f.Sync(); err != nil {
314+
return fmt.Errorf("error syncing data to tmp file: %w", err)
306315
}
307316

308-
if err == nil {
309-
err = f.Sync()
317+
if err = f.Close(); err != nil {
318+
return fmt.Errorf("error closing tmp file: %w", err)
310319
}
311320

312-
if err1 := f.Close(); err == nil {
313-
err = err1
321+
if err = helper.SafeFileRotate(path, tmpName); err != nil {
322+
return fmt.Errorf("error moving tmp file to target: %w", err)
314323
}
315-
return err
324+
return nil
316325
}
317326

318327
// fixStates cleans up the registry states when updating from an older version

filebeat/registrar/migrate_test.go

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
// Licensed to Elasticsearch B.V. under one or more contributor
2+
// license agreements. See the NOTICE file distributed with
3+
// this work for additional information regarding copyright
4+
// ownership. Elasticsearch B.V. licenses this file to you under
5+
// the Apache License, Version 2.0 (the "License"); you may
6+
// not use this file except in compliance with the License.
7+
// You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
//go:build linux || darwin
19+
20+
package registrar
21+
22+
import (
23+
"os"
24+
"path/filepath"
25+
"testing"
26+
27+
"github.com/stretchr/testify/assert"
28+
"github.com/stretchr/testify/require"
29+
)
30+
31+
func TestSafeWriteFile(t *testing.T) {
32+
data := []byte(`{"version": "1"}`)
33+
perm := os.FileMode(0o600)
34+
35+
t.Run("success", func(t *testing.T) {
36+
dir := tempDir(t)
37+
path := filepath.Join(dir, "meta.json")
38+
39+
require.NoError(t, safeWriteFile(path, data, perm))
40+
41+
got, err := os.ReadFile(path)
42+
require.NoError(t, err)
43+
assert.Equal(t, data, got)
44+
45+
fi, err := os.Stat(path)
46+
require.NoError(t, err)
47+
assert.Equal(t, perm, fi.Mode().Perm(), "file permissions")
48+
49+
assertNoTempFiles(t, dir)
50+
})
51+
52+
t.Run("parent_dir_missing", func(t *testing.T) {
53+
// os.CreateTemp fails because the directory does not exist.
54+
path := filepath.Join(tempDir(t), "nonexistent", "meta.json")
55+
56+
err := safeWriteFile(path, data, perm)
57+
require.Error(t, err)
58+
})
59+
60+
t.Run("rotate_fails_cleans_up_temp_file", func(t *testing.T) {
61+
dir := tempDir(t)
62+
// Place a directory at the target path so os.Rename returns EISDIR,
63+
// forcing SafeFileRotate to fail after the temp file has been written.
64+
path := filepath.Join(dir, "meta.json")
65+
mkDir(t, path)
66+
67+
err := safeWriteFile(path, data, perm)
68+
require.Error(t, err)
69+
70+
// The temp file must not be left behind despite the rotate failure.
71+
assertNoTempFiles(t, dir)
72+
})
73+
}
74+
75+
// assertNoTempFiles fails the test if any temp files from safeWriteFile remain
76+
// in dir. The naming pattern is <base>.tmp-<random>.
77+
func assertNoTempFiles(t *testing.T, dir string) {
78+
t.Helper()
79+
matches, err := filepath.Glob(filepath.Join(dir, "*.tmp-*"))
80+
require.NoError(t, err)
81+
assert.Empty(t, matches, "unexpected temp files in %s", dir)
82+
}

0 commit comments

Comments
 (0)