[PATCH datacenter-manager/proxmox v3 00/21] inject application context via API macro for easier integration testing

[Lukas Wagner <l.wagner@proxmox.com>]

TL;DR: Inject essential runtime config, client factory, etc. as an application
context object and allow to retrieve this object easily in an API handler via
the API macro. This allows us directly call API handler implementations from
integration tests. The aim is to make it easier to write good tests on a larger
level.

## Rationale

Considering the different stages of automated software testing, unit testing
(test small software components in isolation) integration testing (test
multiple components together, specifically their interactions) and
end-to-end testing (test the entire application in a context as close to
production as possible), I've found that the middle one, integration testing is
a very challenging one in our stack.

I've found that the challenges with integration testing mostly stem from the
following:

   - "hardcoded" (as in, determined by some constant or literally
     hard-coded) assumptions about storage paths (config, state, caches) and
     users/permissions. This makes it challenging to call into the component
     from a test running as normal user, e.g. from a regular `cargo test`.
 
   - use of global/static instances (examples: Worker task context,
    proxmox-product-config, client factory in PDM...) in application code and
    shared crates. While this can be okay for things that are truly global (e.g.
    logging), it often hinders testing and  especially test isolation due to hidden
    dependencies between test cases and some potential internal state of the global
    instance. This is one of the common causes of flaky tests. Also, the setup of
    these global instances in test cases is always a bit awkward, since the order
    of test execution is not defined (and might actually run in parallel), so some
    kind of synchronization between the test cases is necessary. Furthermore,
    certain test cases might require a *different* setup for these global instances
    (example: client factory that should produce a different kind of mocked PVE
    client); but since we usually put these in OnceLocks, one has to put these
    tests into a separate test binary.

  - tight coupling between major subsystems (e.g. one part calling into the
    other without any clear boundary or abstraction via callbacks or traits)


With regards to PDM, there are a couple of things that need considering when testing:
  - Filesystem access to config, state and caches
  - Interactions with remotes via their API

I feel like if we find a sensible way to abstract these for tests, we can
reasonably cover a huge amount of the code in the backend.

## Implementation

The core idea is to provide a context/application object ('struct
PdmApplication') and "injecting" it into API handlers via special `State`
parameters, automatically handled by the API macro.

This new context object would give access to:
  - base paths for caches, config, state
  - file permissions, user/group
  - client factory
  - api cache

The PdmApplication object is set up during daemon startup, stored in the RpcEnvironment,
and then cloned for each API request, assuming that the handle requested access via
an State<PdmApplication> parameter.

PdmApplication is a thin wrapper around Arc<PdmApplicationInner>, which stores the actual
configuration and trait objects.

This general pattern of injecting application state/context via a parameter
to the handler is something that is also common in other Rust-based
web frameworks, e.g. [actix-web] and [axum].

## References:

[actix-web]: https://actix.rs/docs/application/#state
[axum]: https://docs.rs/axum/latest/axum/#sharing-state-with-handlers


## Bumps needed

 - proxmox-router
 - proxmox-rest-server
 - proxmox-api-macro

## Changes since v1:
  - fix a couple typos, grammar mistakes
  - extend the commit message for the commit that adds the type-keyed map
    to RpcEnvironment
  - fix test case for API macro
  - consistently use app handle in node subscpription api handler
  - in read_captured_response, don't unwrap but return an Error
  - add missing doc comments

## Changes since v2:
  - Mark state arguments using an attribute, not via a wrapper type
    This now supports the following ways to use injected types

    #[state] foo: Foo
    #[state] foo: &Foo
    #[state] foo: Option<Foo>
    #[state] foo: Option<&Foo>

    The optional ones are None if the requested type was never registered.
    In this series we don't use it yet, but I think this is still a
    valuabe addition, which is why I included it for the review.

    The reference variants borrow the type from the registry.

    Using an attribute instead of a wrapper type has the advantage that
    it makes it less likely to have a collision due to type names;
    State<T> was too generic to rule out conflicts

  - Move ProductConfig to proxmox-product-config.
    Also add documentation and further helpers for building CreateOptions

  - Give more context on why the ApiCache wrapper type is added
  - Use CARGO_TARGET_TMPDIR instead of /tmp
  - ParallelFetcher now provides a PdmApplication handle via its args type,
    mainly for future proofing. PdmApplication is now also a mandatory
    argument when building a ParallelFetcher, this gently nudges
    developers to use #[state] PdmApplication at the API level
  - In the firewall API module, consistently use the app context object


proxmox:

Lukas Wagner (4):
  router: introduce shared state
  rest-server: allow to inject shared state
  api-macro: support shared state extraction type
  product-config: add ProductConfig type

 proxmox-api-macro/src/api/method.rs          | 228 +++++++++++++++++-
 proxmox-api-macro/src/api/mod.rs             |   6 +
 proxmox-api-macro/src/lib.rs                 |  56 ++++-
 proxmox-api-macro/tests/state.rs             | 240 +++++++++++++++++++
 proxmox-product-config/src/lib.rs            |   3 +
 proxmox-product-config/src/product_config.rs | 205 ++++++++++++++++
 proxmox-rest-server/src/api_config.rs        |  14 +-
 proxmox-rest-server/src/environment.rs       |   6 +-
 proxmox-router/src/cli/environment.rs        |  12 +-
 proxmox-router/src/lib.rs                    |   2 +
 proxmox-router/src/rpc_environment.rs        |   7 +
 proxmox-router/src/shared_state.rs           |  46 ++++
 12 files changed, 812 insertions(+), 13 deletions(-)
 create mode 100644 proxmox-api-macro/tests/state.rs
 create mode 100644 proxmox-product-config/src/product_config.rs
 create mode 100644 proxmox-router/src/shared_state.rs


proxmox-datacenter-manager:

Lukas Wagner (17):
  context: promote context to a dir-style module
  pdm-config: remotes: rename trait methods to read/write/lock
  pdm-config: subscriptions: rename trait methods to read/write/lock
  remote iterator: pass remote config reader explicitly
  context: introduce a ContextFactory to build application context
  context: establish PdmApplication object
  context: register PdmApplication in router
  connection: use client factory from PdmApplication handle
  parallel fetcher: pass arguments to closure in a single type
  server: migrate existing ParallelFetcher users to use PdmApplication
  tests: add helpers for building API-handler-level integration tests
  tests: add example tests for SDN API routes
  api-cache: add wrapper type
  context: provide api-cache on the app object
  api: subscriptions: use PdmApplication instead of globals
  pdm-config: subscriptions: drop unused accessor functions
  tests: add example tests for remote subscription management

 cli/admin/src/main.rs                         |  10 +-
 lib/pdm-config/src/remotes.rs                 |  26 +-
 lib/pdm-config/src/subscriptions.rs           |  59 +--
 server/src/api/nodes/subscription.rs          |  23 +-
 server/src/api/nodes/tasks.rs                 |   2 +-
 server/src/api/pbs/mod.rs                     |  10 +-
 server/src/api/pve/firewall.rs                |  53 +--
 server/src/api/pve/mod.rs                     |   3 +-
 server/src/api/remotes/mod.rs                 |   5 +-
 server/src/api/remotes/updates.rs             |   3 +-
 server/src/api/resources.rs                   |  39 +-
 server/src/api/sdn/controllers.rs             |  17 +-
 server/src/api/sdn/vnets.rs                   |  20 +-
 server/src/api/sdn/zones.rs                   |  17 +-
 server/src/api/subscriptions/mod.rs           | 223 +++++++-----
 server/src/api_cache.rs                       | 117 ++++--
 server/src/bin/proxmox-datacenter-api/main.rs |  17 +-
 ...proxmox-datacenter-manager-daily-update.rs |  12 +-
 .../bin/proxmox-datacenter-privileged-api.rs  |  22 +-
 server/src/connection.rs                      |  54 ++-
 server/src/context.rs                         |  49 ---
 server/src/context/default.rs                 |   5 +
 server/src/context/faked_remotes.rs           |  42 +++
 server/src/context/mod.rs                     | 165 +++++++++
 server/src/metric_collection/mod.rs           |   6 +-
 .../remote_collection_task.rs                 |  47 +--
 server/src/parallel_fetcher.rs                | 109 +++++-
 server/src/remote_tasks/refresh_task.rs       |  27 +-
 server/src/remote_updates.rs                  |  17 +-
 server/src/test_support/fake_remote.rs        |   8 +-
 .../pve/remote-a/list_vnets.json              |   8 +
 .../pve/remote-a/list_zones.json              |   8 +
 .../pve/remote-b/list_vnets.json              |   8 +
 .../pve/remote-b/list_zones.json              |   8 +
 server/tests/common/environment.rs            |  28 ++
 server/tests/common/mod.rs                    | 111 ++++++
 server/tests/common/test_application.rs       | 337 ++++++++++++++++++
 server/tests/test_sdn.rs                      |  86 +++++
 server/tests/test_subscriptions.rs            | 122 +++++++
 39 files changed, 1509 insertions(+), 414 deletions(-)
 delete mode 100644 server/src/context.rs
 create mode 100644 server/src/context/default.rs
 create mode 100644 server/src/context/faked_remotes.rs
 create mode 100644 server/src/context/mod.rs
 create mode 100644 server/tests/api_responses/pve/remote-a/list_vnets.json
 create mode 100644 server/tests/api_responses/pve/remote-a/list_zones.json
 create mode 100644 server/tests/api_responses/pve/remote-b/list_vnets.json
 create mode 100644 server/tests/api_responses/pve/remote-b/list_zones.json
 create mode 100644 server/tests/common/environment.rs
 create mode 100644 server/tests/common/mod.rs
 create mode 100644 server/tests/common/test_application.rs
 create mode 100644 server/tests/test_sdn.rs
 create mode 100644 server/tests/test_subscriptions.rs


Summary over all repositories:
  51 files changed, 2321 insertions(+), 427 deletions(-)

-- 
Generated by murpp 0.12.1