receiver, compactor, sidecar: use os.Root API#8797
Conversation
a662f3e to
866be3e
Compare
866be3e to
8fd088c
Compare
| return errors.Wrapf(err, "create data dir in %v", conf.dataDir) | ||
| } | ||
|
|
||
| dataDir, err := os.OpenRoot(conf.dataDir) |
There was a problem hiding this comment.
Why not have this in createDefaultTenantTSDB itself?
There was a problem hiding this comment.
dataDir is opened once and used in two places: createDefaultTenantTSDB for the default tenant dir and then handed to the long-living MultiTSDB that uses it for all filesystem interactions.
The alternative I see is to create a Root short-lived object for every dataDir operation; if that's not the intention, a dataDir is created once and used repetedly
There was a problem hiding this comment.
My understanding of the os.Root API is that is is intended to be used as a long-lived, concurrency-safe file descriptor, so you don't have to check the dir validity after the initial open
| } | ||
| }() | ||
|
|
||
| dataDir, err := os.OpenRoot(conf.dataDir) |
There was a problem hiding this comment.
Same here, why not create this in shipper itself?
There was a problem hiding this comment.
Similar to before, even if the root object is only consumed by the shipper: if the intention is to have a long-living Root reference, it is better to create it at the caller level. Moving it to the shipper would mean to change the New(...) func signature to handle the error or recreate the Root when needed.
There was a problem hiding this comment.
But if the old conf.dataDir is passed then we wouldn't have to make any changes to shipper.New() because it would accept the same params, no?
There was a problem hiding this comment.
If we want to have a "reusable" os.Root, we would need to change Shipper.dir from string to os.Root; doing so in the New() func would require the function to return an error.
If we keep Shipper.dir a path string as it is, we would need to instantiate an os.Root on the fly when needed e.g. blockMetasFromOldest() and upload(), which breaks the long-lived os.Root object I thought we wanted to implement. Not a big issue, just wanted to clarify pros and cons of the two approaches
|
The TODO on the |
|
@guidonguido are you still interested in continuing this PR? |
|
Hello @GiedriusS. Yes, I missed the review notifications. I'll follow-up the review in the following days. Sorry for the delay |
db1927b to
6733bf8
Compare
| } | ||
| }() | ||
|
|
||
| dataDir, err := os.OpenRoot(conf.dataDir) |
There was a problem hiding this comment.
But if the old conf.dataDir is passed then we wouldn't have to make any changes to shipper.New() because it would accept the same params, no?
6733bf8 to
10069f3
Compare
|
Edited the minor changes using the logging and error utilities for close operations. The discussion on where to instantiate the os.Root object(s) is still open |
10069f3 to
1a829eb
Compare
GiedriusS
left a comment
GiedriusS
left a comment
There was a problem hiding this comment.
I think let's just merge this as is because it doesn't make much of a difference in the end. And, of course, to avoid bikeshedding. If we ever feel in the future that we need to abstract it away in the shipper then we can do that change. Adding a few comments. Let's always use the helpers with closers.
Signed-off-by: Guido Ricioppo <[email protected]> receive e2e: close mtsdb datadir on object release Signed-off-by: Guido Ricioppo <[email protected]> Fix tests after rebase Signed-off-by: Guido Ricioppo <[email protected]>
Signed-off-by: Guido Ricioppo <[email protected]> Refactor compactor os.Root lifetime Signed-off-by: Guido Ricioppo <[email protected]>
Signed-off-by: Guido Ricioppo <[email protected]>
Signed-off-by: Guido Ricioppo <[email protected]> Fix doc error Signed-off-by: Guido Ricioppo <[email protected]>
Signed-off-by: Guido Ricioppo <[email protected]>
1a829eb to
4779eb8
Compare
|
@GiedriusS checked all FDs are closed now using helpers: using |
4 participants
Fixes #8103
Issue
Current os filesystem access methods do not prevent accidental path traversal access, so directory operations based on user input may access unintended paths.
Changes
Force the use of os.Root to confine access to subdirectories only to the root working folder of the specific service.
Approach
Receiver, Sidecar: long-lived os.Root. The data directory persists for the entire service lifetime, so the Root is stored in the owning struct.
Compactor: transient os.Root. The compaction dir is fully removed after each compaction iteration, so the Root dir is opened on-demand.
Analysis
Receiver: real protection from user input, since the tenants directory paths are retrieved from the tenant IDs, derived either from HTTP Header, TLS cert or metric label.
Compactor: no path is composed from user input. The change only prevents accidental use on the codebase side.
Sidecar: as compactor.