configuration from file

[itaysk]

allow configuring tracee-rules and tracee-ebpf from a config file. this file should be given to the executable with a flag --config which will be parsed to fill the available flags options of each tool. this could be split in 2 issues, but most likely get implemented by the same person in one go.

[itaysk]

notes from offline discussion with @mtcherni95 :

Usually CLI flags are key-value pairs, but in tracee they're key-key-value. For example: --output format:json, we have key1 which is output, key2 which is format, and value which is json. Ideally we would like this hierarchy to be natively represented in the configuration file, e.g:

{
    "tracee-ebpf": {
        "output": {
            "format": "json"
        }
    }
}

BUT as an intermediate step, we are ok if the configuration file will be flat key-value, where the value contains key-value as string, e.g:

{
    "tracee-ebpf": {
        "output": "format: json"
    }
}

(I use json to exemplify the point but I think we need to support yaml as well)

In the future we might want to change the CLI UX to be more conventional (simple k/v) and/or to generalize the inner key-value parsing code

[itaysk]

related to #1288 and #1248

[NDStrahilevitz]

Hey,

I've created a PoC for a json configuration file structure, one for tracee-rules and another for tracee-ebpf you can find it here.

The tracee-rules one is pretty simple since the cli options are pretty simple to understand. The tracee-ebpf one is unsurprisingly more complex since the output, trace and capture have many values that can be passed. As such i've decided to model these config options to be like the parsed structs that are created by the cli (for example for trace we have the tracee.Filter struct), instead of simply throwing in an array of strings and passing to the existing cli structure. As such the config files will have a separate parsing function (similar to the prepare functions we have today).

@itaysk @yanivagman @mtcherni95 i'd like to know what you think :)

[itaysk]

Hello :) IMO we don't need more "prepare" or parsing functions, if anything we want to remove the ones we have. we should have just one way of parsing flags/config. If you want to use your (great) config format, that fine but I think you'll need to also change the flag parsing. If you don't want to touch the UX now, then change the file format to meet the current convention. This is essentially what's written here as discussed with @mtcherni95 https://github.com/aquasecurity/tracee/issues/467#issuecomment-1003112416

[NDStrahilevitz]

Hello :) IMO we don't need more "prepare" or parsing functions, if anything we want to remove the ones we have. we should have just one way of parsing flags/config. If you want to use your (great) config format, that fine but I think you'll need to also change the flag parsing. If you don't want to touch the UX now, then change the file format to meet the current convention. This is essentially what's written here as discussed with @mtcherni95 #467 (comment)

I think that if we use the same function for both it will introduce coupling between the input (and thus UX) of the config file and of the cli, which will bite us back later because:

1. The CLI and the config files will practically need to have the same values which will create a conflict between creating a good UX for the cli and a good UX for the config file, possibly resulting in a bad/mediocore UX for both
2. If some bug pops up in regards to parsing the config file, it might screw up the same parse in the CLI flags and vice versa.

I understand that you want less code duplication and that the cli UX right now is up to change, but I think for the sake of future maintainability between these two features (cli arguments and config arguments) adding a separate function will be healthier in the long run, and won't take so much development time atm too.

[itaysk]

I think that if we use the same function for both it will introduce coupling between the input (and thus UX) of the config file and of the cli

yes, and to me that's a good thing

The CLI and the config files will practically need to have the same values which will create a conflict between creating a good UX for the cli and a good UX for the config file, possibly resulting in a bad/mediocore UX for both

This is what virtually every other CLI tool or framework does (source the same configuration from flags or files). I don't think anyone thinks it's mediocre.

If some bug pops up in regards to parsing the config file, it might screw up the same parse in the CLI flags and vice versa.

Yes. but what about the glass half full :) now you have just one place to fix bugs

won't take so much development time atm too

My consideration is not with how much time/code it takes "atm", but how much complexity it adds to the project and how much maintenance cost it will have over time

Sorry but I wasn't convinced that adding more code to parse the same config from a different source makes sense. Especially given how seamless it would be to achieve using the libraries we already use.

[NDStrahilevitz]

@itaysk Alright, if the vision is having the config file just be a 1 to 1 copy of the cli options, then there are obviously simpler options than what i've proposed.

Would the the altsrc package suffice for the use case then? It's an extension of the already used cli package. The only issue with using it as I see it is this open issue from 2019 about what seems to be deprecating and integrating into the cli package itself, but I don't know when that will happen and if we plan to update versions.

Using altsrc would mean adding a Before field to the cli where code like this would be:

Before: func(c *cli.Context) error {
	inputSource, err := altsrc.NewJSONSourceFromFile(c.String("config"))

	if err != nil {
		return err
	}

	return altsrc.ApplyInputSourceValues(c, inputSource, c.App.Flags)
}

[NDStrahilevitz]

In regards to using altsrc, there are some quirks this package seems to have:

1. Every wrapped flag is required to appear in the configuration file, even if it's not set to be required by the cli package itself.
2. There is no way to set an alternate name for the config field, so the names will be exactly the same as the cli and not in camelCase for example.

The only other option left as far as I can see is parsing a json file to a struct and manually applying it to the flags, which would mean any change to the flags would need to be maintained there as well. Otherwise this will just move back in the direction of a separate parse which was said to be undesirable.

[itaysk]

that's good feedback, thanks. I think the worst case (using altsrc with those limitations) is not that bad, but let's fist evaluate what options we have. do you know about alternatives?

[NDStrahilevitz]

that's good feedback, thanks. I think the worst case (using altsrc with those limitations) is not that bad, but let's fist evaluate what options we have. do you know about alternatives?

@itaysk I found an alternative solution which would take some boilerplate but give us the intended result.

We can define a struct and a respective global value for the configs (in the case of tracee-ebpf i'll replace the complex flag configs simply to string slices so it can be parsed like the cli), load it BEFORE defining the cli in main, and then in the default values for the flag (the cli flags have a Value field for that) we set them to the loaded config.

I'll put up a pull request with this PoC today or sunday.

[NDStrahilevitz]

I've opened a PR as seen above, it avoids using altsrc so the config can be overriden easily by the flags but it doesn't use a flag to find the config location, instead using an environment variable.

I also talked with @mtcherni95 about opening an issue about changing the cli UX to match the PoC configs i've attached earlier (this) but since it has a larger scope it would take more time than the current solution in the PR, but could be done later.

[yanivagman]

Most of the configuration will be given through the new policies feature. Will open a separate issue for that