filestorageextension: fix panic when configured directory cannot be accessed #6103
Conversation
|
It's a bit problematic to add a UT since that would require us to juggle the local file system and permissions to induce a situation described in the description but if that's what we'd want I can try to work on it. |
| info, err := os.Stat(config.Directory) | ||
| if (err != nil && os.IsNotExist(err)) || !info.IsDir() { | ||
| return nil, fmt.Errorf("directory must exist: %v", err) | ||
| if err != nil { | ||
| if os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("directory must exist: %v", err) | ||
| } | ||
| if fsErr, ok := err.(*fs.PathError); ok { | ||
| return nil, fmt.Errorf( | ||
| "problem accessing configured directory: %s, err: %v", | ||
| config.Directory, fsErr, | ||
| ) | ||
| } | ||
|
|
||
| } | ||
| if !info.IsDir() { | ||
| return nil, fmt.Errorf("%s is not a directory: %v", config.Directory, err) | ||
| } |
There was a problem hiding this comment.
May want to have this in "validate" func on the config.
There was a problem hiding this comment.
I've changed it but that required more changes in tests because now the default config might not pass on machines that don't have the default dir created (hence my changes in this PR overriding the default in some tests).
There was a problem hiding this comment.
Any chance that we revisit this?
It seems that load-tests / loadtest (TestBallastMemory|TestLog10kDPS) (pull_request) failed for some reason but I wouldn't connect it to this PR.
pmalek-sumo
force-pushed
the
fix-filestorage-extension-when-dir-cannot-be-accessed
branch
from
6ac822c to
05059e6
Compare
|
@codeboten @bogdandrutu can we revisit this and merge it? All the tests pass now and I've changed the code structure to add this check to validate func as per : #6103 (comment) |
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
This is not stale. |
There was a problem hiding this comment.
@bogdandrutu it appears your comment has been address, PTAL
| } | ||
|
|
||
| } | ||
| if !info.IsDir() { |
There was a problem hiding this comment.
Is a test covering this case?
There was a problem hiding this comment.
I've added an appropriate testcase, @codeboten PTAL
|
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
|
@bogdandrutu, this seems to be pending on you. |
pmalek-sumo
force-pushed
the
fix-filestorage-extension-when-dir-cannot-be-accessed
branch
from
05059e6 to
51493c8
Compare
pmalek-sumo
force-pushed
the
fix-filestorage-extension-when-dir-cannot-be-accessed
branch
from
51493c8 to
dbebd65
Compare
|
Unrelated UT failure https://github.com/open-telemetry/opentelemetry-collector-contrib/runs/4366230936?check_suite_focus=true Seems there's a fix waiting to be merged: #6461 |
|
@pmalek-sumo, could you please rebase this one? I merged the PR fixing main. |
pmalek-sumo
force-pushed
the
fix-filestorage-extension-when-dir-cannot-be-accessed
branch
from
dbebd65 to
d45068b
Compare
Done |
|
Looks good to me. I'm merging, as I believe @bogdandrutu had enough time to comment on this :-) If there are still concerns left, it can be addressed in a future PR. |
pmalek-sumo
deleted the
fix-filestorage-extension-when-dir-cannot-be-accessed
branch
…ccessed (open-telemetry#6103) * filestorageextension: fix panic when configured directory cannot be accessed * filestorageextension: move validation to config.Validate()
Description: When a configured directory cannot be accessed (due to *nix permissions)
erris of type*fs.PathErrorandinfoisnilwhich causes a panic when callinginfo.IsDir()hence check for that scenario.Link to tracking Issue: N/A
Testing: Manual testing done.
Documentation: N/A