Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Duplicate: Add sensitive support #857

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Duplicate: Add sensitive support #857

wants to merge 28 commits into from

Conversation

teluq-pbrideau
Copy link
Contributor

@teluq-pbrideau teluq-pbrideau commented Dec 12, 2022 •
edited
Loading

Pull Request (PR) description

This is a copy of #828, where @bdeferme do not have any more time to work on it. Here I tried to fix the tests.

This Pull Request (PR) fixes the following issues

Fixes #440
Fixes #950

@teluq-pbrideau teluq-pbrideau marked this pull request as draft December 12, 2022 20:25
@teluq-pbrideau
Copy link
Contributor Author

While trying to add some basic tests about this change, I’ve discovered that the zabbix::proxy tests aren’t run at all, it is restricted to when 'RedHat' which is not in the listed OS on_supported_os(baseline_os_hash)…

I’ve fixed the tests for them to run at least for CentOS, but they fail for other OS for reasons outside the scope of this PR:

zabbix::proxy
  on debian-10-x86_64
[...]
      is expected to contain Yumrepo[zabbix-nonsupported] (FAILED - 1)

It should probably be fixed it its own PR…

@teluq-pbrideau teluq-pbrideau marked this pull request as ready for review December 13, 2022 14:05
teluq-pbrideau
teluq-pbrideau commented Dec 13, 2022
View reviewed changes
manifests/params.pp
@@ -182,7 +182,7 @@
$ldap_clientcert = undef
$ldap_clientkey = undef
$ldap_reqcert = undef
$server_api_pass = 'zabbix'
$server_api_pass = Sensitive('zabbix')
Copy link
Contributor Author

@teluq-pbrideau teluq-pbrideau Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure if we should set default values as sensitive.

I’ve changed some tests because of this: if the default value is Sensitive, the output may be masked during tests, and the validation cannot be made. These lines are marked with this comment below:

# cleartext password must be explicitly declared in this test, otherwise the parser will secure content of the file

For example, see spec/classes/proxy_spec.rb:115

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that's okay

root-expert
root-expert reviewed Feb 11, 2023
View reviewed changes
Copy link
Member

@root-expert root-expert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I left some inline comments 😄

manifests/server.pp
Comment on lines +69 to +71
# @param vmwareperffrequency
# Delay in seconds between performance counter statistics retrieval from a single VMware service.
# This delay should be set to the least update interval of any VMware monitoring item that uses VMware performance counters.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to open a new PR for the new parameter

manifests/server.pp
@@ -132,6 +135,7 @@
# @param include_dir You may include individual files or all files in a directory in the configuration file.
# @param loadmodulepath Full path to location of server modules.
# @param loadmodule Module to load at server startup.
# @param sslcalocation_dir Location of certificate authority (CA) files for SSL server certificate verification.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to open a new PR for the new parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is @bdeferme discovered the following parameters in zabbix_server.conf while converting to the new epp template:

  • SSLCALocation
  • VMwarePerfFrequency

As they were present in upstream config, and noticed they were missing in the old erb template, and added them to be consistent with upstream config.

@root-expert I’ve deleted them as requested, but I think they should be included. However, I won’t go out of my way to create a new PR if it is not required in my own production environment.

manifests/params.pp
@@ -182,7 +182,7 @@
$ldap_clientcert = undef
$ldap_clientkey = undef
$ldap_reqcert = undef
$server_api_pass = 'zabbix'
$server_api_pass = Sensitive('zabbix')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO that's okay

@teluq-pbrideau teluq-pbrideau mentioned this pull request Jul 2, 2024
Open
@bastelfreak
Copy link
Member

@teluq-pbrideau thanks for working on this! Can you please rebase against our master branch?

@teluq-pbrideau
Copy link
Contributor Author

@bastelfreak I’m sorry but rebasing was just a pain in the ass with all the conflicts. I solved the conflict once with the merge of your master branch

@teluq-pbrideau
Copy link
Contributor Author

You might see I added back the SSLCALocation and VMwarePerfFrequency parameters as they were already defined in the .erb template, without beeing defined in the class. I think it is cleaner to follow how the template was, and adapting the class accordingly. It is such a pain to modify and update the erb vs the epp to keep them consistent.

@teluq-pbrideau
Copy link
Contributor Author

I’m not quite sure how to cleanup my git tree… Can we just squash to only one commit when we merge into master?

@teluq-pbrideau
Copy link
Contributor Author

I’ve been running the old changes in my production environment since my original PR, but I cannot test these new changes right now in a live environment. I’m stuck with a dependency cycle in my Puppetfile with stdlib9, which I’m in the process of solving right now, but might take a few days still to solve on my side.
Feel free to freeze this PR until I come back with confirmation on my side, or if someone can test in a more complete environment.

@cocker-cc cocker-cc mentioned this pull request Jul 9, 2024
Open
@teluq-pbrideau
Copy link
Contributor Author

I’ve just successfully tested this PR in my development environment. The failing tests are out of my control, it was a timeout after 360 minutes:

Error: The operation was canceled.

Can we force the tests to rerun and move forward with this? Or It can also be the changes #973!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@root-expert root-expert root-expert left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Accept Datatype Sensitive[String] for Passwords Manage resources leaks API password in resource types
3 participants
@teluq-pbrideau @bastelfreak @root-expert