Project

General

Profile

Actions

Feature #921

closed

Override conf parameters

Added by Anoop Saldanha over 10 years ago. Updated about 10 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Effort:
Difficulty:
Label:

Description

It would be nice to support a feature where we can override conf parameters from the command line.

Maybe --override=<key=value>.

Actions #1

Updated by Anoop Saldanha over 10 years ago

  • Tracker changed from Bug to Feature
Actions #2

Updated by Victor Julien over 10 years ago

This PR might be enough? It does require the yaml to use the env vars though: https://github.com/inliniac/suricata/pull/321

E.g. https://github.com/inliniac/suricata/pull/321/files#L3R805

Actions #3

Updated by Anoop Saldanha over 10 years ago

Using env variables is bad as it can lead to situations where you forget that you have set an env var and you go around trying to figure out why something is not working. The reason why the env var feature of logging api was supposed to be for dev/debug purposes only.

Command line in this regard is better.

Actions #4

Updated by Jason Ish over 10 years ago

Perhaps something in the middle would be better? I like environment variable expansion in config's but can see Anoop's point. And I'm not too keen on overrides as its hard to do consistently throughout the config file, for instance where sequences are used.

Instead something like Java system properties could be used... Stick with the syntax I'm using in PR 321, but instead of pulling from the environment, pull from user supplied defines, eg: --define SURICATA_RULE_PATH=/etc/rules, which would expand ${SURICATA_RULE_PATH} to /etc/rules. A wrapper script could then pull define values from environment variables if it desired, but the config would no pull directly from the environment itself making it more predictable.

Actions #5

Updated by Victor Julien over 10 years ago

@ish: Not sure I would like to put these vars in our default config though.

Actions #6

Updated by Jason Ish over 10 years ago

Yes, the would somewhat uglify the default config, in which case the variable expansion is not as useful either, as it would have the same affect on the configuration.

Perhaps a --set or --define command line option the results in a ConfSet (or ConfSetFinal if that gets merged in) to set basic values.. For example:

--set unix-command.enabled=yes

or

--set default-log-dir=/tmp

This will give the desired result keeping variable names out of the configuration. I still may see a need for something like variable or define expansion, but YAML anchor/alias support may handle those requirement.

Actions #7

Updated by Jason Ish over 10 years ago

I've made a branch (dev-config-set) with a single commit implementing:

https://github.com/jasonish/suricata/commit/f46aa7402e312c600d610e7bcebf8bce316d65ff

Its pretty unobtrusive. It could also form the basis of variable expansion down the road, where some token like %{NAME} looks up "NAME" in the set of --set parameters. But YAML aliases/anchors may remove any need for that.

Actions #8

Updated by Victor Julien over 10 years ago

One of the use cases is in #622.

Would this look something like: --set af-packet.interfaces.eth0.threads=8

Actions #9

Updated by Jason Ish over 10 years ago

Unfortunately no, and the reason is because the interfaces are defined as a sequence, and sequences are harder to deal with.

This could potentially work (note that it doesn't work right now, but could look at fixing it):

--set af-packet.0.threads=8

But you can see the issue here, you have to know the index of the interface in the config file to set the threads for it. A better af-packet configuration (based on what is in suricata.yaml.in right now) might look like:

af-packet:
  defaults:
    threads: 2
    use-mmap: yes
  interfaces:
    eth0:
      threads: 1
      cluster-id: 99
      cluster-type: cluster_flow
      defrag: yes
      use-mmap: yes
    eth1:
      threads: 1
      cluster-id: 98
      cluster-type: cluster_flow
      defrag: yes

then --set af-packet.interfaces.eth0.threads=8 would work as expected, plus I think the af-packet configuration section is a little tidier this way.

Actions #10

Updated by Jason Ish over 10 years ago

The use case --set af-packet.interfaces.eth0.threads=8 is a cadidate for variable/define expansion in the configuration file. For instance,

--define AF_PACKET_THREADS=8

would replace all occurrences of ${AF_PACKET_THREADS} in the configuration file with "8". I do consider this feature more useful to integrators than anyone, and I think Doug's case might be that of an integrator (Security Onion)?

Actions #11

Updated by Victor Julien over 10 years ago

  • Assignee changed from Anoop Saldanha to Jason Ish
Actions #12

Updated by Victor Julien over 10 years ago

Jason Ish wrote:

The use case --set af-packet.interfaces.eth0.threads=8 is a cadidate for variable/define expansion in the configuration file. For instance,

--define AF_PACKET_THREADS=8

would replace all occurrences of ${AF_PACKET_THREADS} in the configuration file with "8". I do consider this feature more useful to integrators than anyone, and I think Doug's case might be that of an integrator (Security Onion)?

What I don't like about this approach is that it clutters the yaml (further). What about allowing the code itself to register such vars? So in afpacket parsing code the we would have logic like "if (defined_on_cmndline) use_commandline; else use_yaml_value;" It would simply register it in the code: "ConfRegisterOverride("AF_PACKET_THREADS");" or something. Make sense?

e.g. ConfGetWithOverride("threads", "AF_PACKET_THREADS");

Actions #13

Updated by Jason Ish over 10 years ago

Ok, so for forget about any form of expansion in the config file.

What you suggest is fine, but is not generic. Its very much like individual modules using environment variables set init parameters, where the environment variable overrides the configuration value.

I also don't think this is a replacement for the more generic --set parameter=value, which could be applied almost anywhere, it only fails for af-packet because of the sequence (thought --set af-packet.*.threads=8).

So back to your suggestion of overrides, does it require registration? For arguments sake, lets use --define AF_PACKET_THREAD=8, these defines/overrides (ok, maybe --override is better?) go into their own lookup table. Then wrap some api around that:
```
if (!ConfGetDefine("AF_PACKET_THREADS")) {
ConfGet(...)
}
```
or like your ConfGetWithOverride. But that might get kinda long when replacing ConfGetChildValueWithDefault where you want to resolution order to be, the override, then the configuration value, or the default. but thats an implementation detail.

I do think we should avoid code solutions, when changing up the configuration a bit would do.

Actions #14

Updated by Victor Julien over 10 years ago

  • Target version changed from 2.0beta2 to 2.0rc1
Actions #15

Updated by Victor Julien about 10 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100
Actions #16

Updated by Peter Manev about 10 years ago

Is it described anywhere how can this be used?
A user example or something?

Actions #18

Updated by Peter Manev about 10 years ago

  • Status changed from Closed to Feedback

Reopened due to inconsistency.

It does not work as described in https://github.com/inliniac/suricata/pull/705 :

suricata -c /etc/suricata/suricata.yaml  -i eth0 -v -S empty.rules --set threading.detect-thread-ration=3

does not change the thread ratio.

This line segafults:

suricata -c /etc/suricata/suricata.yaml  --af-packet -v --set  af-packet.0.threads=4

Tested with the latest git.

Actions #19

Updated by Jason Ish about 10 years ago

Pull request to address this issue: https://github.com/inliniac/suricata/pull/839

This addresses the af-packet.0.threads issue. Basically when set on the command line, it is not a proper "sequence" node as we don't know that yet. I suppose we could guess based on the integer portion of the name, but instead the pull request will promote node to a sequence.

Regarding the tread-detect-ratio. It works for me, but not "ratio" vs "ration".

Actions #20

Updated by Peter Manev about 10 years ago

yes - about "ration" wrong copy/paste from the source:
https://github.com/inliniac/suricata/pull/705

--set threading.detect-thread-ration=1.5

Actions #21

Updated by Jason Ish about 10 years ago

Comment fixed in the PR.

Actions #22

Updated by Victor Julien about 10 years ago

  • Status changed from Feedback to Closed

Merged, thanks guys.

Actions

Also available in: Atom PDF