-
Notifications
You must be signed in to change notification settings - Fork 22
feat(dgw): basic network monitoring #1446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Let maintainers know that an action is required on their side
|
ac51029
to
ec20f65
Compare
} | ||
|
||
#[derive(Debug, Clone, Serialize)] | ||
pub struct MonitoringLogResponse { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Should not be part of the public API.
pub struct MonitoringLogResponse { | |
pub(crate) struct MonitoringLogResponse { |
(Or private, but you may need that in the openapi declaration.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! I just did a very high level review, I’ll look at the details of your crate later 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR! I left some notes.
I'd also suggest running cargo clippy -p network-monitor
and fixing the lints.
} | ||
} | ||
|
||
pub fn mock(cache_path: Utf8PathBuf) -> State { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add #[cfg(test)]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mock is created by the mock function in DgwState
, which is not gated in this way. I'm not sure why though, maybe it should be? @CBenoit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The one in Devolutions Gateway is #[doc(hidden)] to express a similar intention, but if we gate with #[cfg(test)], then the function is not available for the integration tests…
It’s not ideal though, I don’t really like that. But for the purpose of getting stuff done, I would say that it’s not a big deal, and we can surely do something about that later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: While .into()
is concise, I strongly discourage its use in non-generic, concrete code because it obscures intent. Prefer TargetType::from(source_value)
:
- The destination type is explicit at the call site.
- It immediately signals that a conversion is taking place and to which type.
- It removes the need for type inference and reduces coupling to surrounding context, aiding readability and future refactoring.
.into()
is better reserved for generic contexts or builder-style code where the target type is already constrained. Outside of those, the clarity benefit of from()
is worth the extra characters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Highlighting this comment: https://github.com/Devolutions/devolutions-gateway/pull/1446/files#r2242524595
suggestion: Look in the config
module, there is a enable_unstable
option. For now, I suggest we gate this feature behind this option, so you can freely change the API as necessary until the 2025.3 release.
afd2221
to
40bcc6c
Compare
40bcc6c
to
3f40070
Compare
613a493
to
9f98c27
Compare
9f98c27
to
59c853c
Compare
Co-authored-by: Benoît Cortier <[email protected]>
Retrieve the logs: | ||
|
||
```shell | ||
curl -v http://127.0.0.1:7171/jet/net/monitor/log/drain -X POST -H "Authorization: Bearer $dgwkey" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: I see you are using the correct methods 💯
The response will look similar to this: | ||
|
||
```json | ||
{"entries":[{"monitor_id":"monitor1","request_start_time":"2025-08-22T17:07:34.3370521Z","response_success":true,"response_messages":null,"response_time":0.0585181}]} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Maybe use skip_serializing_if = Option::is_none in the serde annotations, we typically don’t bother returning null values in the rest of the API
/// Replaces the existing monitoring config with the one provided in the body. | ||
/// This request will immediately start any new monitors, and will stop | ||
/// currently active monitors that are no longer in the config. | ||
/// | ||
/// The configuration is not persisted across restarts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: By the way, I kind of remember you talked about that in the past, but I’m not entirely sure: is it really fine to not persist the configuration? I guess the idea is that if the Gateway is restarted, the monitor order must be reissued by DVLS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea, given that the controlling server needs to be in regular contact with gateway, we might as well keep it stateless. But I've been meaning to ask you: does DVLS have an explicit way to know when the gateway comes online? It's not clear to me how these "Subscribers" work. If not I would have to rethink the approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The subscriber API is only used by Hub Business, so you can ignore all of that when it comes to DVLS. DVLS performs active polling in order to maintain a "one-directed communication" (Gateway inbound and DVLS outbound only). DVLS may never know that a Devolutions Gateway was restarted. Does that change the design in your mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the deets, I'll bring back my code to save the config to disk in a follow-up PR then.
Co-authored-by: Benoît Cortier <[email protected]>
Co-authored-by: Benoît Cortier <[email protected]>
…return" statements
423718a
to
146d9fc
Compare
Introduces an endpoint monitor that checks the uptime of a list of hosts provided through a remotely-submitted configuration file. The monitor results are saved to a temporary in-memory buffer and can be fetched by means of a REST endpoint.
The monitor system is structured as an agent, meant to be driven by a third party (for example our DVLS), so the configuration is ephemeral and monitor results are deleted from the buffer after being fetched. Gateway is not itself the source of truth for the monitor configuration, and it does not persist the log entries.
Two authenticated endpoints are introduced:
Example requests for testing:
curl http://127.0.0.1:7171/jet/net/monitor/log/drain -X POST -H "Authorization: Bearer $dgwkey"
Remaining tasks for a basic scanner:
Issue: DGW-302