Skip to content

PMM-15326: Let an app own its settings without a second config implementation - #1393

Draft
plebioda wants to merge 1 commit into
PMM-15299-open-managerfrom
PMM-15326-app-settings
Draft

PMM-15326: Let an app own its settings without a second config implementation#1393
plebioda wants to merge 1 commit into
PMM-15299-open-managerfrom
PMM-15326-app-settings

Conversation

@plebioda

Copy link
Copy Markdown
Collaborator

Summary

SEP's settings router is admin-gated, and not every caller of an app's configuration is an admin. The case in hand is a deployment-level shared secret with no person behind it - PMM's --sep-token resolves to the synthetic sep-service user, built is_admin=False deliberately - which has to be able to read and change the configuration of the app it drives, and nothing else.

An app should be able to serve its own /config: a schedule change scoped to one app has no business requiring SEP-wide administrative access. What an app must not do is reimplement what a settings PATCH means. "Validate the whole batch before writing any of it, persist atomically, republish the proxy snapshot inline, fire the rebind callbacks for the keys that changed" is four invariants deep, and a second copy drifts into a second set of validation rules - which is how one endpoint starts accepting what the other rejects.

So the two halves of PATCH /{setting_class} and DELETE /{setting_class}/{key} that are not routing come out as apply_class_overrides and clear_class_override, and the router's own handlers call them. Pure extraction - same phases, same errors, same order, no behaviour change - and the existing settings tests cover it unchanged.

clear_class_override comes out with it rather than being left behind, because without a way to remove an override an operator who once set a value can only ever set another one: "no override" stops being a reachable state and the value the deployment shipped becomes unrecoverable through the API.

Two things ride along, both about a setting changing and nothing acting on it

  • build_sep_override_callbacks is lifted out of sep_overrides_lifespan so the registry is a value a test can assert on. Every entry in it is a change with an effect outside the settings snapshot: a client rebound, a logging config re-applied, a beat row rewritten. A missing entry fails silently - the API reports the new value and nothing happens until the process restarts - and a literal buried inside a context manager is not something a test can look at.
  • The reseed callback now re-applies app gating. init_periodic_tasks_db preserves enabled only on its update path, and a schedule an app may set to None - which is how an app-owned periodic task is turned off - contributes no task while it is null, so the orphan cleanup deletes its beat row. Setting it again takes the create path, which builds a fresh row at the model's default enabled, and a disabled app would start running on the next beat tick. init_sep_db already runs init_periodic_tasks_db and sync_app_periodic_task_gating as a pair at startup; this makes the hot path do the same.

One existing test is retargeted rather than extended: the settings-proxy list assertion indexed the last three classes by position, and app-owned groups are appended, so every app that declares a settings class shifted a fixed index. It now asserts the ordering it actually cares about - core, then remote, then app-owned - which is the contract, and which no future app can break by existing.

Why this is its own PR

Nothing here has a caller in this branch. The first is the OpenManager Inventory app, which serves its own /config off these two functions - but they belong to the app framework, not to that app: any app whose configuration a non-admin principal must reach needs exactly this and should not write it again. Reviewing a core settings refactor next to 5k lines of MongoDB probing wastes both reviews.

Second of four PRs replacing the single 21k-line draft #1371, which stays open until the replacements are up. #1390 (the executor-fleet endpoint) is independent of this one; the om_inventory app comes next and imports both these functions; the side-car activation is last.

Base is PMM-15299-open-manager, the integration branch for the epic.

No generated artifacts: this changes no route shapes, so scripts/dump_openapi.py leaves the committed specs alone. Confirmed rather than assumed.

Tested

Note that CI does not run here - .github/workflows/ci.yml triggers on pull_request with branches: [main], so a PR based on pmm or on this integration branch gets no test job. Everything below was run locally against this branch.

  • venv/bin/python -m pytest tests/app -q -n 8 - full suite on this branch
  • tests/app/sep/api/routes/test_settings*.py - the extraction is covered by the settings suite unchanged, which is the point: the handlers behave identically because they now call the same code
  • tests/app/sep/test_override_callbacks.py - the reseed callback re-applies gating over the same task set it seeded, and the registry is asserted entry by entry
  • scripts/dump_openapi.py - no spec drift
  • make run-pre-commit - all 22 hooks pass

Checklist

  • New/modified functions have type hints and rST docstrings
  • New tests added for new features or bug fixes
  • All tests pass locally (make test)
  • Pre-commit hooks pass (make run-pre-commit)
  • Database migrations generated if models changed (make makemigrations) - N/A, no model changes
  • User-facing changes documented (README, inline help, UI text) - N/A, no user-facing surface changes; the admin settings API behaves exactly as before
  • Configuration changes documented with examples - N/A, no configuration changes
  • Changelog fragment added under changelog.d/ - N/A on two counts, and the second one needs a decision for the whole ticket. This PR is an internal refactor with no user-visible effect, so skip-rule 1 applies. But it also cannot carry one: scripts/changelog.py's FRAGMENT_RE only accepts SEP-<n>.<section>.md and raises on anything else, so no PMM-ticketed fragment can exist - which means the OpenManager app PR cannot carry one either. Either the SEP half of this work gets a SEP-xxxx for changelog purposes, or the feature's release note comes from PMM's side only. Flagged on PMM-15326: Report the executor fleet, not only the part of it that works #1390 as well.

…entation

SEP's settings router is admin-gated, and not every caller of an app's configuration
is an admin. The case in hand is a deployment-level shared secret with no person
behind it - PMM's `--sep-token` resolves to the synthetic `sep-service` user, built
`is_admin=False` deliberately - which has to be able to read and change the
configuration of the app it drives, and nothing else.

The app could serve its own `/config`, and an app *should* be able to: a schedule
change scoped to one app has no business requiring SEP-wide administrative access.
What an app must not do is reimplement what a settings PATCH means. "Validate the
whole batch before writing any of it, persist atomically, republish the proxy
snapshot inline, fire the rebind callbacks for the keys that changed" is four
invariants deep, and a second copy of it drifts into a second set of validation
rules - which is how one endpoint starts accepting what the other rejects.

So the two halves of `PATCH /{setting_class}` and `DELETE /{setting_class}/{key}`
that are *not* routing come out as `apply_class_overrides` and
`clear_class_override`, and the router's own handlers call them. Pure extraction:
same phases, same errors, same order, no behaviour change, and the existing settings
tests cover it unchanged.

`clear_class_override` comes out with it rather than being left behind, because
without a way to remove an override an operator who once set a value can only ever
set another one: "no override" stops being a reachable state and the value the
deployment shipped becomes unrecoverable through the API.

Two things ride along, both about the same failure mode - a setting that changes and
nothing acts on it:

- `build_sep_override_callbacks` is lifted out of `sep_overrides_lifespan` so the
  registry is a value a test can assert on. Every entry in it is a change with an
  effect *outside* the settings snapshot: a client rebound, a logging config
  re-applied, a beat row rewritten. A missing entry fails silently - the API reports
  the new value and nothing happens until the process restarts - and a literal buried
  inside a context manager is not something a test can look at.

- The reseed callback now re-applies app gating. `init_periodic_tasks_db` preserves
  `enabled` only on its *update* path, and a schedule an app may set to `None` -
  which is how an app-owned periodic task is turned off - contributes no task while
  it is null, so the orphan cleanup deletes its beat row. Setting it again takes the
  *create* path, which builds a fresh row at the model's default `enabled`. A
  disabled app would start running on the next beat tick. `init_sep_db` already runs
  `init_periodic_tasks_db` and `sync_app_periodic_task_gating` as a pair at startup;
  this makes the hot path do the same.

One test is retargeted rather than extended: the settings-proxy list assertion
indexed the last three classes by position, and app-owned groups are appended, so
every app that declares a settings class shifted a fixed index. It now asserts the
ordering it actually cares about - core, then remote, then app-owned - which is the
contract, and which no future app can break by existing.

Nothing here has a caller yet in this branch. The first is the OpenManager Inventory
app, which serves its own `/config` off these two functions, but they are the app
framework's, not that app's: any app whose configuration a non-admin principal must
reach needs exactly this and should not write it again.
@plebioda

Copy link
Copy Markdown
Collaborator Author

Where the trust-model question lives: #1395 (comment on its /config handler) asks the SEP team whether a per-app, non-admin config endpoint is acceptable at all, and whether sep-service is the right identity for PMM→SEP calls. That is the decision worth having; it is not this PR.

This PR is useful either way. It removes a duplicate implementation of what a settings PATCH means - validate the whole batch, persist atomically, republish the snapshot, fire the rebind callbacks - whoever ends up allowed to call it. If the answer over there is "config changes need an admin", nothing here changes: the app's /config handlers go away and these two functions still have exactly one implementation, called by the admin router.

Question 4 of that thread is the one aimed at this PR: if per-app config endpoints turn out to be the right pattern, these helpers are most of what a second app would need, and this could become framework rather than something each app wires by hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant