(PUP-11326) Make regsubst() sensitive-aware #8799

cocker-cc · 2021-10-22T11:42:34Z

      $_ssl_key = $::facts['os']['family'] ? {                           
        'windows' => regsubst($ssl_key, '\n', "\r\n", 'EMG'),            
        default   => $ssl_key,                                           
      }

puppetlabs-jenkins · 2021-10-22T11:42:36Z

Can one of the admins verify this patch?

ciprianbadescu · 2022-01-04T12:29:09Z

lib/puppet/functions/regsubst.rb

@@ -30,7 +30,7 @@
  #   $i3 = regsubst($ipaddress,'^(\\d+)\\.(\\d+)\\.(\\d+)\\.(\\d+)$','\\3')
  #   ```
  dispatch :regsubst_string do
-    param          'Variant[Array[String],String]',       :target
+    param          'Variant[Array[Variant[String,Sensitive[String]]],Variant[String,Sensitive[String]]]', :target


This change allows you also to call the function with an array of mixed strings and sensitive data.
I think that the change from inner_regsubst implements only Variant[Array[String],Sensitive[String],String], which I think could be enough, if it matches your use case.

You are right, this does not fit together. I refined the Function and amended the Commit. Also I added some basic Tests.

CLAassistant · 2022-02-07T16:29:00Z

All committers have signed the CLA.

joshcooper · 2022-02-22T21:21:47Z

Thanks @cocker-cc you'll want to rebase this PR against #8879 after it's merged to resolve the rspec issue on Ruby 3

joshcooper · 2022-02-23T08:14:56Z

lib/puppet/functions/regsubst.rb

@@ -95,7 +95,27 @@ def regsubst_regexp(target, pattern, replacement, flags = nil)
  end

  def inner_regsubst(target, re, replacement, op)
-    target.respond_to?(op) ? target.send(op, re, replacement) : target.collect { |e| e.send(op, re, replacement) }
+    if target.respond_to?(:map)


I'm not a big fan of checking if something is an Array/Sensitive by checking if it responds to map/unwrap. For example, all classes that include Enumerable will respond to map. I usually prefer checking if the object is an instance of the expected class or a subclass, e.g. item.is_a?(Array)

I'd like to argue opposite-wise: If I want the Duck to quack, then I have to test, if she is able to quack.
If you test for a particular Datatype, then you have to cover all possible Cases.
Not implemented.

Agree with Josh here, use of "responds to" is too brittle and is a style that is not favored throughout the puppet code base. (yes, we have been bitten by that a number of times).

Have changed it…

lib/puppet/functions/regsubst.rb

hlindberg · 2022-08-30T11:47:47Z

lib/puppet/functions/regsubst.rb

+      Puppet::Pops::Types::PSensitiveType::Sensitive.new(target)
+    else
+      # this should be a String
+      target.respond_to?(op) ? target.send(op, re, replacement) : target.collect { |e| e.send(op, re, replacement) }


Suggested change

target.respond_to?(op) ? target.send(op, re, replacement) : target.collect { |e| e.send(op, re, replacement) }

target.respond_to?(op) ? target.send(op, re, replacement) : target.map { |e| e.send(op, re, replacement) }

hlindberg · 2022-08-30T11:52:50Z

There is one more possible Sensitive permutation to support - that of Sensitive[Array] where the array may contain sensitives or strings. That case may be difficult to describe in the dispatch section since a Sensitive type does not present more than the base type of its content. Easiest is probably to have one dispatch for Sensitive[Array] that unwraps the Array and then recursively calls the function to get the type checking.
Not sure how valuable it is to also support that case.

Co-authored-by: Henrik Lindberg <563066+hlindberg@users.noreply.github.com>: map preferred over collect Co-authored-by: Henrik Lindberg <563066+hlindberg@users.noreply.github.com>

cocker-cc requested a review from a team October 22, 2021 11:42

cocker-cc requested review from a team October 22, 2021 11:42

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from e3d1ea9 to ed4a58a Compare October 22, 2021 11:48

cocker-cc mentioned this pull request Oct 22, 2021

Accept Datatype Sensitive for Secrets Icinga/puppet-icinga2#668

Closed

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from ed4a58a to 520b873 Compare October 22, 2021 12:34

cocker-cc changed the title ~~Make regsubst() sensitive-aware~~ (PUP-11326) Make regsubst() sensitive-aware Oct 22, 2021

ciprianbadescu reviewed Jan 4, 2022

View reviewed changes

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 520b873 to 4ec8679 Compare January 17, 2022 12:47

cocker-cc requested a review from a team as a code owner January 17, 2022 12:47

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 4ec8679 to bd61004 Compare February 7, 2022 19:02

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from bd61004 to b8a49d2 Compare February 23, 2022 01:51

joshcooper reviewed Feb 23, 2022

View reviewed changes

lib/puppet/functions/regsubst.rb Outdated Show resolved Hide resolved

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from b8a49d2 to 4a556d2 Compare February 23, 2022 10:11

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 4a556d2 to b998ce7 Compare April 14, 2022 21:00

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch 3 times, most recently from 320fc93 to a7dc145 Compare July 29, 2022 15:18

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from a7dc145 to 10c8b22 Compare August 30, 2022 11:32

hlindberg reviewed Aug 30, 2022

View reviewed changes

lib/puppet/functions/regsubst.rb Outdated Show resolved Hide resolved

hlindberg reviewed Aug 30, 2022

View reviewed changes

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from cff4ef6 to 6ebddd7 Compare October 17, 2022 11:57

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 6ebddd7 to 425a6f3 Compare April 21, 2023 12:40

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 425a6f3 to 6d30ad8 Compare June 19, 2023 12:17

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 6d30ad8 to aac6132 Compare August 4, 2023 11:28

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from aac6132 to 92f6efd Compare September 20, 2023 11:08

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch 2 times, most recently from 92b15f6 to 6b4a0ae Compare October 5, 2023 08:55

(PUP-11326) Make regsubst() sensitive-aware

a580772

Co-authored-by: Henrik Lindberg <563066+hlindberg@users.noreply.github.com>: map preferred over collect Co-authored-by: Henrik Lindberg <563066+hlindberg@users.noreply.github.com>

cocker-cc force-pushed the Make_regsubst_sensitive-aware branch from 6b4a0ae to a580772 Compare November 7, 2023 10:26

joshcooper added the enhancement New feature or request label Nov 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(PUP-11326) Make regsubst() sensitive-aware #8799

(PUP-11326) Make regsubst() sensitive-aware #8799

cocker-cc commented Oct 22, 2021 •

edited

puppetlabs-jenkins commented Oct 22, 2021

ciprianbadescu Jan 4, 2022 •

edited

cocker-cc Jan 17, 2022 •

edited

CLAassistant commented Feb 7, 2022 •

edited

joshcooper commented Feb 22, 2022

joshcooper Feb 23, 2022

cocker-cc Feb 23, 2022

hlindberg Jul 28, 2022

cocker-cc Jul 28, 2022

hlindberg Aug 30, 2022

hlindberg commented Aug 30, 2022

	target.respond_to?(op) ? target.send(op, re, replacement) : target.collect { \|e\| e.send(op, re, replacement) }
	target.respond_to?(op) ? target.send(op, re, replacement) : target.map { \|e\| e.send(op, re, replacement) }

(PUP-11326) Make regsubst() sensitive-aware #8799

Are you sure you want to change the base?

(PUP-11326) Make regsubst() sensitive-aware #8799

Conversation

cocker-cc commented Oct 22, 2021 • edited

puppetlabs-jenkins commented Oct 22, 2021

ciprianbadescu Jan 4, 2022 • edited

Choose a reason for hiding this comment

cocker-cc Jan 17, 2022 • edited

Choose a reason for hiding this comment

CLAassistant commented Feb 7, 2022 • edited

joshcooper commented Feb 22, 2022

joshcooper Feb 23, 2022

Choose a reason for hiding this comment

cocker-cc Feb 23, 2022

Choose a reason for hiding this comment

hlindberg Jul 28, 2022

Choose a reason for hiding this comment

cocker-cc Jul 28, 2022

Choose a reason for hiding this comment

hlindberg Aug 30, 2022

Choose a reason for hiding this comment

hlindberg commented Aug 30, 2022

cocker-cc commented Oct 22, 2021 •

edited

ciprianbadescu Jan 4, 2022 •

edited

cocker-cc Jan 17, 2022 •

edited

CLAassistant commented Feb 7, 2022 •

edited