c8y-configuration-plugin panics when required folder/s are missing #1963
Conversation
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
Robot Results
Passed Tests
|
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| if !Path::new(config_dir).is_file() { | ||
| anyhow::bail!( | ||
| "The configuration file {} does not exists", | ||
| config_dir.display() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why not just that check?
There was a problem hiding this comment.
There might be some times the directory exists but not the config file. So, wanted to be specific.
There was a problem hiding this comment.
We are both incorrect ;-) After some checks, it appears that the directory must exist but not necessarily the configuration file.
There was a problem hiding this comment.
When I moved the check inside ConfigManagerConfig::from_tedge_config() felt that it was better just to check the file.
There was a problem hiding this comment.
Yes but it's okay to have no configuration : the defaults will apply. So one needs to check only the directory.
| let config_manager_config = | ||
| ConfigManagerConfig::from_tedge_config(DEFAULT_TEDGE_CONFIG_PATH, &tedge_config)?; | ||
|
|
||
| validate_config_dir_and_file(config_manager_config.plugin_config_path.as_path())?; |
There was a problem hiding this comment.
My feeling is that checking the existence of the config dir would be simpler and more natural if done inside ConfigManagerConfig::from_tedge_config() because there you will have the path to the conf directory and not to the configuration file.
There was a problem hiding this comment.
moved check inside the ConfigManagerConfig::from_tedge_config()
| match config_dir.parent() { | ||
| Some(cdir) => { | ||
| if !Path::new(cdir).is_dir() { | ||
| anyhow::bail!( | ||
| "The configuration directory {} does not exists", | ||
| cdir.display() | ||
| ); | ||
| } else { | ||
| if !Path::new(config_dir).is_file() { | ||
| anyhow::bail!( | ||
| "The configuration file {} does not exists", | ||
| config_dir.display() | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } | ||
| } | ||
| None => { | ||
| anyhow::bail!("The configuration directory does not exists"); | ||
| } | ||
| } |
There was a problem hiding this comment.
If /etc/tedge or similar that user configured does not exist, I think it's enough reason to quit the process. So, I don't think we need to check the parent.
There was a problem hiding this comment.
moved this check into config.rs. Also, just checking the config dir is enough, if there is no config file then it will fall back to the default file.
| } | ||
| } | ||
| None => { | ||
| anyhow::bail!("The configuration directory does not exists"); |
There was a problem hiding this comment.
Minor correction.
| anyhow::bail!("The configuration directory does not exists"); | |
| anyhow::bail!("The configuration directory does not exist"); |
There was a problem hiding this comment.
not relevant anymore.
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
PradeepKiruvale
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
| fn is_config_dir_exist(config_dir: &Path) -> Result<(), TEdgeConfigError> { | ||
| match config_dir.parent() { | ||
| Some(cdir) => { |
There was a problem hiding this comment.
The name of the parameter is confusing, as this is the path to the config file and not the config directory.
There was a problem hiding this comment.
Agree, renamed it.
| @@ -11,6 +11,7 @@ repository = { workspace = true } | |||
|
|
|||
| [dependencies] | |||
| async-trait = "0.1" | |||
| log = "0.4" | |||
There was a problem hiding this comment.
In most of the other crates, we use tracing instead of log. This inconsistency is not just limited to this crate, but let's try and avoid it at least in newer code.
There was a problem hiding this comment.
What I heard from @didier-wenzek is that trace is heavy, so I am just using the log crate.
There was a problem hiding this comment.
I also agree that log is probably more that sufficient for our simple logging needs. But, the Rust ecosystem seems to be leaning more towards wider usage of tracing instead of log as you can see in rust-lang/compiler-team#331 . Anyway, we just need to stay consistent, whichever one we choose. If the mandate is to stick to log, will keep that in mind.
There was a problem hiding this comment.
This is a point we need to discuss. I have a preference for log simply because in practice our use of tracing is reduced to logging without any trace.
| @@ -95,6 +95,23 @@ impl ConfigManagerConfig { | |||
| mqtt_port, | |||
| tedge_http_address, | |||
| tedge_http_port, | |||
| )) | |||
| ); | |||
| is_config_dir_exist(config.plugin_config_path.as_path())?; | |||
There was a problem hiding this comment.
Rather than creating a "faulty" config object first and then doing an explicit validation with this extra function call, why not make the ConfigManagerConfig::new() (ConfigManagerConfig::try_new()) return the error?
There was a problem hiding this comment.
Many new functions must be converted to try_new, because it's a bit deeper in the code(c8y_config_plugin->plugin-config->RawPluginConfig). So, this is the right way to validate the config path.
There was a problem hiding this comment.
I agree with @albinsuresh.
Many new functions must be converted to try_new,
There are only 2 calls to ConfigManagerConfig::new() ;-)
| } | ||
| } | ||
|
|
||
| fn is_config_dir_exist(config_dir: &Path) -> Result<(), TEdgeConfigError> { |
There was a problem hiding this comment.
| fn is_config_dir_exist(config_dir: &Path) -> Result<(), TEdgeConfigError> { | |
| fn plugin_config_dir_exists(config_dir: &Path) -> Result<(), TEdgeConfigError> { |
Just to avoid any confusion with tedge_config_dir
There was a problem hiding this comment.
Used existing function from tedge-utils
| fn is_config_dir_exist(config_dir: &Path) -> Result<(), TEdgeConfigError> { | ||
| match config_dir.parent() { | ||
| Some(cdir) => { | ||
| if !Path::new(cdir).is_dir() { | ||
| return Err(TEdgeConfigError::ConfigDirNotFound(cdir.to_path_buf())); | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| None => Err(TEdgeConfigError::ConfigDirNotFound( | ||
| config_dir.to_path_buf(), | ||
| )), | ||
| } | ||
| } |
There was a problem hiding this comment.
Could be moved to tedge_utils as a utility function and reused.
There was a problem hiding this comment.
In fact, there is already a function pub fn validate_parent_dir_exists(path: impl AsRef<Path>) -> Result<(), PathsError> { ) that exists for verifying the dir path.
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| for (watch_path, _) in self.messages.get_watch_dirs().iter() { | ||
| fs_notify.add_watcher(watch_path).unwrap(); | ||
| if let Err(err) = fs_notify.add_watcher(watch_path) { | ||
| error!( |
There was a problem hiding this comment.
Although this approach is less invasive I'm starting to doubt if this is the right thing to do.
The issue with it is that, we let the system continue in a broken state by silently logging the issue, which users may not notice immediately. If this actor fails to watch a directly that another actor has requested for, it is a fatal failure, in my opinion. So crashing the daemon is probably the better option I feel, so that the user immediately notices that something is wrong.
Ideally, we should have a mechanism in the actor framework where this actor can communicate this failure to the requesting actor via an error and let that actor itself decide whether to crash or not.
There was a problem hiding this comment.
Since we are doing the validation up-front during build time and hence this error path is less likely to happen (unless someone deletes the dirs during runtime), this is okay for now. But, we'd still need to explore a better build time error exchange mechanism between actors.
| @@ -26,6 +26,9 @@ pub enum TEdgeConfigError { | |||
|
|
|||
| #[error(transparent)] | |||
| Multi(#[from] Multi), | |||
|
|
|||
| #[error(transparent)] | |||
There was a problem hiding this comment.
Since this error variant is generic enough, it's okay.
A better option to avoid "corrupting" TEdgeConfigError would have been to add a similar variant to ConfigManagementError and LogRetrievalError enums respectively and have the ConfigManagerConfig::from_tedge_config() and LogManagerConfig::from_tedge_config() functions return these specific errors instead of TEdgeConfigError.
There was a problem hiding this comment.
Adding this error case is really not ideal, as it depends on tedge_utils::paths::PathsError.
Albin's suggestion would avoid that.
| for (watch_path, _) in self.messages.get_watch_dirs().iter() { | ||
| fs_notify.add_watcher(watch_path).unwrap(); | ||
| if let Err(err) = fs_notify.add_watcher(watch_path) { | ||
| error!( |
There was a problem hiding this comment.
Since we are doing the validation up-front during build time and hence this error path is less likely to happen (unless someone deletes the dirs during runtime), this is okay for now. But, we'd still need to explore a better build time error exchange mechanism between actors.
| @@ -1,4 +1,5 @@ | |||
| use anyhow::Context; | |||
| use anyhow::Ok; | |||
There was a problem hiding this comment.
The changes in this file looks like accidental left-overs.
Check if the config directory exists or not. Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
PradeepKiruvale
force-pushed
the
c8y-config-panic
branch
from
56dd3b2 to
274dc59
Compare
PradeepKiruvale
temporarily deployed
to
Test Pull Request
GitHub Actions
Inactive
| @@ -26,6 +26,9 @@ pub enum TEdgeConfigError { | |||
|
|
|||
| #[error(transparent)] | |||
| Multi(#[from] Multi), | |||
|
|
|||
| #[error(transparent)] | |||
There was a problem hiding this comment.
Adding this error case is really not ideal, as it depends on tedge_utils::paths::PathsError.
Albin's suggestion would avoid that.
Check if the config directory exists or not. Signed-off-by: Pradeep Kumar K J <pradeepkumar.kj@softwareag.com>
|
QA has thoroughly checked the feature and here are the results:
Test steps:
Result: |
Proposed changes
The issue here is in
tedge_file_system_extin theFsWatchActorrunfunction, the errors are not handled, and usingunwrap, Because of this when there are no directories to addfs watchit panics.The proposed solution is to handle these panics gracefully by removing unwrap.
Types of changes
Paste Link to the issue
#1955
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments