intelsdi-x / snap

The open telemetry framework
http://snap-telemetry.io
Apache License 2.0
1.8k stars 294 forks source link

Can't use "$" (dollar-sign) char in snap task files #1691

Open michep opened 7 years ago

michep commented 7 years ago

Due to calling os.ExpandEnv() in snap\core\task.go:331 [func UnmarshalBody()] and in snap\cmd\snaptel\task.go:355 [func createTaskUsingTaskManifest()] it is not possible to use "$" char in snap task files, it will be unconditionally removed, and there is no way to escape this symbol.

Example here.

Now Im creating a processor plugin in which one can specify command line to execute, and if it contains "$" char in it, like cat /etc/fstab |grep ext[2,3,4]| awk '{print($2);}', it will be broken.

kjlyon commented 7 years ago

Good catch @michep, we'll add this to our backlog. Though if you have a fix in mind, go ahead and submit a PR. We appreciate all contributions.

michep commented 7 years ago

Approach to resolve this issue is depend on how current functionality supposed to work. Why, in the first place, it was implemented. Is it really necessary to use environment variables expansion in tasks?

One way is to use regexp \$\{{0,1}[A-Z_]+(\}{0,1}|s+). Take a look here: https://play.golang.org/p/l__RnjmqnE This will be signicantly slower and more resource intensive solution, but this is not so frequently used, actually.

kjlyon commented 7 years ago

@michep, do you mind sharing your task file with me? I am curious to see where exactly you are using the command cat /etc/fstab |grep ext[2,3,4]| awk '{print($2);}'

michep commented 7 years ago

This is all about my plugin - https://github.com/michep/snap-plugin-processor-maptag Dollar-sign can be used in regex and in cmd&args in task.

IzabellaRaulin commented 7 years ago

Hello @michep, I see your point that you want to directly pass e.g. cat /etc/fstab |grep ext[2,3,4]| awk '{print($2);}' as a value of one of config params in your processor plugin. However, there is functionality in Snap that in task manifest you can point to the value of environment variable by its name - for example, you can control where your data are published by setting an appropriate value of $INFLUXDB_HOST as below:

[...]
      publish:
        - plugin_name: "influxdb"
          config:
             host: "${INFLUXDB_HOST}"
             port: 8086
             database: "test"
[...]

See full example here

In your case, you need to pass a value of config param (which includes a dollar char) in a different way, not directly. @michep, do you need help in that?

michep commented 7 years ago

Hi, @IzabellaRaulin. So, what is that other, indirect way?

IzabellaRaulin commented 7 years ago

@michep - one way is providing in task manifest a path to file in which you can have the dollar sign. What is more, you as an author of plugin can decide what this structure of the file should look like - it might be just one line file with a command or some json/yaml when user can declare more params, depending on needs.

Example 1 - where command.txt has one line containing the command cat /etc/fstab |grep ext[2,3,4]| awk '{print($2);}':

plugin_name: "my_plugin"
          config:
             commandfile: "/home/user/command.txt"
             param2: "something"
             param3: 100  

As a value of config you receive path to command file, so in plugin needs to be handled opening this file and interpreting its contents.

Example 2 - where setfile.yaml contains all possible parameters with determined its value (notice that there are no param2, neither param3. All config parameters might be determined in setfile.yaml together with command.

plugin_name: "my_plugin"
          config:
             setfile: "/home/user/setfile.yaml"         

Please, let me know if this has sense in your use case and what do you think about it .

michep commented 7 years ago

Following the Occam's razor principle one should not create new entities until this is truly necessary. I believe that solution here is not "you must move your dollar-sign-contained texts into separate files", but "fix how environment variables expansion works". I think unconditional removing of all dollar-sign symbols in tasks and configs is a huge issue. I think something like python's os.path.expandvars() should be implemented, using regexp $(\w+|{[^}]*}): os.path.expandvars() source code

IzabellaRaulin commented 7 years ago

Hello @michep, sorry for delayed response. I think your proposition is valuable. I look on python's os.path.expandvars() and I agree that similar approach should be considered in Snap. I will create RFC and describe the proposition, just to summarize our discussion and continue design discussion under new GH issue. It would be great if you would like to contribute to that. Thank You.