search

intentionet / netconan

netconan - a Network Configuration Anonymizer
Apache License 2.0
146 stars 12 forks source link

Move command line arguments to YAML configuration file #50

Closed saparikh closed 6 years ago

saparikh commented 6 years ago

Moved all existing command line args to a YAML configuration file, except: --log-level

Added command line arg for specifying the configuration file: --config-file

Also added sample YAML configuration file.

This change is Reviewable

ratulm commented 6 years ago

It looks like this PR removes the ability to specify arguments on the command line completely. Is that right? I am not sure if that is desirable because command line arguments are easier to specify than config files and are more suited to fast, experimental use. What you probably want to do is to retain the command line arguments and use what is specified there to override what is in the config file if certain arguments are provided in both places.

dhalperi commented 6 years ago

+1 @ratul's comment. Similarly, no reason not to put log-level in the config file as a default.

dhalperi commented 6 years ago

Reviewed 1 of 2 files at r1, 1 of 1 files at r2. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

dhalperi commented 6 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file): Adding blocking comment re: preserving CLI

Comments from Reviewable

dhalperi commented 6 years ago

Reviewed 2 of 3 files at r3. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…
Adding blocking comment re: preserving CLI

Rather than reinventing the wheel, should we just use the standard Python way to do this?

https://stackoverflow.com/a/5826167/1715495

Comments from Reviewable

dhalperi commented 6 years ago

Reviewed 1 of 3 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

ratulm commented 6 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…
Rather than reinventing the wheel, should we just use the standard Python way to do this? https://stackoverflow.com/a/5826167/1715495

This is cool! I didn't know about it.

The only reason to keep it this way would be to preserve the yaml format for the config file (which folks asked for and are familiar with). Is that a good reason? I could go either way.

Comments from Reviewable

dhalperi commented 6 years ago

Review status: all files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file):

Previously, ratulm wrote…
This is cool! I didn't know about it. The only reason to keep it this way would be to preserve the yaml format for the config file (which folks asked for and are familiar with). Is that a good reason? I could go either way.

I'm betting YAML was meant as an example not a specific technology choice. The Python config files seem perfectly manually editable to me, so would prefer to just use the standard code.

Comments from Reviewable

dhalperi commented 6 years ago

Reviewed 2 of 4 files at r4. Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

a discussion (no related file): Okay, I added testing and a few bug fixes to @ratul 's version. (What a group party!)

Ratul, can you review my code for sanity? (It should be the unreviewed diffs).

Victor, can you review the tests?

Comments from Reviewable

dhalperi commented 6 years ago

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…
Okay, I added testing and a few bug fixes to @ratul 's version. (What a group party!) Ratul, can you review my code for sanity? (It should be the unreviewed diffs). Victor, can you review the tests?

@progwriter ^^

Comments from Reviewable

sfraint commented 6 years ago

Review status: 1 of 3 files reviewed at latest revision, 7 unresolved discussions.

sample-netconan-config.cfg, line 9 at r4 (raw file):

salt=BLAHBLAHBLAH
dump_ip_map=./ip-map-file
sensitive_words=TEST1 TEST2 TEST3 TEST4

This sample param may be a little misleading. Netconan will see TEST1 TEST2 TEST3 TEST4 as a single keyword and replace all occurrences of that full string, while a user might interpret this as trying to replace each of those words separately.

netconan/netconan.py, line 64 at r4 (raw file):

    sensitive_words = None
    if options.sensitive_words is not None:
        sensitive_words = options.sensitive_words.split(',')

Was removing this intentional? I think this will break sensitive words.

netconan/netconan.py, line 77 at r4 (raw file): The syntax of -a, -p, and -u (the 'store_true' flags) has changed. Previously, those params did not require any value to be supplied, i.e. this was valid:

netconan -a -i ./in -o ./out

But now it would need to be:

netconan -a true -i ./in -o ./out

So either 1) documentation (readme) should be updated the reflect the change in these params, or 2) they should be reverted to behave like they did before.

netconan/netconan.py, line 94 at r4 (raw file):

    parser.add_argument('-u', '--undo', type=str2bool,
                        help='Undo reversible anonymization (must specify salt)')
    parser.add_argument('-w', '--sensitive-words', nargs="+",

This changes the syntax of sensitive-words param, so the readme should be updated

netconan/netconan.py, line 98 at r4 (raw file):

    args = parser.parse_args(remaining_argv)

    print(args)

Is this intentionally left in?

netconan/netconan.py, line 116 at r4 (raw file):


    if not os.path.exists(args.output):
        os.makedirs(args["output"])

I would change this to os.makedirs(args.output) for consistency (also not sure if the above works)

Comments from Reviewable

sfraint commented 6 years ago

Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions.

sample-netconan-config.cfg, line 9 at r4 (raw file):

Previously, sfraint (Spencer Fraint) wrote…
This sample param may be a little misleading. Netconan will see `TEST1 TEST2 TEST3 TEST4` as a single keyword and replace all occurrences of that full string, while a user might interpret this as trying to replace each of those words separately.

Actually, this might be seen as an array of characters, each character being considered a "sensitive word"

netconan/netconan.py, line 64 at r4 (raw file):

Previously, sfraint (Spencer Fraint) wrote…
Was removing this intentional? I think this will break sensitive words.

Never mind, ignore this.

Comments from Reviewable

dhalperi commented 6 years ago

sample-netconan-config.cfg, line 6 at r5 (raw file):

log_level=INFO
anonymize_ips=True
anonymize_passwords=True

another bad thing with current implementation is that the constant names in config file do not match argument names (e.g., --anonymize-passwords vs anonymize_passwords).

Comments from Reviewable

progwriter commented 6 years ago

Reviewed 1 of 4 files at r4, 2 of 2 files at r5. Review status: all files reviewed at latest revision, 8 unresolved discussions.

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…
@progwriter ^^

Tests look fine, although I did leave a different comment

netconan/netconan.py, line 44 at r5 (raw file):

        # Turn off help, so that the parser below prints help for all options.
        add_help=False
    )

To me this looks overly complex: multiple parsers, multiple places defaults are initialized (both dict and in parser)? Why not simply:

Comments from Reviewable