Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More helpful pdnsutil help output #15082

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

miodvallat
Copy link
Contributor

Short description

This is another take at #12350 ("Group pdnsutil usage by topic"). The dictionary of pdnsutil commands is extended with a "command group" and the short help text, for every command. This allows these help texts to be output in groups, with alphabetical ordering within each group, automagically.

Doing this also exposes a few commands which lack help texts, these can either remain hidden or get gifted a nice help text (to be discussed in this PR).

The grouping of commands is stolen borrowed from #12350 and open for discussion as well. The changes in this PR allow for group assignment to be changed very easily, as well as new groups created, if deemed useful.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@coveralls
Copy link

coveralls commented Jan 23, 2025

Pull Request Test Coverage Report for Build 13324705601

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 11494 unchanged lines in 204 files lost coverage.
  • Overall coverage increased (+5.3%) to 64.578%

Files with Coverage Reduction New Missed Lines %
ext/lmdb-safe/lmdb-typed.cc 1 89.66%
pdns/auth-catalogzone.hh 1 66.67%
pdns/dnsdistdist/dnsdist-kvs.hh 1 55.0%
pdns/dnsdistdist/dnsdist-lua-hooks.cc 1 94.59%
pdns/dnsdistdist/dnsdist-session-cache.cc 1 62.86%
pdns/dnswriter.hh 1 76.6%
pdns/dynlistener.hh 1 0.0%
pdns/inflighter.cc 1 90.48%
pdns/qtype.hh 1 86.96%
pdns/recursordist/nod.hh 1 92.59%
Totals Coverage Status
Change from base Build 13317890644: 5.3%
Covered Lines: 127825
Relevant Lines: 166866

💛 - Coveralls

@miodvallat miodvallat added this to the auth-5 milestone Feb 7, 2025
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
pdns/pdnsutil.cc Outdated Show resolved Hide resolved
@miodvallat
Copy link
Contributor Author

As you may or may not have guessed, this PR was not changing any of the existing help texts, only moving them around. Thanks to the review feedback I'll try to make them look better now.

@jsoref
Copy link
Contributor

jsoref commented Feb 10, 2025

I can't remember if my original attempt did. I think I struggled because they were so inconsistent.

@miodvallat
Copy link
Contributor Author

Also, I'm really tempted to make the help text for every command span two lines, as in

frob-zone-knob ZONE NAME [frobbitzim...]
  From the knob NAME of zone ZONE with the given frobbitzim (defaulting to "fnord" if unset)

On one hand, this would prevent some help sentences to span two lines due to them being preceded by about 35 spaces; on the other hand there are nevertheless a fair number of commands where synposis and help fits on a single line.

Opinions?

@jsoref
Copy link
Contributor

jsoref commented Feb 10, 2025

I started doing something like that in places in my PR. The existing wrapping was more trouble than it was worth.

@miodvallat
Copy link
Contributor Author

Here is what the output of pdnsutil without any args look after that last commit. Tell me you like it 😁

Usage:
pdnsutil [options] <command> [params...]

Autoprimary commands:

add-autoprimary IP NAMESERVER [account]
        Add a new autoprimary 
list-autoprimaries
        List all autoprimaries
remove-autoprimary IP NAMESERVER
        Remove an autoprimary

Catalog commands:

set-catalog ZONE CATALOG
        Change the catalog of ZONE to CATALOG. Setting CATALOG to an empty
        value ("") removes ZONE from the catalog it is in

Metadata commands:

add-meta ZONE KIND VALUE [VALUE...]
        Add zone metadata, this adds to the existing KIND
get-meta ZONE [KIND...]
        Get zone metadata. If no KIND given, lists all known
set-meta ZONE KIND METADATANAME [VALUE]
        Set zone metadata, optionally providing a value. A missing value clears
        the metadata.
        Note - this will replace all metadata records of KIND!

Zone commands:

add-record ZONE NAME TYPE [ttl] content [content...]
        Add one or more records to ZONE
change-secondary-zone-primary ZONE primary-ip [primary-ip...]
        Change secondary zone ZONE primary IP address(es) to primary-ip
check-all-zones [exit-on-error]
        Check all zones for correctness. Use exit-on-error to exit immediately
        upon finding the first error in any zone
check-zone ZONE
        Check a zone for correctness
clear-zone ZONE
        Clear all records of a zone, but keep everything else
create-secondary-zone ZONE primary-ip [primary-ip...]
        Create secondary zone ZONE with primary IP address(es) primary-ip
create-zone ZONE [nsname]
        Create empty zone ZONE
delete-zone ZONE
        Delete zone ZONE
edit-zone ZONE
        Edit zone contents using $EDITOR
increase-serial ZONE
        Increases the SOA-serial by 1. Uses SOA-EDIT
list-all-zones [primary|secondary|native|producer|consumer]
        List all active zone names. --verbose or -v will also include disabled
        or empty zones
list-member-zones CATALOG
        List all members of catalog zone CATALOG
list-zone ZONE
        List zone contents
load-zone ZONE FILE
        Load ZONE from FILE, possibly creating zone or atomically replacing
        contents; --verbose or -v will also include the keys for disabled or
        empty zones
set-account ZONE ACCOUNT
        Change the account (owner) of ZONE to ACCOUNT
set-kind ZONE KIND
        Change the kind of ZONE to KIND (primary, secondary, native, producer,
        consumer)
set-option ZONE [producer|consumer] [coo|unique|group] VALUE [VALUE...]
        Set or remove an option for ZONE. Providing an empty value removes the
        option
set-options-json ZONE JSON
        Change the options of ZONE to JSON
zonemd-verify-file ZONE FILE
        Validate ZONEMD for ZONE

RRSet commands:

delete-rrset ZONE NAME TYPE
        Delete named RRSET from zone
replace-rrset ZONE NAME TYPE [ttl] content [content...]
        Replace named RRSET from zone

DNSSEC commands:

create-bind-db FNAME
        Create DNSSEC db for BIND backend (bind-dnssec-db)
disable-dnssec ZONE
        Deactivate all keys and unset PRESIGNED in ZONE
list-algorithms [with-backend]
        List all DNSSEC algorithms supported, optionally also listing the
        cryptographic library used
list-keys [ZONE]
        List DNSSEC keys for ZONE. When ZONE is unset, display all keys
        for all active zones
rectify-all-zones [quiet]
        Rectify all zones. Optionally quiet output with errors only
rectify-zone ZONE [ZONE...]
        Fix up DNSSEC fields (order, auth)
secure-all-zones [increase-serial]
        Secure all zones without keys
secure-zone ZONE [ZONE...]
        Add DNSSEC to zone ZONE
set-presigned ZONE
        Use presigned RRSIGs from storage
show-zone ZONE
        Show DNSSEC (public) key details about a zone
unset-presigned ZONE
        Stop using presigned RRSIGs on ZONE

CDS/CDNSKEY commands:

export-zone-dnskey ZONE KEY-ID
        Export the public DNSKEY with the given ID to stdout
export-zone-ds ZONE
        Export all KSK DS records for ZONE to stdout
set-publish-cdnskey ZONE [delete]
        Enable sending CDNSKEY responses for ZONE. Add 'delete' to publish
        a CDNSKEY with a DNSSEC delete algorithm
set-publish-cds ZONE [DIGESTALGOS]
        Enable sending CDS responses for ZONE, using DIGESTALGOS as signature
        algorithms; DIGESTALGOS should be a comma-separated list of numbers,
        defaulting to '2'
unset-publish-cdnskey ZONE
        Disable sending CDNSKEY responses for ZONE
unset-publish-cds ZONE
        Disable sending CDS responses for ZONE

NSEC3 commands:

hash-zone-record ZONE RNAME
        Calculate the NSEC3 hash for RNAME in ZONE
set-nsec3 ZONE ['PARAMS' [narrow]]
        Enable NSEC3 with PARAMS. Optionally narrow
unset-nsec3 ZONE
        Switch ZONE back to NSEC

TSIG key commands:

activate-tsig-key ZONE NAME {primary|secondary|producer|consumer}
        Enable TSIG authenticated AXFR using the key NAME for ZONE
deactivate-tsig-key ZONE NAME {primary|secondary}
        Disable TSIG authenticated AXFR using the key NAME for ZONE
delete-tsig-key NAME
        Delete TSIG key (warning: will not unmap key!)
generate-tsig-key NAME ALGORITHM
        Generate new TSIG key
import-tsig-key NAME ALGORITHM KEY
        Import TSIG key
list-tsig-keys
        List all TSIG keys

Zone key commands:

activate-zone-key ZONE KEY-ID
        Activate the key with key id KEY-ID in ZONE
add-zone-key ZONE {zsk|ksk} [BITS] [active|inactive] [published|unpublished]
    [rsasha1|rsasha1-nsec3-sha1|rsasha256|rsasha512|ecdsa256|ecdsa384|ed25519|ed448]
        Add a ZSK or KSK to zone with specific algorithm and size in bits
deactivate-zone-key ZONE KEY-ID
        Deactivate the key with key id KEY-ID in ZONE
export-zone-key ZONE KEY-ID
        Export the private key with the given ID to stdout
export-zone-key-pem ZONE KEY-ID
        Export in PEM format the private key with the given ID to stdout
generate-zone-key {zsk|ksk} [ALGORITHM] [BITS]
        Generate a ZSK or KSK to stdout with specified ALGORITHM and BITS
import-zone-key ZONE FILE [active|inactive] [ksk|zsk] [published|unpublished]
        Import from a file a private key, ZSK or KSK; defaults to KSK, active
        and published
import-zone-key-pem ZONE FILE ALGORITHM {ksk|zsk}
        Import a private key from a PEM file

publish-zone-key ZONE KEY-ID
        Publish the zone key with key id KEY-ID in ZONE
remove-zone-key ZONE KEY-ID
        Remove key with KEY-ID from ZONE
unpublish-zone-key ZONE KEY-ID
        Unpublish the zone key with key id KEY-ID in ZONE

Other commands:

b2b-migrate OLD NEW
        Move all data from one backend to another
backend-cmd BACKEND CMD [CMD...]
        Perform one or more backend commands
backend-lookup BACKEND NAME [[TYPE] CLIENT-IP-SUBNET]
        Perform a backend lookup of NAME, TYPE (defaulting to ANY) and
        CLIENT-IP-SUBNET
bench-db [filename]
        Benchmark database backend with queries, one zone per line
hash-password [WORK FACTOR]
        Ask for a plaintext password or API key and output a salted and hashed
        version
hsm assign ZONE ALGORITHM {ksk|zsk} MODULE SLOT PIN LABEL
        Assign a hardware signing module to a ZONE
hsm create-key ZONE KEY-ID [BITS]
        Create a key using hardware signing module for ZONE (use assign first);
        BITS defaults to 2048
ipdecrypt IP passphrase/key [key]
        Decrypt IP address using passphrase or base64 key
ipencrypt IP passphrase/key [key]
        Encrypt IP address using passphrase or base64 key
lmdb-get-backend-version
        Get schema version supported by backend
raw-lua-from-content TYPE CONTENT
        Display record contents in a form suitable for dnsdist's
        `SpoofRawAction`
test-schema ZONE
        Test DB schema - will create ZONE

Common options:
  -h [ --help ]                      produce help message
  --version                          show version
  -v [ --verbose ]                   be verbose
  --force                            force an action
  --config-name arg                  virtual configuration name
  --config-dir arg (=/usr/local/etc) location of pdns.conf
  --no-colors                        do not use colors in output
  --commands arg

@miodvallat
Copy link
Contributor Author

@jsoref I think I addressed all your comments (and used ... for ellipsis instead of .. everywhere). Please let me know if I missed any.

@jsoref
Copy link
Contributor

jsoref commented Feb 12, 2025

set-catalog ZONE CATALOG
        Change the catalog of ZONE to CATALOG. Setting CATALOG to an empty
        value ("") removes ZONE from the catalog it is in
  1. if I call pdnsutil set-catalog my-zone does it error? Do I really have to use pdnsutil set-catalog my-zone ''?
  2. I'd change:
    -        value ("") removes ZONE from the catalog it is in
    +        value ("") removes ZONE from its current catalog

I can't quite figure out when we're using trailing . and when we aren't:

get-meta ZONE [KIND...]
        Get zone metadata. If no KIND given, lists all known
set-meta ZONE KIND METADATANAME [VALUE]
        Set zone metadata, optionally providing a value. A missing value clears
        the metadata.
        Note - this will replace all metadata records of KIND!
-        Get zone metadata. If no KIND given, lists all known
+        Get zone metadata. If no KIND is given, lists all known
-        Set zone metadata, optionally providing a value. A missing value clears
+         Set zone metadata, optionally providing a value. An omitted value clears
-        the metadata.
+        the metadata
-        List all active zone names. --verbose or -v will also include disabled
-        or empty zones
+        List all active zone names.
+        Use --verbose (-v) to include disabled or empty zones
         Change the kind of ZONE to KIND (primary, secondary, native, producer,
-        consumer)
+        or consumer)
        Set or remove an option for ZONE. Providing an empty value removes the
        option

We should standardize on text for empty=>delete

   Validate ZONEMD for ZONE

That ZONEMD is a thing and ZONE is an argument is confusing.

 delete-rrset ZONE NAME TYPE
-        Delete named RRSET from zone
+        Delete named RRSET from ZONE
replace-rrset ZONE NAME TYPE [ttl] content [content...]
-        Replace named RRSET from zone
+        Replace named RRSET from ZONE
-        List DNSSEC keys for ZONE. When ZONE is unset, display all keys
-        for all active zones
+        List DNSSEC keys for ZONE.
+       When ZONE is omitted, display all keys for all active zones
         Enable sending CDS responses for ZONE, using DIGESTALGOS as signature
         algorithms; DIGESTALGOS should be a comma-separated list of numbers,
-        defaulting to '2'
+        (default: '2')
activate-zone-key ZONE KEY-ID
        Activate the key with key id KEY-ID in ZONE

Using - in arguments is bad, it should really be KEY_ID or KEYID ... (same for CLIENT-IP-SUBNET)

@miodvallat
Copy link
Contributor Author

1. if I call `pdnsutil set-catalog my-zone` does it error? Do I really have to use `pdnsutil set-catalog my-zone ''`?

In the current state of the code, yes, you need to explicitly pass an empty argument. I'm fixing this en passant.

I can't quite figure out when we're using trailing . and when we aren't:

get-meta ZONE [KIND...]
        Get zone metadata. If no KIND given, lists all known
set-meta ZONE KIND METADATANAME [VALUE]
        Set zone metadata, optionally providing a value. A missing value clears
        the metadata.
        Note - this will replace all metadata records of KIND!

You can query for multiple kind of metadata, but only set one kind at once.

@miodvallat miodvallat force-pushed the dilnpstu branch 2 times, most recently from 8c98550 to ff66eaf Compare February 13, 2025 11:02
@miodvallat
Copy link
Contributor Author

I think it's good to go now. (which means that there is still a lot of room for improvement but I'd like to be able to push this in before this becomes unmanageable)

@jsoref
Copy link
Contributor

jsoref commented Feb 13, 2025

The command synopsis is now to the command handler.

I think you're missing a verb.

Otherwise, let's ship it and iterate later

@miodvallat
Copy link
Contributor Author

Indeed, the word is "passed", I'll amend the commit (and fix a typo in the docs).

The next bikeshed topic will be the different grouping of commands in the pdnsutil help output compared to the manual page in the documentation...

1. The commands dictionary is extended with a command group, as well as
   the one-liner synopsis and help message. When the list of these
   one-liners get output, they are now output in sorted order within
   each group.

2. Instead of having the syntax on the leftmost 40 columns and the
   description on the rightmost 40 columns, causing many commands to
   span more than one line total, we now display the syntax on a whole
   line, and the description on the next one, indented by a tab.

   This should hopefully improve readability (especially for complex
   syntax lines).

3. The command synopsis is now passed to the command handler. This
   allows error messages in case of invocation error to be consistent
   with the output of `pdnsutil help`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants