[ansible]: mark variables from custom resources as unsafe #4566
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
varshaprasad96
force-pushed
the
unsafe_var/ansible
branch
from
07b20b7
to
c611724
Compare
varshaprasad96
commented
Feb 22, 2021
internal/ansible/runner/runner.go
Outdated
| @@ -337,7 +337,7 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa | |||
| parameters = paramconv.MapToSnake(spec) | |||
| } else { | |||
| for k, v := range spec { | |||
| parameters[k] = v | |||
| parameters[k] = markUnsafe(v) | |||
There was a problem hiding this comment.
We are also specifying the spec key as it is for json to Marshall (https://github.com/operator-framework/operator-sdk/pull/4566/files#diff-40a9a64aa847bb7816a7eb40cf6ec6b3ca70a7d4066e320d95fe9c0e0cb2ac11R349). Do we need to tag only these variables as unsafe?
There was a problem hiding this comment.
yeah we'll need to run this on all pieces of the spec that we pass in.
fabianvf
previously requested changes
Feb 22, 2021
There was a problem hiding this comment.
Two more things:
- Could we also add a setting in the
watches.yamlto toggle this on/off (off by default is good). As far as I can tell there's not a way to mark a variable as safe once it's been marked unsafe, and some authors may actually want to process user-provided templates (or may be relying on that now) - Could we add an integration test with nested values so that we can verify we're properly formatting these variables in a way that Ansible can read
internal/ansible/runner/runner.go
Outdated
| @@ -337,7 +337,7 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa | |||
| parameters = paramconv.MapToSnake(spec) | |||
| } else { | |||
| for k, v := range spec { | |||
| parameters[k] = v | |||
| parameters[k] = markUnsafe(v) | |||
There was a problem hiding this comment.
yeah we'll need to run this on all pieces of the spec that we pass in.
internal/ansible/runner/runner.go
Outdated
| @@ -337,7 +337,7 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa | |||
| parameters = paramconv.MapToSnake(spec) | |||
There was a problem hiding this comment.
We'd also need to mark these as unsafe
User provided values from the custom resource are tagged unsafe by default. Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
varshaprasad96
force-pushed
the
unsafe_var/ansible
branch
from
c611724
to
6e1441c
Compare
Signed-off-by: varshaprasad96 <varshaprasad96@gmail.com>
varshaprasad96
force-pushed
the
unsafe_var/ansible
branch
from
9a9182d
to
bfbeecd
Compare
jmrodri
dismissed
fabianvf’s stale review
Fabian's concerns were addressed by Varsha. Dismissing his change request.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
User provided values from the custom resource are tagged
unsafe by default.
Motivation for the change:
Closes: #4169
Checklist
If the pull request includes user-facing changes, extra documentation is required:
changelog/fragments(seechangelog/fragments/00-template.yaml)website/content/en/docs