-
Notifications
You must be signed in to change notification settings - Fork 441
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
feat: integrate embedded reportserver #3583
Conversation
2c0c998
to
55c2ca1
Compare
55c2ca1
to
8f0447d
Compare
8f0447d
to
c5b7df6
Compare
fecbe14
to
b851879
Compare
@@ -0,0 +1,20 @@ | |||
[package] | |||
name = "reportserver" |
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.
I suggest rename reportserver
to report_server
, also rename the directory.
There are three cases to consider: if ZO_ENABLE_EMBEDDED_REPORT_SERVER=true If running a binary, alertmanager node will spawn an embedded server running at localhost:5082. All the requests from alert-manager will go to localhost:5082 for pdf generation related tasks. For single node k8s deployment, as this is running in a container there is no chromium installed, so even if you set the variable to true, the report server will panic due to missing dependencies. For this situation helm chart will spawn a report-server instance which can be used. Same for cluster deployment.
d6fe1fc
to
bbec38b
Compare
WalkthroughThe recent changes introduce a new Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant ReportServer
participant HeadlessBrowser
participant EmailService
User->>ReportServer: Send report request
ReportServer->>HeadlessBrowser: Generate report
HeadlessBrowser->>ReportServer: Return report data
ReportServer->>EmailService: Send report email
EmailService->>User: Email sent confirmation
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 4
Outside diff range and nitpick comments (2)
src/reportserver/src/models.rs (1)
26-35
: Consider adding documentation comments toReportDashboard
to explain the purpose of each field, especiallyvariables
andtimerange
.src/reportserver/src/router.rs (1)
21-47
: TheHttpResponse
structure is well-defined with appropriate use of serialization controls. Consider adding more detailed documentation for each field, especiallyerror_detail
andtrace_id
, to explain their usage contexts.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lock
is excluded by!**/*.lock
Files selected for processing (12)
- Cargo.toml (3 hunks)
- deny.toml (1 hunks)
- src/config/src/config.rs (4 hunks)
- src/handler/http/request/status/mod.rs (1 hunks)
- src/job/alert_manager.rs (1 hunks)
- src/reportserver/Cargo.toml (1 hunks)
- src/reportserver/src/lib.rs (1 hunks)
- src/reportserver/src/models.rs (1 hunks)
- src/reportserver/src/report.rs (1 hunks)
- src/reportserver/src/router.rs (1 hunks)
- src/reportserver/src/server.rs (1 hunks)
- src/service/dashboards/reports.rs (1 hunks)
Files skipped from review due to trivial changes (4)
- Cargo.toml
- deny.toml
- src/handler/http/request/status/mod.rs
- src/reportserver/Cargo.toml
Additional comments not posted (10)
src/reportserver/src/lib.rs (1)
1-6
: LGTM! The module structure and public export are clearly defined.src/reportserver/src/models.rs (2)
64-71
: The default implementation forReportTimerange
is well-defined. Good use of thedefault
trait to provide fallback values.
5-8
: Ensure that theclient
field inSmtpConfig
is thread-safe and properly managed since it's a static reference.Verification successful
The provided context shows that
AsyncSmtpTransport
is initialized usingLazy
from theonce_cell
crate, which ensures thread-safe, one-time initialization. This pattern is used in bothsrc/reportserver/src/report.rs
andsrc/config/src/config.rs
. TheSmtpConfig
struct references thisLazy
instance, ensuring that theclient
field is properly managed and thread-safe.
src/reportserver/src/report.rs
:pub static SMTP_CLIENT: Lazy<AsyncSmtpTransport<Tokio1Executor>> = Lazy::new(|| { let tls_parameters = TlsParameters::new(CONFIG.smtp.smtp_host.clone()).unwrap(); let mut transport_builder = AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous(&CONFIG.smtp.smtp_host) .port(CONFIG.smtp.smtp_port); let option = &CONFIG.smtp.smtp_encryption; transport_builder = if option == "starttls" { transport_builder.tls(Tls::Required(tls_parameters)) } else if option == "ssltls" { transport_builder.tls(Tls::Wrapper(tls_parameters)) } else { transport_builder }; transport_builder.build() });
src/config/src/config.rs
:pub static SMTP_CLIENT: Lazy<Option<AsyncSmtpTransport<Tokio1Executor>>> = Lazy::new(|| { if !CONFIG.smtp.smtp_enabled { None } else { let tls_parameters = TlsParameters::new(CONFIG.smtp.smtp_host.clone()).unwrap(); let mut transport_builder = AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous(&CONFIG.smtp.smtp_host) .port(CONFIG.smtp.smtp_port); let option = &CONFIG.smtp.smtp_encryption; transport_builder = if option == "starttls" { transport_builder.tls(Tls::Required(tls_parameters)) } else if option == "ssltls" { transport_builder.tls(Tls::Wrapper(tls_parameters)) } else { transport_builder }; Some(transport_builder.build()) } });Given this context, the
client
field inSmtpConfig
is indeed thread-safe and properly managed.
- : The
client
field inSmtpConfig
is thread-safe and properly managed usingLazy
from theonce_cell
crate.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify thread safety and lifecycle management of `AsyncSmtpTransport` instance. # Test: Search for the initialization and usage of `AsyncSmtpTransport`. ast-grep --lang rust --pattern $'let $_ = AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous($$$)'Length of output: 104
Script:
#!/bin/bash # Description: Verify thread safety and lifecycle management of `AsyncSmtpTransport` instance. # Test: Search for the initialization and usage of `AsyncSmtpTransport` with different patterns. ast-grep --lang rust --pattern $'let $_ = AsyncSmtpTransport::<Tokio1Executor>::new($$$)' ast-grep --lang rust --pattern $'let $_ = AsyncSmtpTransport::<Tokio1Executor>::builder($$$)' ast-grep --lang rust --pattern $'let $_ = AsyncSmtpTransport::<Tokio1Executor>::new_with($$$)' ast-grep --lang rust --pattern $'let $_ = AsyncSmtpTransport::<Tokio1Executor>::builder_dangerous($$$)' # Test: Search for the usage of `SmtpConfig` to understand how `client` is managed. ast-grep --lang rust --pattern $'SmtpConfig { $$$ }'Length of output: 440
Script:
#!/bin/bash # Description: Broaden the search to find any references to `AsyncSmtpTransport` and `SmtpConfig`. # Test: Search for any references to `AsyncSmtpTransport` throughout the codebase. rg 'AsyncSmtpTransport' # Test: Search for any references to `SmtpConfig` throughout the codebase. rg 'SmtpConfig'Length of output: 1015
Script:
#!/bin/bash # Description: Extract relevant code snippets to analyze the initialization and management of `AsyncSmtpTransport` and `SmtpConfig`. # Extract code snippets from `src/reportserver/src/report.rs` related to `AsyncSmtpTransport`. rg -A 10 'AsyncSmtpTransport' src/reportserver/src/report.rs # Extract code snippets from `src/config/src/config.rs` related to `AsyncSmtpTransport`. rg -A 10 'AsyncSmtpTransport' src/config/src/config.rs # Extract code snippets from `src/reportserver/src/router.rs` related to `SmtpConfig`. rg -A 10 'SmtpConfig' src/reportserver/src/router.rsLength of output: 2505
src/reportserver/src/server.rs (1)
51-87
: The graceful shutdown function handles different signals appropriately. Ensure that the signal handling is tested across different operating systems to confirm its reliability.src/job/alert_manager.rs (1)
28-38
: The logic to spawn the embedded report server based on configuration is clear and well-implemented. Ensure that the configuration flags are documented and validated during application startup.src/reportserver/src/report.rs (1)
325-377
: Thesend_email
function is well-implemented with comprehensive error handling. Ensure that the SMTP configuration and credentials are securely managed and not logged.Verification successful
The search results indicate that SMTP configuration and credentials are used in various parts of the codebase, but there is no evidence of them being logged directly. This suggests that the SMTP credentials are managed securely without being exposed in log statements.
src/reportserver/src/router.rs
src/reportserver/src/models.rs
src/reportserver/src/report.rs
src/service/dashboards/reports.rs
src/service/alerts/destinations.rs
src/service/alerts/mod.rs
src/config/src/config.rs
These files contain references to SMTP configuration and credentials, but no logging statements were found that expose sensitive information.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify secure management of SMTP credentials. # Test: Search for logging statements that might expose sensitive information. rg --type rust 'log::info' | grep smtpLength of output: 38
Script:
#!/bin/bash # Description: Verify secure management of SMTP credentials. # Test: Search for logging statements that might expose sensitive information at different logging levels. rg --type rust 'log::warn' | grep smtp rg --type rust 'log::error' | grep smtp rg --type rust 'log::debug' | grep smtpLength of output: 118
Script:
#!/bin/bash # Description: Verify secure management of SMTP credentials. # Test: Search for the usage of SMTP configuration and credentials in the codebase. rg --type rust 'SmtpConfig' rg --type rust 'smtp'Length of output: 3179
src/config/src/config.rs (4)
245-245
: AddedReportServer
struct to theConfig
struct.This addition aligns with the PR's objective to integrate an embedded report server, ensuring that the server's configuration can be managed through environment variables.
267-281
: IntroducedReportServer
struct with configuration fields.This struct is well-defined with appropriate use of environment variables for configuration. It includes essential settings such as enabling the report server, user credentials, and network settings. This is crucial for the flexibility and security of the report server configuration.
299-304
: Added new fields to theChrome
struct for managing Chrome browser options.These additions provide more control over the Chrome browser's behavior when used by the report server, such as checking for default paths and managing downloads. This is important for ensuring that the embedded browser operates correctly in various deployment environments.
615-618
: Added fields to configure the report server's URL and TLS verification behavior.These fields are essential for defining the connection to the report server and its security settings. The ability to skip TLS verification (
report_server_skip_tls_verify
) can be useful in development or internal environments but should be used cautiously in production.
There are three cases to consider:
if ZO_ENABLE_EMBEDDED_REPORT_SERVER=true
If running a binary, alertmanager node will spawn an embedded server running at localhost:5082.
All the requests from alert-manager will go to localhost:5082 for pdf generation related tasks.
For single node k8s deployment, as this is running in a container there is no chromium installed, so even if you set the variable to true, the report server will panic due to missing dependencies. For this situation helm chart will spawn a report-server instance which can be used.
Same for cluster deployment.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
Documentation