feat: Implement Lightning kickstart - #5026
Conversation
|
Based on my review of the PR changes, here are my findings. Security Review ✅
|
midigofrank
left a comment
There was a problem hiding this comment.
Thanks @doc-han ,
Comments below are grouped: things I think need fixing, two decisions that get much more expensive after merge, and a list of smaller stuff.
1. ${...} interpolation corrupts JS job bodies
interpolate_env/1 walks every string in the document, including jobs[].body. Job bodies are the most ${}-dense strings in Lightning.
Regex.scan(~r/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/, "fn(state => http.get(`/patients/${id}`))")
#=> [["${id}", "id"]]Two failure modes:
- Hard failure: any body using a template literal on a bare identifier (
${id},${name},${state}) aborts the run withScenario references ${id}, but that environment variable is not set. That job simply can't be expressed in a scenario file. - Silent corruption: when the identifier happens to exist in the environment —
`${USER}`,`${HOME}`,`${PATH}`— it's substituted into the job source at seed time, and the seeded workflow runs code nobody wrote.
Suggestion: make interpolation explicit rather than ambient, e.g. a YAML tag (apiKey: !env DHIS2_PASSWORD) or a namespaced sigil (${env:VAR}). Then bodies can never collide with it. Failing that, only walk the positions that are meant to hold secrets (credentials[].body, credentials[].credential_bodies[].body) and never recurse into jobs[].body.
2. Two role: owner members — or moving ownership — crashes with a raw constraint error
project_users has a partial unique index, so a project can only have one owner:
# priv/repo/migrations/20240502053312_add_unique_index_on_project_users.exs
create unique_index("project_users", [:project_id], where: "role = 'owner'", name: "project_owner_unique_index")parse_members/3 validates at least one owner but never at most one, and reconcile_members/2 writes with bare Repo.insert!(%ProjectUser{...}) and Ecto.Changeset.change/2 |> Repo.update!(). No constraint is registered on those changesets, so the violation surfaces as an Ecto.ConstraintError rather than a scenario-level message. Two ways in:
- a scenario declaring two members with
role: owner; - a re-run that moves ownership (
a@owner →b@owner) — promotingb@collides witha@'s existing row.
Those raw writes also bypass ProjectUser.changeset/2 validation entirely. Projects.add_project_users(project, members, false) and Projects.update_project_user/2 are the canonical paths — the false suppresses the project_addition notification emails, which is what you want for seeding, and you get the owner validation and friendly constraint messages for free. (No audit trail exists on project_users, so nothing is lost audit-wise by the current approach — this is only about validation and error quality.)
3. Unknown / mistyped keys are silently ignored at the top two levels
The moduledoc and README both promise that "Unknown fields are rejected by the provisioner's own validation … typos fail loudly rather than being silently dropped". That holds inside workflow/trigger/job/edge maps, because those are passed through to validate_extraneous_params/1. It doesn't hold above them, because run/1 and ensure_project/3 read named keys and assemble the document by hand:
- top level:
usres:,projcts:,credential:→ silent no-op; - per project:
workflowss:, plus three keys the provisioner genuinely supports —description:,collections:,channels:(provisioner.ex:629-662) → silently dropped; - per user and per credential: any misspelled key.
For a hand-written file this is the worst failure mode: the config does nothing and says nothing. Minimum fix for this PR is an unknown-key check at the scenario, user, credential and project levels; passing description/collections/channels through to the document is nearly free at the same time.
4. superuser: yes silently creates a regular user
truthy/1 accepts only true and "true", and yamerl doesn't treat YAML 1.1 booleans as booleans:
YamlElixir.read_from_string!("a: yes\nb: true\nc: on\nd: 1\n")
#=> %{"a" => "yes", "b" => true, "c" => "on", "d" => 1}So superuser: yes produces a normal user and api_token: yes produces no token (and a null in the manifest, which then breaks whatever reads it). Lightning's own env parsing (Lightning.Config.Utils.ensure_boolean/1) accepts yes/true/1, so people will write yes. Either reuse that coercion or raise on anything that isn't a real boolean — silently defaulting to false is the option to avoid.
5. Existing users aren't converged, but are silently confirmed
ensure_users/1 matches on email and returns the existing user unchanged: an existing regular user declared superuser: true stays a regular user, and a changed password:, first_name: or last_name: is ignored. Meanwhile confirm_user/1 does mutate pre-existing accounts — confirmed_at is force-set on any unconfirmed user matched by email.
That's backwards for a declarative file: the fields the operator explicitly declared are ignored, and the one they didn't is written. Worth deciding and documenting; at minimum, promote-to-superuser should either happen or fail loudly rather than silently not happening.
6. The manifest writes live API tokens to a 0644 file, for a consumer that doesn't exist
run_file/2 → File.write!(manifest_path, ...) gets mode 0644, and both the README and the task docs suggest /tmp. The contents include usable personal access tokens (Lightning.Tokens.PersonalAccessToken JWTs; the "api" context has no expiry check).
Nothing in the branch reads it — bin/e2e's seed_data() runs mix lightning.bootstrap '${SCENARIO_FILE}' with no --manifest, and the only other references are the tests and the README.
It's worth being explicit about what the manifest is actually for, because it's a one-value feature with a general-looking surface. Of what it emits: ids and webhook_path are already knowable by the caller (derived from names, or pinnable with id:, and the path is just /i/<trigger-id>); emails and names are echoes of the input; only api_token is genuinely unknowable up front, because it's a JWT signed with the instance key.
Suggestion: drop api_token: from the scenario file, drop the manifest with it, and mint tokens on demand from a task alongside the existing gen_worker_keys / gen_encryption_key:
TOKEN=$(mix lightning.gen_api_token [email protected])
curl -H "Authorization: Bearer $TOKEN" localhost:4003/api/projectsThat's a thin wrapper over Accounts.generate_api_token/1 plus an email lookup, and it means: the config file stops emitting secrets (matters if these are meant to be committed or mounted from a ConfigMap), there's no file-permission question because nothing hits disk, and tokens become obtainable at any time rather than only at seed time — right now a harness that wants a second token, or one for a user seeded last week, has to re-run the whole scenario.
If you'd rather keep the manifest, it needs File.chmod!(path, 0o600) and the docs should stop suggesting /tmp. Emitting JSON on stdout (--output json, piped to jq) is the middle option: no file, no mode, caller decides.
Related: don't be tempted to let the file declare a literal token value
It looks attractive — declare api_token: "1234", no output artifact needed — and it half-works, which is worse than not working, because the two auth paths validate differently:
| route | plug | check |
|---|---|---|
/api/* (provision, projects, workflows, runs, …) |
authenticate_bearer |
token_and_context_query/2 — plain where: [token: ^token, context: "api"] string equality, no signature check |
/collections/* |
Plugs.ApiAuth |
Tokens.verify/1 — JWT signature against the instance key, plus iss/sub/iat claims, plus DB existence |
So a declared "1234" would authenticate against the REST API and be rejected by collections, with the failure landing inside job runtime. (The asymmetry predates this PR and isn't exploitable — forging a token means writing to user_tokens.)
7. bin/e2e scenario snapshots go stale, and scenario paths are cwd-relative
- The snapshot slug is the scenario file's basename (
/tmp/lightning_e2e_snapshot__example.sql), so two scenarios with the same basename in different directories share a snapshot, and — the one that'll bite — editing a scenario doesn't invalidate its snapshot.bin/e2e start --scenario exampleafter an edit silently boots the old data. Hashing the file contents into the slug fixes both. SCENARIOS_DIR="$(dirname "$0")/scenarios", and a user-supplied--scenario ./my.yaml, are resolved relative to the caller's cwd but consumed insiderun_in_project_root, whichcds to$PROJECT_ROOT. Works from the repo root, breaks from anywhere else. Resolving to an absolute path inresolve_scenariocovers it.- Cosmetic:
cmd_start→cmd_serverboth callresolve_scenario, so the📋 Using scenario/📸 Snapshotbanner prints twice.
Two decisions that get much more expensive after merge
The name. bootstrap now means four different things in the repo: bin/bootstrap (dev machine setup), Lightning.Config.Bootstrap (runtime env-var configuration), ALLOW_BOOTSTRAP, and this (data seeding). Lightning.Config.Bootstrap vs Lightning.Bootstrap is the sharp edge — one namespace apart, opposite meanings. Lightning already has vocabulary for this artifact: a project export is a state file (mix lightning.merge_projects takes "state files"; the API is /api/provision with a state document). So my suggestion is Lightning.InstanceState + mix lightning.state.apply FILE + LIGHTNING_STATE_FILE, which composes with the existing term; Lightning.Blueprint is the alternative if you'd rather the feature had its own word. Either way, keep scenario as the e2e-local term only, and reserve bootstrap for env config and the dev script.
Whether workflows: should be the workflow spec format we already have. Lightning has a hand-writable workflow YAML format with a published JSON Schema — assets/js/yaml/schema/workflow-spec.json, implemented in assets/js/yaml/util.ts — used by the editor's YAML import/export and by workflow templates. It keys jobs/triggers/edges by slug and references them by key, and already has a credential field:
name: My Workflow
jobs:
transform:
name: Transform data
adaptor: "@openfn/language-common@latest"
body: fn(state => state)
credential: dhis2-prod
triggers:
webhook:
type: webhook
enabled: true
edges:
webhook->transform:
source_trigger: webhook
target_job: transform
condition_type: alwaysThis PR introduces a third dialect (lists, from:/to:, condition:). Practical consequence: a workflow exported from the editor can't be pasted into a scenario file, and we end up maintaining two hand-written workflow formats plus the provisioner document. The cost of aligning is writing the Elixir side of convertWorkflowSpecToState — which is independently useful, since server-side YAML import/export currently depends on browser code. Worth settling now, because changing the file format after people have written files is the expensive kind of change.
Longer write-up of both, plus the parser structure and the boot-time story, in a separate doc — happy to drop it here if useful.
Smaller things
- Duplicate names collapse silently. Two projects called
x, two workflows in one project, or two jobs in one workflow derive the same id. The project/workflow cases upsert one over the other; duplicate job names reach the provisioner as a single job and fail with an id-level message. Cheap to validate up front. - Soft-deleted projects.
Repo.get(Project, id)ignoresdeleted_at, so a re-run updates (effectively resurrects) a project queued for purge. manifest/1iterates maps, sousersandcredentialsordering is unspecified — anything indexing into them is relying on luck. Emit in declaration order.{:ok, result} = Repo.transaction(...)inrun/1wouldMatchErrorif the transaction ever returned{:error, _}. The code always raises instead today (the provisioner-error path is covered by tests), so it's latent — but the assertive match hides the case.timeout: :timer.minutes(2)is hard-coded on the transaction. Fine now; worth an option before this points at a large instance file.- Config accessor. The enabled flag is read with
Application.get_env(:lightning, __MODULE__)directly; everything else goes throughLightning.Config(mockable behaviour +Utils.get_env). ALightning.Config.bootstrap_enabled?/0would match convention and make the test'sApplication.put_envdance unnecessary. Application.ensure_all_started(:yamerl)inload_file!/1can go —yaml_elixiris a real dep now, so it starts with the release.
7b46cae to
e6aca76
Compare
ef88c75 to
124705b
Compare
@midigofrank resolved. Please I'll love another round of review. thanks |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5026 +/- ##
=======================================
- Coverage 90.7% 90.6% -0.1%
=======================================
Files 420 421 +1
Lines 19994 20246 +252
=======================================
+ Hits 18132 18343 +211
- Misses 1862 1903 +41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
export DATABASE_URL=postgres://<user>:<password>@localhost:5432/<database>
mix ecto.migrate
mix lightning.kickstart <path-to-scenario-file>
iex -S mix phx.server |
midigofrank
left a comment
There was a problem hiding this comment.
Hey @doc-han , great job on tightening things here. One big thing that you haven't addressed is the introduction of a new format. We should have the workflow/project spec work one to one with the kickstart.
Ideally, what I'd like to see before merge — happy to be talked out of any of it:
workflows:in a scenario file isworkflow-spec.json, unchanged. Nofrom:/to:/condition:, no singulartrigger:.- An Elixir
Lightning.Workflows.Spec(name up for grabs) that doesspec map -> provisioner document, mirroringconvertWorkflowSpecToStatein the collab editor, validated against the same published JSON Schema. Lightning.Kickstartcalls it and stops building trigger/job/edge documents itself — which deletesbuild_trigger/2,build_job/3,build_edge/5,edge_job_id!/3anddefault_condition/1fromkickstart.ex.
If the scope is unwelcome in this PR, the alternative I'd prefer is landing the spec conversion first as its own PR, but I don't think we should merge a different workflow dialect and plan to reconcile it later.
Description
This PR adds
Lightning.Kickstart: declarative, idempotent seeding of aLightning instance from a YAML/JSON scenario file.
Users (with API tokens), credentials, projects and workflows are provisioned
through the same engine as
/api/provision.mix lightning.kickstart FILE [--manifest out.json]for dev/sourceLightning.Setup.kickstart/2for releases viabin/lightning eval, gatedbehind
ALLOW_KICKSTART=truebin/e2e {start,setup,reset} --scenario <name|path>with per-scenariosnapshots (snapshot names include a content hash, so editing a scenario
invalidates its snapshot instead of silently reusing stale data)
derived from their names, so everything upserts
${env:VAR}interpolation keeps secrets out of committed scenario files —the explicit
env:namespace means${...}in JS job bodies passesthrough untouched
ignored
--manifestwrites a JSON file (API tokens, record ids, webhook paths) forscripts and test harnesses
Groundwork for the kit↔lightning integration test harness (#4784), with
standalone value for seeding deployments to a known state.
Closes #4974
Validation steps
mix test test/lightning/kickstart_test.exsmix lightning.kickstart bin/e2e.d/scenarios/example.yaml --manifest /tmp/m1.jsontwice (second time with
/tmp/m2.json) — both succeed, manifests areidentical, and no duplicate records are created.
bin/e2e start --scenario example, then log in as[email protected]/welcome12345and check the example project/workflow.Additional notes for the reviewer
bin/e2e.d/scenarios/README.md.never removed, existing credential bodies aren't updated on re-run.
AI Usage
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)