MON-106121-pull-wss-add-gorgone-parameter-to-get-vault-credentials #5483
Conversation
Evan-Adam
force-pushed
the
MON-106121-pull-wss-add-gorgone-parameter-to-get-vault-credentials
branch
7 times, most recently
from
216a8d7 to
8df3c55
Compare
Evan-Adam
requested review from
tuntoja,
mushroomempires,
lucie-dubrunfaut and
smau-centreon
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several significant changes, including a new GitHub Actions workflow for automated unit testing of Perl code, enhancements to documentation, the addition of a new Perl module for interacting with HashiCorp Vault, and updates to logging functionality. It also introduces new YAML configuration files for packaging and modifies existing ones to reflect changes in the structure and dependencies of Centreon Perl libraries. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Outside diff range and nitpick comments (8)
centreon/packaging/centreon-perl-libs.yaml (2)
11-12: Fix grammar in package description
The description has a grammatical error. "for various script" should be "for various scripts" (plural form).
- This package contains Centreon Perl libraries for various script.
+ This package contains Centreon Perl libraries for various scripts.Line range hint 1-89: Consider documenting package split rationale
The package split into centreon-perl-libs and centreon-perl-libs-common is a significant architectural change. Consider:
- Adding comments in the YAML file explaining the package's scope and relationship with centreon-perl-libs-common
- Creating/updating documentation about which modules belong in each package
- Adding validation in the CI pipeline to prevent common modules from being accidentally added to this package
This will help maintain the clean separation of concerns and guide future contributors.
centreon/lib/perl/t/common/logger.t (2)
12-23: Enhance test organization and documentation.
While the test coverage is good, consider these improvements:
- Add POD documentation for the test function
- Use subtests for better organization
- Add explicit tests for case sensitivity
sub test_severity {
+ # Add documentation
+ # =head2 test_severity
+ # Tests the severity level functionality of the logger:
+ # - Default severity
+ # - Setting/getting severity levels
+ # - Invalid severity handling
+
my $logger = centreon::common::logger->new();
- is($logger->severity(), "WARNING", "default severity should be warning.");
+ subtest 'default severity' => sub {
+ is($logger->severity(), "WARNING", "default severity should be warning.");
+ };
- for my $sev ('FATAL', 'fatal', 'ERROR', 'Error', 'WARNING', 'NOTICE', 'INFO', 'DEBUG') {
+ subtest 'severity levels' => sub {
+ for my $sev ('FATAL', 'fatal', 'ERROR', 'Error', 'WARNING', 'NOTICE', 'INFO', 'DEBUG') {
+ is($logger->severity($sev), uc($sev), "severity $sev was correctly set.");
+ is($logger->severity(), uc($sev), "getter was correct.");
+ }
+ };
- $logger->severity("FATAL");
- is($logger->severity("toto"), -1, "if severity is unknown we don't change anything.");
+ subtest 'invalid severity' => sub {
+ $logger->severity("FATAL");
+ is($logger->severity("toto"), -1, "if severity is unknown we don't change anything.");
+ is($logger->severity(), "FATAL", "original severity remains unchanged");
+ };
}85-97: Improve test organization in main.
Consider using Test2::V0's subtest feature to group related tests and provide better test output organization.
sub main {
- test_severity();
- test_writeLogInfo_Stdout(pid => 1, severity => "info");
- test_writeLogInfo_Stdout(pid => 0, severity => "info");
- test_writeLogInfo_Stdout(pid => 1, severity => "debug");
- test_writeLogInfo_Stdout(pid => 0, severity => "debug");
- # let's check the log is not written when the severity is not high enough
- test_writeLogInfo_Stdout_is_empty(pid => 1, severity => "error");
- test_writeLogInfo_Stdout_is_empty(pid => 0, severity => "notice");
- test_writeLogFunc(pid => 1);
+ subtest 'severity tests' => \&test_severity;
+
+ subtest 'stdout logging tests' => sub {
+ test_writeLogInfo_Stdout(pid => 1, severity => "info");
+ test_writeLogInfo_Stdout(pid => 0, severity => "info");
+ test_writeLogInfo_Stdout(pid => 1, severity => "debug");
+ test_writeLogInfo_Stdout(pid => 0, severity => "debug");
+ };
+
+ subtest 'severity threshold tests' => sub {
+ test_writeLogInfo_Stdout_is_empty(pid => 1, severity => "error");
+ test_writeLogInfo_Stdout_is_empty(pid => 0, severity => "notice");
+ };
+
+ subtest 'logging function tests' => sub {
+ test_writeLogFunc(pid => 1);
+ };
done_testing;
}centreon/lib/perl/centreon/common/logger.pm (1)
277-279: Document that 'writeLogFatal' terminates the program
The writeLogFatal method logs a fatal message and then calls die, which terminates the program execution:
sub writeLogFatal {
shift->writeLog(2, @_);
die("FATAL: " . $_[0] . "\n");
}To prevent unexpected termination, ensure that this behavior is clearly documented so that users of the logger module are aware that calling writeLogFatal will exit the program.
centreon/lib/perl/t/common/centreonvault.t (3)
256-256: Use diag instead of print for diagnostic messages
To ensure that diagnostic messages are properly captured in test outputs, use diag from Test2::V0 instead of print. This integrates better with the testing framework.
Apply this diff to make the change:
- print(Dumper($required_option));
+ diag(Dumper($required_option));28-28: Offer assistance in implementing the encryption verification using salt
The comment mentions that the function to use the salt for encryption verification is not implemented yet. Implementing this functionality can enhance security by verifying that data is decrypted correctly.
Would you like assistance in implementing this feature?
204-206: Use local for temporarily overriding environment variables
To ensure that the APP_SECRET environment variable is safely restored even if an exception occurs during testing, use local to temporarily override it within a scope.
Apply this diff for safer handling:
- my $old_app_secret = $ENV{'APP_SECRET'};
- $ENV{'APP_SECRET'} = $set->{default_app_secret};
+ local $ENV{'APP_SECRET'} = $set->{default_app_secret};This way, APP_SECRET will automatically be restored at the end of the scope.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (8)
- .github/workflows/perl-unit-tests.yml (1 hunks)
- centreon/lib/perl/README.md (1 hunks)
- centreon/lib/perl/centreon/common/centreonvault.pm (1 hunks)
- centreon/lib/perl/centreon/common/logger.pm (7 hunks)
- centreon/lib/perl/t/common/centreonvault.t (1 hunks)
- centreon/lib/perl/t/common/logger.t (1 hunks)
- centreon/packaging/centreon-perl-libs-common.yaml (1 hunks)
- centreon/packaging/centreon-perl-libs.yaml (2 hunks)
🧰 Additional context used
🪛 LanguageTool
centreon/lib/perl/README.md
[grammar] ~7-~7: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...centreon-perl-libs-common This package contain libraries located in centreon::common n...
(PLURAL_VERB_AFTER_THIS)
[grammar] ~29-~29: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...` ## centreon-perl-libs This package contain libraries associated to specific binary...
(PLURAL_VERB_AFTER_THIS)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...ted in centreon::[area] namespace. for exemple the package centreon-trap contain a bin...
(AI_HYDRA_LEO_MISSING_COMMA)
[uncategorized] ~31-~31: A comma might be missing here.
Context: ...entreon::[area] namespace. for exemple the package centreon-trap contain a binary ...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~31-~31: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... for exemple the package centreon-trap contain a binary calling centreon::trap namespa...
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
[uncategorized] ~31-~31: This verb does not appear to agree with the subject. Consider using a different form.
Context: ... calling centreon::trap namespace which contain all the logic.
(AI_EN_LECTOR_REPLACEMENT_VERB_AGREEMENT)
🔇 Additional comments (10)
centreon/packaging/centreon-perl-libs-common.yaml (4)
4-5: Verify version schema configuration.
Setting version_schema: "none" might affect version tracking and upgrades. Consider using semantic versioning to ensure proper package version management.
26-55: Review vault-related dependencies.
Given that this package includes vault integration (as mentioned in the description), consider adding these dependencies:
perl(Vault::HTTPS)or equivalent HashiCorp Vault client library- SSL/TLS related Perl modules for secure vault communication
Also, update the provides section to include the vault-related modules that will be part of this package.
59-61: Verify RPM signing configuration.
Ensure that RPM_SIGNING_KEY_FILE and RPM_SIGNING_KEY_ID environment variables are properly configured in the CI/CD pipeline.
11-12: Verify commit hash expansion mechanism.
The @COMMIT_HASH@ placeholder needs a mechanism to be expanded during package building.
.github/workflows/perl-unit-tests.yml (1)
5-8: LGTM! Good concurrency control.
The concurrency configuration with cancellation of in-progress runs is a good practice to prevent queue buildup and ensure latest code is always tested.
centreon/packaging/centreon-perl-libs.yaml (2)
18-42:
Review file permissions
The mode 0755 (rwxr-xr-x) grants execute permissions to all users, which is typically unnecessary for Perl library files. Consider using 0644 (rw-r--r--) instead, as these are source files that don't need direct execution.
Let's verify the file types in these directories:
Line range hint 48-76: Verify package split implementation
The dependency changes align with the PR objective of splitting the package. However, let's verify:
- No common modules are accidentally included in this package
- All script-related modules are properly included
centreon/lib/perl/t/common/centreonvault.t (1)
185-186: Verify the correctness of the test assertions in test_get_secret
After calling $vault->get_secret("secret::hashicorp_vault::..."), the test assertion checks $vault->get_secret("token") expecting "token" as the result. Please ensure that this is the intended behavior and the assertion is verifying the correct outcome.
To confirm, should the test assertion be checking the value of $clear_password instead?
centreon/lib/perl/centreon/common/centreonvault.pm (2)
75-77: Enable SSL certificate verification for secure HTTPS communication
The code initializes Net::Curl::Easy but does not explicitly enable SSL certificate verification. To prevent man-in-the-middle attacks, it's important to verify SSL certificates when making HTTPS requests.
[security]
Apply this diff to enable SSL verification:
$self->{curl_easy} = Net::Curl::Easy->new();
$self->{curl_easy}->setopt( CURLOPT_USERAGENT, "Centreon VMware daemon's centreonvault.pm");
+$self->{curl_easy}->setopt( CURLOPT_SSL_VERIFYPEER, 1 );
+$self->{curl_easy}->setopt( CURLOPT_SSL_VERIFYHOST, 2 );Ensure that the CA certificate bundle is properly configured for certificate verification.
275-278: Review handling of unrecognized secret formats for security concerns
The get_secret method returns the input secret unchanged if it does not match the expected vault path format. This could lead to unintended use of plaintext secrets, potentially bypassing security mechanisms.
[security]
Consider handling unrecognized secrets by raising an error or warning, or by enforcing the use of the vault for all secrets to enhance security.
Evan-Adam
added
the
upload-artifacts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.github/actions/package-nfpm/action.yml (1)
117-118: LGTM! Consider enhancing the documentation.
The conditional artifact upload based on PR labels is a good security practice. The implementation correctly uses a pinned version of the upload action and sets an appropriate retention period.
Consider adding this documentation to the PR template or contributing guidelines to ensure developers are aware of this requirement. Example addition:
## Artifacts
To obtain package artifacts from your PR:
1. Add the `upload-artifacts` label to your PR
2. Wait for the workflow to complete
3. Download artifacts from the Actions tab
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
- .github/workflows/perl-unit-tests.yml (1 hunks)
- centreon/lib/perl/centreon/common/centreonvault.pm (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/perl-unit-tests.yml
🔇 Additional comments (5)
centreon/lib/perl/centreon/common/centreonvault.pm (5)
35-35: Consider encapsulating object attributes for better maintainability
In the constructor sub new, the object is blessed directly with %options, which may expose internal implementation details. It's recommended to use a separate hash reference for object attributes to ensure encapsulation and prevent unintended side effects.
85-85: Avoid using die to allow graceful error handling
Using die inside sub check_options can terminate the program unexpectedly. Instead, consider logging the error and returning undef to let the caller handle it appropriately.
209-215: Properly encode POST data to handle special characters
Constructing the POST data by concatenating strings may lead to issues if role_id or secret_id contains special characters. It's recommended to send the data in JSON format or URL-encode the parameters.
244-246: Validate authentication token before use
When storing the authentication token, the code assumes that $auth_result_obj->{auth}->{client_token} is valid. If it's undefined or empty, this could lead to authentication failures in subsequent requests.
328-338: Ensure JSON decoding errors are properly propagated
In sub transform_json_to_object, if decode_json fails, the method returns an error message. Ensure that all callers of this method check for the error_message key and handle the error appropriately to prevent unexpected behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
centreon/lib/perl/centreon/common/logger.pm (5)
33-33: Consider using a more standardized format for update notices.The update notice could be more informative by including the nature of changes. Consider using this format:
-# updated by Evan ADAM on 10/2024 +# Updated by Evan ADAM <email> on 10/2024 +# Changes: Refactored severity handling and added new logging methods
67-74: Add documentation for severity levels.The severity mapping would benefit from documentation explaining:
- The meaning and use case for each level
- Why level 1 is skipped
- That lower numbers indicate higher severity
Consider adding this documentation above the hash:
# Severity levels (lower number = higher severity): # 2 (FATAL) - Unrecoverable errors that require immediate shutdown # 3 (ERROR) - Recoverable errors that affect functionality # 4 (WARNING) - Important events that don't affect normal operation # 5 (NOTICE) - Normal but significant events # 6 (INFO) - General operational information # 7 (DEBUG) - Detailed information for debugging
155-159: Add parameter validation for boolean options.Both
flush_outputandwithdatemethods accept parameters that should be boolean. Consider adding validation:sub flush_output { my ($self, %options) = @_; - $| = 1 if (defined($options{enabled})); + $| = 1 if (defined($options{enabled}) && $options{enabled}); } sub withdate { my $self = shift; if (@_) { - $self->{withdate} = $_[0]; + $self->{withdate} = $_[0] ? 1 : 0; } return $self->{withdate}; }Also applies to: 217-223
179-194: Improve severity level validation pattern.The current pattern
/^[0234567]$/allows some invalid severity levels. Consider:- if ($input_severity =~ /^[0234567]$/) { + if ($input_severity =~ /^[2-7]$/) {Also, consider using a hash for string-to-number mapping to reduce the number of
elsifstatements:my %severity_names = ( 'none' => 0, 'fatal' => 2, 'error' => 3, 'warning' => 4, 'notice' => 5, 'info' => 6, 'debug' => 7 );
275-278: Enhance fatal error handling.The
writeLogFatalmethod could be improved by:
- Making the error message consistent with the logged message
- Including the program name in the die message
sub writeLogFatal { - shift->writeLog(2, @_); - die("FATAL: " . $_[0] . "\n"); + my ($self, $msg) = @_; + $self->writeLog(2, $msg); + die(sprintf("[%s] FATAL: %s\n", $0, $msg)); }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (2)
centreon/lib/perl/centreon/common/logger.pm(7 hunks)centreon/lib/perl/t/common/logger.t(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- centreon/lib/perl/t/common/logger.t
🔇 Additional comments (1)
centreon/lib/perl/centreon/common/logger.pm (1)
232-251:
Revisit syslog severity level handling.
While the previous suggestion to add a severity mapping hash was declined, the current implementation still passes numeric values directly to syslog. Consider using the built-in syslog constants directly:
} elsif ($self->{log_mode} == 2) {
- syslog($severity, $msg);
+ my %syslog_levels = (
+ 2 => LOG_CRIT,
+ 3 => LOG_ERR,
+ 4 => LOG_WARNING,
+ 5 => LOG_NOTICE,
+ 6 => LOG_INFO,
+ 7 => LOG_DEBUG
+ );
+ syslog($syslog_levels{$severity} // LOG_ERR, $msg);
}This approach:
- Uses standard syslog constants
- Provides a fallback for unknown severity levels
- Maintains correct syslog priority levels
Likely invalid or redundant comment.
…one/centreon_vmware Refs:MON-106121
* add new log level to be used by clients * add getter/setter for new option withdate and fix withpid * simplify writeLog sub Refs:MON-106121
* Add a readme file * Add a github workflow to run all tests on every changes. This workflow use centreon-plugin docker image. Refs:MON-106121
Refs:MON-106121
* this library was initialy started in centreon-plugin repository, but need to be used by centreon_vmware and gorgone, so it should be in a common package. Change since centreon-plugins: * update error message to be less alarming for the user. * app_secret can be retrieved from .env file if env variable is not set. * remove error log when vault is not enabled. As vault is optionnal if it is not configured it should be warning at most. Refs:MON-106121
Evan-Adam
force-pushed
the
MON-106121-pull-wss-add-gorgone-parameter-to-get-vault-credentials
branch
from
82b56bd to
49fb3f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
centreon/lib/perl/README.md (3)
1-4: Enhance the introduction to clarify package separation.The introduction should explicitly mention that these are two separate packages with different purposes and installation targets. This aligns with the PR objective of splitting the packages.
# centreon-perl-libs / centreon-perl-libs-common -This directory contains the common Perl libraries used by Centreon. +This directory contains Perl libraries used by Centreon, split into two packages: +- centreon-perl-libs-common: Core libraries required by all Centreon installations +- centreon-perl-libs: Additional libraries specifically for web application functionality
7-8: Fix grammar issues in the description.The text contains grammatical errors that should be corrected for better clarity.
-This package contain libraries located in centreon::common namespace. -they are generic library used by multiples centreon modules. +This package contains libraries located in the centreon::common namespace. +These are generic libraries used by multiple Centreon modules.🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...centreon-perl-libs-common This package contain libraries located in centreon::common n...(PLURAL_VERB_AFTER_THIS)
12-24: Enhance test documentation.The test instructions should include information about expected outcomes and test coverage requirements.
-to execute the test, see t/ directory in the package. -First install the dependency : +To execute the tests: + +1. Install the required dependencies: ```bash # distro package to install : openssl-dev # cpan package to install : Test2::V0 Test2::Plugin::NoWarnings Crypt::OpenSSL::AES-run the following command passing the path to t/ folder as argument :
+2. Run the test suite:prove -r t/
+3. Verify that all tests pass and maintain 100% coverage for new code.
</blockquote></details> <details> <summary>centreon/packaging/centreon-perl-libs-common.yaml (1)</summary><blockquote> `10-12`: **Enhance package description and standardize variable format.** The description could be more detailed about the specific functionalities provided. Also, consider using consistent variable syntax: ```diff description: | - This package contains Centreon Perl library for logging and vault integration. - Commit: @COMMIT_HASH@ + This package contains essential Centreon Perl libraries for core functionalities: + - Logging infrastructure + - Vault credential management + - Database connectivity + - Locking mechanisms + Commit: ${COMMIT_HASH}centreon/lib/perl/centreon/common/logger.pm (5)
Line range hint
33-74: Enhance module documentation with severity level details.The module's documentation should be updated to include:
- Available severity levels and their numeric values
- Default severity level
- Usage examples for different severity levels
Add the following to the DESCRIPTION section:
=head1 DESCRIPTION This module offers a simple interface to write log messages to various output: * standard output * file * syslog Severity Levels: * FATAL (2) - Fatal errors that cause immediate termination * ERROR (3) - Error conditions that should be addressed * WARNING (4) - Warning messages (default) * NOTICE (5) - Normal but significant conditions * INFO (6) - Informational messages * DEBUG (7) - Debug-level messages Example: $logger->severity("warning"); # Set severity level $logger->writeLogWarning("Warning message"); $logger->writeLogError("Error message"); =cut
155-165: Add input validation to configuration methods.The new configuration methods should validate their inputs to prevent unexpected behavior.
Consider these improvements:
sub flush_output { my ($self, %options) = @_; - $| = 1 if (defined($options{enabled})); + if (defined($options{enabled})) { + $| = $options{enabled} ? 1 : 0; + } } sub force_default_severity { my ($self, %options) = @_; - $self->{old_severity} = defined($options{severity}) ? $options{severity} : $self->{severity}; + if (defined($options{severity})) { + return unless exists $human_severities{$options{severity}}; + $self->{old_severity} = $options{severity}; + } else { + $self->{old_severity} = $self->{severity}; + } }
177-201: Optimize severity validation logic.The current implementation uses multiple
if/elsifstatements for severity validation. This could be simplified using a hash lookup.Consider this more maintainable approach:
sub severity { my $self = shift; if (@_) { my $input_severity = lc($_[0]); my $save_severity = $self->{severity}; - if ($input_severity =~ /^[0234567]$/) { - $self->{severity} = $input_severity; - } elsif ($input_severity eq "none") { - $self->{severity} = 0; - } elsif ($input_severity eq "fatal") { - $self->{severity} = 2; - } elsif ($input_severity eq "error") { - $self->{severity} = 3; - } elsif ($input_severity eq "warning") { - $self->{severity} = 4; - } elsif ($input_severity eq "notice") { - $self->{severity} = 5; - } elsif ($input_severity eq "info") { - $self->{severity} = 6; - } elsif ($input_severity eq "debug") { - $self->{severity} = 7; + my %severity_map = ( + none => 0, + fatal => 2, + error => 3, + warning => 4, + notice => 5, + info => 6, + debug => 7 + ); + if ($input_severity =~ /^[0234567]$/) { + $self->{severity} = $input_severity; + } elsif (exists $severity_map{$input_severity}) { + $self->{severity} = $severity_map{$input_severity}; } else { $self->writeLogError("Wrong severity value set."); return -1; } $self->{old_severity} = $save_severity; } return $human_severities{$self->{severity}}; }
207-223: Maintain consistent coding style in utility methods.The
withpidandwithdatemethods have different formatting styles. Let's make them consistent.Apply this style update:
sub withpid { my $self = shift; if (@_) { - if ($_[0]){ - $self->{withpid} = 1; - }else{ - $self->{withpid} = 0; - } + $self->{withpid} = $_[0] ? 1 : 0; } return $self->{withpid}; }
275-277: Enhance fatal error handling.The
writeLogFatalmethod's die message duplicates the severity level indication.Consider this improvement:
sub writeLogFatal { shift->writeLog(2, @_); - die("FATAL: " . $_[0] . "\n"); + die($_[0] . "\n"); }The severity is already included in the log message via
writeLog, so there's no need to duplicate it in the die message.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (9)
.github/actions/package-nfpm/action.yml(1 hunks).github/workflows/perl-unit-tests.yml(1 hunks)centreon/lib/perl/README.md(1 hunks)centreon/lib/perl/centreon/common/centreonvault.pm(1 hunks)centreon/lib/perl/centreon/common/logger.pm(7 hunks)centreon/lib/perl/t/common/centreonvault.t(1 hunks)centreon/lib/perl/t/common/logger.t(1 hunks)centreon/packaging/centreon-perl-libs-common.yaml(1 hunks)centreon/packaging/centreon-perl-libs.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- .github/actions/package-nfpm/action.yml
- .github/workflows/perl-unit-tests.yml
- centreon/lib/perl/centreon/common/centreonvault.pm
- centreon/lib/perl/t/common/centreonvault.t
- centreon/lib/perl/t/common/logger.t
- centreon/packaging/centreon-perl-libs.yaml
🧰 Additional context used
🪛 LanguageTool
centreon/lib/perl/README.md
[grammar] ~7-~7: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...centreon-perl-libs-common This package contain libraries located in centreon::common n...
(PLURAL_VERB_AFTER_THIS)
[grammar] ~29-~29: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...` ## centreon-perl-libs This package contain libraries associated to specific binary...
(PLURAL_VERB_AFTER_THIS)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...ted in centreon::[area] namespace. for exemple the package centreon-trap contain a bin...
(AI_HYDRA_LEO_MISSING_COMMA)
🔇 Additional comments (3)
centreon/packaging/centreon-perl-libs-common.yaml (2)
17-22: Previous review comment about file permissions and expansion settings is still applicable.
59-61: Verify RPM signing key configuration in CI.
The RPM signing configuration looks correct, but ensure these environment variables are properly set in the CI environment.
✅ Verification successful
RPM signing configuration is properly set up in CI
The RPM signing configuration is correctly implemented in the CI environment:
- The package workflow has a dedicated "sign" job that uses a specialized rpm-signing container
- The signing process is automatically triggered for RPM packages
- The signing keys are properly parameterized across all package YAML files
- The signing environment variables (RPM_SIGNING_KEY_FILE and RPM_SIGNING_KEY_ID) are expected to be set in the container environment
The review comment's concern has been addressed as the CI infrastructure has proper RPM signing support in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if RPM signing variables are defined in GitHub Actions workflows
rg -l "RPM_SIGNING_KEY" .github/workflows/
Length of output: 42
Script:
#!/bin/bash
# Let's check for any workflow files first to ensure the directory exists
fd . .github/workflows/
# Also check if there are any other CI configuration files that might contain these variables
fd "ci|jenkins|gitlab" .
# Check for environment files that might contain these variables
fd ".env" .
# Look for any documentation about RPM signing
rg -i "rpm.*sign|signing.*rpm" .
Length of output: 11549
Script:
#!/bin/bash
# Let's check the package.yml workflow which is likely responsible for building packages
cat .github/workflows/package.yml
# Also check create-repo-yum.yml as it might contain RPM-related configurations
cat .github/workflows/create-repo-yum.yml
Length of output: 8190
Script:
#!/bin/bash
# Let's check the package action to see how RPM signing is configured
cat .github/actions/package/action.yml
# Also check if there are any environment files in the repository root that might define these variables
ls -la .env* 2>/dev/null || true
# Check if there's any documentation about RPM signing setup
rg -i "rpm.*sign.*key|signing.*key" .
Length of output: 9930
centreon/lib/perl/centreon/common/logger.pm (1)
232-251:
Improve syslog severity handling.
The syslog call still uses numeric severity levels directly. While this works, it's better to use the standard syslog constants for clarity and maintainability.
As mentioned in the previous review, consider mapping severity levels to syslog constants:
+use constant {
+ SEVERITY_TO_SYSLOG => {
+ 2 => LOG_CRIT,
+ 3 => LOG_ERR,
+ 4 => LOG_WARNING,
+ 5 => LOG_NOTICE,
+ 6 => LOG_INFO,
+ 7 => LOG_DEBUG
+ }
+};
sub writeLog($$$%) {
my ($self, $severity, $msg, %options) = @_;
# ... existing code ...
} elsif ($self->{log_mode} == 2) {
- syslog($severity, $msg);
+ syslog(SEVERITY_TO_SYSLOG->{$severity}, $msg);
}
}| # centreon-perl-libs / centreon-perl-libs-common | ||
|
|
||
| This directory contains the common Perl libraries used by Centreon. | ||
|
|
||
| ## centreon-perl-libs-common | ||
|
|
||
| This package contain libraries located in centreon::common namespace. | ||
| they are generic library used by multiples centreon modules. | ||
|
|
||
| ### tests | ||
|
|
||
| to execute the test, see t/ directory in the package. | ||
| First install the dependency : | ||
| ```bash | ||
| # distro package to install : | ||
| openssl-dev | ||
| # cpan package to install : | ||
| Test2::V0 Test2::Plugin::NoWarnings Crypt::OpenSSL::AES | ||
| ``` | ||
|
|
||
| run the following command passing the path to t/ folder as argument : | ||
| ```bash | ||
| prove -r t/ | ||
| ``` | ||
|
|
||
|
|
||
| ## centreon-perl-libs | ||
|
|
||
| This package contain libraries associated to specific binary, they are located in centreon::[area] namespace. | ||
|
|
||
| for exemple the package centreon-trap contain a binary calling centreon::trap namespace which contain all the logic. | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add installation verification instructions.
The documentation should include information about package installation and verification, as mentioned in the PR objectives. This is crucial for users to confirm proper setup of both packages.
Add a new section:
## Installation
1. Install both packages:
```bash
# Install commands for your package manager- Verify the installation by testing library accessibility:
perl -e 'use centreon::common::centreonvault;'
Both commands should complete without errors, confirming successful installation.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 LanguageTool</summary>
[grammar] ~7-~7: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...centreon-perl-libs-common This package contain libraries located in centreon::common n...
(PLURAL_VERB_AFTER_THIS)
---
[grammar] ~29-~29: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...` ## centreon-perl-libs This package contain libraries associated to specific binary...
(PLURAL_VERB_AFTER_THIS)
---
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...ted in centreon::[area] namespace. for exemple the package centreon-trap contain a bin...
(AI_HYDRA_LEO_MISSING_COMMA)
</details>
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| ## centreon-perl-libs | ||
|
|
||
| This package contain libraries associated to specific binary, they are located in centreon::[area] namespace. | ||
|
|
||
| for exemple the package centreon-trap contain a binary calling centreon::trap namespace which contain all the logic. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix grammar and enhance documentation of available namespaces.
The section contains grammatical errors and could benefit from more detailed information about available namespaces.
## centreon-perl-libs
-This package contain libraries associated to specific binary, they are located in centreon::[area] namespace.
+This package contains libraries associated with specific binaries, located in the centreon::[area] namespace.
-for exemple the package centreon-trap contain a binary calling centreon::trap namespace which contain all the logic.
+For example:
+- centreon::trap: Contains trap management logic
+- centreon::reporting: Contains reporting functionality
+- centreon::dashboard: Contains dashboard script logicCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: The verb ‘contain’ is plural. Did you mean: “contains”? Did you use a verb instead of a noun?
Context: ...` ## centreon-perl-libs This package contain libraries associated to specific binary...
(PLURAL_VERB_AFTER_THIS)
[uncategorized] ~31-~31: Possible missing comma found.
Context: ...ted in centreon::[area] namespace. for exemple the package centreon-trap contain a bin...
(AI_HYDRA_LEO_MISSING_COMMA)
| deb: | ||
| depends: | ||
| - libconfig-inifiles-perl | ||
| - libcrypt-openssl-aes-perl | ||
| - libjson-xs-perl | ||
| - libnet-curl-perl | ||
| - libcrypt-des-perl | ||
| - librrds-perl | ||
| - libdigest-hmac-perl | ||
| - libdigest-sha-perl | ||
| - libgd-perl | ||
|
|
There was a problem hiding this comment.
Review Debian package dependencies.
Two issues to address:
- Some packages seem unrelated to the common functionality (librrds-perl, libgd-perl)
- Missing Vault-related dependencies
deb:
depends:
- libconfig-inifiles-perl
- libcrypt-openssl-aes-perl
- libjson-xs-perl
- libnet-curl-perl
- libcrypt-des-perl
- - librrds-perl # Should be in centreon-perl-libs
- - libgd-perl # Should be in centreon-perl-libs
- libdigest-hmac-perl
- libdigest-sha-perl
+ - libwww-perl # For Vault HTTP interactions
+ - libjson-perl # For Vault response parsing
+ - liburi-perl # For URL handling📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deb: | |
| depends: | |
| - libconfig-inifiles-perl | |
| - libcrypt-openssl-aes-perl | |
| - libjson-xs-perl | |
| - libnet-curl-perl | |
| - libcrypt-des-perl | |
| - librrds-perl | |
| - libdigest-hmac-perl | |
| - libdigest-sha-perl | |
| - libgd-perl | |
| deb: | |
| depends: | |
| - libconfig-inifiles-perl | |
| - libcrypt-openssl-aes-perl | |
| - libjson-xs-perl | |
| - libnet-curl-perl | |
| - libcrypt-des-perl | |
| - libdigest-hmac-perl | |
| - libdigest-sha-perl | |
| - libwww-perl | |
| - libjson-perl | |
| - liburi-perl | |
| depends: | ||
| - perl-interpreter | ||
| - perl(Crypt::OpenSSL::AES) | ||
| - perl(JSON::XS) | ||
| - perl(DBI) | ||
| - perl(FindBin) | ||
| - perl(IO::Handle) | ||
| - perl(POSIX) | ||
| - perl(Pod::Usage) | ||
| - perl(Sys::Syslog) | ||
| provides: |
There was a problem hiding this comment.
Add missing Vault-related dependencies.
Since this package includes vault integration functionality, consider adding these dependencies:
depends:
- perl-interpreter
- perl(Crypt::OpenSSL::AES)
- perl(JSON::XS)
- perl(DBI)
- perl(FindBin)
- perl(IO::Handle)
- perl(POSIX)
- perl(Pod::Usage)
- perl(Sys::Syslog)
+ - perl(LWP::UserAgent) # For HTTP interactions with Vault
+ - perl(MIME::Base64) # For token handling
+ - perl(URI::Escape) # For URL encoding in API calls📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| depends: | |
| - perl-interpreter | |
| - perl(Crypt::OpenSSL::AES) | |
| - perl(JSON::XS) | |
| - perl(DBI) | |
| - perl(FindBin) | |
| - perl(IO::Handle) | |
| - perl(POSIX) | |
| - perl(Pod::Usage) | |
| - perl(Sys::Syslog) | |
| provides: | |
| depends: | |
| - perl-interpreter | |
| - perl(Crypt::OpenSSL::AES) | |
| - perl(JSON::XS) | |
| - perl(DBI) | |
| - perl(FindBin) | |
| - perl(IO::Handle) | |
| - perl(POSIX) | |
| - perl(Pod::Usage) | |
| - perl(Sys::Syslog) | |
| - perl(LWP::UserAgent) # For HTTP interactions with Vault | |
| - perl(MIME::Base64) # For token handling | |
| - perl(URI::Escape) # For URL encoding in API calls | |
| provides: |
|
This pull request is no longer revelent. |
Evan-Adam
deleted the
MON-106121-pull-wss-add-gorgone-parameter-to-get-vault-credentials
branch
Description
move the centreonvault.pm file made in centreon-plugin to the generic library stored here.
splitted the package in 2, centreon-perl-libs-common will be installed by everyone, centreon-perl-libs will only be installed by webapp (it contain mainly logic for script about dashboard, trap management and reporting)
Fixes # mon-106121
Type of change
Target serie
How this pull request can be tested ?
install both package, they should not have any error. The library inside should all be callable by using a command of the form ``` perl -e 'use strict ; use centreon::common::logger; print "ok\n" ' ```Checklist
Community contributors & Centreon team