Fix blind replacement of paths in Heat template referencing other files#2815
Fix blind replacement of paths in Heat template referencing other files#2815mandre merged 6 commits intogophercloud:masterfrom
Conversation
|
Thanks for the PR @dverbeir. I'll look at your changes today. You just made us realize we were skipping these tests in CI because they're not in the Running the test locally indeed shows the failure: |
mandre
left a comment
There was a problem hiding this comment.
Thanks for breaking your work in multiple commits, this makes it easier to review. I just have a simple question, otherwise lgtm.
| if err := opts.TemplateOpts.getFileContents(opts.TemplateOpts.Parsed, ignoreIfTemplate, true); err != nil { | ||
| return nil, err | ||
| } | ||
| opts.TemplateOpts.fixFileRefs() |
There was a problem hiding this comment.
Was that the only call to fixFileRefs() that was problematic, or should we drop more of them?
A quick grep shows more occurrences in the same file.
There was a problem hiding this comment.
Good point. I had looked into that at some point but because I didn't know what kind of manipulation was expected on the Environment part (just a few lines below, and where getRRFileContents() is used instead of getFileContents()), I decided to only touch the Template part that I was interested in and understood.
But I missed the fact that Adopt, Update and Preview also call fixFileRefs() for their template parts (after doing getFileContents()). I have now removed the calls from those other operations so they will also benefit from the fix.
Looking into getRRFileContents(), I saw it also uses getFileContents() but simply with a different filter function (ignoreIfEnvironment()). Hence, it is in fact already affected by my change since some of the files it loads could be re-generated with full paths expanded, as now done by getFileContents() without the need for fixFileRefs().
So I went ahead and made a separate commit that completes similar changes for the env part and completely removes fixFileRefs(). I have however not tested this part other than with the included unit tests and am somewhat unsure as to whether it's okay to re-generate env content from the parsed structure as the code is now doing. Note that this was already the case with the previous commit (templates), just not for the top-level env content. I hope my explanation is not too unclear....
There was a problem hiding this comment.
Your explanation is perfectly clear, and I believe it is fine. It looks like a bug that was never noticed.
|
please rebase |
538518e to
5b89e88
Compare
Applying filepath.Dir() to a URL used to give the expected result in go1.18. But with more recent versions, the library function cleans the provided path and removes any duplicate path separator. As a result, filepath.Dir() applied to "file:///some_abs_path/here" will give "file:/some_abs_path" instead of "file:///some_abs_path". This commit ensures filepath.Dir() is only applied to the Path element of the URL by first Parse()ing the URL. Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
Refactor getFileContents() into smaller functions to make it easier to modify in a follow-up commit that will replace fixFileRefs() by something more appropriate for replacing child-template paths. Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
Simple cleanup of the stack template tests: * Move long duplicated template into a constant * Move function invocations into their AssertNoErr() when the resulting line is not too long (reduces number of lines for the tests, making their flow and intent more visible) Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
Reduce duplication and make tests shorter by consolidating all Mux.HandleFunc(...) invocations into a serveFile() convenience function. Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
Existing fixFileRefs() does a simple string replacement regardless of
context. This is not appropriate as the same keyord could appear outside
of the valid template/file reference cases ('type: ...' and
'get_file: ...').
Example:
init-cmd:
type: OS::Heat::CloudConfig
properties:
cloud_config:
write_files:
- path: /etc/somefile
content: { get_file: somefile }
Here "somefile" in "{ get_file: somefile }" should be replaced by the
absolute path to somefile, but the "path: /etc/somefile" part should not
be modified as that path refers here to the target destination in a
completely different context (in this example, a cloud-init target path
on a to-be-created VM).
Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
An earlier commit removed the usage of fixFileRefs() for templates, by having the getFileContents() do the replacement directly on the parsed data structure as it goes through it and gathers referenced files. This commit does the same for environment files that also ultimately use the now improved getFileContents() function. As the fixFileRefs() was not used anywhere else, it has been removed, together with its test. Signed-off-by: David Verbeiren <david.verbeiren@tessares.net>
5b89e88 to
257b069
Compare
|
Rebased to have the tests run on the modified code. |
mandre
left a comment
There was a problem hiding this comment.
Really good work, thanks a lot.
This change fixes an issue in the handling of Heat templates that reference other files (child templates or files included by means of
get_file:) and that also contain, in some other context, the same string as the relative path being referenced (that must indeed be substituted for the absolute local path).The existing fixFileRefs() does a simple blind string replacement inside the referencing template, regardless of the context. This is not appropriate as the same string as the path being replaced could possibly appear outside of the valid template/file reference cases (
type: ...andget_file: ...), as in the below example:Here "somefile" in "{ get_file: somefile }" should be replaced by the absolute local path to 'somefile', but the "path: /etc/somefile" part should not be modified as that path refers here to the target destination in a completely different context (in this example, a cloud-init target path on a to-be-created VM).
The change also fixes another issue when using go v1.20 where the usage of
filepath.Dir()on a URL removes any duplicate path separator when they should in fact be preserved (in "file:///some_abs_path/here", the "///" must remain).Finally, the stack template tests have been slightly cleaned up (removing duplications, more concise code).