Fix save_to_file to be indentation independent #10283

ShimShtein · 2024-08-20T10:09:52Z

Heredoc notation is very sensitive to leading spaces.
This implementation makes the save_to_file macro safe for indentation.

ekohl

I don't think you're going to find any solution that works together with indent.

ekohl · 2024-08-20T11:04:06Z

app/services/foreman/renderer/scope/macros/base.rb

-              content += "\n" unless content.end_with?("\n")
-              "cat << #{delimiter} > #{filename}\n#{content}#{delimiter}"
+              content_echos = content.split("\n").map do |content_line|
+                "echo #{Shellwords.shellescape(content_line)} >> #{filename}"


Previously the code allowed to use variables like $var but now that's going to be escaped, making it backwards incompatible. If you want it really verbatim, we have the base64 method above.

This should be possible now

Now the verbatim will escape everything and the else will escape only double quotes.

@adamruzicka you initially introduced this because you found a case where it broke. Do you think this is sufficient?

I don't really see where this wouldn't work, but I don't have a good feeling about it.

As a counter-proposal, how about we keep base64, but on the ruby side we drop the newlines from it and then echo it? Something like

content = Base64.encode64(content).tr("\n", "") "echo #{content} | base64 -d > #{filename}

@adamruzicka https://serverfault.com/questions/163371/linux-command-line-character-limit says that there is some limit.

I agree that it's quite long and we probably won't hit it soon, but I preferred not to rely on any of it.
I have tried to work with base64, but somehow I wasn't able to make it work properly with multiple lines and variable name. I can use a tempfile, but it seems a bit of overkill.

says that there is some limit.

Well, yes and no. From what I've seen, what's written there applies when launching things that are external to the shell, but it doesn't seem to apply to shell builtins, such as echo in this case.

I have tried to work with base64, but somehow I wasn't able to make it work properly with multiple lines and variable name

How about we mix the two approaches?

b64 = Base64.encode64(content) content_echos = b64.each_line.map { |l| "echo #{l}" }.join "(#{content_echos}) | base64 -d >#{filename}"

If you go the echo path, you need to accumulate the result somewhere. and once you have the whole base64 string accumulated, you can pass it into base64.
Let me try to accumulate the base64 value without newlines and then pipe it into base64.
Something like:

b64 = Base64.encode64(content) content_var = b64.each_line.map { |l| "FOO+=#{l}" }.join("\n") content_var + "echo $FOO | base64 -d >#{filename}"

Mostly I had trouble with the line endings. Maybe in this approach it will be a bit easier.

But let's say this works, we will have different implementations for verbatim and else use cases.
On the other hand, if we go with the escaping approach, we will be able to DRY the code by extracting most of the generation into a single method, and just changing the escaping method of the input.

If you go the echo path, you need to accumulate the result somewhere

Well, you can always spawn a subshell just like I did in the previous suggestion.

On the other hand, if we go with the escaping approach, we will be able to DRY the code by extracting most of the generation into a single method, and just changing the escaping method of the input.

True, at the same time we lose the option to write out binary blobs

Reimplemented as base64 single string.

ekohl · 2024-08-20T11:04:37Z

app/services/foreman/renderer/scope/macros/base.rb

@@ -124,8 +124,12 @@ def save_to_file(filename, content, verbatim: false)
              content = Base64.encode64(content)
              "cat << #{delimiter} | base64 -d > #{filename}\n#{content}#{delimiter}"


This case will still have an issue with indent().

Do you think we will have \n symbols there? I can implement the same solution here too

Changed the verbatim option to stop using heredoc too

ShimShtein · 2024-08-21T09:25:40Z

This should fix the indentation error mentioned in #10283

ekohl

This is not a change I feel comfortable cherry picking so I'd still prefer to revert the offending change in the short term and then figure this out.

ShimShtein · 2024-08-22T10:54:51Z

but then we will be left with broken cloud-init, unless we redesign it as you suggest. Additionally we will be left with save_to_file that is unusable if indented

ShimShtein · 2024-08-22T15:54:35Z

So I tracked down the whole chain of events:

Fixes #37601 - Add Foreman CA refresh template #10208 introduced a bug into cloud-init (it made the code indented incorrectly, so cloud-init yaml bcame invalid)
Fixes #37433 - Set indentation when calling subscription_manager_setup #10153 fixed the indentation problem, but exposed a lingering problem with save_to_file, since this helper method does not work well with indentation

Now this PR tries to avoid reverting the previous two

ekohl · 2024-08-22T15:58:39Z

* [Fixes #37433 - Set indentation when calling subscription_manager_setup #10200](https://github.com/theforeman/foreman/pull/10200) fixed the indentation problem, but exposed a lingering problem with `save_to_file`, since this helper method does not work well with indentation

I think you mean #10153

ShimShtein · 2024-08-22T16:12:57Z

Updated the original comment

ekohl · 2024-08-26T12:51:51Z

test/static_fixtures/script.txt

+#!/bin/sh
+
+echo "Calling Ansible AWX/Tower provisioning callback..."
+/usr/bin/curl -v -k -s --data "host_config_key=" https:///api/v2/job_templates//callback/


This is missing some interpolated values. Can we somehow set up the environment so those are set?

Added another test with interpolated values.

ShimShtein · 2024-08-27T18:53:33Z

I have replaced base64 implementation with shellescape one. The whole idea of verbatim flag is to place the exact strings in the file as opposed to parameter interpolation in verbatim=false use case.

I have implemented them both in a similar manner with the only difference what symbols should be escaped: in verbatim everything is escaped, in non-verbatim only " is escaped.

adamruzicka · 2024-08-28T11:30:35Z

app/services/foreman/renderer/scope/macros/base.rb

+              end.join("\n")
+
+              # prefix the append commands with a cleanup command
+              "> #{filename}\n#{content_echos}"


This relies on stdin being closed, right? Can we safely assume that stdin will be closed in all the scenarios where the save_to_file macro might be used?

Thanks! Now the implementation does not rely on stdin.

adamruzicka · 2024-08-29T08:21:17Z

app/services/foreman/renderer/scope/macros/base.rb

-              content += "\n" unless content.end_with?("\n")
-              "cat << #{delimiter} > #{filename}\n#{content}#{delimiter}"
+              content_echos = content.split("\n").map do |content_line|
+                "echo #{Shellwords.shellescape(content_line)} >> #{filename}"


I don't really see where this wouldn't work, but I don't have a good feeling about it.

As a counter-proposal, how about we keep base64, but on the ruby side we drop the newlines from it and then echo it? Something like

content = Base64.encode64(content).tr("\n", "") "echo #{content} | base64 -d > #{filename}

ShimShtein · 2024-09-17T10:39:08Z

@adamruzicka forgot to push the version. Now it should work on all OS-es. What do you think about this implementation?

adamruzicka

One last question, otherwise lgtm

adamruzicka · 2024-09-18T08:06:56Z

app/services/foreman/renderer/scope/macros/base.rb

+              base64 = Base64.encode64(content)
+              content_assignments = base64.split("\n").map { |l| "echo #{Shellwords.shellescape(l)}" }.join("\n")


Is the escaping even needed? Base64 shouldn't contain any problematic characters.

pr-processor bot added Not yet reviewed Waiting on contributor labels Aug 20, 2024

ShimShtein marked this pull request as draft August 20, 2024 10:10

ekohl requested changes Aug 20, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Aug 20, 2024

ShimShtein force-pushed the fix_save_to_file branch from 01a722f to f464a5b Compare August 20, 2024 15:11

pr-processor bot added Needs re-review and removed Needs re-review labels Aug 20, 2024

ekohl requested changes Aug 22, 2024

View reviewed changes

ShimShtein force-pushed the fix_save_to_file branch from f464a5b to 100eece Compare August 22, 2024 17:26

pr-processor bot added the Needs re-review label Aug 22, 2024

ekohl reviewed Aug 26, 2024

View reviewed changes

ShimShtein force-pushed the fix_save_to_file branch 12 times, most recently from 18101e9 to 7b9056c Compare August 27, 2024 18:25

ShimShtein marked this pull request as ready for review August 27, 2024 18:45

adamruzicka reviewed Aug 29, 2024

View reviewed changes

pr-processor bot removed the Needs re-review label Aug 29, 2024

ShimShtein force-pushed the fix_save_to_file branch from 7b9056c to 9ac7e27 Compare September 5, 2024 09:45

pr-processor bot added Needs re-review and removed Waiting on contributor Needs re-review labels Sep 5, 2024

ShimShtein force-pushed the fix_save_to_file branch 2 times, most recently from 23660fd to bde58c1 Compare September 5, 2024 10:28

Fixes #37741 - Fix save_to_file to be indentation independent

132a978

ShimShtein force-pushed the fix_save_to_file branch from bde58c1 to 132a978 Compare September 17, 2024 10:20

adamruzicka reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix save_to_file to be indentation independent #10283

Fix save_to_file to be indentation independent #10283

ShimShtein commented Aug 20, 2024

ekohl left a comment

ekohl Aug 20, 2024

ShimShtein Aug 20, 2024

ShimShtein Aug 27, 2024

ekohl Aug 27, 2024

adamruzicka Aug 29, 2024

ShimShtein Sep 2, 2024

adamruzicka Sep 2, 2024

ShimShtein Sep 2, 2024

adamruzicka Sep 2, 2024

ShimShtein Sep 5, 2024

ekohl Aug 20, 2024

ShimShtein Aug 20, 2024

ShimShtein Aug 20, 2024

ShimShtein commented Aug 21, 2024

ekohl left a comment

ShimShtein commented Aug 22, 2024

ShimShtein commented Aug 22, 2024 •

edited

Loading

ekohl commented Aug 22, 2024

ShimShtein commented Aug 22, 2024

ekohl Aug 26, 2024

ShimShtein Aug 27, 2024

ShimShtein commented Aug 27, 2024

adamruzicka Aug 28, 2024

ShimShtein Sep 5, 2024

adamruzicka Aug 29, 2024

ShimShtein commented Sep 17, 2024

adamruzicka left a comment

adamruzicka Sep 18, 2024

		@@ -124,8 +124,12 @@ def save_to_file(filename, content, verbatim: false)
		content = Base64.encode64(content)
		"cat << #{delimiter} \| base64 -d > #{filename}\n#{content}#{delimiter}"

		base64 = Base64.encode64(content)
		content_assignments = base64.split("\n").map { \|l\| "echo #{Shellwords.shellescape(l)}" }.join("\n")

Fix save_to_file to be indentation independent #10283

Are you sure you want to change the base?

Fix save_to_file to be indentation independent #10283

Conversation

ShimShtein commented Aug 20, 2024

ekohl left a 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

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

Choose a reason for hiding this comment

ShimShtein commented Aug 21, 2024

ekohl left a comment

Choose a reason for hiding this comment

ShimShtein commented Aug 22, 2024

ShimShtein commented Aug 22, 2024 • edited Loading

ekohl commented Aug 22, 2024

ShimShtein commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Sep 17, 2024

adamruzicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShimShtein commented Aug 22, 2024 •

edited

Loading