Skip to content

Accept Sensitive for Secrets #436

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

Open
cocker-cc wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Accept Sensitive for Secrets #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion manifests/candlepin.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
Optional[Stdlib::Port] $db_port = undef,
String $db_name = 'candlepin',
String $db_user = 'candlepin',
Optional[String] $db_password = undef,
Optional[Variant[Sensitive[String], String]] $db_password = undef,
Boolean $db_ssl = false,
Boolean $db_ssl_verify = true,
Optional[Stdlib::Absolutepath] $db_ssl_ca = undef,
Expand Down
2 changes: 1 addition & 1 deletion manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
Optional[Stdlib::Port] $candlepin_db_port = undef,
String $candlepin_db_name = 'candlepin',
String $candlepin_db_user = 'candlepin',
Optional[String] $candlepin_db_password = undef,
Optional[Variant[Sensitive[String], String]] $candlepin_db_password = undef,
Copy link
Member

@ekohl ekohl Feb 9, 2022

Choose a reason for hiding this comment

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

I found with puppet-foreman testing that this wouldn't work well with our installer. It writes out all the answers so you end up with Sensitive[Undef], which isn't accepted here.

So what you end with in the installer answers file is:

katello:
  candlepin_db_password:

I think this translates to Hiera as:

katello::candlepin_db_password:

So that ends up sending undef via Hiera. If you add a conversion to sensitive in Hiera using lookup_options, this ends up as Sensitive[Undef].

Suggested change
Optional[Variant[Sensitive[String], String]] $candlepin_db_password = undef,
Variant[Sensitive[Optional[String]], Optional[String]] $db_password = undef,

This would then also need to be reflected in all other types.

I merged theforeman/kafo#345, will release it shortly. I'm also considering a solution where we add the lookup option and use this option:

Suggested change
Optional[Variant[Sensitive[String], String]] $candlepin_db_password = undef,
Sensitive[Optional[String]] $candlepin_db_password = sensitive(undef),

That would be a lot more straight forward. It may be breaking to users who pass in arguments directly from Puppet, but we can look at that.

Copy link
Author

@cocker-cc cocker-cc Feb 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I have no Knowledge, how the Installer-Answer-File works, but Hiera does not translate

katello:
  candlepin_db_password:

to katello::candlepin_db_password. Instead what Hiera does here, is: Lookup a Hash named katello, find the Hash named katello, which turns out to have a Key named candlepin_db_password.

Isn't it possible to state an empty String "" instead of "nothing"

katello:
  candlepin_db_password: ""

in the Installer-Answer-File?

Copy link
Member

@ekohl ekohl Feb 9, 2022

Choose a reason for hiding this comment

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

Not really. What the installer does is that it extracts the default value from Puppet and stores that. Thinking more about it, other code would probably also fail, like if $candlepin_db_password {} is "false" with undef but perhaps Sensitive[Undef] may not.

I wonder if this is a more fundamental problem in Puppet and the lookup option may provide to only convert it if it isn't undef. Perhaps we need a conversion function that essentially is to_optional_sensitive(Optional[Any]) -> Optional[Sensitive[Any]]) (compared to sensitive(Any) -> Sensitive[Any]). Does that make sense?

Copy link
Author

@cocker-cc cocker-cc Feb 9, 2022

Choose a reason for hiding this comment

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

Yes, this totally makes Sense.
I created an Issue in the Hiera-Board: https://tickets.puppetlabs.com/browse/HI-628

Copy link
Member

@ekohl ekohl Feb 9, 2022

Choose a reason for hiding this comment

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

Thanks! In theforeman/puppet-foreman#1018 we had a less structured discussion but I think I now understand the problem properly and I'm an issue there is helpful moving it forward. Thanks for distilling it down to the minimal problem statement.

Boolean $candlepin_db_ssl = false,
Boolean $candlepin_db_ssl_verify = true,
Optional[Stdlib::Absolutepath] $candlepin_db_ssl_ca = undef,
Expand Down