Skip to content

Commit bff7666

Browse files
jetstack-botjetstack-bot
authored
Merge pull request #65 from inteon/update_atomic_writer
Update atomic writer based on upstream: avoiding chown race condition
2 parents 3ba2694 + 1f84151 commit bff7666

File tree

3 files changed

+230
-81
lines changed

3 files changed

+230
-81
lines changed

‎storage/filesystem.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -258,23 +258,23 @@ func (f *Filesystem) WriteFiles(meta metadata.Metadata, files map[string][]byte)
258258
}
259259

260260
payload := makePayload(files)
261-
if err := writer.Write(payload); err != nil {
262-
return err
263-
}
264-
265-
// If a fsGroup is defined, Chown all files just written.
266-
if fsGroup != nil {
267-
// "..data" is the well-known location where the atomic writer links to the
268-
// latest directory containing the files just written. Chown these real
269-
// files.
270-
dirName := filepath.Join(f.dataPathForVolumeID(meta.VolumeID), "..data")
261+
setPerms := func(tsDirName string) error {
262+
if fsGroup == nil {
263+
return nil
264+
}
271265

266+
// If a fsGroup is defined, Chown all files in the timestamped directory.
272267
for filename := range files {
273268
// Set the uid to -1 which means don't change ownership in Go.
274-
if err := os.Chown(filepath.Join(dirName, filename), -1, int(*fsGroup)); err != nil {
269+
if err := os.Chown(filepath.Join(tsDirName, filename), -1, int(*fsGroup)); err != nil {
275270
return err
276271
}
277272
}
273+
274+
return nil
275+
}
276+
if err := writer.Write(payload, setPerms); err != nil {
277+
return err
278278
}
279279

280280
return nil

‎third_party/util/atomic_writer.go‎

Lines changed: 96 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,21 @@
11
/*
22
Copyright 2016 The Kubernetes Authors.
3+
34
Licensed under the Apache License, Version 2.0 (the "License");
45
you may not use this file except in compliance with the License.
56
You may obtain a copy of the License at
7+
68
http://www.apache.org/licenses/LICENSE-2.0
9+
710
Unless required by applicable law or agreed to in writing, software
811
distributed under the License is distributed on an "AS IS" BASIS,
912
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
1013
See the License for the specific language governing permissions and
1114
limitations under the License.
1215
*/
1316

17+
// copied from https://raw.githubusercontent.com/kubernetes/kubernetes/20d0ab7ae808aaddb1556c3c38ca0607663c50ac/pkg/volume/util/atomic_writer.go
18+
1419
package util
1520

1621
import (
@@ -82,6 +87,10 @@ const (
8287

8388
// Write does an atomic projection of the given payload into the writer's target
8489
// directory. Input paths must not begin with '..'.
90+
// setPerms is an optional pointer to a function that caller can provide to set the
91+
// permissions of the newly created files before they are published. The function is
92+
// passed subPath which is the name of the timestamped directory that was created
93+
// under target directory.
8594
//
8695
// The Write algorithm is:
8796
//
@@ -94,37 +103,42 @@ const (
94103
// portion of the payload was deleted and is still present on disk.
95104
//
96105
// 4. The data in the current timestamped directory is compared to the projected
97-
// data to determine if an update is required.
106+
// data to determine if an update to data directory is required.
98107
//
99-
// 5. A new timestamped dir is created
108+
// 5. A new timestamped dir is created if an update is required.
100109
//
101-
// 6. The payload is written to the new timestamped directory
110+
// 6. The payload is written to the new timestamped directory.
111+
//
112+
// 7. Permissions are set (if setPerms is not nil) on the new timestamped directory and files.
113+
//
114+
// 8. A symlink to the new timestamped directory ..data_tmp is created that will
115+
// become the new data directory.
102116
//
103-
// 7. Symlinks and directory for new user-visible files are created (if needed).
117+
// 9. The new data directory symlink is renamed to the data directory; rename is atomic.
118+
//
119+
// 10. Symlinks and directory for new user-visible files are created (if needed).
104120
//
105121
// For example, consider the files:
106122
// <target-dir>/podName
107123
// <target-dir>/user/labels
108124
// <target-dir>/k8s/annotations
109125
//
110126
// The user visible files are symbolic links into the internal data directory:
111-
// <target-dir>/podName -> ..data/podName
112-
// <target-dir>/usr -> ..data/usr
113-
// <target-dir>/k8s -> ..data/k8s
127+
// <target-dir>/podName -> ..data/podName
128+
// <target-dir>/usr -> ..data/usr
129+
// <target-dir>/k8s -> ..data/k8s
114130
//
115131
// The data directory itself is a link to a timestamped directory with
116132
// the real data:
117-
// <target-dir>/..data -> ..2016_02_01_15_04_05.12345678/
118-
//
119-
// 8. A symlink to the new timestamped directory ..data_tmp is created that will
120-
// become the new data directory
133+
// <target-dir>/..data -> ..2016_02_01_15_04_05.12345678/
134+
// NOTE(claudiub): We need to create these symlinks AFTER we've finished creating and
135+
// linking everything else. On Windows, if a target does not exist, the created symlink
136+
// will not work properly if the target ends up being a directory.
121137
//
122-
// 9. The new data directory symlink is renamed to the data directory; rename is atomic
138+
// 11. Old paths are removed from the user-visible portion of the target directory.
123139
//
124-
// 10. Old paths are removed from the user-visible portion of the target directory
125-
//
126-
// 11. The previous timestamped directory is removed, if it exists
127-
func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
140+
// 12. The previous timestamped directory is removed, if it exists.
141+
func (w *AtomicWriter) Write(payload map[string]FileProjection, setPerms func(subPath string) error) error {
128142
// (1)
129143
cleanPayload, err := validatePayload(payload)
130144
if err != nil {
@@ -147,6 +161,7 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
147161
oldTsPath := filepath.Join(w.targetDir, oldTsDir)
148162

149163
var pathsToRemove sets.Set[string]
164+
shouldWrite := true
150165
// if there was no old version, there's nothing to remove
151166
if len(oldTsDir) != 0 {
152167
// (3)
@@ -161,64 +176,89 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
161176
klog.Errorf("%s: error determining whether payload should be written to disk: %v", w.logContext, err)
162177
return err
163178
} else if !should && len(pathsToRemove) == 0 {
164-
klog.V(4).Infof("%s: no update required for target directory %v", w.logContext, w.targetDir)
165-
return nil
179+
klog.V(4).Infof("%s: write not required for data directory %v", w.logContext, oldTsDir)
180+
// data directory is already up to date, but we need to make sure that
181+
// the user-visible symlinks are created.
182+
// See https://github.com/kubernetes/kubernetes/issues/121472 for more details.
183+
// Reset oldTsDir to empty string to avoid removing the data directory.
184+
shouldWrite = false
185+
oldTsDir = ""
166186
} else {
167187
klog.V(4).Infof("%s: write required for target directory %v", w.logContext, w.targetDir)
168188
}
169189
}
170190

171-
// (5)
172-
tsDir, err := w.newTimestampDir()
173-
if err != nil {
174-
klog.V(4).Infof("%s: error creating new ts data directory: %v", w.logContext, err)
175-
return err
176-
}
177-
tsDirName := filepath.Base(tsDir)
191+
if shouldWrite {
192+
// (5)
193+
tsDir, err := w.newTimestampDir()
194+
if err != nil {
195+
klog.V(4).Infof("%s: error creating new ts data directory: %v", w.logContext, err)
196+
return err
197+
}
198+
tsDirName := filepath.Base(tsDir)
178199

179-
// (6)
180-
if err = w.writePayloadToDir(cleanPayload, tsDir); err != nil {
181-
klog.Errorf("%s: error writing payload to ts data directory %s: %v", w.logContext, tsDir, err)
182-
return err
183-
}
184-
klog.V(4).Infof("%s: performed write of new data to ts data directory: %s", w.logContext, tsDir)
200+
// (6)
201+
if err = w.writePayloadToDir(cleanPayload, tsDir); err != nil {
202+
klog.Errorf("%s: error writing payload to ts data directory %s: %v", w.logContext, tsDir, err)
203+
return err
204+
}
205+
klog.V(4).Infof("%s: performed write of new data to ts data directory: %s", w.logContext, tsDir)
185206

186-
// (7)
187-
if err = w.createUserVisibleFiles(cleanPayload); err != nil {
188-
klog.Errorf("%s: error creating visible symlinks in %s: %v", w.logContext, w.targetDir, err)
189-
return err
190-
}
207+
// (7)
208+
if setPerms != nil {
209+
if err := setPerms(tsDirName); err != nil {
210+
klog.Errorf("%s: error applying ownership settings: %v", w.logContext, err)
211+
return err
212+
}
213+
}
191214

192-
// (8)
193-
newDataDirPath := filepath.Join(w.targetDir, newDataDirName)
194-
if err = os.Symlink(tsDirName, newDataDirPath); err != nil {
195-
os.RemoveAll(tsDir)
196-
klog.Errorf("%s: error creating symbolic link for atomic update: %v", w.logContext, err)
197-
return err
198-
}
215+
// (8)
216+
newDataDirPath := filepath.Join(w.targetDir, newDataDirName)
217+
if err = os.Symlink(tsDirName, newDataDirPath); err != nil {
218+
if err := os.RemoveAll(tsDir); err != nil {
219+
klog.Errorf("%s: error removing new ts directory %s: %v", w.logContext, tsDir, err)
220+
}
221+
klog.Errorf("%s: error creating symbolic link for atomic update: %v", w.logContext, err)
222+
return err
223+
}
199224

200-
// (9)
201-
if runtime.GOOS == "windows" {
202-
os.Remove(dataDirPath)
203-
err = os.Symlink(tsDirName, dataDirPath)
204-
os.Remove(newDataDirPath)
205-
} else {
206-
err = os.Rename(newDataDirPath, dataDirPath)
225+
// (9)
226+
if runtime.GOOS == "windows" {
227+
if err := os.Remove(dataDirPath); err != nil {
228+
klog.Errorf("%s: error removing data dir directory %s: %v", w.logContext, dataDirPath, err)
229+
}
230+
err = os.Symlink(tsDirName, dataDirPath)
231+
if err := os.Remove(newDataDirPath); err != nil {
232+
klog.Errorf("%s: error removing new data dir directory %s: %v", w.logContext, newDataDirPath, err)
233+
}
234+
} else {
235+
err = os.Rename(newDataDirPath, dataDirPath)
236+
}
237+
if err != nil {
238+
if err := os.Remove(newDataDirPath); err != nil && err != os.ErrNotExist {
239+
klog.Errorf("%s: error removing new data dir directory %s: %v", w.logContext, newDataDirPath, err)
240+
}
241+
if err := os.RemoveAll(tsDir); err != nil {
242+
klog.Errorf("%s: error removing new ts directory %s: %v", w.logContext, tsDir, err)
243+
}
244+
klog.Errorf("%s: error renaming symbolic link for data directory %s: %v", w.logContext, newDataDirPath, err)
245+
return err
246+
}
207247
}
208-
if err != nil {
209-
os.Remove(newDataDirPath)
210-
os.RemoveAll(tsDir)
211-
klog.Errorf("%s: error renaming symbolic link for data directory %s: %v", w.logContext, newDataDirPath, err)
248+
249+
// (10)
250+
if err = w.createUserVisibleFiles(cleanPayload); err != nil {
251+
klog.Errorf("%s: error creating visible symlinks in %s: %v", w.logContext, w.targetDir, err)
212252
return err
213253
}
214254

215-
// (10)
255+
// (11)
216256
if err = w.removeUserVisiblePaths(pathsToRemove); err != nil {
217257
klog.Errorf("%s: error removing old visible symlinks: %v", w.logContext, err)
218258
return err
219259
}
220260

221-
// (11)
261+
// (12)
222262
if len(oldTsDir) > 0 {
223263
if err = os.RemoveAll(oldTsPath); err != nil {
224264
klog.Errorf("%s: error removing old data directory %s: %v", w.logContext, oldTsDir, err)

0 commit comments

Comments
 (0)