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

[DROP] Added listener command group to pywbemcli #962

Closed
wants to merge 1 commit into from

Conversation

andy-maier
Copy link
Contributor

@andy-maier andy-maier commented May 4, 2021

DROP: This PR has been abandoned in favor of PR #971, but we keep it around until that PR is merged.

Initial stab at listener support. This PR adds a 'listener' command group to pywbemcli. At this point, the listener does nothing with any received indications.

Ready for a concept review and early test.

TODO:

  • Add documentation
  • Add tests
  • Add support for using the general logging options to log the output of 'listener run'. (see discussion item, below)
  • Change --certfile and --keyfile to use the corresponding global options instead. (see discussion item, below)
  • Add support for processing indications: display (subject to being logged), log to file (separate from logging), call Python function. (see discussion item, below)

DISCUSSION:

  • How to treat stdout/stderr of the running listeners. Should they log to a file?
    KS/COMMENT: I think we have a real discussion about how we log/display/count, etc. received indications.
    Result: We go with the general logging options, for now.
    Status: OPEN

  • The general options --certfile, --keyfile and --ca-certs could be used instead of adding similar options at the level of the listener run/start commands.
    Status: OPEN

  • What types of indication processing would we support: display, log to file, call Python function, execute command?
    Result: For now, we do: display (subject to being logged), log to file (separate from logging), call Python function.
    Status: OPEN

  • Default listener protocol? (currently http)
    KS/COMMENT: This one does not really bother me other than the default for the server is https so this is "different".
    Andy: Changed the default to https for security by default and for consistency.
    Result: Agreed to https as a default.
    Status: DONE

  • Default listener port? (currently 5988)
    KS/COMMENT: I think that Jim did set it up that way and certainly we did not claim any standard port. However 5988 always bothered me because there was always the real chance that the server was on the same system. That is why I always pick some high number like 50000.
    Andy: Changed to 25989 to have a more unique value with less chance of collisions.
    Result: Agreed to that port as a default.
    Status: DONE

  • Need for a certificate trust store: Somehow the single (!) server certificate to be presented during SSL/TLS handshake needs to be determined. How would that be done with multiple certificates in a trust store? The underlying WBEMListener class takes certfile and keyfile and it passes them on to ssl.SSLContext.load_cert_chain(). That is straight forward. What would a trust store improve?
    KS/COMMENTS: Our listener may be more primitive but the listener should be capable of handling certs from a CA tree, possibly even multiple CAs since, in all probably the same cert store as the server(s) with which it is defining subscriptions. For our listener we need to determine how much of that we want to do since we do not have an implemented multiple certificate store that we can simply attach to. I should have recognized this much earlier in the project but (to be honest) I tried to avoid the whole certificate subject as long as possible (i.e. until I had to fix pegasus).
    Andy: This does not answer the question why a server would need more than one server certificate?
    Result: We agree that for the server certificate presented by the listener, only one certificate is needed. However, when supporting mutual TLS, a set of CA certs is needed, which Karl meant with "trust store". This item is now about adding support for mutual TLS verification.
    Status: DEFERRED to issue Add support for mutual TLS to listener commands #965

  • Do we want a send or test command? KS: I like the idea of an indication sender that we can use in tests but is part of pywbemcli.
    Result: For now, we do only "send". It is not clear what the addiitonal value of test would be given that we cannot really test the indication was processed.
    Status: DEFERRED to issue Add 'listener send' command #966

  • Do we want to show the general options in the 'show' command? They are not generically available but are properties on the context object.
    Result: We want to show them. We have to, if we use the general options --certfile etc. It is also easier than I thought because they come from parsing the command line of the run commands, not from the context of the currently running list/show command.
    Status: DEFERRED to issue Show general options in 'listener show' command #967

  • Does the WBEMListener class need to be extended with:

  • Idea: During startup of the "run" command, have its stdout/err/in redirected to/from the parent process, in order to get success and error messages in the parent process, and also the potential prompt for keyfile password. Needs investigation.
    Status: DONE - all messages of the run process are now displayed on the correct stream before the start process terminates, and the start process exit code is also correct (i.e. shows error if the child process had an error). The password prompt is also properly displayed and is not subject to redirection.

@andy-maier andy-maier self-assigned this May 4, 2021
@andy-maier andy-maier added this to the 1.0.0 milestone May 4, 2021
@andy-maier andy-maier requested a review from KSchopmeyer May 4, 2021 17:54
@andy-maier andy-maier force-pushed the andy/add-listener-commands branch from 1d20edc to 7d604fb Compare May 4, 2021 18:00
@andy-maier andy-maier linked an issue May 4, 2021 that may be closed by this pull request
@andy-maier andy-maier force-pushed the andy/add-listener-commands branch 3 times, most recently from ee108f9 to 3f4604a Compare May 4, 2021 18:59
@coveralls
Copy link

coveralls commented May 4, 2021

Coverage Status

Coverage decreased (-4.8%) to 88.108% when pulling 6d7d15d on andy/add-listener-commands into e66b1b0 on master.

Copy link
Contributor

@KSchopmeyer KSchopmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made some particular comments and a couple of general thoughts. Generally I like this. Fairly simple and straight forward but covers the options.

pywbemtools/pywbemcli/_cmd_listener.py Outdated Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Outdated Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Outdated Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Outdated Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
pywbemtools/pywbemcli/_cmd_listener.py Show resolved Hide resolved
@KSchopmeyer
Copy link
Contributor

Made first overview review but have not yet run tests against OpenPegasus. Will do that Wednesday.

@andy-maier
Copy link
Contributor Author

andy-maier commented May 5, 2021

I resolved most of your comments, some by adding an item to the TODO list. Please review that, and particularly the comments not yet marked as resolved. Also, please review the new help text of the listener group again.

Also, I added support for honoring the output format in the show and list commands, and removed the general options field from the output since there is no generic way to get to the general options (they can be accessed one by one as context attributes).

@andy-maier andy-maier force-pushed the andy/add-listener-commands branch 2 times, most recently from bb02c68 to 641b2b8 Compare May 5, 2021 06:51
@andy-maier andy-maier force-pushed the andy/add-listener-commands branch 11 times, most recently from aa4ede4 to f87d9ed Compare May 9, 2021 12:00
@andy-maier andy-maier force-pushed the andy/add-listener-commands branch 3 times, most recently from 03bedeb to 7a4f076 Compare May 9, 2021 14:07
Details:

* Added support for a 'listener' command group that manages WBEM indication
  listeners. (see issue #430)

* Removed Python 3.4 on Windows from GitHub Actions tests, because this environment
  does not have the Microsoft Visual C++ 10.0 compiler needed for building the
  'psutils' package.

Signed-off-by: Andreas Maier <[email protected]>
@andy-maier andy-maier force-pushed the andy/add-listener-commands branch from 7a4f076 to 6d7d15d Compare May 9, 2021 17:12
@andy-maier
Copy link
Contributor Author

andy-maier commented May 10, 2021

DISCUSSION: Should we move the listener functionality into a separate command?

A major consideration for this are the general options. It turns out the majority of options of pywbemcli is only for targeting a WBEM server, or has different meaning (caused by opposite direction of server/client role in TLS handshake).

Options only for targeting a WBEM server:

  -n, --name NAME
  -m, --mock-server FILE
  -s, --server URL
  -u, --user TEXT
  -p, --password TEXT
  -t, --timeout INT
  -U, --use-pull [yes|no|either]
  --pull-max-cnt INT
  -d, --default-namespace NAMESPACE
  -T, --timestats / --no-timestats
  -C, --connections-file FILE PATH

Common options with different meanings:

  --verify / --no-verify
  --ca-certs CACERTS
  -c, --certfile FILE
  -k, --keyfile FILE

Common options with same meaning:

  -o, --output-format FORMAT
  -l, --log COMP[=DEST[:DETAIL]],...
  -v, --verbose / --no-verbose
  --warn / --no-warn
  --pdb
  --version
  -h, --help

@andy-maier andy-maier changed the title [WIP] Added listener command group [WIP] Added listener command group to pywbemcli May 11, 2021
@andy-maier
Copy link
Contributor Author

DISCUSSION: If a new 'pywbemlistener' command is the direction (see PR #971), we can abandon this PR.

@andy-maier andy-maier changed the title [WIP] Added listener command group to pywbemcli [DROP] Added listener command group to pywbemcli May 14, 2021
@andy-maier andy-maier closed this May 21, 2021
@andy-maier andy-maier deleted the andy/add-listener-commands branch June 11, 2021 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an indication listener to pywbemtools
3 participants