Parse dialog options as Extra Vars #53

agrare · 2024-06-03T14:33:28Z

When you have a dialog pass a value for a service order it comes in as:

options
{:dialog=>{"dialog_text_box_1"=>"hello world"}

We need to extract this and pass through to the Terraform::Runner as Extra Vars

With this change the Job options now look like:

 options:
  {:template_id=>4,
   :env_vars=>{},
   :input_vars=>{"execution_ttl"=>"", "extra_vars"=>{"text_box_1"=>"hello world"}, "verbosity"=>"0"},
   :credentials=>[3],
   :poll_interval=>1 minute,
   :git_checkout_tempdir=>"/tmp/embedded-terraform-runner-git20240603-334161-40j3os"},

Fryguy · 2024-06-03T17:02:53Z

app/models/service_terraform_template.rb

+    dialog_options = options[:dialog] || {}
+
+    params = dialog_options.each_with_object({}) do |(attr, val), obj|
+      var_key = attr.sub(/^(password::)?dialog_/, '')


are the password values encrypted? If so, do they need to be decrypted?

They can be, and yes they do need to be

Fryguy · 2024-06-03T17:03:50Z

app/models/service_terraform_template.rb

+
+    params = dialog_options.each_with_object({}) do |(attr, val), obj|
+      var_key = attr.sub(/^(password::)?dialog_/, '')
+      obj[var_key] = val unless var_key == attr


Is this saying to not clobber the value if it's already an extra var? If so, I'm not sure that's right. Typically the dialog overrides any hardcoded values.

So background most of this method is pulled form embedded ansible, https://github.com/ManageIQ/manageiq/blob/master/app/models/service_ansible_playbook.rb#L167

I don't think that is the intent, since this is only looking at options[:dialog] and that isn't where hard-coded extra vars are stored.

I'm trying to dig into why ansible does this and the password decryption, we might be able to drop both and just go with a simpler approach.

Ah okay I thought that was an ansible specific thing but protected dialog fields look like "password::dialog_password"=>"v2:{1aCicMVgRQFLeGN5K/LWcA==}"}

I'm not sure that I like where the decrypt is done with embedded_ansible because it looks like the Job.options will have the plaintext value...

right, the password:: prefix is there to ensure that we don't print it in logs (if we see password in the key we are certain it's a sensitive value)

it does have to be removed at the last minute before sending it over, so that part is correct - I just don't know where the decryption also happens.

This was added to embedded_ansible here ManageIQ/manageiq#14636 and it is decrypted here https://github.com/ManageIQ/manageiq/pull/14636/files#diff-43965f59b3501c850937c667f9712b5d46a04da94da5385d74f2fc187f170ca0R16

I've opted to decrypt right before sending to Terraform::Runner so the plaintext values don't make it to the Job.options column. I might be missing something on the embedded_ansible side maybe they're re-encrypting them before creating the Job but this seemed like a good way to go.

That was also where the check for unless var_key == attr maybe so that password and non-password text boxes don't overwrite each other?

Fryguy · 2024-06-03T17:38:10Z

app/models/manageiq/providers/embedded_terraform/automation_manager/job.rb

@@ -21,13 +21,9 @@ def pre_execute
  def execute
    template_path = File.join(options[:git_checkout_tempdir], template_relative_path)
    credentials   = Authentication.where(:id => options[:credentials])
+    input_vars    = decrypt_input_vars(options[:input_vars])


Please double check that we aren't printing the input vars down in Terraform::Runner.run. If we are, then, alternatively, we could decrypt them after any printing that is done inside of the Terraform::Runner.run. I think Ansible::Runner decrypts inside of itself as well, right before it sends it over, but I could be wrong

Not seeing anywhere that logs the input_vars, https://github.com/ManageIQ/manageiq-providers-embedded_terraform/blob/master/lib/terraform/runner.rb#L29

Here is what it looks like in the logs with debug on

[----] I, [2024-06-03T15:07:06.459082#457302:9ee8] INFO -- evm: MIQ(MiqQueue#deliver) Message id: [240], Delivering... [----] I, [2024-06-03T15:07:06.494993#457302:9ee8] INFO -- evm: MIQ(ManageIQ::Providers::EmbeddedTerraform::AutomationManager::Job#checkout_git_repository) Checking out git repository to "/tmp/embedded-terraform-runner-git20240603-457302-yitu28"... [----] I, [2024-06-03T15:07:06.610938#457302:9ee8] INFO -- evm: MIQ(GitRepository#update_repo) Updating [email protected]:manoj-puthran/cam-sample-templates.git in /home/grare/adam/src/manageiq/manageiq/data/git_repos/1... [----] I, [2024-06-03T15:07:06.971855#457302:9ee8] INFO -- evm: MIQ(GitRepository#update_repo) Updating [email protected]:manoj-puthran/cam-sample-templates.git in /home/grare/adam/src/manageiq/manageiq/data/git_repos/1......Complete [----] I, [2024-06-03T15:07:06.972215#457302:9ee8] INFO -- evm: MIQ(GitRepository#checkout) Checking out [email protected]:manoj-puthran/cam-sample-templates.git@master to /tmp/embedded-terraform-runner-git20240603-457302-yitu28... [----] I, [2024-06-03T15:07:06.981529#457302:9ee8] INFO -- evm: MIQ(GitRepository#checkout) Checking out [email protected]:manoj-puthran/cam-sample-templates.git@master to /tmp/embedded-terraform-runner-git20240603-457302-yitu28......Complete [----] D, [2024-06-03T15:07:07.059332#457302:9ee8] DEBUG -- evm: MIQ(Terraform::Runner.run_async) Run_aysnc template: /tmp/embedded-terraform-runner-git20240603-457302-yitu28/terraform/hello-world [----] I, [2024-06-03T15:07:07.059475#457302:9ee8] INFO -- evm: MIQ(Terraform::Runner.create_stack_job) start stack_job for template: /tmp/embedded-terraform-runner-git20240603-457302-yitu28/terraform/hello-world [----] D, [2024-06-03T15:07:07.059719#457302:9ee8] DEBUG -- evm: MIQ(Terraform::Runner.encoded_zip_from_directory) Create #<File:0x00007ff4f3e81ff8> [----] D, [2024-06-03T15:07:07.059865#457302:9ee8] DEBUG -- evm: MIQ(Terraform::Runner.encoded_zip_from_directory) Adding camtemplate.json [----] D, [2024-06-03T15:07:07.059969#457302:9ee8] DEBUG -- evm: MIQ(Terraform::Runner.encoded_zip_from_directory) Adding camvariables.json [----] D, [2024-06-03T15:07:07.060044#457302:9ee8] DEBUG -- evm: MIQ(Terraform::Runner.encoded_zip_from_directory) Adding main.tf

No input_vars printed (thought the #<File:0x00007ff4f3e81ff8> looks like a bug)

I think Ansible::Runner decrypts inside of itself as well, right before it sends it over, but I could be wrong

I don't see anything in Ansible::Runner calling ManageIQ::Password

Fryguy · 2024-06-03T18:08:54Z

app/models/manageiq/providers/embedded_terraform/automation_manager/job.rb

+    input_vars
+      .dup
+      .fetch("extra_vars", {})
+      .transform_values { |val| val.kind_of?(String) ? ManageIQ::Password.try_decrypt(val) : val }


This dup does not work as expected (it only dups the outer hash, but not the inner hash), however it's also not needed since transform_values will create a new hash anyway:

[1] pry(main)> input_vars = {"extra_vars" => {}} [2] pry(main)> [input_vars.object_id, input_vars["extra_vars"].object_id] => [320, 340] [3] pry(main)> iv2 = input_vars.dup [4] pry(main)> [iv2.object_id, iv2["extra_vars"].object_id] => [360, 340] [5] pry(main)> ev = iv2.fetch("extra_vars", {}) [6] pry(main)> ev.object_id => 340 [7] pry(main)> ev2 = ev.transform_values {} [8] pry(main)> ev2.object_id => 400

?> def decrypt_input_vars(input_vars) ?> result = input_vars.deep_dup ?> result.fetch("extra_vars", {}).transform_values! { |val| val.kind_of?(String) ? ManageIQ::Password.try_decrypt(val) : val } ?> result >> end => :decrypt_input_vars >> input_vars = {"extra_vars" => {"foo" => "bar", "bar" => ManageIQ::Password.encrypt("baz")}} => {"extra_vars"=>{"foo"=>"bar", "bar"=>"v2:{8v956e2OHCGXgGW7Q7xxDw==}"}} >> decrypt_input_vars(input_vars) => {"extra_vars"=>{"foo"=>"bar", "bar"=>"baz"}} >> input_vars => {"extra_vars"=>{"foo"=>"bar", "bar"=>"v2:{8v956e2OHCGXgGW7Q7xxDw==}"}}

I want to avoid modifying the original options and saving a decrypted value, went with deep_dup

Fryguy · 2024-06-03T18:53:20Z

@agrare This now returns a different thing than the original commit (can't tell if intentional or not)

  def decrypt_input_vars(input_vars)
    result = input_vars.deep_dup
    result.fetch("extra_vars", {}).transform_values! { |val| val.kind_of?(String) ? ManageIQ::Password.try_decrypt(val) : val }
    result
  end

  def decrypt_input_vars_orig(input_vars)
    input_vars
      .dup
      .fetch("extra_vars", {})
      .transform_values { |val| val.kind_of?(String) ? ManageIQ::Password.try_decrypt(val) : val }
  end

[7] pry(main)> decrypt_input_vars("extra_vars" => {"foo" => "bar"}, "flarp" => {})
=> {"extra_vars"=>{"foo"=>"bar"}, "flarp"=>{}}
[8] pry(main)> decrypt_input_vars_orig("extra_vars" => {"foo" => "bar"}, "flarp" => {})
=> {"foo"=>"bar"}

agrare · 2024-06-03T18:54:56Z

@Fryguy I originally intended to return the entire input_vars payload since that is what embedded_ansible is doing

agrare · 2024-06-03T19:08:42Z

Actually looking at the Terraform::Runner.run_async docs they might want the extra_vars passed in rather than the whole input_vars

agrare · 2024-06-03T19:21:42Z

Okay updated, should be good to go now.

miq-bot · 2024-06-03T19:27:17Z

Checked commit agrare@cd750cd with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Fryguy · 2024-06-03T19:30:03Z

Backported to radjabov in commit 4830ad3.

commit 4830ad3a3f2ccfd8cc541ed43a04fdb158ee9cab
Author: Jason Frey <[email protected]>
Date:   Mon Jun 3 15:29:43 2024 -0400

    Merge pull request #53 from agrare/parse_dialog_options_as_extra_vars
    
    Parse dialog options as Extra Vars
    
    (cherry picked from commit 41c4a5c946ef88b0841bfe17eaa3e7cea393620e)

Parse dialog options as Extra Vars (cherry picked from commit 41c4a5c)

agrare added bug Something isn't working radjabov/yes? labels Jun 3, 2024

agrare assigned Fryguy Jun 3, 2024

Fryguy reviewed Jun 3, 2024

View reviewed changes

agrare force-pushed the parse_dialog_options_as_extra_vars branch from d70e3fd to 2ed1f08 Compare June 3, 2024 17:35

Fryguy reviewed Jun 3, 2024

View reviewed changes

agrare force-pushed the parse_dialog_options_as_extra_vars branch from 2ed1f08 to 47b10d3 Compare June 3, 2024 18:07

Fryguy reviewed Jun 3, 2024

View reviewed changes

agrare force-pushed the parse_dialog_options_as_extra_vars branch from 47b10d3 to ba60ab9 Compare June 3, 2024 18:16

Parse dialog options as Extra Vars

cd750cd

agrare force-pushed the parse_dialog_options_as_extra_vars branch from ba60ab9 to cd750cd Compare June 3, 2024 19:20

Fryguy approved these changes Jun 3, 2024

View reviewed changes

Fryguy merged commit 41c4a5c into ManageIQ:master Jun 3, 2024
4 checks passed

Fryguy added radjabov/yes and removed radjabov/yes? labels Jun 3, 2024

Fryguy added a commit that referenced this pull request Jun 3, 2024

Merge pull request #53 from agrare/parse_dialog_options_as_extra_vars

4830ad3

Parse dialog options as Extra Vars (cherry picked from commit 41c4a5c)

Fryguy added radjabov/backported and removed radjabov/yes labels Jun 3, 2024

agrare deleted the parse_dialog_options_as_extra_vars branch June 3, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse dialog options as Extra Vars #53

Parse dialog options as Extra Vars #53

agrare commented Jun 3, 2024 •

edited

Loading

Fryguy Jun 3, 2024

agrare Jun 3, 2024

Fryguy Jun 3, 2024

agrare Jun 3, 2024

agrare Jun 3, 2024

Fryguy Jun 3, 2024 •

edited

Loading

agrare Jun 3, 2024

agrare Jun 3, 2024

Fryguy Jun 3, 2024

agrare Jun 3, 2024

Fryguy Jun 3, 2024

agrare Jun 3, 2024

Fryguy commented Jun 3, 2024

agrare commented Jun 3, 2024 •

edited

Loading

agrare commented Jun 3, 2024

agrare commented Jun 3, 2024

miq-bot commented Jun 3, 2024

Fryguy commented Jun 3, 2024

Parse dialog options as Extra Vars #53

Parse dialog options as Extra Vars #53

Conversation

agrare commented Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jun 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jun 3, 2024

agrare commented Jun 3, 2024 • edited Loading

agrare commented Jun 3, 2024

agrare commented Jun 3, 2024

miq-bot commented Jun 3, 2024

Fryguy commented Jun 3, 2024

agrare commented Jun 3, 2024 •

edited

Loading

Fryguy Jun 3, 2024 •

edited

Loading

agrare commented Jun 3, 2024 •

edited

Loading