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.

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

Allow the password to be a Sensitive string. #150

Merged
merged 1 commit into from
Jun 10, 2022
Merged

Allow the password to be a Sensitive string. #150

merged 1 commit into from
Jun 10, 2022

Conversation

jcpunk
Copy link
Contributor

@jcpunk jcpunk commented Jun 1, 2022

Pull Request (PR) description

For folks doing mass casting of .password. to Sensitive,
this should help keep chrony working as expected.

This Pull Request (PR) fixes the following issues

N/A

@alexjfisher
Copy link
Member

I guess we already use Sensitive here.

content => Sensitive(epp($chrony::config_keys_template)),

but this means that we won't store the password as a parameter of the main class in PuppetDB??

@jcpunk
Copy link
Contributor Author

jcpunk commented Jun 1, 2022

In theory that should only impact folks who pass in a Sensitive value. Normal strings are still accepted.

@alexjfisher
Copy link
Member

But only if you're on Puppet 6.24.0 or later. Before then, unwrap would error if called on a plain String

@alexjfisher
Copy link
Member

See puppetlabs/puppet@41991b2

@alexjfisher
Copy link
Member

Which is probably fine. But you'd need to update the metadata.json and mark this as breaking. Alternatively, you need to conditionally unwrap the variable depending on its type.

Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

A decision needs to be made about either bumping the minimum puppet version in metadata.json and marking this as breaking, or adding the additional logic so that unwrap isn't called if the data type isn't Sensitive.

templates/chrony.keys.epp Outdated Show resolved Hide resolved
@jcpunk
Copy link
Contributor Author

jcpunk commented Jun 1, 2022

In theory I've reworked the template to better behavior with older puppet.

I may be missing something on the breaking change part.... as I understand it, unless someone explicitly passes in the argument as Sensitive, the behavior is the same as before.

@jcpunk jcpunk requested a review from alexjfisher June 1, 2022 17:34
@bastelfreak bastelfreak added the enhancement New feature or request label Jun 2, 2022
manifests/config.pp Outdated Show resolved Hide resolved
@jcpunk jcpunk requested a review from alexjfisher June 7, 2022 13:44
For folks doing mass casting of .*password.* to Sensitive,
this should help keep chrony working as expected.
@alexjfisher alexjfisher merged commit a48d2bd into voxpupuli:master Jun 10, 2022
@jcpunk jcpunk deleted the password-sensitive branch June 10, 2022 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants