Add uw ecflow server tool#923
Conversation
) Check $HOME/.ecflowrc/ssl for the three required ecFlow SSL files (dh2048.pem, server.crt, server.key). If all exist, log and reuse them. If the directory is incomplete, raise UWError. If the directory is absent, create it and generate the files via openssl, creating each file with 600 permissions before writing to it.
…ver (ufs-community#868) - Implement the previously no-op --report flag: emit JSON {"vars": {ECF_HOST, ECF_PORT, ECF_SSL}} to stdout right after a port is secured. ECF_SSL is included only when SSL is enabled. - Secure the TCP port up front via socket bind (_secure_port/_port_available) so the report reflects a real, available port before the blocking server call. - Accept dict, YAMLConfig, or Path config in server() and api.ecflow.server(), with stdin_ok routing consistent with realize/validate. - Add test coverage and refresh server CLI docs/help output.
Port Paul's demo architecture into uwtools.ecflow.server(): run ecflow_server in a thread that hunts for a free port (catching 'address in use'), while the main thread handles SIGINT and shuts the server down via ecflow_client --terminate. Drop the pre-check _secure_port/_port_available helpers. - Port-attempt logging at DEBUG (1a) - Logs to stderr, JSON to stdout (1b) - JSON report emitted only when --report set (1c) - --report echoes all server vars: config ECF_* plus ECF_HOST/ECF_PORT/ECF_SSL (1d) - Export ECF_SSL to server env (1=secure, unset=insecure); cwd=ECF_HOME (2b) Rewrite ecflow server tests for the threaded model; 100% module coverage. Note: race condition in port selection deferred to a follow-up issue.
Live testing against a real ecflow_server surfaced three issues: - Port range: ecFlow accepts only 1024-49151, not the dynamic/private range 49152-65535, so the default (auto-port) path always failed. - Report buffering: the --report JSON was block-buffered and did not appear when piped; now flushed. - SSL client calls: ecflow_client --ping and --terminate omitted --ssl, so secure servers could not be pinged or shut down. Updates code, CLI help, API docstrings, docs, and tests accordingly.
- Compute the config-to-string var dict once and reuse it for both the server environment and the report vars. - Name the SSL filenames as constants (_SSL_KEY/_SSL_CERT/_SSL_DHPARAM) and reference them in _provision_ssl instead of repeating literals. No behavior change; ecflow suite stays at 100% coverage.
There was a problem hiding this comment.
Pull request overview
Adds a new uw ecflow server CLI/API action to provision SSL materials (by default) and launch an ecflow_server process in the foreground, with an optional JSON report for downstream tooling.
Changes:
- Implement
uwtools.ecflow.server()with SSL provisioning, server startup on fixed/random port, SIGINT shutdown, and optional JSON reporting. - Wire the new action through the public API module and CLI subparser/dispatcher, plus update CLI docs/help text.
- Add unit tests for SSL provisioning, server lifecycle/reporting behavior, and CLI/API wiring.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/uwtools/ecflow.py | Implements SSL provisioning helpers and the ecFlow server runner (threaded start, ping wait, report, SIGINT shutdown). |
| src/uwtools/api/ecflow.py | Exposes ecflow.server() via the public API with stdin/config handling. |
| src/uwtools/cli.py | Adds uw ecflow server subcommand, arguments (--insecure, --port, --report), and dispatch wiring. |
| src/uwtools/strings.py | Adds string constants used by the new CLI arguments/subcommand. |
| src/uwtools/resources/jsonschema/ecflow-server.jsonschema | Introduces an internal schema for validating server config inputs. |
| src/uwtools/tests/test_ecflow.py | Adds tests for SSL provisioning + server start/wait/report/shutdown behavior. |
| src/uwtools/tests/api/test_ecflow.py | Adds API-level tests for the new ecflow.server() wrapper. |
| src/uwtools/tests/test_cli.py | Adds CLI parser/dispatch tests for the new ecflow server action. |
| docs/sections/user_guide/cli/tools/ecflow/server-help.cmd | Captures the command used to generate uw ecflow server --help output. |
| docs/sections/user_guide/cli/tools/ecflow/server-help.out | Documents the CLI help output for uw ecflow server. |
| docs/sections/user_guide/cli/tools/ecflow/help.out | Updates ecflow tool help to include the new server action. |
| docs/sections/user_guide/cli/tools/ecflow.rst | Adds user-guide documentation and examples for uw ecflow server. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Pre-create the SSL private key and certificate files with 0600 permissions before openssl writes them, matching the dh2048.pem path and the issue's requirement to never write key material with open permissions. - Allow server() config to be None (read stdin), aligning the internal signature with the public API and the realize() entrypoint. ecflow suite stays at 100% coverage.
- Catch OSError in _server_start so a failure to launch ecflow_server (e.g. not on PATH or a missing ECF_HOME) sets the terminal event and error instead of leaving _server_wait to loop forever. - Replace preexec_fn (unsafe in a multi-threaded process per the Python docs) with start_new_session=True to keep the terminal's SIGINT from reaching the server child. ecflow suite stays at 100% coverage.
Assert thread.error is not None before calling .startswith, since _ServerThread.error is typed str | None and CI runs mypy over tests.
Sleep briefly between ping attempts so polling does not spin a CPU core or hammer ecflow_client while the server starts up or fails to.
maddenp-cu
left a comment
There was a problem hiding this comment.
First pass comments... I want to read more, especially the tests, and to test manually from this branch, but can provide this initial feedback until then.
|
I did a test with I think we should create the directory the user requests on their behalf and not fail. We usually just execute something like |
openssl req/dhparam honor the umask when writing the cert and DH params, and whether a pre-touched 0600 file's perms survive the write is openssl version/platform dependent. Chmod each file to 0600 after generation so the result is deterministic regardless of umask or openssl behavior.
…trings - Move the standalone ecflow-server.jsonschema into the server sub-schema of ecflow.jsonschema (ECF_HOME required, additionalProperties: false), alongside suitedef under the top-level ecflow key, and delete the standalone file. - server() now validates via the unified ecflow schema and reads the nested cfg.data[ecflow][server] config. - Update docstrings/help: 'optionally with SSL security enabled' and '(e.g. hostname, port)' in api/ecflow.py, cli.py, and server-help.out. - Update ecflow/api/schema tests to the nested server config shape; add test_schema_ecflow_server.
- Restrict SSL file perms via 'umask 0077 &&' prefix on openssl commands (removes _ssl_touch/chmod and the open-permissions window). - DRY out the ecFlow port range into ECFLOW_PORT_MIN/ECFLOW_PORT_MAX. - Drop the ssl_dir parameter from _provision_ssl; tests patch _SSL_DIR.
- _server_start now creates the run directory (ECF_HOME) with mkdir(parents=True, exist_ok=True) instead of failing when it does not exist. - Drop the 'several minutes' DH-parameters claim (it takes ~seconds). - _provision_ssl test no longer mocks our own _ssl_generate_* helpers; it mocks only run_shell_cmd and asserts the real SSL files on disk. - _server_start tests use real tmp_path run dirs.
Exercise real openssl in the key and cert _ssl_generate tests and assert the resulting files are 0600, verifying the 'umask 0077 &&' prefix for real. The dhparam test stays mocked because DH-parameter generation is slow and entropy-dependent (~tens of seconds).
maddenp-cu
left a comment
There was a problem hiding this comment.
This is looking good. I'm down to mostly nitpicks and style suggestions.
… refs, expand ecFlow server schema tests
maddenp-cu
left a comment
There was a problem hiding this comment.
I think the only open questions for me were 1. Whether to shift some strings from STR to EC (hopefully other reviewers will weigh in) and 2. Can some of the unit tests be shortened for less boilerplate (the fixture @AnnaSmoot-NOAA suggests might be a good solution).
But whatever decisions are made on those two items, I'm happy with this update if it doesn't change drastically. Nice work!
Synopsis
Adds a new
uw ecflow serveraction (CLI + API) that provisions and runs and ecflow server. Closes issue #868.The action:
ecflow_serveron an available TCP port, picking a random port in the registered range (1024-49151) and retrying until a free one is found, or using an explicit--port.$HOME/.ecflowrc/ssl, or generates them (server.key,server.crt,dh2048.pem) viaopenssl, creating each file with600permissions before writing. A--insecureswitch disables SSL.CTRL-C(SIGINT), terminating the server viaecflow_client --terminaterather than emitting a stack trace.stderr; with--report, a JSON object of server connection variables (ECF_HOST,ECF_PORT, andECF_SSLwhen secure) is written tostdoutfor downstream consumers (e.g. piped to jq).ecflow-server.jsonschema).ECF_HOMEis required and any of the documentedECF_*server settings (enumerated in the schema) are optional and exported into the server environment.ECF_PORTandECF_SSLare intentionally excluded from config. The tool manages the port (CLI--port) and SSL state (--insecure) itself.CLI:
The feature was verified against a live
ecflow_server(ecFlow 5.15.2), exercising both secure and insecure paths, port auto-selection, the--reportJSON, and cleanCTRL-Cshutdown.Note for Reviewers:
This branch was rebuilt cleanly on top of
mainafter theecflow_toolbranch was squash-merged (#920); it contains server-tool changes only. The arbitraryECF_SSL-cert-path option is intentionally deferred (MVP uses the default$HOME/.ecflowrc/ssl) and will be a follow-up.Type
Impact
Checklist