Add nsec3-hash command. #6
Conversation
ximon18
requested review from
partim,
tertsdiepraam,
bal-e,
Philip-NLnetLabs and
a team
and removed request for
partim,
tertsdiepraam,
mozzieongit,
Philip-NLnetLabs and
bal-e
…g is not specified which will be fixed by PR #428.
… an existing 3rd party ldns Docker image.
…t 1 for backward compatibility when invoked as ldns-nsec3-hash.
…st-nsec3-hash page to match the current help output of the command.
Co-authored-by: Jannik <[email protected]>
…l tree slightly to allow args to be parsed from within a module without knowing how arg parsing works for dnst vs ldns.
| let binary_path = args_iter.next()?; | ||
| let binary_path = args_iter | ||
| .next() | ||
| .ok_or::<Error>("Missing binary name".into())?; |
There was a problem hiding this comment.
I think the type annotation is only necessary because the ? adds an into as well, so removing the .into() might clean this up a bit.
There was a problem hiding this comment.
The latest code (in the packaging PR that merges into this branch) changes this behaviour, but I noted there still seems to be unwanted error output in some cases, e.g. on --help, so I would rather just merge your env PR to main and merge that into this and see what we end up with.
| // nsec-hash defaults NSEC3 iterations to 0. | ||
| assert_cmds_eq( | ||
| &[LDNS_NSEC3_CMD, TEST_ZONE_NAME], | ||
| &[DNST_NSEC3_SUBCMD, "--iterations", "1", TEST_ZONE_NAME], |
There was a problem hiding this comment.
I'm not sure about how useful these tests are when the defaults are different? It would make more sense to run this with the ldns compatibility mode I suppose.
There was a problem hiding this comment.
I think both make sense, otherwise we don't have a complete sanity check of the dnst command as a black box (though we do have that in the packaging workflow sanity check).
Co-authored-by: Terts Diepraam <[email protected]>
| try_ldns_compatibility(args_provider()).or_else(|_| { | ||
| Args::try_parse_from(args_provider()).map_err(|err| Error::new(err.to_string().as_str())) | ||
| }) |
There was a problem hiding this comment.
This is subtly wrong. The try_ldns_compatibility function has three outcomes:
- The command was not matched
- The command matched and parsing succeeded
- The command matched and parsing failed
You've now combined the first and third case even though they should be handled differently.
I think you're also removing the color from the clap output by turning it into a string.
Note: A minimal working version of this command already existed in
mainbut used code copied fromdomainrather than depending ondomain.Currently depends on
domainbranchinitial-nsec3-generation, see NLnetLabs/domain#416.Contains several kinds of tests:
tests/(ignored by default) that compare outputs to a host installedldns-nsec3-hashcommand.Fuzz tests and man page changes were moved out to #24 and #25.