Compare commits

...
Author SHA1 Message Date
zhangwenjian 2c50317a98 fix🐛: stop the duplicate check refusing what the index would accept
The check grouped by (app_code, path, action) and refused whatever
appeared more than once. GROUP BY treats two NULLs as the same value; a
unique index treats them as different ones and allows both. So a
database holding rows with a null path or action was refused for
duplicates the index it was blocking would have accepted - and the
migration stopped, on a database with nothing wrong with it.

Both columns are nullable: neither carries a not-null tag, so gorm built
them that way. Measured on MySQL 8.0, PostgreSQL 15 and SQLite: two rows
with both columns null are one group to GROUP BY, and the unique index
builds over them without complaint. Standard SQL, not a dialect quirk.

They are now excluded from the check rather than grouped. Each column
needs its own exclusion and has its own test: one null column is enough
to make the index accept the pair, so removing either condition alone
lets that half through - which is what the two subtests are for, and
each fails only for its own half.

The message could not name the rows either. MySQL's CONCAT returns NULL
when any argument is, and scanning that into a string fails with
"converting NULL to string is unsupported" - so the check reported a
driver error instead of the duplicates it exists to report. SQLite and
PostgreSQL treat a null argument as empty and say nothing, which is why
this never surfaced in the tests: they run on SQLite, and this
repository has no MySQL in CI. The comment says so, so that a
postgres-only test file is not mistaken for cover.

No COALESCE was added to paper over that. It would have had nothing left
to guard once the nulls are excluded, and it would make a future
regression quieter: someone dropping the exclusions would get a report
naming rows that are not duplicates, which reads as a real answer,
rather than a scan error that reads as a broken query.

Leaving path and action nullable is deliberate. Tightening them is a
migration of its own - existing null rows have to be given values, and
what those values should be belongs to whoever owns the data, not to a
migration whose job is adding an index.
2026-09-08 20:17:50 +08:00
zhangwenjian 28350a15bb fix🐛: repair the row a retry reuses instead of walking past it
The idempotency check added in the previous commit stopped a retry
inserting a second copy, and introduced a quieter failure in its place:
a retry that found an existing sys_menu row skipped everything after the
insert. Those are the sys_menu_api_rule bindings and the materialized
path, and neither is written by the statement that writes the menu -
paths is a separate UPDATE, and on MySQL an earlier DDL has already
committed the transaction that was supposed to hold them together.

So an install interrupted between those steps left a menu that exists,
sits outside the tree with an empty path, and is bound to no API. What
core's contract.md says about such a menu is that it is invisible to
every role and its apis are authorized for no one - while the installer
reports success.

Reusing now repairs. paths is compared before it is written, so a row
that is already right is not touched. Bindings are inserted with WHERE
NOT EXISTS rather than deleted and rebuilt: an administrator can bind an
api to a menu from the menu screen, and delete-then-rebuild would take
that with it on the next retry - the same accident as sys_role.go's
Association.Delete, pointing the other way.

Confirmed as a defect before it was fixed, by building the half-written
state and watching the assertions fail:

    dir.Paths = "", want "/0/1"
    binding count for list = 0, want 1

Four paths through the repair, each with a degradation that reds its own
test and leaves the others green: missing bindings only, missing paths
only, both, and neither. The fourth asserts no UPDATE is issued for a
row already correct.

A fifth covers what the repair must not do. Rebuilding bindings instead
of inserting them leaves every other test green while silently deleting
a binding this code did not create; that one now fails with "a retry
silently deleted a binding it does not own".

Bindings an older version of a manifest created and a newer one no
longer lists are left alone. Removing them is a delete, and a delete
needs the same certainty about ownership that uninstall does - this
function cannot tell a stale binding from one somebody added by hand.
2026-09-08 19:51:22 +08:00
zhangwenjian c6d3ea5f81 fix🐛: stop a retried seed from inserting a second copy
seedApis and seedMenuTree were bare tx.Create calls. A migration that
failed partway and was run again re-inserted everything it had already
written - which is not hypothetical: the demo site collected eighteen
duplicate sys_menu rows this way, and three duplicate menus were visible
in its sidebar.

Both now look for a live row already holding the natural key and reuse
it. Only live rows count: a row an earlier soft-delete retired does not
stand in the way of a fresh insert under the same key, which is also
what the unique indexes allow.

The app_code half of each key has a test of its own. Without it the
lookups still passed every existing test while quietly letting one
application adopt another's rows - and an uninstall would then delete
rows the other application believed were its own, on both sides without
an error. Removing app_code from either lookup now fails with
"has 1 row(s) ... want 2 - one per app".
2026-09-08 19:26:42 +08:00
zhangwenjian 7fadb4b585 feat✨: give the seeded rows a natural key to be found by
Seeding needs something to look for before it inserts, or a retry writes
a second copy of everything it already wrote. sys_api already had one in
(app_code, path, action). sys_menu had nothing usable: menu_name is
pascalCase(appCode) + pascalCase(code), which is not injective -
"list-all", "listAll" and "list_all" all become "ListAll" - so the
original code cannot be recovered from it. Hence a new column.

seed_code is nullable, against this repository's habit of NOT NULL
DEFAULT '' for a new column, and deliberately. Every row that predates
it has no meaningful value, and under a unique index an empty string
collides with every other empty string while NULL collides with nothing.
The convention exists because deleted_at's nullability broke a unique
index; here nullability is what makes one possible.

The unique index on sys_api cannot simply be created: a live database is
known to hold historical duplicates - the demo site had eighteen. The
migration looks first and refuses while naming the offending rows,
rather than letting CREATE UNIQUE INDEX fail with a constraint error
that names none. Same shape as 1786700003000's refuseOnDuplicates.

Run against SQLite, MySQL 8.0 and PostgreSQL 15, including the CONCAT
duplicate check, which had only ever been executed by SQLite's driver.
2026-09-08 19:26:42 +08:00
zhangwenjian 691df82016 feat✨: add the sys_app registry and the casbin grant ledger
sys_app is one row per installed application. It is physically deleted
on uninstall rather than following the millisecond soft-delete marker
the other sys_ tables use: an installed-app registry has no "deleted by
accident, needs recovering" case, and a physical delete is what lets the
same code be installed again afterwards.

status is installing/installed/failed rather than a boolean, because an
install spanning several migration files is not atomic on MySQL - DDL
commits implicitly, so a run can stop in the middle. failed_version and
last_error are diagnostic snapshots for a person to read; nothing may
decide anything from them, and the field comments say so. Where to
resume is answered by sys_migration, which cannot drift from what was
actually applied.

sys_app_casbin_grant records which casbin_rule rows an install created,
keyed by casbin_rule's own natural key. That table is not extended
instead: gorm-adapter's SavePolicy truncates and reloads it from an
in-memory model, which would drop any column added here without a word.

Built against SQLite, MySQL 8.0 and PostgreSQL 15.
2026-09-08 19:26:40 +08:00
zhangwenjian ea348fa9d1 build📦: pin go-admin-core v2.8.0
v2.8.0 adds sdk/contract/app - an application's manifest, and the one
comparator for its version - which the installer in this batch is built
on. Nothing here uses it yet; this is the dependency arriving.

Checked that the release is consumable rather than only tagged: a
program built against the published module registers a manifest, reads
it back, and gets -1 from Compare("1.9.0", "1.10.0"), which is the
multi-digit case a string comparison would order backwards.

25 packages pass and checksilent is clean on the new version.
2026-09-08 18:35:13 +08:00
wenjianzhang 925c6772a6 Merge pull request #921 from go-admin-team/fix/920-schema-readiness
Fail readiness while the database is behind the migrations
2026-09-08 17:18:34 +08:00
wenjianzhang 0c60e44aee Merge pull request #922 from go-admin-team/fix/919-drop-index-postgres
Drop the index with SQL this dialect can parse
2026-09-08 17:18:08 +08:00
zhangwenjian a716086295 test✅: fail these tests when the database does not answer
Every Create and Raw dropped its error. Most of them would have failed
an assertion further down anyway, with a message describing the wrong
problem - but one of them would not.

The last count in the index test reads how many indexes survived and
expects zero. An unchecked query that fails leaves the variable at zero,
and zero is what success looks like: a test that cannot reach the
database reports that the indexes were dropped.

Demonstrated rather than assumed, by breaking that one query both ways:

    with the check:    FAIL  counting the indexes after: relation
                             "pg_indexes_nope" does not exist
    without it:        PASS

Raised by Copilot on #922, as a consistency point with the SQLite tests
in this package. It is that as well, but the reason it is worth doing is
the row above.
2026-09-08 17:04:31 +08:00
zhangwenjian f8a5066a40 test✅: run the migration against PostgreSQL in CI
The rest of this package's tests run on SQLite, where dropping an index
through the migrator works. That is why a migration which failed on
every PostgreSQL database it was pointed at had a green suite: the
defect cannot occur on the backend being tested.

A test alone would not have helped either - without a service it skips,
and a test that never runs is the same as no test. So the workflow gains
a postgres service and the DSN, and the helper refuses to skip when CI
is set: a workflow that drops the service or renames the variable fails
rather than going quietly green, which is the shape of the original
defect.

Counter-proved by putting Migrator().DropIndex back, which reproduces
the statement verbatim:

    DROP INDEX CURRENT_SCHEMA()."idx_sd_pg_user_deleted_at"
    ERROR: syntax error at or near "CURRENT_SCHEMA" (SQLSTATE 42601)

The conversion test also checks the timestamp survives as a marker,
since a conversion that dropped it would bring deleted rows back live
while still passing a column-type assertion.
2026-09-08 16:57:17 +08:00
zhangwenjian f978967ef1 fix🐛: drop the index with SQL this dialect can parse
The soft-delete conversion dropped the indexes on deleted_at through
gorm's Migrator().DropIndex. Its PostgreSQL driver resolves a schema for
the statement and falls back to an expression when it cannot:

    currentSchema, _ := m.CurrentSchema(stmt, stmt.Table)
    m.DB.Exec("DROP INDEX ?.?", currentSchema, clause.Column{Name: name})

DROP INDEX takes an identifier in that position, so what reached the
server was

    DROP INDEX CURRENT_SCHEMA()."idx_sys_api_deleted_at"
    ERROR: syntax error at or near "CURRENT_SCHEMA" (SQLSTATE 42601)

The schema is unresolvable for every call this migration makes, because
it passes a table name as a string rather than a model. So it failed on
every PostgreSQL database rather than intermittently, and stopped the
whole conversion at the first table.

What that looked like from outside is go-admin#919: an upgrade that
could not complete, and a login rejecting a correct password, because
deleted_at was still a timestamptz while the current query compares it
to 0. Neither symptom names a migration.

Written per dialect, for the same reason addBigIntColumn and
renameColumn already are. MySQL and SQL Server name the table and have
no IF EXISTS for it; PostgreSQL and SQLite name the index alone.

Verified by running the shipped migration against PostgreSQL 15 and
MySQL 8.0 in containers, and SQLite through this package's tests. The
SQL Server form is from its documentation and has not been run - there
is no SQL Server here to run it against, and saying so is better than
implying four dialects were checked.
2026-09-08 16:57:15 +08:00
zhangwenjian d6e2c02fda fix🐛: fail readiness while the database is behind the migrations
The process started, both probes passed, and the first sign that the
schema did not match was a login failing with a driver-level encoding
error - go-admin#919, where an operator upgraded the binary and
restarted the API without running migrate. Nothing between those two
events had an opinion about the schema.

Readiness is where this belongs. Liveness asks "restart me", and a
process whose database is on the wrong schema comes back to the same
schema. Readiness asks "send me requests", and the answer is no. A
rolling update then stalls at the deploy - new instances never become
ready, the old ones keep serving - rather than at somebody's login, and
running migrate clears it without a restart because the check is
evaluated per request.

Any tenant database being behind fails the check, not only the one being
served: migrations are applied to every database in one run, so one
behind means that run did not finish, and serving the rest would let a
half-applied deploy look like a partial success.

A missing sys_migration table is nothing applied rather than an error.
That is a first deploy, where every migration is pending and the
operator can act on being told so.

The one test that matters is the one that cannot be written normally.
The registry is filled by init() in packages cmd/api does not import, so
a test that imported them to look at it would pass whatever the real
binary links - and a binary that links none of them gives a check that
reports every database current, forever, with every other test here
still green. TestTheServingBinaryLinksTheMigrationRegistry asks the
build instead, with a negative control so that a query matching
everything fails rather than passes.

Closes #920.
2026-09-08 15:42:14 +08:00
zhangwenjian e98b65cf90 feat✨: let the host add a readiness check this package cannot make
Whether the schema matches what the binary expects is answered by the
migration registry, which lives under cmd/. common/ has never imported
cmd/, and starting with this would put the shared layer behind the
command layer for one check.

Register instead, from where both are already in scope. A duplicate name
panics rather than appending: two checks under one name make the failing
one impossible to identify from the response body, and registering the
same one twice is a wiring mistake better heard at start-up than never.

The registered check is run through the same guard as the built-in ones,
so one that panics fails its check rather than taking down the probe
that asked.
2026-09-08 15:42:14 +08:00
zhangwenjian bc5411c30c feat✨: report what the migration registry holds without a database
Status answers what is registered, what is applied, and what is applied
while nothing registers it - and needs a database to do it. A readiness
check needs only the first half, and it already holds the databases it
is asking about.

Without this it would have to call SetDb to reuse Status, writing this
package's shared state from a request path, for a question that does not
depend on any database at all.
2026-09-08 15:42:11 +08:00
wenjianzhang 27f23121f0 Merge pull request #918 from go-admin-team/fix/911-queue-shutdown
Drain the queue on the way out instead of abandoning it
2026-09-07 17:39:15 +08:00
zhangwenjian 047b23872c fix🐛: let a drain that finishes on the deadline count as finished
The wait was one select over done and ctx.Done(). Both can be ready when
it runs, select picks at random among ready cases, and so a queue that
drained in the same instant the budget expired was reported as an
overrun about half the times it landed there - often enough to be read
as noise, and pointing at the wrong thing when it was not. core's own
RunShutdown re-checks for this reason; this did not.

The tie-break is now a function taking channels rather than a queue,
which is what lets a test hand it a closed done and an expired ctx
together. That state is the whole of the bug and cannot be arrived at
reliably from the outside; over 1000 iterations the single-select
version fails, and the second look does not.

The test for giving up on the deadline read the call counter straight
after shutdownQueue returned, while Shutdown runs on a goroutine nobody
joins. It passed because the goroutine is scheduled promptly, not
because anything ordered the two. The fake now signals that Shutdown has
been entered and the test waits for it.

Both raised by Copilot on #918.

The first attempt at the tie-break test was wrong and is not what
landed: it asserted that an immediately-returning Shutdown always counts
as drained under an already-expired context, which is not true and
should not be - if the goroutine has not run, nothing has drained. That
test failed, correctly. What is being claimed is narrower: when both are
ready, done wins.
2026-09-07 17:21:03 +08:00
zhangwenjian 84bd87dcc9 fix🐛: drain the queue on the way out instead of abandoning it
Nothing stopped the queue when the process exited. core v2.7.0 made the
drain work - Memory.Shutdown closes the queue and waits for every
consumer to finish what it holds, and the legacy adapter cancels its
context and closes the underlying queue - but no call site ever reached
it. The only Shutdown() in this repository applies to the previous
adapter during a reload, so the installed one was simply left. The login
log, the operation log and the API sync all publish through it, so a
rolling restart dropped whatever had not been consumed, on the path
where the process exits 0 and reports "Server exiting".

Setup now registers a BeforeExit callback that shuts down the adapter
this package installed.

Three things it has to get right, each with a test.

It reads `installed` when it runs, not when it registers. A reload
replaces the adapter, and the one from start-up is a queue nobody has
published to since.

It never goes through sdk.Runtime.GetQueueAdapter. That accessor never
returns nil - with no queue section configured it wraps the runtime's
own fallback - so it would look like it worked while closing a queue
this package neither built nor started. That is the same trap setupQueue
already had to drop an `if q != nil` for.

It registers once. Setup is re-run on every configuration change, and a
callback per reload would leave the shutdown phase holding a row of
identical entries, each eligible to be named as the one that overran the
budget.

That last one needed a seam. shutdownQueue takes the adapter on its
first run, so the second and third callbacks find nothing and return -
three registrations produce exactly the same observable result as one,
and a test going through the effect passes either way. It did: the
counter-proof for "register on every reload" came back green until the
registration was counted at the seam instead.

The wait is bounded here rather than left to the phase. Shutdown takes
no context, so a consumer that never finishes would hold the process
until SIGKILL; the callback gives up and says what is being lost, which
the phase's generic overrun message cannot.

Ordering falls out of the phase rather than being arranged: callbacks
run in reverse registration order, this one registers during setup and
the job scheduler's registers on AfterListen, so the schedulers stop
before the queue drains. Verified against core v2.7.0 rather than read
off the source.

Closes #911.
2026-09-07 17:10:13 +08:00
wenjianzhang 0e7a13aeba Merge pull request #917 from go-admin-team/fix/914-gen-write-guard
Register the generator's writing endpoints only in a development mode
2026-09-07 15:32:13 +08:00
zhangwenjian 85d50da494 fix🐛: tell the start-up warning's reader to restart, not only to reconfigure
The warning said to set application.mode and stopped there. Following
that on a running process does not close anything: buildRouter has one
call site, in run(), and route registration is on no phase and no reload
callback, so a configuration reload moves the mode and leaves the routes
exactly where they were.

The reader is then worse off than before they acted. The mode now says
prod, GenWriteRoutesEnabled agrees, and the endpoints are still served -
so the one thing they could check to confirm the fix reports success
while the exposure is untouched, until something restarts the process.

A test pins the gap rather than the prose: build under dev, move the
mode to prod, and the routes are still in the engine. It fails if
registration ever becomes dynamic, which is the change that would make
the new sentence wrong.

That test degrades differently from the others - making it fail means
rewriting registration, not weakening it - so what was checked instead
is that it cannot go vacuous. Both of its premises are guarded: with the
gate always refusing it reports building under dev without the writing
routes, and with the gate always allowing it reports the predicate still
allowing prod. Neither failure can be mistaken for the assertion passing.

Raised by Copilot on #917.
2026-09-07 15:20:36 +08:00
zhangwenjian 1d9def4314 test✅: restore the mode before the route helper returns
registeredRoutes set config.ApplicationConfig.Mode and gave it back with
t.Cleanup, which runs at the end of the test rather than at the end of
the helper. Everything the caller did after the call therefore ran under
the mode the helper had been asked about, not one the caller chose.

Nothing was wrong yet: the one caller that reads the mode afterwards
sets it itself, and the two cleanups happen to unwind in an order that
leaves the right value. Both of those are accidents, and neither is
visible at the call site.

A defer inside the helper makes the borrowing end where it starts. The
doc comment said the mode was put back before returning while the code
did not, so that is now true rather than aspirational.

The counter-proof is the reason this has a test of its own: with
t.Cleanup back in place TestRegisteredRoutesRestoresTheModeBeforeReturning
fails and nothing else does, which is what a leak this quiet looks like
when something is actually watching for it.

Raised by Copilot on #917.
2026-09-07 15:14:39 +08:00
zhangwenjian ed9bbd01e2 feat✨: warn at start-up when the generator can write to this host
The gate in the previous commit is decided by application.mode, and the
shipped configuration says dev. So the deployment most likely to be
serving the writing endpoints is the one that changed nothing, and that
is also the one least likely to go looking for them. A gate whose
default is open needs to say so.

Nothing is said in demo mode. The routes are registered there, but
DemoEvn refuses all three by name, so a warning would describe an
exposure that is not present.

The decision is split from the logging so it can be tested. Three
counter-proofs: warning in demo as well fails mode=demo; a warning that
never fires fails mode=dev, which is what shows the line can be reached
at all; and one that always fires fails every mode but dev.
2026-09-07 15:07:07 +08:00
zhangwenjian 523d6a3649 fix🐛: register the generator's writing endpoints only in a development mode
Three of the code generator's endpoints do not read. /gen/toproject
writes seven Go and Vue source files onto the host, one of them under
the path gen.frontpath names; /gen/apitofile writes a migration;
/gen/todb inserts menus and APIs. All three are GET, and all three are
listed in CasbinExclude - which AuthCheckRole skips - so Enforce never
runs for them. Any account that can log in could call them, on every
deployment.

They are now registered only where application.mode is dev or demo. dev
is the shipped default and is where the generator is meant to be used.
demo keeps them because demo mode already has a better answer than a
404: DemoEvn refuses these three by name and explains itself, which is
what the demo is for. test and prod get nothing, and so does a process
whose mode was never set.

This does not make the endpoints safe where they exist; it stops them
existing where nobody should be calling them. A host left on the shipped
dev is still open, which is why the next commit says so at start-up.

CasbinExclude is left alone on purpose. Taking the three off that list
would make them require a permission no existing deployment has granted,
so every non-admin user would start getting 403 from a tool that worked
yesterday. That is a migration, not a guard, and it belongs with a
release that can carry one.

Four counter-proofs, each red on the test that names the behaviour and
green everywhere else: a gate that always allows fails test/prod/unset
only; a gate that always refuses fails dev/demo and takes
TestEveryRouteDemoModeRefusesStillExists with it; moving a read-only
route inside the gate fails the reading test; and spelling the condition
at the registration site instead of calling the predicate fails the
agreement test, which is what keeps that test from being a tautology.
2026-09-07 15:05:24 +08:00
zhangwenjian 6326962862 style🎨: gofmt gen_router.go
Two pre-existing deviations: a space before the comma in
sysNoCheckRoleRouter's parameter list, and no newline at end of file.
Separated from the change that follows so its diff is only the change.
2026-09-07 15:02:33 +08:00
wenjianzhang cd7c8375c0 Merge pull request #916 from go-admin-team/docs/replicas-constraint
Name the job scheduler as the other reason for one replica, and stop the manifests redeploying the demo
2026-09-07 14:52:53 +08:00
zhangwenjian 63bcc912ef ci👷: stop the k8s manifests from redeploying the demo
The comment at the top of this workflow says documentation-only changes
skip it, because a push to master pushes an image, runs the migrations
and restarts the demo container. The ignore list did not cover
scripts/k8s, so editing a manifest that the deploy never reads bought
the site an outage.

The pattern is scripts/k8s/** rather than scripts/** because
scripts/Dockerfile is a build input - go.yml builds the release image
from it on a tag.

This workflow file stays outside the list on purpose. paths-ignore skips
only when every changed path matches, so a change that edits the deploy
still runs it, which is the point.
2026-09-07 14:14:39 +08:00
zhangwenjian adcdd2edcd docs📝: name the job scheduler as the other reason for one replica
The comment on replicas gave one obstacle to raising it, the shared log
volume, which reads as the only one. Someone who moves the log path off
that volume would conclude the way is clear.

The scheduler in app/jobs is the second, and it is the one that does not
announce itself. Its handle on a job lives in sys_job.entry_id, one
column shared by every process, and startup zeroes the whole column
before writing its own ids. A second pod therefore erases the first
pod's, and both pods run the full enabled list. Stopping a job from the
UI then removes an entry from whichever process is asked, by an id that
belongs to another one, and answers 200.

See #915.
2026-09-07 13:32:43 +08:00
wenjianzhang 29406f839e Merge pull request #913 from go-admin-team/fix/demo-mode-guard
fix🐛: demo 模式放行了注册成 GET 的写接口
2026-09-07 08:11:14 +08:00
zhangwenjian a43133ab7b fix🐛: stop demo mode serving the writes that are registered as GET
DemoEvn decided by HTTP method: GET and OPTIONS through, everything else
refused. Three of the code generator's routes are registered as GET and write
anyway - two emit Go source files onto the server's filesystem, and the third
inserts menus, APIs and casbin rules into the database. They sit in a group
whose own name says it does no role check, and a demo deployment lets anybody
log in. So on the demo host any visitor could write to the machine and to the
database, and the one that writes menus had in fact been used: three generated
SysCasbinRule entries is how this was noticed.

The guard now also looks at the matched route. The method cannot answer the
question - whether a request changes anything is not something the verb reports
truthfully here - so the three are named, as gin route patterns, which is what
Context.FullPath returns and how CasbinExclude already spells them.

Changing them to POST would be the better shape and is not this change. sys_api
records an endpoint by method and path and the casbin policy follows it, so
flipping the verb needs a migration and a policy resync; until both land, every
existing deployment would start answering 403 to a role that could use the
generator the day before.

The read-only half stays reachable: preview, the table tree, and the two
database listings. A demo host that cannot demonstrate the generator is as
broken as one that lets visitors write to it - refusing too much is the same
defect facing the other way, and there is a test for that direction too.

Half of the general hole is closed and the other half is written down. The
closed half is a test beside the route registrations: it builds the generator's
routes, enumerates them, and fails if any entry in the guard has stopped being
a real route, so renaming one turns the list red instead of quietly making it
match nothing. It lives there because common/ may not import app/ - which is
also why the guard cannot check its own list from where it is. The open half is
that no static check can tell a handler that writes from one that reads, so the
next GET that writes has to be added by hand. The comment says that rather than
leaving the impression the class is covered.

application.demomsg was configuration nothing read. The message was hard-coded
in the middleware, and the demo host's configured string happened to be
identical, so the setting looked like it worked and never had. It is read now,
with the old string kept verbatim as the fallback, so a deployment that never
set it is answered exactly as before.

This covers demo mode only. On a deployment that is not a demo those three
routes remain in CasbinExclude and stay reachable by any authenticated user
whatever their role; that is a separate decision and is not touched here.
2026-09-07 08:03:40 +08:00
wenjianzhang 7002cd4065 Merge pull request #912 from go-admin-team/feat/007-drain-window
feat✨: 关闭时先排空再停止接收——让 /ready 的 503 真的能被采到
2026-09-07 08:01:59 +08:00
zhangwenjian 8faa8d2aed feat✨: check the shutdown budget against every stop deadline
The budget is one number in config/settings.yml. The deadlines that have to
cover it are in four other files, none of which anybody edits while thinking
about shutdown - so raising the budget passes every test, deploys, and has the
cleanup callbacks killed on the next release.

Two checks share one arithmetic and one five-second margin.

shutdown-budget-overruns-grace compares preStop + drain + server + cleanup
against terminationGracePeriodSeconds in the shipped manifest. Those two files
are not merely adjacent examples: scripts/k8s/prerun.sh builds the
settings-admin ConfigMap out of config/settings.yml and the Deployment mounts
it, so the manifest deploys that file.

docker-stop-cuts-shutdown-short covers the three ways this container is
stopped: `docker stop` in the release workflow, the same in the Makefile, and
stop_grace_period on a compose service that runs this repository's own image. A
service running a database is not this process and is left alone. The duration
is parsed rather than scanned for digits - compose accepts 1m30s, and reading
the first number out of it would call ninety seconds one.

All three spellings of the deadline are read: --timeout, the deprecated --time,
and the short -t. A deadline the check cannot read is reported as no deadline at
all, so recognising only one of them would call a correct command broken and
send whoever fixed it towards the spelling docker is retiring. The message
quotes the flag back in the spelling it was written in, for the same reason:
suggesting a flag the line does not use is how a tool teaches people to
disbelieve it.

What neither covers is `docker rm -f`, which has no deadline to compare
against: it is SIGKILL by definition. That gap is deliberate, and it is why the
previous commit changed the one place that used it on a container that might
still be running.

Both report at two levels. A budget that already overruns is an ERROR; one that
fits with nothing to spare is a WARN, because it works today and failing the
build on a working configuration is how a project teaches people to ignore its
warnings. The two are exclusive: an overrun satisfies the headroom condition as
well, and an ERROR that always drags a duplicate WARN behind it teaches the same
lesson.

preStop is in the sum although the shipped manifest has no hook. That is the
point - a hook added later is spent before the process is told anything, and a
self-check that could not see it would understate the real budget by however
long somebody set it to, which is worse than not checking. A hook whose duration
cannot be read is reported rather than counted as zero.

The fallbacks for fields the settings file leaves out are read from the
constants in the scanned tree, not copied here; if they are renamed the run
stops instead of going quiet with the wrong numbers.

The wording differs by audience on purpose. At run time this is somebody else's
deployment under constraints the process cannot see, so the log states a
minimum. These checks read files this repository owns, where there is standing
to ask for headroom, so they name a target.

The table in AGENTS.md is relisted while it is being touched: the two new
checks, plus datascope-route-unguarded, which has been missing since it was
added. The hard-coded count is gone - it said seven and there were ten, which is
what a written-down count does. AGENTS.md and docs/contract.md both sent readers
to `go run ./tools/checksilent -h` for the list of checks; that prints
command-line flags and has never printed a check, so both now point at
runChecks.

The yaml parser moves from an indirect requirement to a direct one - it was
already in the module graph - and tidy drops four go.sum lines left over from
two older releases of core.
2026-09-06 22:07:08 +08:00
zhangwenjian 705427178d fix🐛: give every stop path enough time for the shutdown budget
Stopping this process takes drain + server + cleanup seconds: eight out of the
box, and more for anyone who configures a drain window. Three places decide
whether it gets that long, and none of them was written with it in mind.

The release workflow stopped the previous container with the default deadline,
which docker sets at ten seconds. The compose file - which the Makefile calls
the first way to run this - set no stop_grace_period, so it took the same ten.
Under either, a drain window over two seconds would have been cut off by
SIGKILL part-way through the cleanup callbacks: this project's own deployments
could not have run the capability it ships.

The third was worse. `make run` removed the previous container with `docker rm
-f`, and the force flag kills a running container outright - "uses SIGKILL", in
docker's own words - with no grace at all. Restarting locally cut every
shutdown short, so the drain window would never once have been reached on a
developer's machine. It now stops with a deadline and then removes, which
leaves what gets removed unchanged: on a container that has already stopped,
stop is a no-op.

So: --timeout 30 in the workflow, stop_grace_period: 30s on the compose
service, and stop --timeout 30 before the removal in the Makefile. --timeout
rather than --time, which docker still honours but has deprecated - it prints a
warning on every use, and a deploy log that always carries a warning is one
nobody reads.

The three remaining `docker rm -f` calls in the workflow are left alone. Two
remove containers that have already been stopped and one is the rollback path,
and nothing static can tell those apart from a container that is still running -
which is also why the check added next does not look at `rm -f` at all: a forced
removal has no deadline to compare against. What keeps that path honest is the
line above it, not a check.

Thirty will drift the first time somebody raises a budget. The next commit is
what notices, which is also why these comments name a check that does not exist
yet.
2026-09-06 22:06:35 +08:00
zhangwenjian 8f10d202e6 feat✨: give the shipped manifest probes and a stop grace period
The manifest in this repository had no probes at all. A pod was sent traffic as
soon as its container was running, whether or not the database it needs was
reachable, and it was stopped with whatever grace period Kubernetes defaults to
rather than one chosen against what this process actually spends shutting down.

It now mounts both probes, at the endpoint that answers each question:
readiness at /ready, which fails while a dependency is unreachable, and
liveness at /health, which is a bare 200 because restarting a process whose
database is down turns one outage into a crash loop. Both skip the rate
limiter, which is why that had to land first.

timeoutSeconds is 3, not the default 1. The handler allows its checks two
seconds, so at the default a database answering in 1.2s would be recorded as a
failed check while the handler was returning 200 - the probe would be failing on
the orchestrator's stopwatch, not on its own. The comment beside that constant
said the constraint was the polling period; the constraint is the per-check
timeout, and it is now written down correctly.

terminationGracePeriodSeconds is 30, against a shipped budget of 0 + 5 + 3.
Raising drain means raising this too, in the same commit; the check that
notices when somebody does not arrives two commits from here.

replicas stays at 1, and the comment says why that makes the drain window worth
nothing: there is nowhere to send the traffic this pod stops taking. Raising it
needs one more change than the number - the volume is shared by every replica
and the log path lives on it, so a second pod would append to the same rotating
file. The reason not to raise it is not the one the review assumed: the claim
was that the PVC is ReadWriteOnce, and it is not, it is ReadWriteMany on nfs-csi.

There is no preStop hook. How long one should sleep depends on how fast the
thing in front removes this instance, which the repository cannot know, and a
manifest carrying both a preStop sleep and a drain window is the double-counting
trap - the budget would be spent twice and the start-up line would report half
of it.
2026-09-06 21:49:17 +08:00
zhangwenjian 5648bd1dcf feat✨: state the shutdown budget at start-up
The three budgets are spent one after the other, so what has to fit inside the
orchestrator's grace period is their sum - and nothing said what that sum was.
Working it out meant reading a configuration file, remembering which fields
were absent, and knowing what each one falls back to.

Start-up now prints it: the three values and the total, taken from the resolved
budget rather than from the file. A field left out still costs its default, so
adding up what was written down understates the total by exactly the fields
nobody wrote - which is the arithmetic somebody doing this by hand gets wrong.

Whether the total fits is a separate question, and the framework cannot answer
it alone: `docker stop` allows ten seconds and Kubernetes thirty, three times
apart. A fixed threshold would have warned about the manifest this repository
is about to ship. So extend.shutdown.grace is optional, nothing reads it during
a shutdown, and when it is absent the line says so and prints both reference
values instead of judging.

When it is set and the budget does not fit, the warning names the shortfall:
how many more seconds are needed. A minimum, not a target - this is somebody
else's deployment under constraints this process cannot see, and asking them to
leave headroom on top is not this line's business. Equal does not fit either;
the grace period is when SIGKILL is sent, so a budget that ends exactly then
leaves the last callback no time to return.
2026-09-06 21:49:17 +08:00
zhangwenjian a442eadb96 feat✨: keep serving for a configurable window before the listener closes
/ready has failed from the moment shutdown begins since the readiness probe was
added, and the order it does that in is right: reversed, the state would be
reported after the connections were already cut. But order alone does not
produce a window. Nothing waited between the flip and Shutdown, so the two were
microseconds apart, and a poller on a multi-second interval never saw the 503 -
it saw a refused connection, which is the thing the probe was supposed to
avoid. Polling a container through a SIGTERM on the demo host recorded exactly
that: 200, then connection refused, and no 503 in between.

extend.shutdown.drain is that wait. The process keeps serving normally for it -
answering requests, not refusing them, because refusing them would move the
outage earlier rather than avoid it - and only then closes the listener.

It is zero by default, so nothing changes for a deployment that does not ask
for it. That is not timidity: the budgets are spent one after another, and a
non-zero default would push every existing shutdown closer to the orchestrator's
grace period, where being cut off part-way through the cleanup callbacks is
worse than never draining at all.

Keep-alive is switched off with the flip. The server keeps connections alive
until Shutdown sets shuttingDown() itself, so without this the pooled
connections a balancer holds would sit untouched for the whole window and be
cut at the end of it anyway - the cost of the window without its benefit. This
is the switch Shutdown flips, moved earlier by the window's length.

The signal disposition is restored after the window rather than on the first
signal. Before there was a window, the interval where a second signal killed
the process outright was only reachable while a cleanup callback hung; putting
a multi-second wait inside it would have made every ordinary shutdown
interruptible for the length of the drain. A second signal during the window is
taken by the channel and ends the window early instead - somebody sending
another kill wants this over with sooner - and the escape hatch comes back the
moment the window does.

What the window is worth depends on who removes this instance. A balancer that
polls /ready acts on the 503 and needs the window to cover its check interval
times its failure threshold; a Kubernetes Service withdraws the endpoint when
the Pod is deleted, concurrently with SIGTERM and regardless of what the probe
returns, and there the window covers the delay in that removal reaching every
node. The three comments that used to say a balancer "has a chance to" take the
instance out said it without either qualification, which is how a claim comes to
be repeated after a live test has refuted it.

The subprocess test polls the real probes on a connection it opens after the
signal - a reused one can be served after the listener is closed, which would
let this pass against a shutdown that had already broken it - and asserts on the
draining answer in the body, not on the status code. With no database the status
is 503 from start-up, so a status-code assertion would hold even with
BeginDraining deleted. Two window lengths, because one proves only that
something takes that long.
2026-09-06 21:49:17 +08:00
zhangwenjian f3b67e9abc fix🐛: keep the rate limiter away from the health probes
The limiter is installed on the engine and the probes are routes like any
other, so above the threshold they are answered with 429 too. Point a liveness
probe at one and the failure mode writes itself: traffic crosses the threshold,
the probe collects three 429s, the kubelet restarts the container, the capacity
that was already short gets shorter, and the instances that are left are pushed
further past the threshold. The limiter working exactly as designed is what
kills the pod.

It is the argument common/health already makes about restarting a process whose
database is unreachable, applied to load: turning one outage into a crash loop
is not an improvement on the outage.

Nothing points a liveness probe at these routes yet. The manifest that will is
two commits away, and this has to land first, because that manifest without
this change would be actively harmful.

The exemption wraps the middleware rather than teaching the limiter about these
paths. common/ may not import app/ - the contract check enforces it - so the
limiter cannot name routes that are registered over there. Wrapping it in the
command package, which imports both, is what keeps the boundary.

Naming those routes needs them exported, so the group prefix and the two paths
become constants and the router function becomes RegisterMonitorRouter. That
also gives a test something real to mount: a probe asserted against a
re-implementation of itself is a test of the copy.

The check that the middleware never runs is separate from the check that the
answer is not 429, because a probe can produce a 429 on its own. What has to be
true is that the request never reached the limiter.
2026-09-06 21:49:16 +08:00
zhangwenjian 4e51f56623 feat✨: make the shutdown budgets configurable
How long a shutdown may spend waiting for in-flight requests, and how long the
cleanup callbacks get after that, were compile-time constants. The two together
have to fit inside whatever grace period the orchestrator allows before it
sends SIGKILL, and that number is not the same everywhere - `docker stop`
allows ten seconds, Kubernetes thirty by default - so the one deployment shape
these constants suited was the one they were written for.

They now come from extend.shutdown, beside rateLimit. Not from application:
that section is a fixed struct in core, and the decoder discards keys it has no
field for without an error, so a budget written there would be accepted and
never applied. That is the failure this whole change is about, and putting the
configuration where it cannot be read would have reproduced it.

Both fields are pointers, following RateLimit.InboundQPS: nil means "not
configured" and takes the default, and a number that was written down is spent
literally, zero included. Without that separation `server: 0` - do not wait for
in-flight requests at all, which is a reasonable thing to ask when the grace
period is very short - could not be expressed, and the section would need a
paragraph explaining which zeros mean what.

A negative is refused rather than clamped. Correcting a value quietly is the
same failure in a different costume, and Budget returns the error instead of
ending the process so that the rule can be tested without a subprocess.

The defaults live in config as seconds and in cmd/api as durations, both from
the same constants, and a test asserts the two agree - a deployment that
configures nothing is entitled to one answer about what it spends, not two.

The last test loads the two settings files this repository ships through the
real loader and asserts the section arrives with the documented values. Nothing
weaker can tell "the key is read" from "the key is discarded": the struct
compiles either way.
2026-09-06 21:49:16 +08:00
zhangwenjian 7e4e17bbcf test✅: run the real shutdown sequence in the signal tests
The child process built a server, restored its own signal disposition and
called shutdownServer and runShutdownHooks itself, in an order it chose. It
never called anything run() calls. So the assertions were about a copy of the
sequence: move a step in the real one, or drop it, and every test here stays
green. The acceptance criteria these back are worth exactly as much as that.

The child now calls gracefulShutdown and asserts on what comes out of it. The
budget it spends is defaultBudget with one field shortened where a test needs a
deterministic timeout, which is also how the two waits stop being wired by
hand.

The stuck-shutdown case changes shape as a result. It used to sleep inside the
child, between the steps it had copied; there is no "between" to sleep in any
more, so it registers a BeforeExit callback that never returns and gives it a
budget long enough to hang on. That is where a shutdown actually hangs, and it
now runs through the same function - which means this test also pins where the
signal disposition is restored, rather than just asserting that the child dies.

It signals repeatedly rather than once. The marker is printed immediately
before gracefulShutdown is entered, so a single signal sent on seeing it can
still arrive before the disposition is restored, land in the buffered channel
and be dropped. Which signal does the killing is not the assertion; that one of
them can is.
2026-09-06 21:49:16 +08:00
zhangwenjian 799e892a68 refactor♻️: run the shutdown sequence from one function
The steps between the stop signal and the last log line were written inline in
run(), which left nothing for a test to call. The signal tests reproduce that
sequence instead: they build their own server, restore their own disposition,
and call shutdownServer and runShutdownHooks in an order of their own. So they
assert against a copy - reorder the real sequence, or drop a step from it, and
they stay green.

The sequence now lives in gracefulShutdown, and the waits it spends are a
budget rather than two constants read at the point of use. Nothing changes
about what happens or in what order: the same disposition is restored first,
the same two announcements are made, the same waits are spent, and run() logs
the same two errors with the same messages.

Returning those errors instead of logging them inside is what lets a caller
other than run() react to them. That matters for the next commit, where the
tests stop reproducing this sequence and start running it.
2026-09-06 21:49:16 +08:00
wenjianzhang 0e2adb3165 Merge pull request #909 from go-admin-team/fix/queue-swap-order
fix🐛: 热重载期间生产者被指向已关闭的队列
2026-09-06 21:23:01 +08:00
wenjianzhang 249e044ded Merge pull request #910 from go-admin-team/docs/readiness-claim
docs📝: 摘除实例的不是 readiness——订正 #908 的三处注释
2026-09-06 20:32:51 +08:00
zhangwenjian d6309c75be docs📝: state what the draining answer is worth
Three comments said readiness failing before the server stops accepting gives
a load balancer the chance to withdraw the instance before its connections are
cut. Nothing between the two lines makes that possible: BeginDraining is
immediately followed by the shutdown, and a poller on a multi-second interval
never observes the flip.

On Kubernetes the endpoint is withdrawn when the Pod receives a
deletionTimestamp, concurrently with SIGTERM and independent of what the probe
returns, so the probe result is not the mechanism there either.

The order itself stands - reporting the state after the connections are cut is
worse - so the comments now say the order is necessary and not sufficient, and
that a window needs a configured delay that does not exist yet.
2026-09-06 19:04:42 +08:00
50 changed files with 5905 additions and 157 deletions
+24 -2
View File
@@ -1,12 +1,25 @@
name: Build
# Documentation-only changes skip this workflow entirely.
# Documentation-only changes, and changes confined to the Kubernetes
# manifests, skip this workflow entirely.
#
# A push to master here does not just build - it pushes an image, runs the
# migrations and restarts the demo container, so the site takes a short outage.
# Paying that for a README edit is waste at best; at worst a deploy fails for a
# reason unrelated to anything in the change. Code coverage is unaffected,
# because go.yml still builds every push and pull request.
#
# scripts/k8s holds deploy.yml, storage.yml and prerun.sh, and the deploy below
# reads none of them - it is an ssh into one host that runs docker, building the
# Dockerfile at the repository root. Those manifests are for people deploying to
# a cluster of their own. The pattern is scripts/k8s/** rather than scripts/**
# because scripts/Dockerfile is a build input: go.yml builds the release image
# from it on a tag.
#
# A file outside these patterns still runs the workflow even when the rest of
# the change is ignorable: paths-ignore skips only when every changed path
# matches. Editing this file is one such case, on purpose - a deploy script
# that is never exercised by the change that broke it is worse than an outage.
on:
push:
branches: [ master ]
@@ -15,6 +28,7 @@ on:
- 'docs/**'
- 'LICENSE*'
- '.github/ISSUE_TEMPLATE/**'
- 'scripts/k8s/**'
pull_request:
branches: [ master ]
paths-ignore:
@@ -22,6 +36,7 @@ on:
- 'docs/**'
- 'LICENSE*'
- '.github/ISSUE_TEMPLATE/**'
- 'scripts/k8s/**'
# One deploy at a time. Two merges seconds apart raced here: both runs did
# docker rm -f then docker run, the second removed the container the first had
@@ -115,7 +130,14 @@ jobs:
if sudo docker ps -a --format '{{.Names}}' | grep -qx "$NAME"; then
sudo docker rm -f "$PREV" >/dev/null 2>&1 || true
sudo docker rename "$NAME" "$PREV"
sudo docker stop "$PREV" >/dev/null
# --timeout, because the default is 10 seconds and the process
# spends drain + server + cleanup from extend.shutdown before it
# exits - 8 seconds out of the box, and more for anyone who
# configures a drain window. Past the deadline docker sends
# SIGKILL and the cleanup callbacks are cut off part-way through.
# checksilent's docker-stop-cuts-shutdown-short check compares
# this number against config/settings.yml.
sudo docker stop --timeout 30 "$PREV" >/dev/null
fi
sudo docker run -d -p 8000:8000 \
+19
View File
@@ -33,8 +33,27 @@ jobs:
--health-timeout 3s
--health-retries 10
postgres:
image: postgres:15-alpine
env:
POSTGRES_PASSWORD: postgres
POSTGRES_DB: goadmin_test
ports:
- 5432:5432
options: >-
--health-cmd "pg_isready -U postgres"
--health-interval 5s
--health-timeout 3s
--health-retries 10
env:
GO_ADMIN_TEST_REDIS_ADDR: 127.0.0.1:6379
# The soft-delete conversion drops an index, and gorm's PostgreSQL driver
# produced unparseable SQL for that - on SQLite, where the rest of these
# tests run, the same code works. The suite reported success for a
# migration that failed on every PostgreSQL database it was pointed at.
# See go-admin#919.
GO_ADMIN_TEST_POSTGRES_DSN: "host=127.0.0.1 port=5432 user=postgres password=postgres dbname=goadmin_test sslmode=disable"
steps:
+10 -2
View File
@@ -227,8 +227,9 @@ go run -tags sqlite3 . server -c config/settings.sqlite.yml
## 静默失败校验
`make checksilent` 检查七类**不报错、不记日志、行为悄悄变得不对**的问题,
CI 会跑,命中 ERROR 即失败:
`make checksilent` 逐条检查那些**不报错、不记日志、行为悄悄变得不对**的问题,
CI 会跑,命中 ERROR 即失败。这里不写条数——写死的数字会悄悄过时,
真正的清单是 `tools/checksilent/checks.go` 里 `runChecks` 跑的那几个:
| 检查 | 级别 | 静默后果 |
|---|---|---|
@@ -238,8 +239,15 @@ CI 会跑,命中 ERROR 即失败:
| `menu-id-collision` | ERROR | 两个模块硬编码同一菜单 ID,互相覆盖 |
| `contract-import-boundary` | ERROR | 契约包 import `app/`,应用无法独立编译 |
| `contract-shim-alias` | ERROR | 契约薄壳写成 defined type 而非别名,方法集丢失,本仓可能照常编译、第三方应用编译不过 |
| `datascope-route-unguarded` | ERROR | handler 读调用方的数据权限,而注册它的路由组没装提供权限的中间件。取不到时拿到零值、走 fail-closed 分支,查询被塞进 `1 = 0`:接口对确实存在的行返回「查不到」,且只在 `enabledp: true` 的部署上出现 |
| `shutdown-budget-overruns-grace` | ERROR / WARN | `settings.yml` 的 `extend.shutdown` 预算(含清单里的 `preStop`)放不进自带 k8s 清单的 `terminationGracePeriodSeconds`,SIGKILL 在清理回调跑到一半时到达 |
| `docker-stop-cuts-shutdown-short` | ERROR / WARN | 停止容器的两条路径——脚本/工作流里的 `docker stop`,和 `docker-compose.yml` 的 `stop_grace_period`——没写或写得不够关闭预算用。两边默认都是 10 秒,而这个数字离命令很远,调大预算的人不会想起它 |
| `menu-name-mismatch` | WARN | 菜单名与前端组件 `name` 不一致,keep-alive 缓存静默失效 |
两条关闭预算检查分两级,用的是同一条算术和同一个 5 秒边际:真的超限报 ERROR,
放得进但余量不足 5 秒报 WARN。余量不足做 WARN 不做 ERROR,是因为那是个技术上
跑得通的配置——**一条在正确配置下也会响的 ERROR,训练的是忽略它**。
最后一条要跨仓库比对,只能做正则启发式,因此是 WARN,**不影响退出码**,
且默认跳过;要跑它得指定前端目录:
+10 -1
View File
@@ -15,7 +15,16 @@ build-sqlite:
# make run
run:
# delete go-admin-api container
@if [ $(shell docker ps -aq --filter name=go-admin --filter publish=8000) ]; then docker rm -f go-admin; fi
#
# stop then rm, rather than `rm -f`. The force flag kills a running
# container with SIGKILL and no grace at all, so restarting locally cut
# short every shutdown this application does - the drain window was never
# once reached on a developer's machine. --timeout has to cover
# extend.shutdown's drain + server + cleanup; checksilent's
# docker-stop-cuts-shutdown-short check compares it against
# config/settings.yml. On a container that has already stopped, stop is a
# no-op and the removal is unchanged.
@if [ $(shell docker ps -aq --filter name=go-admin --filter publish=8000) ]; then docker stop --timeout 30 go-admin && docker rm go-admin; fi
# 启动方法一 run go-admin-api container docker-compose 启动方式
# 进入到项目根目录 执行 make run 命令
+89
View File
@@ -0,0 +1,89 @@
package models
import (
"time"
"go-admin/common/models"
)
// SysApp is the sys_app row model: one row per installed application (PRD
// 008 F2). It deliberately does not embed models.ModelTime - see the design
// doc (docs-prd/008-应用清单与安装器/数据库变更.md) §1.1 for why an
// installed-app registry does not need the millisecond soft-delete marker
// every other sys_* table follows. Uninstalling an app deletes its row
// outright; a later reinstall creates a fresh one.
type SysApp struct {
models.Model // Id int, primary key, autoincrement
// AppCode is the app.Manifest.Code / migration.ForApp / seed.SeedMenus
// identity, already lower-cased by migration.NormalizeAppCode before
// anything reaches this table. Unique: row existence alone answers G2
// ("is app X installed").
AppCode string `json:"appCode" gorm:"type:varchar(64);not null;uniqueIndex:uk_sys_app_app_code;comment:app code"`
Name string `json:"name" gorm:"size:128;not null;comment:display name, from Manifest.Name"`
// Version is the version this row currently reflects - attempted or
// confirmed, disambiguated by Status. It does not drive which
// migrations run next; sys_migration's per-version rows do that (see
// design doc §1.5's resume flow). This field is descriptive, refreshed
// from the manifest on every install/upgrade/resume attempt.
Version string `json:"version" gorm:"size:32;not null;comment:version this row currently reflects, see Status"`
Description string `json:"description" gorm:"size:255;not null;default:'';comment:from Manifest.Description"`
Author string `json:"author" gorm:"size:128;not null;default:'';comment:from Manifest.Author"`
// Requires is a comma-separated list of app codes this app declared as
// dependencies (Manifest.Requires). Stored as plain VARCHAR CSV, not
// JSON - see design doc §1.3 for why. F8 (P1) is what validates and
// orders on this; this batch only stores what the manifest declared.
Requires string `json:"requires" gorm:"size:255;not null;default:'';comment:declared dependency app codes, comma separated"`
// Pricing/License are reserved passthrough fields (PRD 003; PRD 008
// open question 1). This batch stores whatever the manifest carries and
// does not interpret either one.
Pricing string `json:"pricing" gorm:"size:64;not null;default:'';comment:reserved, not interpreted by this batch"`
License string `json:"license" gorm:"size:64;not null;default:'';comment:reserved, not interpreted by this batch"`
// Status: 1=installing 2=installed 3=failed. Three states, not a
// single "1=installed", because a partial, stuck install has to be an
// observable row rather than "the row doesn't exist yet" - see design
// doc §1.5 for why cross-migration-file atomicity is not available on
// MySQL (implicit commit on DDL).
Status int `json:"status" gorm:"size:4;not null;default:1;comment:1=installing 2=installed 3=failed"`
// FailedVersion and LastError are DIAGNOSTIC TEXT ONLY - what a human
// looking at this row is told about the last failure, nothing more. No
// code anywhere may read either one to decide what to do next.
//
// The question "where should a resume pick up" has exactly one
// authoritative answer, and it is not these two columns: subtract
// sys_migration's applied rows for this app_code from what the app's
// own compiled-in code has registered (migration.Snapshot()/ForApp -
// the same set F7's `migrate status` already walks). That answer can
// never go stale, because it is not stored anywhere to go stale - it is
// recomputed from sys_migration every time it is asked. FailedVersion
// is a snapshot of what that computation returned at the moment of
// failure, kept only so an operator does not have to go find the
// process's logs; if it and a fresh recomputation from sys_migration
// ever disagree, sys_migration is right and this column is stale, by
// definition, and nothing should ever notice or care except a human
// reading the row.
FailedVersion string `json:"failedVersion" gorm:"size:64;not null;default:'';comment:diagnostic snapshot only, not a judgment basis; meaningful only when status=3"`
LastError string `json:"lastError" gorm:"size:255;not null;default:'';comment:diagnostic text only, not a judgment basis; meaningful only when status=3"`
// InstalledAt is when this app first reached status=installed - set
// once, never moved by a later upgrade (see design doc §1.4). Nullable,
// unlike every other column here: a row can exist before it has a
// value (a fresh install starts at status=installing). This is not the
// deleted_at problem 1786700003000_soft_delete_marker.go fixed - that
// column sat inside a unique index, where NULL <> NULL let two live
// rows coexist under the same key. InstalledAt is in no index at all,
// so nullability here opens no such hole.
InstalledAt *time.Time `json:"installedAt" gorm:"comment:first successful install time; null until status first reaches installed"`
UpdatedAt time.Time `json:"updatedAt" gorm:"comment:last updated time"`
models.ControlBy // CreateBy/UpdateBy: which operator triggered the attempt
}
func (*SysApp) TableName() string {
return "sys_app"
}
+43
View File
@@ -0,0 +1,43 @@
package models
import "time"
// SysAppCasbinGrant is a ledger of casbin_rule rows an app install created,
// keyed by the exact natural key casbin_rule itself is unique on. It exists
// because casbin_rule is not a table this project owns (see design doc
// docs-prd/008-应用清单与安装器/数据库变更.md §2.2): we cannot add an
// app_code column to it without that column being silently zeroed the first
// time anything calls the gorm-adapter's SavePolicy/SavePolicyCtx. Recording
// the natural key here, instead of a foreign key into casbin_rule, is also
// what survives SysRole.Update's RemoveFilteredPolicy+re-add cycle for a
// role's policies (app/admin/service/sys_role.go): that cycle replaces the
// underlying row (a new auto-increment ID) but reproduces the same
// (ptype,v0,v1,v2) tuple from the same sys_menu/sys_api data, so a
// natural-key match here still finds it. What it does not survive is the
// role being renamed, or the tuple being rebuilt from a completely different
// source (a future SavePolicy call from outside this seeder) - in both cases
// the match legitimately fails, and business rule 3 says the uninstaller
// should report and skip, not delete something else that happens to look
// the same.
type SysAppCasbinGrant struct {
Id int `json:"id" gorm:"primaryKey;autoIncrement"`
AppCode string `json:"appCode" gorm:"type:varchar(64);not null;index:idx_sys_app_casbin_grant_app_code;comment:app code that created this grant"`
// Column widths mirror gorm-adapter's own CasbinRule struct exactly, so
// a value that fits into casbin_rule always fits here, and the unique
// index below matches the one createTable() puts on casbin_rule itself.
Ptype string `json:"ptype" gorm:"size:100;not null;uniqueIndex:uk_sys_app_casbin_grant_rule;comment:casbin ptype, 'p' today"`
V0 string `json:"v0" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:role_key at grant time"`
V1 string `json:"v1" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:api path"`
V2 string `json:"v2" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:http method"`
V3 string `json:"v3" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:unused today"`
V4 string `json:"v4" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:unused today"`
V5 string `json:"v5" gorm:"size:100;not null;default:'';uniqueIndex:uk_sys_app_casbin_grant_rule;comment:unused today"`
CreatedAt time.Time `json:"createdAt" gorm:"comment:when this grant was recorded"`
}
func (*SysAppCasbinGrant) TableName() string {
return "sys_app_casbin_grant"
}
+13
View File
@@ -32,6 +32,19 @@ type SysMenu struct {
// AutoMigrate adding this column to an existing table leaves every
// pre-existing row reading back as "" rather than NULL.
AppCode string `json:"appCode" gorm:"type:varchar(64);not null;default:'';index:idx_sys_menu_app_code;comment:AppCode"`
// SeedCode is the raw seed.MenuSpec.Code this row was created from, kept
// so seedMenuTree can ask "did I already write this node" without
// relying on MenuName's PascalCase concatenation, which is not
// injective (see design doc §1.6). Nullable, unlike AppCode: every row
// seed.SeedMenus writes sets a real value, but every pre-existing row -
// the host's own hand-placed menus, and every app-seeded row written
// before this column existed - has none, and there is no way to
// backfill one that means anything. NULL is what lets an unbounded
// number of those coexist under the same app_code without tripping the
// unique index below: the database never treats two NULLs as equal, so
// only rows that do carry a real code participate in the uniqueness
// check at all.
SeedCode *string `json:"seedCode" gorm:"size:64;uniqueIndex:uk_sys_menu_app_seed_code_del;comment:raw MenuSpec.Code, null for rows not written through SeedMenus"`
models.ControlBy
models.ModelTime
}
+152 -8
View File
@@ -90,6 +90,23 @@ func (adminSeeder) SeedMenus(tx *gorm.DB, appCode string, menus []seed.MenuSpec,
// application's ids in the module cache. Never accepting a caller-chosen id
// here removes the collision this Seeder has no way to detect instead of
// trying to detect it after the fact.
//
// The natural key is (app_code, path, action) - the same three columns
// 1786700002000_remove_refresh_token_api.go already used to identify a
// single API by hand, and the ones 1786700008000_seed_natural_keys.go put a
// unique index on. Before inserting, this looks for a live row (deleted_at
// = 0, applied automatically by the soft-delete plugin on every query
// against models.SysApi) already holding that key and reuses it instead of
// inserting a second one - see the design doc §1.6: a migration retried
// after a partial failure previously re-ran this as a bare tx.Create and
// produced duplicate rows on the demo site.
//
// Unlike seedMenuTree's reuse branch, this one has nothing left to repair
// after finding an existing row: models.SysApi carries no association
// (nothing like SysMenu's many2many SysApi field) and this function writes
// nothing beyond the row itself - no second statement comparable to
// seedMenuTree's paths UPDATE follows tx.Create below. An interrupted retry
// can therefore only ever find this row complete or not find it at all.
func seedApis(tx *gorm.DB, appCode string, apis []seed.ApiSpec) (map[string]models.SysApi, error) {
seen := make(map[string]bool, len(apis))
rows := make(map[string]models.SysApi, len(apis))
@@ -102,6 +119,19 @@ func seedApis(tx *gorm.DB, appCode string, apis []seed.ApiSpec) (map[string]mode
}
seen[a.Code] = true
var existing models.SysApi
err := tx.Where("app_code = ? AND path = ? AND action = ?", appCode, a.Path, a.Method).
First(&existing).Error
switch {
case err == nil:
rows[a.Code] = existing
continue
case errors.Is(err, gorm.ErrRecordNotFound):
// Not seen yet; fall through to insert it.
default:
return nil, fmt.Errorf("api %q: checking for an existing row: %w", a.Code, err)
}
row := models.SysApi{
Handle: a.Handle,
Title: a.Title,
@@ -153,6 +183,12 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma
continue
}
// Resolved before the idempotency check below, whether or not
// this spec's own row turns out to already exist: repairing an
// existing-but-incomplete row's paths needs the parent's
// already-resolved Paths exactly as much as creating a fresh
// row does (see repairExistingMenu), so both have to wait for
// it the same way.
var parentRow models.SysMenu
if s.Parent != "" {
parent, ok := created[s.Parent]
@@ -165,6 +201,32 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma
parentRow = parent
}
// Idempotency check: does this node already have a row, from
// an earlier, possibly-interrupted attempt? The natural key is
// (app_code, seed_code) - menu_name's PascalCase concatenation
// is not injective and cannot be used for this (see menuName's
// doc comment and the design doc §1.6). Only a live row counts;
// the soft-delete plugin scopes deleted_at = 0 automatically on
// every query against models.SysMenu.
var existing models.SysMenu
err := tx.Where("app_code = ? AND seed_code = ?", appCode, s.Code).First(&existing).Error
switch {
case err == nil:
row, err := repairExistingMenu(tx, existing, s, parentRow, apiRows)
if err != nil {
return nil, fmt.Errorf("%q: repairing an existing row: %w", s.Code, err)
}
created[s.Code] = row
ids = append(ids, row.MenuId)
progressed = true
continue
case errors.Is(err, gorm.ErrRecordNotFound):
// Not written yet; fall through to create it below.
default:
return nil, fmt.Errorf("%q: checking for an existing row: %w", s.Code, err)
}
seedCode := s.Code
row := models.SysMenu{
MenuName: menuName(appCode, s.Code),
Title: s.Title,
@@ -179,9 +241,10 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma
// 1786700001000_demo_menu.go seeds its own menu with. A
// freshly installed application's menu should not need an
// administrator to first find and unhide it.
Visible: "0",
IsFrame: "1",
AppCode: appCode,
Visible: "0",
IsFrame: "1",
AppCode: appCode,
SeedCode: &seedCode,
}
for _, code := range s.ApiCodes {
api, ok := apiRows[code]
@@ -205,11 +268,7 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma
// two-step create-then-update 1786700001000_demo_menu.go's
// hand-assigned ids let it do in one literal, sequenced here
// instead.
if s.Parent == "" {
row.Paths = "/0/" + strconv.Itoa(row.MenuId)
} else {
row.Paths = parentRow.Paths + "/" + strconv.Itoa(row.MenuId)
}
row.Paths = expectedPaths(row.MenuId, s.Parent, parentRow)
if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", row.MenuId).
Update("paths", row.Paths).Error; err != nil {
return nil, fmt.Errorf("%q: writing paths: %w", s.Code, err)
@@ -226,6 +285,91 @@ func seedMenuTree(tx *gorm.DB, appCode string, specs []seed.MenuSpec, apiRows ma
return ids, nil
}
// expectedPaths is the materialized path a fresh insert of menuID under
// parent (or at the root, if parent is "") computes - factored out so
// repairExistingMenu can ask the same question about a row it did not just
// create.
func expectedPaths(menuID int, parent string, parentRow models.SysMenu) string {
if parent == "" {
return "/0/" + strconv.Itoa(menuID)
}
return parentRow.Paths + "/" + strconv.Itoa(menuID)
}
// repairExistingMenu brings a row seedMenuTree's idempotency check found up
// to what a fresh insert of the same spec would have produced.
//
// A row can be found and still be incomplete: tx.Create's own association
// write (the sys_menu_api_rule bindings from row.SysApi) and the paths
// UPDATE that follows it are each their own statement, and design doc §1.5
// establishes that nothing after the first DDL in a migration function can
// be rolled back together - a process interrupted between the row insert
// and either of those two steps leaves exactly this row: present, findable
// by its natural key, but missing what makes it a working menu entry. A
// retry that only checked "does the row exist" and stopped there would
// report success while the sys_menu_api_rule binding stays missing (the
// api is granted to no one) or paths stays empty (a materialized-path
// break that orphans the rest of the subtree from the root) - as silent as
// the duplicate-row defect the idempotency check itself was written to
// close.
//
// Both checks are read-before-write, so a row that is already complete -
// the ordinary case on every retry after the first successful one - causes
// no writes at all: existing.Paths already equals what expectedPaths
// computes, and the sys_menu_api_rule INSERT is itself guarded by
// WHERE NOT EXISTS, the same idempotent-insert shape grantToAdminRole
// already uses for sys_role_menu/casbin_rule. Never DELETEs an existing
// binding to rebuild it - that is the FullSaveAssociations mistake
// sys_role.go's SysRole.Update makes for sys_role_menu/casbin_rule
// (app/admin/service/sys_role.go:148-153), the exact pattern this design
// went out of its way to avoid for the tables that do use it.
//
// Insert-only cuts both ways, deliberately. A binding an administrator
// added by hand through the menu management UI, for an api never in
// s.ApiCodes at all, is never touched by this loop and survives every
// later retry (TestSeedMenusPreservesAHandAddedBinding is the reproduction
// case for the opposite mistake: delete-then-reinsert wipes it silently,
// the same shape as sys_role_menu/casbin_rule getting zeroed by a role
// edit, just with this code as the actor instead of the victim). The
// converse case - a MenuSpec that used to list an ApiCode and no longer
// does - is not handled here either, and that half is intentional rather
// than an oversight: this loop only ever adds rows for codes the *current*
// call's ApiCodes names, so a binding for a code an earlier version
// granted and the current one dropped is left in place, stale. Reconciling
// that is deleting something, which needs the same certainty about
// ownership uninstall's design (see design doc §5) already requires -
// this function has no way to tell "stale, from an older version of this
// same app" apart from "hand-added, for a reason", and business rule 3
// ("uninstall deletes only what it can attribute with certainty") applies
// here just as much as it does there. Reconciling stale seed-driven
// bindings, if it is ever wanted, belongs in the upgrade path with that
// same ownership check - not silently inside every retry of every install.
func repairExistingMenu(tx *gorm.DB, existing models.SysMenu, s seed.MenuSpec, parentRow models.SysMenu, apiRows map[string]models.SysApi) (models.SysMenu, error) {
want := expectedPaths(existing.MenuId, s.Parent, parentRow)
if existing.Paths != want {
if err := tx.Model(&models.SysMenu{}).Where("menu_id = ?", existing.MenuId).
Update("paths", want).Error; err != nil {
return models.SysMenu{}, fmt.Errorf("repairing paths: %w", err)
}
existing.Paths = want
}
for _, code := range s.ApiCodes {
api, ok := apiRows[code]
if !ok {
return models.SysMenu{}, fmt.Errorf("ApiCodes references %q, which is not an ApiSpec.Code in this call", code)
}
if err := tx.Exec(
"INSERT INTO sys_menu_api_rule (sys_menu_menu_id, sys_api_id) SELECT ?, ? WHERE NOT EXISTS (SELECT 1 FROM sys_menu_api_rule WHERE sys_menu_menu_id = ? AND sys_api_id = ?)",
existing.MenuId, api.Id, existing.MenuId, api.Id,
).Error; err != nil {
return models.SysMenu{}, fmt.Errorf("binding %q: %w", code, err)
}
}
return existing, nil
}
// validateMenuSpec rejects the malformed input tools/checksilent's
// menu-sort-overflow and Kind-adjacent checks would catch for an in-tree
// seed but cannot for a third-party application's - see menuSortRange's doc
+552
View File
@@ -1,13 +1,17 @@
package service
import (
"context"
"errors"
"strconv"
"strings"
"sync"
"testing"
"time"
"github.com/glebarez/sqlite"
"gorm.io/gorm"
gormlogger "gorm.io/gorm/logger"
contractmodels "github.com/go-admin-team/go-admin-core/v2/sdk/contract/models"
"github.com/go-admin-team/go-admin-core/v2/sdk/contract/seed"
@@ -289,3 +293,551 @@ func TestSeedMenusWithNothingRegisteredWritesNothing(t *testing.T) {
}
}
}
// newSeedTestDB's AutoMigrate builds a unique index on seed_code alone,
// because SysMenu.SeedCode is the only field in the struct carrying the
// uk_sys_menu_app_seed_code_del tag - app_code already carries a different,
// non-unique index name of its own, and the embedded ModelTime's
// DeletedAt (aliased from go-admin-core) cannot be given a third one. The
// real migration (cmd/migrate/migration/version/1786700008000_seed_natural_keys.go)
// never lets AutoMigrate touch this table for exactly that reason: it
// builds the composite (app_code, seed_code, deleted_at) index by hand
// instead. Reproduce that by hand here too, so a test that seeds two rows
// sharing a seed_code under different deleted_at values sees what a real
// install would, not gorm's narrower default.
func useCompositeSeedCodeIndex(t *testing.T, db *gorm.DB) {
t.Helper()
if db.Migrator().HasIndex(&models.SysMenu{}, "uk_sys_menu_app_seed_code_del") {
if err := db.Migrator().DropIndex(&models.SysMenu{}, "uk_sys_menu_app_seed_code_del"); err != nil {
t.Fatalf("drop the single-column seed_code index: %v", err)
}
}
if err := db.Exec(
"CREATE UNIQUE INDEX uk_sys_menu_app_seed_code_del ON sys_menu (app_code, seed_code, deleted_at)",
).Error; err != nil {
t.Fatalf("create the composite seed_code index: %v", err)
}
}
// A retried migration - one that failed partway through and is run again,
// or simply run twice by mistake - must not create a second sys_api or
// sys_menu row for the same (appCode, natural key). This is the defect the
// demo site hit in production: duplicate sys_menu/casbin_rule rows from a
// bare tx.Create on a natural key nothing was checking.
func TestSeedMenusIsIdempotentAcrossARetry(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
menus := []seed.MenuSpec{
{Code: "dir", Kind: contractmodels.Directory, Title: "Order", Sort: 10},
{Code: "list", Parent: "dir", Kind: contractmodels.Menu, Title: "Orders", Sort: 1, ApiCodes: []string{"list"}},
}
apis := []seed.ApiSpec{
{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"},
}
run := func() {
t.Helper()
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus: %v", err)
}
}
run()
firstMenuIDs := allMenuIDs(t, db, "order")
firstApiIDs := allApiIDs(t, db, "order")
run() // the retry
if got := allMenuIDs(t, db, "order"); !sameIDs(got, firstMenuIDs) {
t.Errorf("sys_menu ids after retry = %v, want unchanged %v (a second call inserted new rows)", got, firstMenuIDs)
}
if got := allApiIDs(t, db, "order"); !sameIDs(got, firstApiIDs) {
t.Errorf("sys_api ids after retry = %v, want unchanged %v (a second call inserted new rows)", got, firstApiIDs)
}
assertRowCount(t, db, "sys_api", 1)
assertRowCount(t, db, "sys_menu", 2)
assertRowCount(t, db, "sys_menu_api_rule", 1)
assertRowCount(t, db, "sys_role_menu", 2)
assertRowCount(t, db, "casbin_rule", 1)
}
// Only a live row counts as "already written". A row a prior, unrelated
// soft-delete already retired must not be reused - seedApis/seedMenuTree
// have to insert a fresh one under the same natural key, the same way the
// unique indexes 1786700008000_seed_natural_keys.go builds only bind live
// rows.
func TestSeedMenusOnlyReusesLiveRows(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
menus := []seed.MenuSpec{{Code: "dir", Kind: contractmodels.Directory, Title: "Order", Sort: 10}}
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus: %v", err)
}
// Soft-delete both rows this first call wrote, as if an operator (or an
// earlier uninstall) had retired them, independently of this migration
// ever running again.
if err := db.Exec("UPDATE sys_menu SET deleted_at = 1").Error; err != nil {
t.Fatalf("soft-delete sys_menu: %v", err)
}
if err := db.Exec("UPDATE sys_api SET deleted_at = 1").Error; err != nil {
t.Fatalf("soft-delete sys_api: %v", err)
}
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus after soft-delete: %v", err)
}
// Two rows total: the soft-deleted original, plus a fresh one - not the
// dead row resurrected in place, and not left with zero live rows.
assertRowCount(t, db, "sys_menu", 2)
assertRowCount(t, db, "sys_api", 2)
var liveMenus, liveApis int64
db.Model(&models.SysMenu{}).Where("app_code = ?", "order").Count(&liveMenus)
db.Model(&models.SysApi{}).Where("app_code = ?", "order").Count(&liveApis)
if liveMenus != 1 {
t.Errorf("live sys_menu rows = %d, want 1", liveMenus)
}
if liveApis != 1 {
t.Errorf("live sys_api rows = %d, want 1", liveApis)
}
}
// app_code is part of the natural key, not a descriptive column alongside
// it. Two applications that happen to register an identical (path, action)
// or seed_code must each get their own row - reusing one app's row for
// another's install would make an uninstall of the first delete a row the
// second considers its own.
func TestSeedMenusScopesTheNaturalKeyByAppCode(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
menus := []seed.MenuSpec{{Code: "dir", Kind: contractmodels.Directory, Title: "Dir", Sort: 10}}
apis := []seed.ApiSpec{{Code: "list", Title: "Shared endpoint", Path: "/api/v1/shared", Method: "GET"}}
for _, appCode := range []string{"order", "billing"} {
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, appCode, menus, apis)
}); err != nil {
t.Fatalf("SeedMenus(%q): %v", appCode, err)
}
}
var apiRows []models.SysApi
if err := db.Where("path = ? AND action = ?", "/api/v1/shared", "GET").
Order("app_code").Find(&apiRows).Error; err != nil {
t.Fatalf("read sys_api: %v", err)
}
if len(apiRows) != 2 {
t.Fatalf("sys_api has %d row(s) for the shared (path, action), want 2 - one per app", len(apiRows))
}
if apiRows[0].AppCode != "billing" || apiRows[1].AppCode != "order" {
t.Errorf("sys_api app_codes = [%s %s], want [billing order]", apiRows[0].AppCode, apiRows[1].AppCode)
}
var menuRows []models.SysMenu
if err := db.Where("seed_code = ?", "dir").Order("app_code").Find(&menuRows).Error; err != nil {
t.Fatalf("read sys_menu: %v", err)
}
if len(menuRows) != 2 {
t.Fatalf("sys_menu has %d row(s) for the shared seed_code, want 2 - one per app", len(menuRows))
}
if menuRows[0].AppCode != "billing" || menuRows[1].AppCode != "order" {
t.Errorf("sys_menu app_codes = [%s %s], want [billing order]", menuRows[0].AppCode, menuRows[1].AppCode)
}
}
func allMenuIDs(t *testing.T, db *gorm.DB, appCode string) []int {
t.Helper()
var rows []models.SysMenu
if err := db.Where("app_code = ?", appCode).Order("menu_id").Find(&rows).Error; err != nil {
t.Fatalf("read sys_menu: %v", err)
}
ids := make([]int, len(rows))
for i, r := range rows {
ids[i] = r.MenuId
}
return ids
}
func allApiIDs(t *testing.T, db *gorm.DB, appCode string) []int {
t.Helper()
var rows []models.SysApi
if err := db.Where("app_code = ?", appCode).Order("id").Find(&rows).Error; err != nil {
t.Fatalf("read sys_api: %v", err)
}
ids := make([]int, len(rows))
for i, r := range rows {
ids[i] = r.Id
}
return ids
}
func sameIDs(a, b []int) bool {
if len(a) != len(b) {
return false
}
for i := range a {
if a[i] != b[i] {
return false
}
}
return true
}
func assertRowCount(t *testing.T, db *gorm.DB, table string, want int64) {
t.Helper()
var n int64
if err := db.Table(table).Count(&n).Error; err != nil {
t.Fatalf("count %s: %v", table, err)
}
if n != want {
t.Errorf("%s has %d row(s), want %d", table, n, want)
}
}
// A retried migration does not just risk inserting a second copy of a row
// it already wrote (that gap is closed above) - the reuse path itself has
// to leave the row in the same state a fresh insert would have. Before
// this defect was fixed, the reuse branch (seed.go's "case err == nil")
// stopped at reusing the row's id and skipped everything a fresh insert
// does afterwards: the sys_menu_api_rule binding gorm's association save
// writes as part of Create, and the paths UPDATE that follows Create as a
// separate statement. A row a prior attempt inserted but did not finish -
// exactly the shape design doc §1.5 says a non-transactional retry can
// leave behind - would be "found" and then left broken forever, with the
// migration reporting success.
//
// existingHalfWrittenMenu inserts a sys_menu row the way seedMenuTree's own
// tx.Create leaves one when interrupted immediately afterwards: the row
// exists with its natural key, but paths was never computed and no
// sys_menu_api_rule binding was ever written for it - Create's association
// save and the paths UPDATE are each a separate statement from the row
// insert itself.
func existingHalfWrittenMenu(t *testing.T, db *gorm.DB, appCode, seedCode string, parentID int) models.SysMenu {
t.Helper()
code := seedCode
row := models.SysMenu{
MenuName: menuName(appCode, seedCode),
AppCode: appCode,
SeedCode: &code,
ParentId: parentID,
Visible: "0",
IsFrame: "1",
// Paths deliberately left "" - never computed, the same as a row
// whose Create succeeded but whose follow-up paths UPDATE never ran.
}
if err := db.Create(&row).Error; err != nil {
t.Fatalf("seed half-written menu %q: %v", seedCode, err)
}
return row
}
func bindingCount(t *testing.T, db *gorm.DB, menuID, apiID int) int64 {
t.Helper()
var n int64
if err := db.Table("sys_menu_api_rule").
Where("sys_menu_menu_id = ? AND sys_api_id = ?", menuID, apiID).Count(&n).Error; err != nil {
t.Fatalf("count sys_menu_api_rule: %v", err)
}
return n
}
// TestSeedMenusRepairsAnIncompleteExistingRow is the reproduction case:
// both paths and the api binding are missing on the row seedMenuTree finds
// through its idempotency check, the shape a real interrupted retry leaves
// behind. Run against the unfixed reuse branch, this must fail - that is
// what proves the defect is real rather than a three-way guess.
func TestSeedMenusRepairsAnIncompleteExistingRow(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
apiRows, err := seedApis(db, "order", apis)
if err != nil {
t.Fatalf("seedApis: %v", err)
}
dir := existingHalfWrittenMenu(t, db, "order", "dir", 0)
list := existingHalfWrittenMenu(t, db, "order", "list", dir.MenuId)
menus := []seed.MenuSpec{
{Code: "dir", Kind: contractmodels.Directory, Title: "Order", Sort: 10},
{Code: "list", Parent: "dir", Kind: contractmodels.Menu, Title: "Orders", Sort: 1, ApiCodes: []string{"list"}},
}
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus: %v", err)
}
wantDirPaths := "/0/" + strconv.Itoa(dir.MenuId)
wantListPaths := wantDirPaths + "/" + strconv.Itoa(list.MenuId)
var gotDir, gotList models.SysMenu
if err := db.First(&gotDir, dir.MenuId).Error; err != nil {
t.Fatalf("read dir: %v", err)
}
if err := db.First(&gotList, list.MenuId).Error; err != nil {
t.Fatalf("read list: %v", err)
}
if gotDir.Paths != wantDirPaths {
t.Errorf("dir.Paths = %q, want %q - a retried install left a root menu with no materialized path", gotDir.Paths, wantDirPaths)
}
if gotList.Paths != wantListPaths {
t.Errorf("list.Paths = %q, want %q - a retried install left the seeded subtree with a broken materialized path", gotList.Paths, wantListPaths)
}
if n := bindingCount(t, db, list.MenuId, apiRows["list"].Id); n != 1 {
t.Errorf("sys_menu_api_rule binding count for list = %d, want 1 - a retried install left the menu with its api granted to no one", n)
}
}
// soloMenuSpec is a single, parent-less menu with one api binding - the
// smallest shape that can exhibit "paths wrong" and "binding missing"
// independently of each other, used by the three tests below to isolate
// one repair path at a time from TestSeedMenusRepairsAnIncompleteExistingRow's
// combined (both broken) case.
func soloMenuSpec() []seed.MenuSpec {
return []seed.MenuSpec{{Code: "solo", Kind: contractmodels.Menu, Title: "Solo", Sort: 1, ApiCodes: []string{"list"}}}
}
// Only the binding is missing; paths is already correct. The repair must
// add the binding and must not touch the already-correct paths value.
func TestSeedMenusRepairsOnlyAMissingBinding(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
apiRows, err := seedApis(db, "order", apis)
if err != nil {
t.Fatalf("seedApis: %v", err)
}
solo := existingHalfWrittenMenu(t, db, "order", "solo", 0)
wantPaths := "/0/" + strconv.Itoa(solo.MenuId)
if err := db.Model(&models.SysMenu{}).Where("menu_id = ?", solo.MenuId).
Update("paths", wantPaths).Error; err != nil {
t.Fatalf("set paths: %v", err)
}
// The binding is deliberately left unwritten.
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", soloMenuSpec(), apis)
}); err != nil {
t.Fatalf("SeedMenus: %v", err)
}
var got models.SysMenu
if err := db.First(&got, solo.MenuId).Error; err != nil {
t.Fatalf("read solo: %v", err)
}
if got.Paths != wantPaths {
t.Errorf("paths changed from %q to %q; repairing a missing binding must not touch an already-correct path", wantPaths, got.Paths)
}
if n := bindingCount(t, db, solo.MenuId, apiRows["list"].Id); n != 1 {
t.Errorf("binding count = %d, want 1", n)
}
}
// Only paths is missing; the binding already exists (as if Create's own
// association write had succeeded but the paths UPDATE that follows it
// never ran). The repair must fix paths and must not duplicate the
// already-correct binding.
func TestSeedMenusRepairsOnlyMissingPaths(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
apiRows, err := seedApis(db, "order", apis)
if err != nil {
t.Fatalf("seedApis: %v", err)
}
solo := existingHalfWrittenMenu(t, db, "order", "solo", 0)
if err := db.Exec(
"INSERT INTO sys_menu_api_rule (sys_menu_menu_id, sys_api_id) VALUES (?, ?)",
solo.MenuId, apiRows["list"].Id,
).Error; err != nil {
t.Fatalf("seed binding: %v", err)
}
// solo.Paths is deliberately left "" by existingHalfWrittenMenu.
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", soloMenuSpec(), apis)
}); err != nil {
t.Fatalf("SeedMenus: %v", err)
}
wantPaths := "/0/" + strconv.Itoa(solo.MenuId)
var got models.SysMenu
if err := db.First(&got, solo.MenuId).Error; err != nil {
t.Fatalf("read solo: %v", err)
}
if got.Paths != wantPaths {
t.Errorf("paths = %q, want %q", got.Paths, wantPaths)
}
if n := bindingCount(t, db, solo.MenuId, apiRows["list"].Id); n != 1 {
t.Errorf("binding count = %d, want 1 - repairing paths must not duplicate an already-correct binding", n)
}
}
// capturingLogger records every SQL statement gorm actually executes, so a
// test can assert that a fully-consistent retry performs no write at all -
// not just that its net effect happens to be zero rows changed. Mirrors
// common/actions/crud_shim_test.go's logger of the same name and shape;
// duplicated locally rather than exported and shared, matching how small
// gorm-facing test doubles are kept next to the test that needs them
// elsewhere in this repository.
type capturingLogger struct {
gormlogger.Interface
mu sync.Mutex
stmts []string
}
func (l *capturingLogger) Trace(ctx context.Context, begin time.Time, fc func() (string, int64), err error) {
sql, _ := fc()
l.mu.Lock()
l.stmts = append(l.stmts, sql)
l.mu.Unlock()
}
func (l *capturingLogger) all() string {
l.mu.Lock()
defer l.mu.Unlock()
return strings.Join(l.stmts, "\n")
}
// Both paths and the binding are already correct - the ordinary shape of
// every retry after the first one succeeds in full. Repairing an
// already-consistent row must not touch it: paths is read-before-write and
// so must not be UPDATEd at all (asserted directly, by statement, since the
// code gates that call behind a value comparison); the binding's own
// insert is guarded by WHERE NOT EXISTS the same way grantToAdminRole's
// already are, so its row count staying put is the meaningful claim - the
// guarded statement itself may still be sent, the same way it already is
// for sys_role_menu/casbin_rule.
func TestSeedMenusFullyConsistentRowCausesNoPathsUpdate(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
menus := soloMenuSpec()
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus (first): %v", err)
}
var apiRows []models.SysApi
db.Where("app_code = ?", "order").Find(&apiRows)
var soloRow models.SysMenu
if err := db.Where("app_code = ? AND seed_code = ?", "order", "solo").First(&soloRow).Error; err != nil {
t.Fatalf("read solo after first call: %v", err)
}
if soloRow.Paths == "" {
t.Fatalf("solo.Paths is empty after the first call; the fixture itself is broken, not what this test means to check")
}
wantBindings := bindingCount(t, db, soloRow.MenuId, apiRows[0].Id)
if wantBindings != 1 {
t.Fatalf("binding count after the first call = %d, want 1; the fixture itself is broken", wantBindings)
}
capturing := &capturingLogger{Interface: gormlogger.Default.LogMode(gormlogger.Info)}
captured := db.Session(&gorm.Session{Logger: capturing})
if err := captured.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus (retry): %v", err)
}
all := strings.ToUpper(capturing.all())
if strings.Contains(all, "UPDATE") && strings.Contains(all, "SYS_MENU") && strings.Contains(all, "PATHS") {
t.Errorf("a fully consistent retry executed a paths UPDATE against sys_menu:\n%s", capturing.all())
}
if got := bindingCount(t, db, soloRow.MenuId, apiRows[0].Id); got != 1 {
t.Errorf("binding count after the retry = %d, want 1 (unchanged)", got)
}
}
// An administrator can bind a menu to an additional api by hand through
// the menu management UI - a sys_menu_api_rule row for an api never in
// s.ApiCodes at all. A retried SeedMenus call must not touch it: deleting
// every binding for the menu and reinserting only what s.ApiCodes lists
// would wipe it out silently, the same shape as sys_role_menu/casbin_rule
// getting zeroed by SysRole.Update's FullSaveAssociations save - just with
// this code as the actor instead of the victim this time.
func TestSeedMenusPreservesAHandAddedBinding(t *testing.T) {
db := newSeedTestDB(t)
useCompositeSeedCodeIndex(t, db)
seedAdminRole(t, db)
apis := []seed.ApiSpec{{Code: "list", Title: "Order list", Path: "/api/v1/order", Method: "GET"}}
menus := soloMenuSpec()
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus (first): %v", err)
}
var soloRow models.SysMenu
if err := db.Where("app_code = ? AND seed_code = ?", "order", "solo").First(&soloRow).Error; err != nil {
t.Fatalf("read solo: %v", err)
}
// An api this call's ApiSpec list never mentions - standing in for one
// belonging to some other feature entirely, bound to this menu by an
// administrator, not by any SeedMenus call.
handAdded := models.SysApi{Path: "/api/v1/order/export", Action: "GET", Type: "SYS", AppCode: "order"}
if err := db.Create(&handAdded).Error; err != nil {
t.Fatalf("seed the hand-added api: %v", err)
}
if err := db.Exec(
"INSERT INTO sys_menu_api_rule (sys_menu_menu_id, sys_api_id) VALUES (?, ?)",
soloRow.MenuId, handAdded.Id,
).Error; err != nil {
t.Fatalf("seed the hand-added binding: %v", err)
}
// A retry with the exact same specs - solo's ApiCodes still names only
// "list".
if err := db.Transaction(func(tx *gorm.DB) error {
return adminSeeder{}.SeedMenus(tx, "order", menus, apis)
}); err != nil {
t.Fatalf("SeedMenus (retry): %v", err)
}
if n := bindingCount(t, db, soloRow.MenuId, handAdded.Id); n != 1 {
t.Errorf("hand-added binding count = %d, want 1 - a retry silently deleted a binding it does not own", n)
}
var apiRows []models.SysApi
db.Where("app_code = ? AND path = ?", "order", "/api/v1/order").Find(&apiRows)
if len(apiRows) != 1 {
t.Fatalf("seeded api not found as expected: %+v", apiRows)
}
if n := bindingCount(t, db, soloRow.MenuId, apiRows[0].Id); n != 1 {
t.Errorf("the seed's own binding count = %d, want 1 - it must survive the retry too", n)
}
}
+91
View File
@@ -0,0 +1,91 @@
package router
import (
"testing"
"github.com/gin-gonic/gin"
jwt "github.com/go-admin-team/go-admin-core/v2/jwtauth"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
"go-admin/common/middleware"
)
// registeredRoutes builds the generator's routes on an engine of its own and
// reports the patterns they were registered under.
//
// The mode has to be given rather than inherited, because it now decides what
// gets registered: a test that leaves it at the zero value would be asking
// about a mode no deployment runs in, and would pass whether or not the gate
// works.
//
// It is put back before this returns, not at the end of the test. t.Cleanup
// would leave the mode set for everything the caller does afterwards, so a
// caller that went on to assert something mode-dependent would be reading a
// value this helper left behind rather than one it chose. A caller that does
// want the mode set has to set it, which is visible where it happens.
//
// The JWT middleware is a zero value. MiddlewareFunc only closes over the
// receiver and is never called here - no request is served, the engine is
// asked what it has - so nothing dereferences it.
func registeredRoutes(t *testing.T, mode string) map[string]bool {
t.Helper()
gin.SetMode(gin.TestMode)
previous := config.ApplicationConfig.Mode
defer func() { config.ApplicationConfig.Mode = previous }()
config.ApplicationConfig.Mode = mode
r := gin.New()
v1 := r.Group("/api/v1")
sysNoCheckRoleRouter(v1, &jwt.GinJWTMiddleware{})
registerDBRouter(v1, &jwt.GinJWTMiddleware{})
out := map[string]bool{}
for _, route := range r.Routes() {
out[route.Path] = true
}
return out
}
// The list of routes demo mode refuses lives in common/middleware, which may
// not import app/ and therefore cannot see whether any of them is still a
// route. This is the half that can be checked, and it is checked here because
// this is where the routes are declared: rename one, and the entry over there
// stops matching anything, demo mode silently starts serving it again, and
// nothing else would say so.
func TestEveryRouteDemoModeRefusesStillExists(t *testing.T) {
routes := registeredRoutes(t, "demo")
for _, guarded := range middleware.DemoWriteRoutes() {
if !routes[guarded] {
t.Errorf("demo mode refuses %q, but no route is registered under that pattern - "+
"either it was renamed, or it moved to another file; the guard now matches nothing",
guarded)
}
}
}
// The other direction, and the one the demo host cares about: the generator's
// read-only routes have to stay reachable, or a demo deployment cannot show
// the feature at all. Refusing too much is as much of a defect as refusing too
// little.
func TestTheGeneratorsReadOnlyRoutesAreNotRefused(t *testing.T) {
refused := map[string]bool{}
for _, guarded := range middleware.DemoWriteRoutes() {
refused[guarded] = true
}
for _, readOnly := range []string{
"/api/v1/gen/preview/:tableId",
"/api/v1/gen/tabletree",
"/api/v1/db/tables/page",
"/api/v1/db/columns/page",
} {
if !registeredRoutes(t, "demo")[readOnly] {
t.Fatalf("%s is not registered, so this test is asserting against nothing", readOnly)
}
if refused[readOnly] {
t.Errorf("demo mode refuses %s, which only reads - the demo host needs it to "+
"demonstrate the generator", readOnly)
}
}
}
+41 -5
View File
@@ -3,15 +3,49 @@ package router
import (
"github.com/gin-gonic/gin"
jwt "github.com/go-admin-team/go-admin-core/v2/jwtauth"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
"go-admin/app/admin/apis"
"go-admin/app/other/apis/tools"
)
// GenWriteRoutesEnabled reports whether the code generator's writing endpoints
// are registered in this process.
//
// Three of the generator's endpoints do not read. /gen/toproject writes seven
// Go and Vue source files onto this host, one of them under the path
// gen.frontpath names; /gen/apitofile writes a migration; /gen/todb inserts
// menus and APIs. All three are GET, all three are listed in CasbinExclude,
// and AuthCheckRole skips what is on that list - so Enforce never runs for
// them and any account that can log in may call them. That is a bargain a
// workstation can make and a deployment cannot.
//
// dev is the shipped default and is where the generator is meant to be used.
// demo keeps them because demo mode already has a better answer than a 404:
// DemoEvn refuses these three by name and explains itself, which is the thing
// the demo exists to show. test and prod get nothing.
//
// The mode is read once, while the routes are being built. Changing
// application.mode in a running process adds and removes nothing - a
// configuration reload rebuilds neither the engine nor its routes.
//
// core has constants for dev, test and prod but none for demo, which this
// repository spells as a literal in common/middleware/demo.go. Both are
// literals here so that the two read as one set rather than two conventions.
func GenWriteRoutesEnabled() bool {
switch config.ApplicationConfig.Mode {
case "dev", "demo":
return true
default:
return false
}
}
func init() {
routerCheckRole = append(routerCheckRole, sysNoCheckRoleRouter, registerDBRouter, registerSysTableRouter)
}
func sysNoCheckRoleRouter(v1 *gin.RouterGroup ,authMiddleware *jwt.GinJWTMiddleware) {
func sysNoCheckRoleRouter(v1 *gin.RouterGroup, authMiddleware *jwt.GinJWTMiddleware) {
r1 := v1.Group("")
{
sys := apis.System{}
@@ -22,9 +56,11 @@ func sysNoCheckRoleRouter(v1 *gin.RouterGroup ,authMiddleware *jwt.GinJWTMiddlew
{
gen := tools.Gen{}
r.GET("/gen/preview/:tableId", gen.Preview)
r.GET("/gen/toproject/:tableId", gen.GenCode)
r.GET("/gen/apitofile/:tableId", gen.GenApiToFile)
r.GET("/gen/todb/:tableId", gen.GenMenuAndApi)
if GenWriteRoutesEnabled() {
r.GET("/gen/toproject/:tableId", gen.GenCode)
r.GET("/gen/apitofile/:tableId", gen.GenApiToFile)
r.GET("/gen/todb/:tableId", gen.GenMenuAndApi)
}
sysTable := tools.SysTable{}
r.GET("/gen/tabletree", sysTable.GetSysTablesTree)
}
@@ -53,4 +89,4 @@ func registerSysTableRouter(v1 *gin.RouterGroup, authMiddleware *jwt.GinJWTMiddl
tablesInfo.GET("", sysTable.GetSysTablesInfo)
}
}
}
}
+151
View File
@@ -0,0 +1,151 @@
package router
import (
"testing"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
)
// genWritingRoutes are the three that do not read. They write Go and Vue
// source onto the host, a migration, and rows in sys_menu.
var genWritingRoutes = []string{
"/api/v1/gen/toproject/:tableId",
"/api/v1/gen/apitofile/:tableId",
"/api/v1/gen/todb/:tableId",
}
// genReadingRoutes are the rest of the generator's surface. Gating the three
// above must not cost any of these: a deployment that cannot list its tables
// or preview a template has lost the feature, not secured it.
var genReadingRoutes = []string{
"/api/v1/gen/preview/:tableId",
"/api/v1/gen/tabletree",
"/api/v1/db/tables/page",
"/api/v1/db/columns/page",
}
// The endpoints that write are registered where the mode says development and
// nowhere else.
//
// They are in CasbinExclude, so Enforce never runs for them and any account
// that can log in may call them. dev is the shipped default and is where the
// generator is meant to be used; demo keeps them because DemoEvn refuses these
// three by name and saying so is the thing the demo is for. Everything else,
// including the empty mode a process gets when nothing set one, is refused by
// not existing.
func TestGeneratorWritingRoutesExistOnlyWhereTheModeAllowsIt(t *testing.T) {
for _, tc := range []struct {
mode string
expected bool
why string
}{
{"dev", true, "the shipped default, and where the generator is used"},
{"demo", true, "registered so demo mode can refuse them by name"},
{"test", false, "a deployment, however much it is called a test"},
{"prod", false, "a deployment"},
{"", false, "no mode configured is not a reason to trust the caller"},
} {
t.Run("mode="+tc.mode, func(t *testing.T) {
routes := registeredRoutes(t, tc.mode)
for _, writing := range genWritingRoutes {
if got := routes[writing]; got != tc.expected {
t.Errorf("mode %q: %s registered = %v, want %v (%s)",
tc.mode, writing, got, tc.expected, tc.why)
}
}
})
}
}
// The other direction. Refusing too much is as much of a defect as refusing
// too little, and the read-only half of the generator is what a demo shows.
func TestGeneratorReadingRoutesExistInEveryMode(t *testing.T) {
for _, mode := range []string{"dev", "demo", "test", "prod", ""} {
t.Run("mode="+mode, func(t *testing.T) {
routes := registeredRoutes(t, mode)
for _, reading := range genReadingRoutes {
if !routes[reading] {
t.Errorf("mode %q: %s is not registered - the gate took a route that only reads",
mode, reading)
}
}
})
}
}
// GenWriteRoutesEnabled is what cmd/api reads to decide whether to warn at
// start-up. If it and the registration ever disagree, the log says one thing
// and the engine does another, so they are checked against each other rather
// than each against a list.
func TestGenWriteRoutesEnabledAgreesWithWhatWasRegistered(t *testing.T) {
for _, mode := range []string{"dev", "demo", "test", "prod", ""} {
t.Run("mode="+mode, func(t *testing.T) {
routes := registeredRoutes(t, mode)
// registeredRoutes puts the mode back before it returns, so ask
// the predicate under a mode set here - about the same value the
// engine was just built under.
previous := config.ApplicationConfig.Mode
t.Cleanup(func() { config.ApplicationConfig.Mode = previous })
config.ApplicationConfig.Mode = mode
claimed := GenWriteRoutesEnabled()
actual := routes["/api/v1/gen/todb/:tableId"]
if claimed != actual {
t.Errorf("mode %q: GenWriteRoutesEnabled() = %v but the route was registered = %v",
mode, claimed, actual)
}
})
}
}
// The helper restores the mode before it returns, so nothing it was asked
// about leaks into what the caller does next.
//
// Worth a test of its own because the failure is silent: a helper that left
// the mode set would make every assertion after the call read a value the
// caller did not choose, and each of those assertions would still pass for as
// long as the leaked value happened to be the right one.
func TestRegisteredRoutesRestoresTheModeBeforeReturning(t *testing.T) {
const sentinel = "not-a-mode"
previous := config.ApplicationConfig.Mode
t.Cleanup(func() { config.ApplicationConfig.Mode = previous })
config.ApplicationConfig.Mode = sentinel
registeredRoutes(t, "prod")
if got := config.ApplicationConfig.Mode; got != sentinel {
t.Errorf("mode after the helper returned = %q, want %q - it was left set to what "+
"the helper was asked about", got, sentinel)
}
}
// Changing the mode after the routes were built unregisters nothing.
//
// buildRouter has one call site, in run(), and route registration is not on
// any phase or reload callback - so a configuration reload moves
// config.ApplicationConfig.Mode without moving the routes. From that moment
// GenWriteRoutesEnabled answers about a mode the engine was not built under.
//
// That gap is why the start-up warning tells the reader to restart rather than
// only to change the mode. This pins it: if registration ever becomes dynamic,
// this test fails and the message it justifies has to be revisited.
func TestChangingTheModeDoesNotUnregisterWhatWasAlreadyBuilt(t *testing.T) {
built := registeredRoutes(t, "dev")
if !built["/api/v1/gen/todb/:tableId"] {
t.Fatal("built under dev without the writing routes, so this test asserts nothing")
}
previous := config.ApplicationConfig.Mode
t.Cleanup(func() { config.ApplicationConfig.Mode = previous })
config.ApplicationConfig.Mode = "prod"
if GenWriteRoutesEnabled() {
t.Fatal("the predicate still allows prod, so the disagreement below is not the one meant")
}
if !built["/api/v1/gen/todb/:tableId"] {
t.Error("the route left the engine when the mode changed - registration has become " +
"dynamic, and the start-up warning's advice to restart is now wrong")
}
}
+29 -10
View File
@@ -13,16 +13,34 @@ import (
)
func init() {
routerNoCheckRole = append(routerNoCheckRole, registerMonitorRouter)
routerNoCheckRole = append(routerNoCheckRole, RegisterMonitorRouter)
}
// readyTimeout bounds the whole probe. It has to stay under whatever period
// the orchestrator polls on, or a slow dependency turns a readiness check into
// a queue of readiness checks.
// readyTimeout bounds the whole probe. What constrains it is the orchestrator's
// per-check timeout rather than its polling period: Kubernetes allows a probe
// one second by default, so a dependency that answers in 1.2s is recorded as a
// failed check however promptly this handler returns. A manifest that mounts
// this probe has to raise timeoutSeconds above this value, and
// scripts/k8s/deploy.yml does.
const readyTimeout = 2 * time.Second
// HealthPath and ReadyPath are the two probe routes, relative to APIPrefix.
//
// Exported for the same reason as the prefix: the rate limiter has to be told
// to skip them, and it is installed in a package that cannot import this one.
const (
HealthPath = "/health"
ReadyPath = "/ready"
)
// RegisterMonitorRouter mounts the metrics endpoint and the two probes on v1.
//
// Exported so that a test can put the real probes on a server of its own. The
// alternative - a test that re-implements the handler it means to check - is
// how a probe comes to be asserted against a copy of itself.
//
// 无需认证的路由代码
func registerMonitorRouter(v1 *gin.RouterGroup) {
func RegisterMonitorRouter(v1 *gin.RouterGroup) {
v1.GET("/metrics", transfer.Handler(promhttp.Handler()))
// 健康检查(存活)
@@ -31,17 +49,18 @@ func registerMonitorRouter(v1 *gin.RouterGroup) {
// you", and a process whose database is unreachable does not want
// restarting - that turns one outage into a crash loop and throws away the
// connection pool, the cache and every in-flight request along the way.
v1.GET("/health", func(c *gin.Context) {
v1.GET(HealthPath, func(c *gin.Context) {
c.Status(http.StatusOK)
})
// 就绪检查
//
// The answer to "should I send you requests". It fails while a dependency
// is unreachable, and from the moment shutdown begins - which is before
// the server stops accepting, so a load balancer can take this instance
// out of the pool while it can still finish what it has.
v1.GET("/ready", func(c *gin.Context) {
// is unreachable, and from the moment shutdown begins - for as long as
// extend.shutdown.drain says, which is zero unless it is configured. The
// package comment in common/health says what that window is worth, and to
// whom.
v1.GET(ReadyPath, func(c *gin.Context) {
if health.Draining() {
c.JSON(http.StatusServiceUnavailable, gin.H{
"status": "draining",
+9 -2
View File
@@ -10,6 +10,13 @@ var (
routerCheckRole = make([]func(v1 *gin.RouterGroup, authMiddleware *jwt.GinJWTMiddleware), 0)
)
// APIPrefix is the group every route below is registered under.
//
// Exported because the middleware chain in cmd/api has to name two of those
// routes in full - the rate limiter is installed on the engine and must skip
// the probes - and a prefix spelled in two places is a prefix that drifts.
const APIPrefix = "/api/v1"
// initRouter 路由示例
func initRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) *gin.Engine {
@@ -24,7 +31,7 @@ func initRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) *gin.Engine
// noCheckRoleRouter 无需认证的路由示例
func noCheckRoleRouter(r *gin.Engine) {
// 可根据业务需求来设置接口版本
v1 := r.Group("/api/v1")
v1 := r.Group(APIPrefix)
for _, f := range routerNoCheckRole {
f(v1)
@@ -34,7 +41,7 @@ func noCheckRoleRouter(r *gin.Engine) {
// checkRoleRouter 需要认证的路由示例
func checkRoleRouter(r *gin.Engine, authMiddleware *jwt.GinJWTMiddleware) {
// 可根据业务需求来设置接口版本
v1 := r.Group("/api/v1")
v1 := r.Group(APIPrefix)
for _, f := range routerCheckRole {
f(v1, authMiddleware)
+38
View File
@@ -0,0 +1,38 @@
package api
import (
"testing"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
)
// The warning fires exactly where the generator's writing endpoints are served
// and nothing else refuses them.
//
// dev is the case the warning exists for: it is the shipped default, so it is
// the mode a deployment that changed nothing is running in. demo serves the
// routes too, but DemoEvn refuses all three by name, so warning there would
// describe an exposure that is not there.
func TestGeneratorWriteRoutesWarningFiresWhereTheExposureIs(t *testing.T) {
for _, tc := range []struct {
mode string
want bool
why string
}{
{"dev", true, "shipped default, endpoints served and not refused"},
{"demo", false, "served, but DemoEvn refuses all three"},
{"test", false, "not served"},
{"prod", false, "not served"},
{"", false, "not served"},
} {
t.Run("mode="+tc.mode, func(t *testing.T) {
previous := config.ApplicationConfig.Mode
t.Cleanup(func() { config.ApplicationConfig.Mode = previous })
config.ApplicationConfig.Mode = tc.mode
if got := generatorWriteRoutesNeedWarning(); got != tc.want {
t.Errorf("mode %q: warning = %v, want %v (%s)", tc.mode, got, tc.want, tc.why)
}
})
}
}
+117
View File
@@ -0,0 +1,117 @@
package api
import (
"context"
"fmt"
"sort"
"strings"
"github.com/go-admin-team/go-admin-core/v2/sdk"
"gorm.io/gorm"
"go-admin/cmd/migrate/migration"
"go-admin/common/health"
commonmodels "go-admin/common/models"
)
// schemaCheckName is what a failing schema reports itself as in /ready's body.
const schemaCheckName = "schema"
// registerSchemaCheck adds the pending-migration check to readiness.
//
// Readiness rather than a refusal to start, and rather than a log line alone.
// The two probes answer different questions: liveness is "restart me", and a
// process whose database is on the wrong schema comes back to the same schema,
// so restarting is not the answer. Readiness is "send me requests", and with a
// schema the binary does not match the answer is no.
//
// Issue #919 is what the absence of this looked like: the process started,
// both probes passed, and the first sign of trouble was a login failing with a
// driver-level encoding error. Refusing to start would have been the wrong fix
// - a process that exits tells an operator less than one that runs and says
// why, and under an orchestrator it crash-loops - while a log line alone is
// not something an orchestrator can act on.
func registerSchemaCheck() {
health.Register(schemaCheckName, schemaCheck)
}
// schemaCheck fails while any tenant database is behind the migrations this
// binary registers.
//
// Any one of them, rather than only the tenant being served: migrations are
// applied to every database in one run, so one database behind means that run
// did not finish. Serving the rest would let a half-applied deploy look like a
// partial success.
//
// Evaluated per request rather than decided at start-up, so that running
// migrate clears it without a restart.
func schemaCheck(ctx context.Context) error {
registered := migration.RegisteredVersions()
if len(registered) == 0 {
// Nothing registered means nothing can be pending, which is the honest
// answer for a tree with no migrations. It is also what a broken build
// would produce - the registry is filled by init() in packages the
// binary has to link - so cmd/api's dependency test asserts the real
// binary links them.
return nil
}
behind := make([]string, 0, 2)
for name, db := range sdk.Runtime.GetAllDb() {
applied, err := appliedVersions(ctx, db)
if err != nil {
return fmt.Errorf("reading applied migrations for %q: %w", name, err)
}
if pending := pendingVersions(registered, applied); len(pending) > 0 {
behind = append(behind, fmt.Sprintf("%s is %d behind, first pending %s",
name, len(pending), pending[0]))
}
}
if len(behind) == 0 {
return nil
}
sort.Strings(behind)
return fmt.Errorf("%s; run `go-admin migrate -c <config>` and see `go-admin migrate status`",
strings.Join(behind, "; "))
}
// appliedVersions reads what sys_migration records for one database.
//
// A missing table is not an error: a database that has never been migrated has
// applied nothing, which is exactly what the caller needs to hear, and is the
// state a first deploy is in.
func appliedVersions(ctx context.Context, db *gorm.DB) (map[string]bool, error) {
if db == nil {
return nil, fmt.Errorf("no database")
}
db = db.WithContext(ctx)
if !db.Migrator().HasTable(&commonmodels.Migration{}) {
return map[string]bool{}, nil
}
var rows []commonmodels.Migration
if err := db.Select("version").Find(&rows).Error; err != nil {
return nil, err
}
out := make(map[string]bool, len(rows))
for _, r := range rows {
out[r.Version] = true
}
return out, nil
}
// pendingVersions returns the registered versions applied does not contain.
//
// Split out and taking both sides as arguments because the registry is
// process-wide and filled by init() in packages cmd/api does not import: a
// test in this package cannot arrange it, so the arranging part is the part
// that is not tested here.
func pendingVersions(registered []string, applied map[string]bool) []string {
out := make([]string, 0)
for _, v := range registered {
if !applied[v] {
out = append(out, v)
}
}
sort.Strings(out)
return out
}
+124
View File
@@ -0,0 +1,124 @@
package api
import (
"context"
"os/exec"
"strings"
"testing"
"github.com/glebarez/sqlite"
"gorm.io/gorm"
commonmodels "go-admin/common/models"
)
func TestPendingVersionsReportsOnlyWhatIsNotApplied(t *testing.T) {
registered := []string{"1000_a", "2000_b", "3000_c"}
applied := map[string]bool{"1000_a": true, "3000_c": true}
got := pendingVersions(registered, applied)
if len(got) != 1 || got[0] != "2000_b" {
t.Errorf("pending = %v, want [2000_b]", got)
}
}
func TestPendingVersionsIsEmptyWhenTheDatabaseIsCurrent(t *testing.T) {
registered := []string{"1000_a", "2000_b"}
applied := map[string]bool{"1000_a": true, "2000_b": true}
if got := pendingVersions(registered, applied); len(got) != 0 {
t.Errorf("pending = %v, want none", got)
}
}
// A row recorded that this binary no longer registers is not pending. It is
// the orphan `migrate status` already reports, and readiness has nothing to
// say about it: the schema is ahead, not behind, and requests will be served
// correctly.
func TestPendingVersionsIgnoresAppliedRowsNothingRegisters(t *testing.T) {
registered := []string{"1000_a"}
applied := map[string]bool{"1000_a": true, "9999_gone": true}
if got := pendingVersions(registered, applied); len(got) != 0 {
t.Errorf("pending = %v, want none - an orphaned row is not a pending migration", got)
}
}
func memoryDB(t *testing.T) *gorm.DB {
t.Helper()
db, err := gorm.Open(sqlite.Open("file::memory:?cache=shared"), &gorm.Config{})
if err != nil {
t.Fatalf("open: %v", err)
}
t.Cleanup(func() { db.Migrator().DropTable(&commonmodels.Migration{}) })
return db
}
// A first deploy has no sys_migration table. That is "nothing applied", not an
// error: reporting it as one would make the check fail for a reason the
// operator cannot act on, on the one deployment where every migration really
// is pending.
func TestAppliedVersionsTreatsAMissingTableAsNothingApplied(t *testing.T) {
db := memoryDB(t)
db.Migrator().DropTable(&commonmodels.Migration{})
got, err := appliedVersions(context.Background(), db)
if err != nil {
t.Fatalf("appliedVersions: %v", err)
}
if len(got) != 0 {
t.Errorf("applied = %v, want empty", got)
}
}
func TestAppliedVersionsReadsWhatTheTableHolds(t *testing.T) {
db := memoryDB(t)
if err := db.AutoMigrate(&commonmodels.Migration{}); err != nil {
t.Fatalf("automigrate: %v", err)
}
db.Create(&commonmodels.Migration{Version: "1000_a"})
db.Create(&commonmodels.Migration{Version: "2000_b"})
got, err := appliedVersions(context.Background(), db)
if err != nil {
t.Fatalf("appliedVersions: %v", err)
}
if !got["1000_a"] || !got["2000_b"] || len(got) != 2 {
t.Errorf("applied = %v, want the two rows written", got)
}
}
// The check is only worth anything if the registry it reads is populated in
// the binary that serves requests, and it is filled by init() in packages
// cmd/api does not import - cmd/migrate blank-imports them, and cmd wires both
// subcommands into one binary.
//
// This cannot be asserted from an ordinary test: importing the version package
// to look at the registry would put it in the test binary's dependency graph
// and pass whatever the real binary links. So ask the build instead.
//
// Without this, dropping those blank imports leaves a check that reports
// "nothing pending" for every database forever, and every test above still
// passes.
func TestTheServingBinaryLinksTheMigrationRegistry(t *testing.T) {
out, err := exec.Command("go", "list", "-deps", "go-admin").Output()
if err != nil {
t.Skipf("go list unavailable: %v", err)
}
deps := string(out)
const versions = "go-admin/cmd/migrate/migration/version"
if !strings.Contains(deps, versions+"\n") {
t.Errorf("the main package does not link %s, so the schema check would "+
"read an empty registry and report every database as current", versions)
}
// Negative control: a package the binary genuinely must not link, so that a
// `deps` that somehow contained everything would fail here rather than pass
// the assertion above for the wrong reason.
const notLinked = "go-admin/tools/checksilent"
if strings.Contains(deps, notLinked+"\n") {
t.Errorf("%s is in the binary's dependency closure, so this test cannot "+
"tell a real link from a query that matches anything", notLinked)
}
}
+247 -29
View File
@@ -28,6 +28,7 @@ import (
"go-admin/app/admin/models"
"go-admin/app/admin/router"
"go-admin/app/jobs"
otherrouter "go-admin/app/other/router"
"go-admin/common/database"
"go-admin/common/global"
"go-admin/common/health"
@@ -81,6 +82,11 @@ func setup() {
// can call the API, which is only true once the socket is accepting.
sdk.Runtime.SetPhase(runtime.AfterListen, startCronJobs)
// Registered before the configuration is read, because it registers a
// callback rather than reading anything: the check runs per request and
// asks the databases that exist then.
registerSchemaCheck()
//1. 读取配置
bootstrap.SetupConfig(
file.NewSource(file.WithPath(configYml)),
@@ -158,10 +164,25 @@ func attachConsumersOnce(gen uint64, q corestorage.AdapterQueue) {
}
func run() error {
// Resolved first, and used both for the line it prints and for the
// shutdown that spends it. Reading the configuration again at signal time
// would let the two disagree, and the sum that gets printed is the whole
// point of printing it.
//
// Refused rather than corrected, and refused before anything is built: a
// budget that cannot be spent as written is a configuration error, and the
// moment to say so is while nothing depends on this process yet.
seconds, err := ext.ExtConfig.Shutdown.Budget()
if err != nil {
return err
}
reportShutdownBudget(seconds)
if config.ApplicationConfig.Mode == pkg.ModeProd.String() {
gin.SetMode(gin.ReleaseMode)
}
buildRouter()
reportGeneratorWriteRoutes()
srv := &http.Server{
Addr: fmt.Sprintf("%s:%d", config.ApplicationConfig.Host, config.ApplicationConfig.Port),
@@ -209,50 +230,219 @@ func run() error {
fmt.Printf("%s Enter Control + C Shutdown Server \r\n", pkg.GetCurrentTimeStr())
<-quit
// Restored here, not deferred: from this point a second signal must reach
// the default handler, so a shutdown that hangs can still be interrupted.
disarmStopSignals()
serverErr, cleanupErr := gracefulShutdown(srv, quit, disarmStopSignals, budgetFrom(seconds))
if serverErr != nil {
// Not log.Fatal: that is an unconditional os.Exit(1), and Shutdown
// reports an error exactly when connections were still in flight -
// which is when the cleanup that ran after it mattered most.
log.Error("Server Shutdown: ", serverErr)
}
if cleanupErr != nil {
log.Error("Cleanup: ", cleanupErr)
}
return nil
}
// budget is the three waits a shutdown spends, in the order it spends them.
type budget struct {
drain time.Duration
server time.Duration
cleanup time.Duration
}
// budgetFrom turns the resolved seconds into the durations the sequence waits
// on.
func budgetFrom(s ext.ShutdownBudget) budget {
return budget{
drain: time.Duration(s.Drain) * time.Second,
server: time.Duration(s.Server) * time.Second,
cleanup: time.Duration(s.Cleanup) * time.Second,
}
}
// defaultBudget is what a process with no extend.shutdown section spends.
func defaultBudget() budget {
return budget{drain: drainTimeout, server: shutdownTimeout, cleanup: cleanupTimeout}
}
// gracefulShutdown takes the process down in the order that gives something
// else a chance to notice first.
//
// The whole order lives here, and run() is not the only caller: the signal
// tests run this function rather than reproducing it. A test that reproduces a
// sequence asserts against its own copy and stays green while the sequence it
// was written for regresses.
//
// The caller has already taken the first signal off quit. quit is handed on
// because a second signal during the drain window ends the window early -
// somebody sending another kill wants this over with sooner - and because
// until the window is over that signal must not reach the default handler and
// kill the process outright.
//
// disarm is therefore called at the end of the window rather than on the first
// signal. After it, a second signal is handled by the default disposition
// again, which is the only way out of a Shutdown or a cleanup callback that
// never returns. Restoring it any earlier would put every ordinary shutdown
// inside that escape hatch for the whole length of the drain, where before
// this window existed only a hung callback could reach it.
//
// The two waits' errors are returned separately rather than logged: they fail
// for different reasons, and the caller decides what each is worth.
func gracefulShutdown(srv *http.Server, quit <-chan os.Signal, disarm func(), b budget) (serverErr, cleanupErr error) {
// Said before anything is taken apart. A configuration reload arriving in
// this window would otherwise re-run AfterResource - rebuilding the pool
// and the queue adapter, and re-registering consumers - on top of cleanup
// that has already run.
sdk.Runtime.BeginShutdown()
// Readiness fails from here, which is before the server stops accepting.
// The order is the whole point: a load balancer that is told "not ready"
// while this instance can still finish what it has in flight takes it out
// of the pool without dropping anything. Reversed, the connections are cut
// first and the health check reports it afterwards.
// That order is necessary and not sufficient: with nothing between this
// line and the listener closing, the two are microseconds apart and a
// poller on a multi-second interval sees the refused connection instead of
// the 503. The window below is what turns the order into something
// observable - extend.shutdown.drain, which is zero unless it is
// configured.
health.BeginDraining()
log.Info("Shutdown Server ... ")
if err := shutdownServer(srv, shutdownTimeout); err != nil {
// Not log.Fatal: that is an unconditional os.Exit(1), and Shutdown
// reports an error exactly when connections were still in flight -
// which is when the cleanup that follows matters most.
log.Error("Server Shutdown: ", err)
}
// Keep-alive off for the same window, and for the same reason. The server
// keeps connections alive while !disableKeepAlives && !shuttingDown(), and
// shuttingDown() is only set by Shutdown itself - so without this line
// every pooled connection stays open for the whole drain and is cut at the
// end of it anyway, which is the cost of the window without its benefit.
// This is the switch Shutdown flips, moved earlier by the window's length:
// answers now carry Connection: close, and the idle connections a balancer
// is holding are closed at once rather than when it next tries to use one.
srv.SetKeepAlivesEnabled(false)
// Runs whether or not the line above reported an error, for that reason.
if err := runShutdownHooks(cleanupTimeout); err != nil {
log.Error("Cleanup: ", err)
}
drain(quit, b.drain)
// Restored here, not on the first signal: from this point a second signal
// must reach the default handler, so a shutdown that hangs can still be
// interrupted.
disarm()
log.Info("Shutdown Server ... ")
serverErr = shutdownServer(srv, b.server)
// Runs whether or not the wait above failed, and deliberately so: Shutdown
// reports an error exactly when connections were still in flight, which is
// when there is most left to clean up after.
cleanupErr = runShutdownHooks(b.cleanup)
log.Info("Server exiting")
return nil
return serverErr, cleanupErr
}
// shutdownTimeout is how long Shutdown waits for in-flight requests, and
// cleanupTimeout how long the BeforeExit callbacks get after it.
// drain keeps serving for d, or until another stop signal arrives.
//
// They are consumed one after the other, so the two together are what has to
// stay inside the orchestrator's grace period: `docker stop` allows 10s by
// default before it sends SIGKILL, and 5+3 leaves room for the process to
// finish returning. Raising either without lowering the other buys nothing -
// the budget that runs out is the orchestrator's.
// Requests are answered normally throughout. Refusing them would move the
// outage earlier rather than avoid it - the point of the window is that this
// instance is still able to work while whoever routes to it stops routing.
func drain(quit <-chan os.Signal, d time.Duration) {
if d <= 0 {
return
}
log.Infof("Draining for %s: still serving, /ready answers 503 from here", d)
timer := time.NewTimer(d)
defer timer.Stop()
select {
case <-quit:
log.Info("Second signal during the drain window, closing the listener now")
case <-timer.C:
}
}
// Reference stop grace periods, printed when nothing was configured to compare
// against. They are three times apart, which is why the check below needs a
// configured value rather than a constant of its own: a budget that overruns
// under one of them fits comfortably under the other.
const (
shutdownTimeout = 5 * time.Second
cleanupTimeout = 3 * time.Second
dockerStopGraceSeconds = 10
kubernetesGraceSeconds = 30
)
// reportGeneratorWriteRoutes says whether this process serves the code
// generator's writing endpoints, and to whom.
//
// The endpoints are gated on the mode, and the shipped configuration says dev -
// so the deployment most likely to be exposed is the one that changed nothing,
// and the one least likely to go looking. Silence there would leave the gate
// technically correct and practically useless.
//
// Nothing is said in demo mode. The routes are registered, but DemoEvn refuses
// all three by name, so a warning would describe an exposure that is not there.
func reportGeneratorWriteRoutes() {
if !generatorWriteRoutesNeedWarning() {
return
}
log.Warnf("the code generator's writing endpoints are served in mode %q: "+
"/api/v1/gen/{toproject,apitofile,todb} write Go and Vue source onto this host and rows "+
"into this database, and they are in CasbinExclude, so any account that can log in may "+
"call them. Set application.mode to prod or test on anything that is not a workstation, "+
"then restart: these routes were registered at start-up and a configuration reload does "+
"not rebuild them.",
config.ApplicationConfig.Mode)
}
// generatorWriteRoutesNeedWarning reports whether there is an exposure to warn
// about: the endpoints are served, and nothing else is refusing them.
//
// Split from the logging so the decision can be tested. A warning nobody can
// make fire is indistinguishable from no warning at all, and this one exists
// precisely for the case nobody is looking at.
func generatorWriteRoutesNeedWarning() bool {
return otherrouter.GenWriteRoutesEnabled() && config.ApplicationConfig.Mode != "demo"
}
// reportShutdownBudget states what a shutdown will spend and whether it fits.
//
// The sum is taken from the resolved values, not from the configuration file:
// a field left out of extend.shutdown still costs its default, so adding up
// what was written down understates the budget by exactly the fields nobody
// wrote.
func reportShutdownBudget(s ext.ShutdownBudget) {
log.Infof("shutdown budget: drain %ds + server %ds + cleanup %ds = %ds",
s.Drain, s.Server, s.Cleanup, s.Total())
if s.Grace <= 0 {
log.Infof("shutdown budget: extend.shutdown.grace is not set, so nothing is compared against it - "+
"for reference `docker stop` allows %ds and Kubernetes terminationGracePeriodSeconds defaults to %ds",
dockerStopGraceSeconds, kubernetesGraceSeconds)
return
}
if over := s.Overrun(); over > 0 {
// A minimum, not a target. This is somebody else's deployment under
// constraints this process cannot see, so the honest thing to state is
// how much is missing - the repository's own files are where there is
// standing to ask for headroom on top, and checksilent does that.
log.Warnf("shutdown budget of %ds does not fit inside the %ds of extend.shutdown.grace: "+
"SIGKILL arrives while the cleanup callbacks are still running, and the work they "+
"were about to finish is lost. It needs at least %ds more, or %ds less budget.",
s.Total(), s.Grace, over, over)
return
}
log.Infof("shutdown budget of %ds fits inside the %ds of extend.shutdown.grace", s.Total(), s.Grace)
}
// The budgets a shutdown spends when extend.shutdown configures nothing:
// drainTimeout keeps the process serving after the stop signal, then
// shutdownTimeout waits for in-flight requests, then cleanupTimeout is what
// the BeforeExit callbacks get.
//
// The seconds come from config, which is where an absent field falls back, so
// the default is one number rather than two that can drift apart.
//
// They are consumed one after the other, so their sum is what has to stay
// inside the orchestrator's grace period: `docker stop` allows 10s by default
// before it sends SIGKILL, and 0+5+3 leaves room for the process to finish
// returning. Raising one without lowering another buys nothing - the budget
// that runs out is the orchestrator's, and reportShutdownBudget is what says
// so at start-up.
var (
drainTimeout = time.Duration(ext.DefaultDrainSeconds) * time.Second
shutdownTimeout = time.Duration(ext.DefaultServerSeconds) * time.Second
cleanupTimeout = time.Duration(ext.DefaultCleanupSeconds) * time.Second
)
// armStopSignals registers for the stop signals and returns the channel they
@@ -420,10 +610,38 @@ func initRouter() {
r.Use(handler.TlsHandler())
}
//r.Use(middleware.Metrics())
r.Use(common.Sentinel()).
r.Use(exemptProbes(common.Sentinel())).
Use(common.RequestId(pkg.TrafficKey)).
Use(api.SetRequestLogger)
common.InitMiddleware(r)
}
// probePaths are the two routes the rate limiter must not answer for.
var probePaths = map[string]bool{
otherrouter.APIPrefix + otherrouter.HealthPath: true,
otherrouter.APIPrefix + otherrouter.ReadyPath: true,
}
// exemptProbes wraps a middleware so the health and readiness routes skip it.
//
// The limiter is installed on the engine and the probes are routes like any
// other, so above the threshold they are answered with 429 as well. A liveness
// probe that collects 429s fails its threshold and the container is restarted,
// which takes capacity out of a deployment that is already short of it and
// pushes the rest closer to the threshold - the limiter working exactly as
// intended is what causes it. It is the argument common/health makes about
// restarting a process whose database is unreachable, applied to load.
//
// Wrapping rather than teaching the limiter about these paths: the limiter
// lives under common/, which may not import the package that registers them.
func exemptProbes(h gin.HandlerFunc) gin.HandlerFunc {
return func(c *gin.Context) {
if probePaths[c.FullPath()] {
c.Next()
return
}
h(c)
}
}
+139
View File
@@ -0,0 +1,139 @@
package api
import (
"net/http"
"net/http/httptest"
"testing"
"time"
"github.com/gin-gonic/gin"
otherrouter "go-admin/app/other/router"
"go-admin/common/health"
ext "go-admin/config"
)
// The seconds in the configuration and the durations the sequence waits on are
// two spellings of one budget, and only one of them is printed at start-up.
func TestBudgetFromSeconds(t *testing.T) {
got := budgetFrom(ext.ShutdownBudget{Drain: 10, Server: 5, Cleanup: 3})
want := budget{
drain: 10 * time.Second,
server: 5 * time.Second,
cleanup: 3 * time.Second,
}
if got != want {
t.Errorf("budgetFrom = %+v, want %+v", got, want)
}
}
// The package variables and config.Default*Seconds have to say the same thing.
// They are the same default written twice - once as durations for the shutdown
// and once as seconds for the fallback - and a deployment that configures
// nothing is entitled to one answer, not two.
func TestDefaultBudgetIsTheConfiguredFallback(t *testing.T) {
unconfigured, err := ext.Shutdown{}.Budget()
if err != nil {
t.Fatalf("the empty section did not resolve: %v", err)
}
if got, want := defaultBudget(), budgetFrom(unconfigured); got != want {
t.Errorf("defaultBudget = %+v, want the unconfigured budget %+v", got, want)
}
}
// The rate limiter must not answer for the probes.
//
// It is installed on the engine, so without this the probes are limited like
// any other route and answer 429 above the threshold. A liveness probe that
// collects 429s fails its threshold and the container is restarted - taking
// capacity out of a deployment that is already short of it and pushing the
// rest closer to the threshold. The limiter working exactly as designed is
// what would cause it.
//
// The stand-in rejects everything rather than being a real limiter: what is
// under test is which requests reach it, and a real one would need the traffic
// to cross a threshold before it said anything.
func TestTheProbesSkipTheRateLimiter(t *testing.T) {
gin.SetMode(gin.TestMode)
var reached []string
r := gin.New()
r.Use(exemptProbes(func(c *gin.Context) {
reached = append(reached, c.FullPath())
c.AbortWithStatus(http.StatusTooManyRequests)
}))
v1 := r.Group(otherrouter.APIPrefix)
otherrouter.RegisterMonitorRouter(v1)
v1.GET("/business", func(c *gin.Context) { c.Status(http.StatusOK) })
for _, tc := range []struct {
path string
limited bool
}{
{otherrouter.APIPrefix + otherrouter.HealthPath, false},
{otherrouter.APIPrefix + otherrouter.ReadyPath, false},
// Not a probe, and deliberately not exempt: the exemption is for the
// two routes an orchestrator acts on, not for everything under
// /api/v1 that happens to be unauthenticated.
{otherrouter.APIPrefix + "/metrics", true},
{otherrouter.APIPrefix + "/business", true},
} {
t.Run(tc.path, func(t *testing.T) {
w := httptest.NewRecorder()
r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, tc.path, nil))
if tc.limited {
if w.Code != http.StatusTooManyRequests {
t.Errorf("answered %d, want the middleware's 429 - it was skipped for a route that is not a probe", w.Code)
}
return
}
if w.Code == http.StatusTooManyRequests {
t.Errorf("answered 429; a probe that can be rate-limited gets the container restarted under load")
}
})
}
// Said separately, because a probe could also answer 429 by itself: what
// has to be true is that the middleware never saw the request.
for _, p := range reached {
if probePaths[p] {
t.Errorf("the middleware ran for %s", p)
}
}
}
// /health has to stay 200 while draining, and it is the assertion most easily
// lost by accident: making the liveness probe follow the readiness flag reads
// like tidying up, and it turns every rolling restart into a kubelet-issued
// kill part-way through the drain.
func TestHealthStaysUpWhileDraining(t *testing.T) {
gin.SetMode(gin.TestMode)
r := gin.New()
v1 := r.Group(otherrouter.APIPrefix)
otherrouter.RegisterMonitorRouter(v1)
ask := func(path string) int {
w := httptest.NewRecorder()
r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, path, nil))
return w.Code
}
if got := ask(otherrouter.APIPrefix + otherrouter.HealthPath); got != http.StatusOK {
t.Fatalf("/health answered %d before draining, want 200", got)
}
// Process-wide and one-way - nothing clears it - so this is the last thing
// in this package that may run in-process and care. Everything else that
// exercises draining does so in a child process of its own.
health.BeginDraining()
if got := ask(otherrouter.APIPrefix + otherrouter.HealthPath); got != http.StatusOK {
t.Errorf("/health answered %d while draining, want 200 - liveness is "+
"\"should I restart you\", and the answer during a drain is no", got)
}
if got := ask(otherrouter.APIPrefix + otherrouter.ReadyPath); got != http.StatusServiceUnavailable {
t.Errorf("/ready answered %d while draining, want 503", got)
}
}
+426 -56
View File
@@ -3,44 +3,77 @@ package api
import (
"context"
"fmt"
"io"
"net"
"net/http"
"os"
"os/exec"
"strconv"
"strings"
"sync"
"syscall"
"testing"
"time"
"github.com/gin-gonic/gin"
"github.com/go-admin-team/go-admin-core/v2/sdk"
otherrouter "go-admin/app/other/router"
)
// The signal path cannot be exercised in-process: delivering a signal to the
// test binary would race with the test framework, and the disposition changes
// are global. So the test re-executes itself as a child, and the child runs the
// same armStopSignals / shutdownServer the server does.
// are global. So the test re-executes itself as a child, and the child runs
// gracefulShutdown - the same function run() runs, not a second copy of the
// sequence. A test that reproduces the sequence asserts against its own copy:
// move BeginDraining after the drain window and the process regresses while
// the test stays green, which is the failure mode this file exists to avoid.
//
// The child deliberately serves an empty http.Server rather than the real one:
// this repository's CI has no database (.github/workflows/go.yml runs neither
// MySQL nor a sqlite-tagged build), and none of what is under test needs one.
// The child serves the real probe routes on an http.Server of its own rather
// than the configured one: this repository's CI has no database
// (.github/workflows/go.yml runs neither MySQL nor a sqlite-tagged build), and
// none of what is under test needs one. /ready answers 503 either way - with
// no database its checks fail - so the assertions below are on the draining
// answer specifically, not on the status code alone.
const (
childEnv = "GO_ADMIN_SIGNAL_CHILD"
childStuckEnv = "GO_ADMIN_SIGNAL_CHILD_STUCK"
childHangConn = "GO_ADMIN_SIGNAL_CHILD_HANGCONN"
childSlowCleanup = "GO_ADMIN_SIGNAL_CHILD_SLOWCLEANUP"
childDrainMS = "GO_ADMIN_SIGNAL_CHILD_DRAIN_MS"
markerAddr = "CHILD-ADDR"
markerReady = "CHILD-READY"
markerSignal = "CHILD-SIGNAL"
markerShutdown = "CHILD-SHUTDOWN-OK"
markerCleanup = "CHILD-CLEANUP-RAN"
markerTook = "CHILD-TOOK-NS"
markerExiting = "CHILD-EXITING"
)
// childPingRoute is an ordinary route, registered beside the probes so the
// window can be checked for what it promises: requests arriving inside it are
// served, not refused. Refusing them would move the outage earlier instead of
// avoiding it.
const childPingRoute = "/signal-test-ping"
var (
readyPath = otherrouter.APIPrefix + otherrouter.ReadyPath
healthPath = otherrouter.APIPrefix + otherrouter.HealthPath
pingPath = otherrouter.APIPrefix + childPingRoute
)
// TestSignalChild is the child process. It is skipped in a normal run.
func TestSignalChild(t *testing.T) {
if os.Getenv(childEnv) != "1" {
t.Skip("child process entry point")
}
gin.SetMode(gin.TestMode)
engine := gin.New()
v1 := engine.Group(otherrouter.APIPrefix)
otherrouter.RegisterMonitorRouter(v1)
v1.GET(childPingRoute, func(c *gin.Context) { c.String(http.StatusOK, "pong") })
ln, err := net.Listen("tcp", "127.0.0.1:0")
if err != nil {
fmt.Println("listen:", err)
@@ -52,7 +85,7 @@ func TestSignalChild(t *testing.T) {
// finds nothing to wait for and returns immediately.
accepted := make(chan struct{}, 1)
srv := &http.Server{
Handler: http.NewServeMux(),
Handler: engine,
ConnState: func(_ net.Conn, state http.ConnState) {
if state == http.StateNew {
select {
@@ -64,22 +97,43 @@ func TestSignalChild(t *testing.T) {
}
go func() { _ = srv.Serve(ln) }()
// The budget the child spends. Nothing here calls bootstrap.SetupConfig, so
// with no environment set this is the budget of a deployment that
// configures no extend.shutdown section at all.
b := defaultBudget()
if ms := os.Getenv(childDrainMS); ms != "" {
n, err := strconv.Atoi(ms)
if err != nil {
fmt.Println("drain:", err)
os.Exit(4)
}
b.drain = time.Duration(n) * time.Millisecond
}
// A BeforeExit callback, registered the way a module would. What the tests
// below care about is whether it runs at all - after a Shutdown that
// failed, and after its own budget has been spent.
cleanupBudget := cleanupTimeout
sdk.Runtime.SetShutdown(func(ctx context.Context) {
if os.Getenv(childSlowCleanup) == "1" {
// Outlasts the budget on purpose, and does not consult ctx -
// which is the case the contract is explicit about: what the
// context bounds is the wait, not the work.
switch {
case os.Getenv(childStuckEnv) == "1":
// Stands in for a cleanup hook that never finishes. The point of
// restoring the signal disposition after the drain window is that
// a second signal still reaches the default handler and kills this.
time.Sleep(2 * time.Minute)
case os.Getenv(childSlowCleanup) == "1":
// Outlasts the budget on purpose, and does not consult ctx - which
// is the case the contract is explicit about: what the context
// bounds is the wait, not the work.
time.Sleep(2 * time.Second)
}
fmt.Println(markerCleanup)
os.Stdout.Sync()
_ = os.Stdout.Sync()
})
if os.Getenv(childSlowCleanup) == "1" {
cleanupBudget = 300 * time.Millisecond
switch {
case os.Getenv(childStuckEnv) == "1":
b.cleanup = 2 * time.Minute
case os.Getenv(childSlowCleanup) == "1":
b.cleanup = 300 * time.Millisecond
}
// Arm before announcing readiness. Doing it the other way round leaves a
@@ -89,22 +143,14 @@ func TestSignalChild(t *testing.T) {
// accident.
quit, disarm := armStopSignals()
fmt.Println(markerAddr, ln.Addr().String())
fmt.Println(markerReady)
os.Stdout.Sync()
_ = os.Stdout.Sync()
sig := <-quit
disarm()
fmt.Println(markerSignal, sig)
os.Stdout.Sync()
_ = os.Stdout.Sync()
if os.Getenv(childStuckEnv) == "1" {
// Stand in for a cleanup hook that never finishes. The point of
// restoring the signal disposition is that a second signal still
// reaches the default handler and kills this.
time.Sleep(2 * time.Minute)
}
timeout := shutdownTimeout
if os.Getenv(childHangConn) == "1" {
// Dialled here, not at start-up. net/http stops counting a StateNew
// connection against Shutdown once it is more than five seconds old,
@@ -130,27 +176,29 @@ func TestSignalChild(t *testing.T) {
// only treats a StateNew connection as idle once it is more than five
// seconds old. A short budget makes the timeout deterministic without
// waiting out the real one.
timeout = 300 * time.Millisecond
b.server = 300 * time.Millisecond
}
sdk.Runtime.BeginShutdown()
started := time.Now()
serverErr, cleanupErr := gracefulShutdown(srv, quit, disarm, b)
spent := time.Since(started)
if err := shutdownServer(srv, timeout); err != nil {
if serverErr != nil {
// Deliberately not fatal, and deliberately not a bare return: the
// point is that whatever follows still runs.
fmt.Println("shutdown error:", err)
fmt.Println("shutdown error:", serverErr)
} else {
fmt.Println(markerShutdown)
}
if err := runShutdownHooks(cleanupBudget); err != nil {
fmt.Println("cleanup error:", err)
if cleanupErr != nil {
fmt.Println("cleanup error:", cleanupErr)
}
fmt.Println(markerTook, spent.Nanoseconds())
fmt.Println(markerExiting)
os.Stdout.Sync()
_ = os.Stdout.Sync()
}
func startChild(t *testing.T, stuck bool, extraEnv ...string) (*exec.Cmd, *os.File, chan string) {
func startChild(t *testing.T, stuck bool, extraEnv ...string) (*exec.Cmd, chan string) {
t.Helper()
r, w, err := os.Pipe()
@@ -170,7 +218,7 @@ func startChild(t *testing.T, stuck bool, extraEnv ...string) (*exec.Cmd, *os.Fi
}
_ = w.Close()
lines := make(chan string, 64)
lines := make(chan string, 256)
go func() {
defer close(lines)
buf := make([]byte, 4096)
@@ -204,12 +252,13 @@ func startChild(t *testing.T, stuck bool, extraEnv ...string) (*exec.Cmd, *os.Fi
_, _ = cmd.Process.Wait()
_ = r.Close()
})
return cmd, r, lines
return cmd, lines
}
// await drains lines until one contains want, or the deadline passes. It
// returns everything it saw, so a failure says what the child actually did.
func await(t *testing.T, lines chan string, want string, d time.Duration) []string {
// returns everything it saw, so a failure says what the child actually did,
// and the matching line, so a marker can carry a value.
func await(t *testing.T, lines chan string, want string, d time.Duration) ([]string, string) {
t.Helper()
var seen []string
deadline := time.After(d)
@@ -221,7 +270,7 @@ func await(t *testing.T, lines chan string, want string, d time.Duration) []stri
}
seen = append(seen, l)
if strings.Contains(l, want) {
return seen
return seen, l
}
case <-deadline:
t.Fatalf("timed out waiting for %q; saw:\n%s", want, strings.Join(seen, "\n"))
@@ -229,6 +278,134 @@ func await(t *testing.T, lines chan string, want string, d time.Duration) []stri
}
}
// childAddr waits for the address the child is listening on.
func childAddr(t *testing.T, lines chan string) string {
t.Helper()
_, line := await(t, lines, markerAddr, 30*time.Second)
fields := strings.Fields(line)
return fields[len(fields)-1]
}
// took reads the nanoseconds gracefulShutdown spent, as the child measured
// them. Measured inside the child on purpose: the parent's own clock includes
// process scheduling, which is the noise the tightest assertion here cannot
// afford.
func took(t *testing.T, lines chan string, d time.Duration) time.Duration {
t.Helper()
_, line := await(t, lines, markerTook, d)
fields := strings.Fields(line)
ns, err := strconv.ParseInt(fields[len(fields)-1], 10, 64)
if err != nil {
t.Fatalf("unreadable %s line %q: %v", markerTook, line, err)
}
return time.Duration(ns)
}
// sample is one answer, or the refusal that replaced it.
type sample struct {
at time.Time
path string
// status is zero when the connection could not be made at all, which is
// what a closed listener looks like from outside.
status int
draining bool
// willClose is what the server answered about the connection: the header
// it sends is Connection: close, which the transport consumes and reports
// here rather than leaving in Response.Header.
willClose bool
}
// probe asks once, on a connection of its own.
//
// A new transport per request, because a connection opened before the signal
// can still be served after the listener is closed: reusing one would let this
// test pass against a shutdown that had already broken the listener. Keep-alive
// is left enabled so the server's own Connection: close is observable - a
// client that asked for close would get that header back either way, and the
// assertion would prove nothing.
func probe(addr, path string) sample {
tr := &http.Transport{}
defer tr.CloseIdleConnections()
c := &http.Client{Transport: tr, Timeout: 3 * time.Second}
s := sample{at: time.Now(), path: path}
resp, err := c.Get("http://" + addr + path)
if err != nil {
return s
}
defer func() { _ = resp.Body.Close() }()
body, _ := io.ReadAll(resp.Body)
s.status = resp.StatusCode
s.willClose = resp.Close
s.draining = strings.Contains(string(body), `"status":"draining"`)
return s
}
// watcher polls the child until it stops accepting connections, keeping every
// answer.
type watcher struct {
mu sync.Mutex
samples []sample
done chan struct{}
}
func watch(addr string, paths ...string) *watcher {
w := &watcher{done: make(chan struct{})}
go func() {
defer close(w.done)
for {
refused := false
for _, p := range paths {
s := probe(addr, p)
w.mu.Lock()
w.samples = append(w.samples, s)
w.mu.Unlock()
if s.status == 0 {
refused = true
}
}
if refused {
return
}
time.Sleep(20 * time.Millisecond)
}
}()
return w
}
// sawDraining reports whether /ready has answered "draining" yet.
func (w *watcher) sawDraining() bool {
w.mu.Lock()
defer w.mu.Unlock()
for _, s := range w.samples {
if s.path == readyPath && s.draining {
return true
}
}
return false
}
func (w *watcher) wait(t *testing.T, d time.Duration) []sample {
t.Helper()
select {
case <-w.done:
case <-time.After(d):
t.Fatal("the child never stopped accepting connections")
}
w.mu.Lock()
defer w.mu.Unlock()
return w.samples
}
func describe(samples []sample) string {
var b strings.Builder
for _, s := range samples {
fmt.Fprintf(&b, " %s %s -> %d draining=%v willClose=%v\n",
s.at.Format("15:04:05.000"), s.path, s.status, s.draining, s.willClose)
}
return b.String()
}
// Acceptance 19. Registering only os.Interrupt meant SIGTERM - the signal
// `docker stop`, Kubernetes and systemd all send - terminated the process
// before any of the shutdown path ran. Both must now reach it.
@@ -241,7 +418,7 @@ func TestBothSignalsRunTheShutdownPath(t *testing.T) {
{"SIGTERM", syscall.SIGTERM},
} {
t.Run(tc.name, func(t *testing.T) {
cmd, _, lines := startChild(t, false)
cmd, lines := startChild(t, false)
await(t, lines, markerReady, 30*time.Second)
if err := cmd.Process.Signal(tc.sig); err != nil {
@@ -263,8 +440,14 @@ func TestBothSignalsRunTheShutdownPath(t *testing.T) {
// without restoring the disposition a second signal only refills the buffer:
// once SIGTERM is registered, a shutdown that hangs could not be interrupted by
// anything short of SIGKILL.
//
// The hang is now a cleanup callback that never returns, which is where a
// shutdown actually hangs, and it is reached through gracefulShutdown - so this
// also pins where the disposition is restored. Restore it before the drain
// window and the window itself becomes the interruptible part; restore it never
// and this test hangs.
func TestASecondSignalStillKillsAStuckShutdown(t *testing.T) {
cmd, _, lines := startChild(t, true)
cmd, lines := startChild(t, true)
await(t, lines, markerReady, 30*time.Second)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
@@ -272,21 +455,33 @@ func TestASecondSignalStillKillsAStuckShutdown(t *testing.T) {
}
await(t, lines, markerSignal, 10*time.Second)
// The child is now inside a cleanup that will not finish on its own.
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("second signal: %v", err)
}
// The child is now on its way into a cleanup that will not finish on its
// own. Signalled repeatedly rather than once: the marker is printed just
// before gracefulShutdown is entered, and the disposition is not restored
// until the drain window is over - zero seconds here, but not zero
// instructions - so a single signal sent immediately after the marker can
// still land in the buffered channel and be dropped. Which of them does
// the killing is not the assertion; that one of them can is.
done := make(chan error, 1)
go func() { done <- cmd.Wait() }()
select {
case err := <-done:
if err == nil {
t.Fatal("child exited cleanly; it was supposed to be killed by the second signal")
retry := time.NewTicker(200 * time.Millisecond)
defer retry.Stop()
deadline := time.After(15 * time.Second)
for {
select {
case err := <-done:
if err == nil {
t.Fatal("child exited cleanly; it was supposed to be killed by the second signal")
}
return
case <-retry.C:
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("second signal: %v", err)
}
case <-deadline:
t.Fatal("the second signal did not kill a stuck shutdown - the escape hatch is gone")
}
case <-time.After(15 * time.Second):
t.Fatal("the second signal did not kill a stuck shutdown - the escape hatch is gone")
}
}
@@ -295,7 +490,7 @@ func TestASecondSignalStillKillsAStuckShutdown(t *testing.T) {
// unconditional os.Exit(1). Everything after it, which is where the cleanup
// hooks will hang, never ran. A failed Shutdown must not end the process.
func TestShutdownTimeoutDoesNotStopWhatFollows(t *testing.T) {
cmd, _, lines := startChild(t, false, childHangConn+"=1")
cmd, lines := startChild(t, false, childHangConn+"=1")
await(t, lines, markerReady, 30*time.Second)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
@@ -303,7 +498,7 @@ func TestShutdownTimeoutDoesNotStopWhatFollows(t *testing.T) {
}
await(t, lines, markerSignal, 10*time.Second)
seen := await(t, lines, markerExiting, 20*time.Second)
seen, _ := await(t, lines, markerExiting, 20*time.Second)
var timedOut bool
for _, l := range seen {
@@ -336,7 +531,7 @@ func TestShutdownTimeoutDoesNotStopWhatFollows(t *testing.T) {
// the contract that is easy to get backwards - the context bounds the wait, not
// the work, because Go cannot cancel a function that does not check for it.
func TestACleanupThatOutlastsItsBudgetIsAbandonedNotAwaited(t *testing.T) {
cmd, _, lines := startChild(t, false, childSlowCleanup+"=1")
cmd, lines := startChild(t, false, childSlowCleanup+"=1")
await(t, lines, markerReady, 30*time.Second)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
@@ -347,7 +542,7 @@ func TestACleanupThatOutlastsItsBudgetIsAbandonedNotAwaited(t *testing.T) {
// The budget is 300ms and the callback sleeps two seconds. If RunShutdown
// waited for it, this marker would not arrive for two seconds; the one
// second here is what makes "abandoned, not awaited" the thing asserted.
seen := await(t, lines, markerExiting, 1*time.Second)
seen, _ := await(t, lines, markerExiting, 1*time.Second)
var reported bool
for _, l := range seen {
@@ -367,3 +562,178 @@ func TestACleanupThatOutlastsItsBudgetIsAbandonedNotAwaited(t *testing.T) {
t.Fatalf("child exited with %v, want a clean exit despite the abandoned callback", err)
}
}
// The core acceptance: with a drain window configured, something outside the
// process can observe that this instance is draining, on a connection it opens
// after the signal, and can still be served while it does.
//
// Two windows rather than one. A single value proves only that something takes
// that long, which a hard-coded sleep anywhere in the sequence would satisfy;
// two say the wait is the configured one.
//
// What each answer is for:
//
// - /ready reporting "draining" is the window being observable at all. The
// status code alone would not say it: with no database configured the
// probe's own checks fail and 503 is also the answer before the signal.
// - The server refusing to keep those connections alive is the window being
// useful. It keeps them alive until Shutdown sets shuttingDown(), so
// without switching keep-alive off here a balancer's pool would sit
// untouched for the whole window and be cut at the end of it anyway. The
// header saying so is Connection: close; the transport consumes it and
// reports it as Response.Close, which is what a sample records.
// - /health staying 200 is the window not asking to be restarted, and the
// ordinary route staying 200 is the window not refusing work. Draining is
// "stop sending me new work", not "reject what arrives".
func TestTheDrainWindowIsObservableWhileStillServing(t *testing.T) {
for _, drain := range []time.Duration{300 * time.Millisecond, 1200 * time.Millisecond} {
t.Run(drain.String(), func(t *testing.T) {
cmd, lines := startChild(t, false,
fmt.Sprintf("%s=%d", childDrainMS, drain.Milliseconds()))
addr := childAddr(t, lines)
await(t, lines, markerReady, 30*time.Second)
w := watch(addr, readyPath, healthPath, pingPath)
// Long enough for a round of answers from a server that is not yet
// draining, which is what the keep-alive assertion below compares
// against.
time.Sleep(150 * time.Millisecond)
signalAt := time.Now()
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("signal: %v", err)
}
samples := w.wait(t, drain+30*time.Second)
spent := took(t, lines, 10*time.Second)
await(t, lines, markerExiting, 10*time.Second)
if spent < drain {
t.Errorf("the shutdown took %s, want at least the %s window", spent, drain)
}
var refusedAt = -1
for i, s := range samples {
if s.status == 0 {
refusedAt = i
break
}
}
if refusedAt < 0 {
t.Fatalf("the child never stopped accepting; saw:\n%s", describe(samples))
}
var keptAliveBefore, drainingInside, closedInside bool
for _, s := range samples[:refusedAt] {
switch s.path {
case readyPath:
if s.at.Before(signalAt) && !s.draining && !s.willClose {
keptAliveBefore = true
}
if s.at.After(signalAt) && s.draining {
drainingInside = true
if s.willClose {
closedInside = true
}
}
case healthPath, pingPath:
if s.status != http.StatusOK {
t.Errorf("%s answered %d before the listener closed, want 200;\n%s",
s.path, s.status, describe(samples))
}
}
}
if !keptAliveBefore {
t.Fatalf("no answer before the signal kept the connection alive, so the header assertion below proves nothing;\n%s",
describe(samples))
}
if !drainingInside {
t.Errorf("no answer inside the window reported draining; the flip and the closed listener were not far enough apart to observe;\n%s",
describe(samples))
}
if !closedInside {
t.Errorf("answers inside the window still kept the connection alive, so a pooled connection survives the whole window and is cut at the end of it anyway;\n%s",
describe(samples))
}
if err := cmd.Wait(); err != nil {
t.Fatalf("child exited with %v, want a clean exit", err)
}
})
}
}
// The default has to be no window at all: a process that configures no
// extend.shutdown section must shut down the way it did before the section
// existed.
//
// Asserted as a sequence rather than as a duration. How long a shutdown takes
// is decided by how much the cleanup callbacks have to do, so "as fast as
// before" is not falsifiable; "nothing was inserted between the signal and the
// listener closing" is.
func TestAnUnconfiguredShutdownAddsNoWindow(t *testing.T) {
cmd, lines := startChild(t, false)
await(t, lines, markerReady, 30*time.Second)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("signal: %v", err)
}
await(t, lines, markerSignal, 10*time.Second)
spent := took(t, lines, 10*time.Second)
if spent > 100*time.Millisecond {
t.Errorf("an unconfigured shutdown spent %s between the signal and exiting; "+
"with no drain window and no cleanup callbacks it must be immediate", spent)
}
if err := cmd.Wait(); err != nil {
t.Fatalf("child exited with %v, want a clean exit", err)
}
}
// A second signal during the window ends it early rather than killing the
// process. Somebody sending another kill wants this over with sooner, and the
// answer to that is to stop draining - not to skip the cleanup, which is what
// the default disposition would do.
//
// This is the pair to TestASecondSignalStillKillsAStuckShutdown: the escape
// hatch has to be closed for the length of the window and open after it.
func TestASecondSignalEndsTheDrainWindowEarly(t *testing.T) {
// Long enough that the shutdown cannot plausibly have taken this long on
// its own, short enough that the test does not sit out the whole window
// when the early exit is missing - it fails on the reported duration
// instead of on a timeout, which says which of the two broke.
const window = 10 * time.Second
cmd, lines := startChild(t, false,
fmt.Sprintf("%s=%d", childDrainMS, window.Milliseconds()))
addr := childAddr(t, lines)
await(t, lines, markerReady, 30*time.Second)
w := watch(addr, readyPath)
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("first signal: %v", err)
}
deadline := time.Now().Add(15 * time.Second)
for !w.sawDraining() {
if time.Now().After(deadline) {
t.Fatal("the child never reported draining, so the second signal below would not land inside the window")
}
time.Sleep(20 * time.Millisecond)
}
if err := cmd.Process.Signal(syscall.SIGTERM); err != nil {
t.Fatalf("second signal: %v", err)
}
spent := took(t, lines, window+20*time.Second)
if spent >= window {
t.Errorf("the window ran its full %s despite a second signal (%s); the signal was ignored", window, spent)
}
await(t, lines, markerExiting, 10*time.Second)
if err := cmd.Wait(); err != nil {
t.Fatalf("child exited with %v; a second signal inside the window must end the window, not the process", err)
}
}
+22
View File
@@ -185,6 +185,28 @@ func (e *Migration) mergedEntries() map[string]versionEntry {
return out
}
// RegisteredVersions returns every migration version this binary registers,
// sorted, without touching a database.
//
// Status answers a richer question - what is registered, what is applied, and
// what is applied while nothing registers it - and needs a database to do it.
// This is the half that can be asked of the process alone, which is what a
// readiness check needs: the check holds the databases it is asking about, and
// reusing Status would mean calling SetDb from a request handler, writing this
// package's shared state from a request path.
func (e *Migration) RegisteredVersions() []string {
all := e.mergedEntries()
out := make([]string, 0, len(all))
for k := range all {
out = append(out, k)
}
sort.Strings(out)
return out
}
// RegisteredVersions reports what the process-wide registry holds.
func RegisteredVersions() []string { return Migrate.RegisteredVersions() }
// StatusEntry is one row of migrate status.
type StatusEntry struct {
AppCode string
@@ -237,13 +237,53 @@ func dropIndexesOn(db *gorm.DB, table, column string) error {
if !m.HasIndex(table, name) {
continue
}
if err := m.DropIndex(table, name); err != nil {
if err := db.Exec(dropIndex(db, table, name)).Error; err != nil {
return fmt.Errorf("dropping index %s: %w", name, err)
}
}
return nil
}
// dropIndex spells DROP INDEX for one dialect, rather than going through
// Migrator().DropIndex.
//
// The migrator cannot be used here on PostgreSQL. Its driver resolves a schema
// for the statement and falls back to an expression when it cannot:
//
// currentSchema, _ := m.CurrentSchema(stmt, stmt.Table) // CURRENT_SCHEMA()
// m.DB.Exec("DROP INDEX ?.?", currentSchema, clause.Column{Name: name})
//
// DROP INDEX takes an identifier in that position, not an expression, so the
// statement does not parse. The schema is unresolvable for every call made
// here, because this passes a table name as a string rather than a model - so
// it failed on every PostgreSQL database rather than intermittently, and took
// the whole conversion with it. Reported as go-admin#919, where the visible
// symptom was a login rejecting a correct password: the migration had stopped
// here, leaving deleted_at a timestamptz that the current query compares to 0.
//
// Written per dialect for the same reason addBigIntColumn and renameColumn
// already are.
//
// MySQL and SQL Server name the table in the statement and have no IF EXISTS
// for it; PostgreSQL and SQLite name the index alone, in its own namespace.
// The caller has already checked HasIndex, so IF EXISTS is only there to make
// the two that support it say nothing rather than fail on a race with another
// migrator.
//
// Verified against PostgreSQL 15, MySQL 8.0 and SQLite. The SQL Server form is
// from its documentation and has not been run - this repository has no SQL
// Server to run it against.
func dropIndex(db *gorm.DB, table, index string) string {
switch db.Dialector.Name() {
case "mysql":
return fmt.Sprintf("DROP INDEX `%s` ON `%s`", index, table)
case "sqlserver":
return fmt.Sprintf("DROP INDEX [%s] ON [%s]", index, table)
default:
return fmt.Sprintf(`DROP INDEX IF EXISTS "%s"`, index)
}
}
// indexNamesFor asks the database which indexes cover column.
func indexNamesFor(db *gorm.DB, table, column string) ([]string, error) {
indexes, err := db.Migrator().GetIndexes(table)
@@ -0,0 +1,156 @@
package version
import (
"os"
"testing"
"time"
"gorm.io/driver/postgres"
"gorm.io/gorm"
)
// postgresDSNEnv points these tests at a database. They are skipped without
// it, so a developer with no PostgreSQL running still gets a green run.
//
// The whole file exists because the rest of this package's tests run on
// SQLite, where the defect they cover cannot happen: dropping an index through
// gorm's migrator works there and produces unparseable SQL on PostgreSQL. A
// suite that only ever exercised SQLite reported success for a migration that
// failed on every PostgreSQL database it was pointed at - go-admin#919.
const postgresDSNEnv = "GO_ADMIN_TEST_POSTGRES_DSN"
func postgresDB(t *testing.T) *gorm.DB {
t.Helper()
dsn := os.Getenv(postgresDSNEnv)
if dsn == "" {
// Skipping locally is the point; skipping in CI is the failure this
// file exists to prevent. A workflow that renamed the variable or
// dropped the service would otherwise go green while these tests
// quietly did nothing - the same shape as the defect they cover.
if os.Getenv("CI") != "" {
t.Fatalf("%s is not set while CI is: the PostgreSQL migration tests must not skip here", postgresDSNEnv)
}
t.Skipf("%s is not set; skipping the PostgreSQL migration tests", postgresDSNEnv)
}
db, err := gorm.Open(postgres.Open(dsn), &gorm.Config{})
if err != nil {
t.Fatalf("connecting to %s: %v", postgresDSNEnv, err)
}
return db
}
// pgOldUser is the pre-migration shape: a nullable timestamp with an index on
// it, which is what makes dropping the column require dropping the index.
type pgOldUser struct {
UserId int64 `gorm:"column:user_id;primaryKey;autoIncrement"`
Username string
DeletedAt *time.Time `gorm:"index"`
}
func (pgOldUser) TableName() string { return "sd_pg_user" }
// The conversion completes on PostgreSQL.
//
// It did not. dropIndexesOn went through Migrator().DropIndex, whose
// PostgreSQL driver falls back to an expression when it cannot resolve a
// schema - which is every call made here, because the migration passes a table
// name as a string:
//
// DROP INDEX CURRENT_SCHEMA()."idx_sd_pg_user_deleted_at"
//
// DROP INDEX takes an identifier there, so it failed to parse and took the
// whole conversion with it. Every PostgreSQL deployment stopped at this
// migration, and the visible symptom was a login rejecting a correct password
// because deleted_at was still a timestamptz being compared to 0.
func TestConversionCompletesOnPostgres(t *testing.T) {
db := postgresDB(t)
t.Cleanup(func() { db.Migrator().DropTable(&pgOldUser{}) })
db.Migrator().DropTable(&pgOldUser{})
if err := db.AutoMigrate(&pgOldUser{}); err != nil {
t.Fatalf("building the old shape: %v", err)
}
deleted := time.Now().Add(-time.Hour)
// Checked rather than fired and forgotten: a failed insert leaves the
// assertions below reading an empty table, and "no rows" is a shape some
// of them cannot tell from success.
if err := db.Create(&pgOldUser{Username: "gone", DeletedAt: &deleted}).Error; err != nil {
t.Fatalf("seeding the deleted row: %v", err)
}
if err := db.Create(&pgOldUser{Username: "live"}).Error; err != nil {
t.Fatalf("seeding the live row: %v", err)
}
if err := convertDeletedAt(db, "sd_pg_user"); err != nil {
t.Fatalf("convertDeletedAt: %v", err)
}
var dataType string
if err := db.Raw(`SELECT data_type FROM information_schema.columns
WHERE table_name = 'sd_pg_user' AND column_name = 'deleted_at'`).Scan(&dataType).Error; err != nil {
t.Fatalf("reading the column type: %v", err)
}
if dataType != "bigint" {
t.Errorf("deleted_at is %q after the conversion, want bigint", dataType)
}
// The marker has to carry the timestamp across, or a row that was deleted
// comes back live.
var markers []int64
if err := db.Raw(`SELECT deleted_at FROM sd_pg_user ORDER BY user_id`).Scan(&markers).Error; err != nil {
t.Fatalf("reading the markers: %v", err)
}
if len(markers) != 2 {
t.Fatalf("read %d rows, want 2", len(markers))
}
if markers[0] == 0 {
t.Error("the deleted row came back live")
}
if markers[1] != 0 {
t.Errorf("the live row is marked deleted at %d", markers[1])
}
}
// The index on deleted_at is gone afterwards, which is the step that failed.
//
// Asserted separately from the conversion because the conversion can succeed
// on a table with no index at all, and this migration exists for tables that
// have one.
func TestTheIndexOnDeletedAtIsDroppedOnPostgres(t *testing.T) {
db := postgresDB(t)
t.Cleanup(func() { db.Migrator().DropTable(&pgOldUser{}) })
db.Migrator().DropTable(&pgOldUser{})
if err := db.AutoMigrate(&pgOldUser{}); err != nil {
t.Fatalf("building the old shape: %v", err)
}
var before int64
if err := db.Raw(`SELECT count(*) FROM pg_indexes
WHERE tablename = 'sd_pg_user' AND indexdef LIKE '%deleted_at%'`).Scan(&before).Error; err != nil {
t.Fatalf("counting the indexes before: %v", err)
}
if before == 0 {
t.Fatal("the old shape has no index on deleted_at, so this test asserts nothing")
}
if err := dropIndexesOn(db, "sd_pg_user", "deleted_at"); err != nil {
t.Fatalf("dropIndexesOn: %v", err)
}
// This one is why the errors are checked at all rather than as a matter of
// habit: a query that fails leaves after at zero, and zero is what success
// looks like. An unchecked error here is a test that passes when it cannot
// reach the database.
var after int64
if err := db.Raw(`SELECT count(*) FROM pg_indexes
WHERE tablename = 'sd_pg_user' AND indexdef LIKE '%deleted_at%'`).Scan(&after).Error; err != nil {
t.Fatalf("counting the indexes after: %v", err)
}
if after != 0 {
t.Errorf("%d index(es) on deleted_at survived", after)
}
}
@@ -0,0 +1,42 @@
package version
import (
"runtime"
"gorm.io/gorm"
adminmodels "go-admin/app/admin/models"
"go-admin/cmd/migrate/migration"
common "go-admin/common/models"
)
// Create sys_app (PRD 008 F2) and sys_app_casbin_grant (F4/F6's casbin
// attribution ledger - see the design doc's (docs-prd/008-应用清单与安装器/
// 数据库变更.md) §2.2/§3 for why casbin_rule itself is not touched:
// gorm-adapter's SavePolicyCtx truncates and reloads that table from its
// in-memory model, and any column this migration added to it would be
// silently zeroed the first time anything calls SavePolicy.
//
// Ordered after 1786700003000 (the soft-delete conversion), so importing
// cmd/migrate/migration/models is banned here - see
// schema_coverage_test.go's TestPostConversionMigrationsAvoidFrozenSeedModels.
// Both new tables are AutoMigrate'd from their runtime model shape under
// app/admin/models directly, which is also why neither one is added to
// 1786700003000's frozen softDeleteTables list: neither embeds
// common.ModelTime in the first place (see design doc §1.1).
func init() {
_, fileName, _, _ := runtime.Caller(0)
migration.Migrate.SetVersion(migration.GetFilename(fileName), _1786700007000AppRegistryTables)
}
func _1786700007000AppRegistryTables(db *gorm.DB, version string) error {
return db.Transaction(func(tx *gorm.DB) error {
if err := tx.Migrator().AutoMigrate(
new(adminmodels.SysApp),
new(adminmodels.SysAppCasbinGrant),
); err != nil {
return err
}
return tx.Create(&common.Migration{Version: version}).Error
})
}
@@ -0,0 +1,62 @@
package version
import (
"testing"
common "go-admin/common/models"
adminmodels "go-admin/app/admin/models"
)
// postgresDB is defined in 1786700003000_soft_delete_marker_postgres_test.go
// and shared across this package's PostgreSQL-only tests.
//
// This migration is plain AutoMigrate on two brand-new tables, unlike
// 1786700003000's DROP INDEX (go-admin#919's actual defect), so there is no
// dialect-specific SQL here for AutoMigrate itself to get wrong on
// PostgreSQL specifically. What is worth a real PostgreSQL run is
// 1786700008000's CONCAT()-based duplicate check next door - PostgreSQL has
// had CONCAT() since 9.1, but it was never verified against a real server
// until this file, only inferred from documentation - and the same
// AutoMigrate call this test makes, so a schema/character-set mistake
// AutoMigrate might make silently on a dialect nobody ran it against here
// has somewhere to surface.
func TestAppRegistryTablesAreCreatedOnPostgres(t *testing.T) {
db := postgresDB(t)
const version = "1786700007000-pg"
cleanup := func() {
db.Migrator().DropTable(&adminmodels.SysAppCasbinGrant{}, &adminmodels.SysApp{})
// Only this test's own row, not the whole shared sys_migration
// table: postgresDB points at a real, persistent database (unlike
// the SQLite tests' fresh in-memory one per run), so a version left
// behind by a previous run of this same binary collides with the
// wrapper's own INSERT the next time this test runs.
db.Exec("DELETE FROM sys_migration WHERE version = ?", version)
}
t.Cleanup(cleanup)
cleanup()
if err := db.AutoMigrate(&common.Migration{}); err != nil {
t.Fatalf("automigrate sys_migration: %v", err)
}
if err := _1786700007000AppRegistryTables(db, version); err != nil {
t.Fatalf("migrate: %v", err)
}
if err := db.Create(&adminmodels.SysApp{AppCode: "order", Name: "Order", Version: "v1"}).Error; err != nil {
t.Fatalf("insert sys_app: %v", err)
}
if err := db.Create(&adminmodels.SysApp{AppCode: "order", Name: "dup", Version: "v1"}).Error; err == nil {
t.Fatal("a second sys_app row with the same app_code was accepted on PostgreSQL")
}
grant := adminmodels.SysAppCasbinGrant{AppCode: "order", Ptype: "p", V0: "admin", V1: "/api/v1/order", V2: "GET"}
if err := db.Create(&grant).Error; err != nil {
t.Fatalf("insert sys_app_casbin_grant: %v", err)
}
dup := grant
dup.Id = 0
if err := db.Create(&dup).Error; err == nil {
t.Fatal("a second sys_app_casbin_grant row with the same natural key was accepted on PostgreSQL")
}
}
@@ -0,0 +1,130 @@
package version
import (
"testing"
"github.com/glebarez/sqlite"
"gorm.io/gorm"
adminmodels "go-admin/app/admin/models"
common "go-admin/common/models"
)
func openAppRegistryDB(t *testing.T) *gorm.DB {
t.Helper()
db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
if err != nil {
t.Fatalf("open: %v", err)
}
if err := db.AutoMigrate(&common.Migration{}); err != nil {
t.Fatalf("automigrate sys_migration: %v", err)
}
return db
}
// The migration has to build both tables and record itself as applied -
// F2/F6's acceptance case is a row landing in either one, and neither is
// possible if the table it belongs to was never created.
func TestAppRegistryTablesAreCreated(t *testing.T) {
db := openAppRegistryDB(t)
if err := _1786700007000AppRegistryTables(db, "1786700007000"); err != nil {
t.Fatalf("migrate: %v", err)
}
if !db.Migrator().HasTable(&adminmodels.SysApp{}) {
t.Fatal("sys_app was not created")
}
if !db.Migrator().HasTable(&adminmodels.SysAppCasbinGrant{}) {
t.Fatal("sys_app_casbin_grant was not created")
}
// A row that exercises every column, not just HasTable/HasColumn -
// AutoMigrate can build a column with the wrong type and still report
// that it exists.
if err := db.Create(&adminmodels.SysApp{
AppCode: "order", Name: "Order", Version: "v1", Description: "d", Author: "a",
Requires: "payment", Pricing: "free", License: "MIT", Status: 1,
}).Error; err != nil {
t.Fatalf("insert sys_app: %v", err)
}
if err := db.Create(&adminmodels.SysAppCasbinGrant{
AppCode: "order", Ptype: "p", V0: "admin", V1: "/api/v1/order", V2: "GET",
}).Error; err != nil {
t.Fatalf("insert sys_app_casbin_grant: %v", err)
}
var applied common.Migration
if err := db.Where("version = ?", "1786700007000").First(&applied).Error; err != nil {
t.Fatalf("sys_migration was not recorded: %v", err)
}
}
// Running it twice must be safe: DDL does not roll back on MySQL, so an
// operator whose first attempt failed partway through has nothing to do but
// run it again. This calls AutoMigrate directly rather than the wrapper,
// which also inserts a sys_migration row that a second call would collide
// on - a collision Migrate.run() itself prevents by never calling a
// function twice for the same recorded version, so it is not this
// migration's job to tolerate.
func TestAppRegistryTablesAutoMigrateIsRepeatable(t *testing.T) {
db := openAppRegistryDB(t)
for i := 0; i < 3; i++ {
if err := db.Migrator().AutoMigrate(
new(adminmodels.SysApp),
new(adminmodels.SysAppCasbinGrant),
); err != nil {
t.Fatalf("automigrate %d: %v", i, err)
}
}
}
// sys_app.app_code is the unique key G2 ("is app X installed") answers with
// - a second row for the same app code must be rejected, not tolerated.
func TestSysAppAppCodeIsUnique(t *testing.T) {
db := openAppRegistryDB(t)
if err := _1786700007000AppRegistryTables(db, "1786700007000"); err != nil {
t.Fatalf("migrate: %v", err)
}
if err := db.Create(&adminmodels.SysApp{AppCode: "order", Name: "Order", Version: "v1"}).Error; err != nil {
t.Fatalf("first insert: %v", err)
}
if err := db.Create(&adminmodels.SysApp{AppCode: "order", Name: "Order dup", Version: "v1"}).Error; err == nil {
t.Fatal("a second sys_app row with the same app_code was accepted")
}
}
// sys_app_casbin_grant's unique index mirrors casbin_rule's own natural key
// (ptype,v0..v5) exactly - see design doc §3. A duplicate grant for the
// same rule must be rejected the same way gorm-adapter's own unique index
// on casbin_rule would reject it.
func TestSysAppCasbinGrantNaturalKeyIsUnique(t *testing.T) {
db := openAppRegistryDB(t)
if err := _1786700007000AppRegistryTables(db, "1786700007000"); err != nil {
t.Fatalf("migrate: %v", err)
}
grant := adminmodels.SysAppCasbinGrant{AppCode: "order", Ptype: "p", V0: "admin", V1: "/api/v1/order", V2: "GET"}
if err := db.Create(&grant).Error; err != nil {
t.Fatalf("first insert: %v", err)
}
dup := grant
dup.Id = 0
if err := db.Create(&dup).Error; err == nil {
t.Fatal("a second sys_app_casbin_grant row with the same natural key was accepted")
}
// A grant for a different app, but the identical casbin natural key, is
// exactly the collision two applications granting the same api/role
// pair would produce - the natural key has to be the one thing that
// rejects it, app_code is descriptive only and not part of the index.
other := grant
other.Id = 0
other.AppCode = "another-app"
if err := db.Create(&other).Error; err == nil {
t.Fatal("a duplicate natural key under a different app_code was accepted")
}
}
@@ -0,0 +1,145 @@
package version
import (
"fmt"
"runtime"
"gorm.io/gorm"
adminmodels "go-admin/app/admin/models"
"go-admin/cmd/migrate/migration"
common "go-admin/common/models"
)
// Give seed.SeedMenus's two write paths (seedApis, seedMenuTree in
// app/admin/service/seed.go) a real natural key to check before inserting,
// so a retried, partially-failed migration (see the design doc
// docs-prd/008-应用清单与安装器/数据库变更.md §1.5/§1.6) does not insert the
// same row twice. This has already happened in production once (duplicate
// sys_menu/casbin_rule rows on the demo site), not a theoretical risk.
func init() {
_, fileName, _, _ := runtime.Caller(0)
migration.Migrate.SetVersion(migration.GetFilename(fileName), _1786700008000SeedNaturalKeys)
}
func _1786700008000SeedNaturalKeys(db *gorm.DB, version string) error {
if err := seedNaturalKeys(db); err != nil {
return err
}
return db.Create(&common.Migration{Version: version}).Error
}
// seedNaturalKeys is split out from the wrapper above so tests can call it
// against a database that only has sys_menu/sys_api, without also standing
// up sys_migration - and so it can be called more than once in the same
// test to prove the re-run tolerance the doc comment above promises: DDL
// does not roll back on MySQL, so an operator whose first attempt failed
// partway through has nothing to do but run the whole migration again.
func seedNaturalKeys(db *gorm.DB) error {
m := db.Migrator()
// sys_menu.seed_code is a brand-new column: every existing row becomes
// NULL, and NULL never collides in the unique index built below, so
// this needs no pre-check.
if !m.HasColumn(&adminmodels.SysMenu{}, "SeedCode") {
if err := m.AddColumn(&adminmodels.SysMenu{}, "SeedCode"); err != nil {
return err
}
}
if !m.HasIndex(&adminmodels.SysMenu{}, "uk_sys_menu_app_seed_code_del") {
if err := db.Exec(
"CREATE UNIQUE INDEX uk_sys_menu_app_seed_code_del ON sys_menu (app_code, seed_code, deleted_at)",
).Error; err != nil {
return err
}
}
// sys_api reuses existing, already-populated columns, which the demo
// site has already proven can hold duplicates. Refuse rather than let
// CREATE UNIQUE INDEX fail on an operator with no idea which rows to
// reconcile - same shape as 1786700003000_soft_delete_marker.go's
// refuseOnDuplicates.
if err := refuseOnDuplicateApis(db); err != nil {
return err
}
if !m.HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") {
if err := db.Exec(
"CREATE UNIQUE INDEX uk_sys_api_app_path_action_del ON sys_api (app_code, path, action, deleted_at)",
).Error; err != nil {
return err
}
}
return nil
}
// refuseOnDuplicateApis reports the (app_code, path, action) values that
// would make the unique index impossible, rather than the index failing to
// build and saying only that it did. Only live rows count: a soft-deleted
// duplicate does not block the index it will never occupy a slot in.
//
// sys_api.path/action (app/admin/models/sys_api.go) carry no NOT NULL
// constraint, and that stays true here on purpose: tightening it is an
// independent, backward-incompatible change of its own - existing NULL
// rows in a real database would need reconciling or backfilling before
// ALTER TABLE ... NOT NULL could even run, which is a decision for
// whoever owns that data, not something this migration should force as a
// side effect of adding an unrelated index. So this function has to
// tolerate NULL path/action rather than assume they cannot occur - see the
// query below for how it does that without either crashing on them
// (MySQL's CONCAT) or wrongly flagging them (GROUP BY's NULL-equals-NULL).
//
// The two are independent bugs that happened to share one root cause, and
// SQLite's own test suite for this file would have caught neither on its
// own: MySQL's CONCAT() returns NULL if any argument is NULL, which turned
// a duplicate check against a NULL-holding library into "converting NULL
// to string is unsupported" instead of a report - but SQLite's (and
// PostgreSQL's) CONCAT() treats a NULL argument as an empty string
// instead, so the exact same query never errors there no matter how it is
// called. A suite that only ever ran on SQLite would report success for
// both defects; only a real MySQL server surfaces the first one at all -
// this migration's PostgreSQL-only sibling test file
// (1786700008000_seed_natural_keys_postgres_test.go) rules out one more
// dialect, but MySQL specifically has to be checked by hand, since this
// repository's test suite has no MySQL service to run against in CI.
func refuseOnDuplicateApis(db *gorm.DB) error {
var dupes []string
if err := db.Raw(
// This has to agree with what the unique index it guards actually
// enforces, not just with what looks like a duplicate at a glance.
// Two different SQL rules collide on a NULL: GROUP BY treats two
// NULLs as equal, so a naive query flags every pair of rows that
// share a NULL path or action - even a pair with only one of the
// two NULL, since GROUP BY's equality still holds on whichever
// column both rows leave NULL - but a UNIQUE INDEX treats every
// NULL as distinct from every other value, including another
// NULL, so the index itself accepts every one of those pairs
// without complaint. Excluding any row missing either column from
// consideration entirely is what makes the two agree: a row
// missing path, or missing action, or missing both, can never
// violate the index no matter how many other rows are also
// missing the same one, so none of them belong in this count.
//
// No COALESCE: with both columns excluded whenever either is
// NULL, CONCAT here never receives a NULL argument for path or
// action - app_code cannot be NULL at all (see its own NOT NULL
// tag) - so there is nothing left for COALESCE to guard against,
// and leaving it out is deliberate rather than an oversight. A
// future regression that removed the two IS NOT NULL conditions
// above would fail loudly on MySQL (the same Scan error this
// query used to produce) instead of quietly reporting a made-up
// "duplicate" whose path and action both print as empty - the
// failure this function exists to prevent in the first place.
`SELECT CONCAT(app_code, '|', path, '|', action) FROM sys_api
WHERE deleted_at = 0 AND path IS NOT NULL AND action IS NOT NULL
GROUP BY app_code, path, action HAVING COUNT(*) > 1`,
).Scan(&dupes).Error; err != nil {
return fmt.Errorf("checking sys_api for duplicates: %w", err)
}
if len(dupes) > 0 {
return fmt.Errorf(
"sys_api already holds duplicate (app_code,path,action) %v; reconcile them before this migration can add its unique index",
dupes)
}
return nil
}
@@ -0,0 +1,105 @@
package version
import (
"testing"
)
// postgresDB is defined in 1786700003000_soft_delete_marker_postgres_test.go.
//
// This file exists because refuseOnDuplicateApis's duplicate check is
// spelled with CONCAT(), a function this migration's design assumed
// PostgreSQL has carried since 9.1 but that nothing had run against a real
// PostgreSQL server before this test - only against the pure-Go SQLite
// driver, which happens to bundle a SQLite new enough to have grown its own
// CONCAT() only recently. A dialect where that assumption were wrong would
// otherwise only be discovered the first time an operator's install hit a
// genuine sys_api duplicate on PostgreSQL in production.
func TestSeedNaturalKeysRefusesDuplicateApisOnPostgres(t *testing.T) {
db := postgresDB(t)
t.Cleanup(func() { db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{}) })
db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{})
if err := db.AutoMigrate(&oldSeedMenu{}, &oldSeedApi{}); err != nil {
t.Fatalf("automigrate: %v", err)
}
for i := 0; i < 2; i++ {
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed duplicate %d: %v", i, err)
}
}
err := seedNaturalKeys(db)
if err == nil {
t.Fatal("PostgreSQL accepted sys_api rows that already hold a duplicate (app_code, path, action)")
}
if !contains(err.Error(), "order") || !contains(err.Error(), "/api/v1/order") {
t.Errorf("the error does not name the offending row: %v", err)
}
if db.Migrator().HasIndex(&oldSeedApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was built despite the migration refusing")
}
}
// GROUP BY treats two NULLs as equal for grouping; a UNIQUE INDEX treats
// every NULL as distinct from every other value, including another NULL.
// Both are standard SQL, not a SQLite/PostgreSQL/MySQL difference - this
// file exists to confirm that on a real server rather than assume it, the
// same reason TestSeedNaturalKeysRefusesDuplicateApisOnPostgres above
// exists for CONCAT(). See TestSeedNaturalKeysDoesNotFlagWhatTheIndexWouldAccept
// in the SQLite-backed test file for the full account of why this matters:
// a naive duplicate check that does not exclude NULL path/action refuses
// an install the unique index itself would accept without complaint.
func TestSeedNaturalKeysDoesNotFlagWhatTheIndexWouldAcceptOnPostgres(t *testing.T) {
db := postgresDB(t)
t.Cleanup(func() { db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{}) })
db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{})
if err := db.AutoMigrate(&oldSeedMenu{}, &oldSeedApi{}); err != nil {
t.Fatalf("automigrate: %v", err)
}
for i := 0; i < 2; i++ {
if err := db.Exec(
"INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', NULL, NULL, 0)",
).Error; err != nil {
t.Fatalf("seed NULL row %d: %v", i, err)
}
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("seedNaturalKeys refused a library the unique index itself accepts on PostgreSQL: %v", err)
}
if !db.Migrator().HasIndex(&oldSeedApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was not built on PostgreSQL even though seedNaturalKeys reported success")
}
}
// The success path, on the same server: both columns and both unique
// indexes have to actually build on PostgreSQL, not merely fail to error
// out on SQLite. Mirrors TestSeedNaturalKeysIsRepeatable's SQLite coverage.
func TestSeedNaturalKeysBuildsOnPostgres(t *testing.T) {
db := postgresDB(t)
t.Cleanup(func() { db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{}) })
db.Migrator().DropTable(&oldSeedMenu{}, &oldSeedApi{})
if err := db.AutoMigrate(&oldSeedMenu{}, &oldSeedApi{}); err != nil {
t.Fatalf("automigrate: %v", err)
}
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed: %v", err)
}
for i := 0; i < 2; i++ {
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate %d: %v", i, err)
}
}
if !db.Migrator().HasColumn(&oldSeedMenu{}, "seed_code") {
t.Error("sys_menu.seed_code was not added on PostgreSQL")
}
if !db.Migrator().HasIndex(&oldSeedMenu{}, "uk_sys_menu_app_seed_code_del") {
t.Error("the sys_menu unique index was not built on PostgreSQL")
}
if !db.Migrator().HasIndex(&oldSeedApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the sys_api unique index was not built on PostgreSQL")
}
}
@@ -0,0 +1,305 @@
package version
import (
"testing"
"time"
"github.com/glebarez/sqlite"
"gorm.io/gorm"
adminmodels "go-admin/app/admin/models"
common "go-admin/common/models"
)
// oldSeedMenu/oldSeedApi are the shape of sys_menu/sys_api immediately
// before this migration: post-1786700003000 (deleted_at is the NOT NULL
// millisecond marker) and post-1786700006000 (app_code exists), but before
// seed_code or either unique index. They stand in for the real runtime
// models, which by the time this file is read already carry the columns
// this migration adds - the same relationship oldUser bears to sys_user in
// 1786700003000_soft_delete_marker_test.go.
type oldSeedMenu struct {
MenuId int `gorm:"column:menu_id;primaryKey;autoIncrement"`
AppCode string `gorm:"column:app_code;type:varchar(64);not null;default:''"`
DeletedAt int64 `gorm:"column:deleted_at;not null;default:0"`
}
func (oldSeedMenu) TableName() string { return "sys_menu" }
type oldSeedApi struct {
Id int `gorm:"column:id;primaryKey;autoIncrement"`
AppCode string `gorm:"column:app_code;type:varchar(64);not null;default:''"`
Path string `gorm:"column:path;type:varchar(128)"`
Action string `gorm:"column:action;type:varchar(16)"`
DeletedAt int64 `gorm:"column:deleted_at;not null;default:0"`
}
func (oldSeedApi) TableName() string { return "sys_api" }
func openSeedNaturalKeysDB(t *testing.T) *gorm.DB {
t.Helper()
db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
if err != nil {
t.Fatalf("open: %v", err)
}
if err := db.AutoMigrate(&oldSeedMenu{}, &oldSeedApi{}, &common.Migration{}); err != nil {
t.Fatalf("automigrate: %v", err)
}
return db
}
// The host's own hand-placed menus, and every app-seeded row written
// before this column existed, have no seed_code at all - an unbounded
// number of those must coexist under the same app_code without tripping
// the new unique index (design doc §1.6: "NULL never treated as equal to
// NULL").
func TestSeedNaturalKeysToleratesManyPreExistingMenusWithNoSeedCode(t *testing.T) {
db := openSeedNaturalKeysDB(t)
for i := 0; i < 3; i++ {
if err := db.Create(&oldSeedMenu{AppCode: ""}).Error; err != nil {
t.Fatalf("seed pre-existing menu %d: %v", i, err)
}
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate: %v", err)
}
if !db.Migrator().HasColumn(&adminmodels.SysMenu{}, "SeedCode") {
t.Fatal("sys_menu.seed_code was not added")
}
}
// The point of adding seed_code at all: a second row with the same
// (app_code, seed_code) while both are live is what seedMenuTree's
// idempotency check depends on the database to reject if the Go-level
// check above it is ever bypassed or raced.
func TestSeedNaturalKeysMenuUniqueIndexBindsLiveRowsOnly(t *testing.T) {
db := openSeedNaturalKeysDB(t)
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate: %v", err)
}
if err := db.Exec(
"INSERT INTO sys_menu (app_code, seed_code, deleted_at) VALUES ('order', 'dir', 0)",
).Error; err != nil {
t.Fatalf("seed: %v", err)
}
t.Run("a second live row with the same natural key is rejected", func(t *testing.T) {
err := db.Exec(
"INSERT INTO sys_menu (app_code, seed_code, deleted_at) VALUES ('order', 'dir', 0)",
).Error
if err == nil {
t.Fatal("a duplicate (app_code, seed_code) was accepted while both rows were live")
}
})
t.Run("the key is free again once the row is soft-deleted", func(t *testing.T) {
if err := db.Exec("UPDATE sys_menu SET deleted_at = ? WHERE seed_code = 'dir'", time.Now().UnixMilli()).Error; err != nil {
t.Fatalf("soft-delete: %v", err)
}
if err := db.Exec(
"INSERT INTO sys_menu (app_code, seed_code, deleted_at) VALUES ('order', 'dir', 0)",
).Error; err != nil {
t.Errorf("the key stayed taken after its row was soft-deleted: %v", err)
}
})
}
// The demo site has already proven sys_api can hold historical duplicates;
// the migration has to name them and refuse, not let CREATE UNIQUE INDEX
// fail on an operator with no idea which rows to reconcile.
func TestSeedNaturalKeysRefusesDuplicateApis(t *testing.T) {
db := openSeedNaturalKeysDB(t)
for i := 0; i < 2; i++ {
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed duplicate %d: %v", i, err)
}
}
err := seedNaturalKeys(db)
if err == nil {
t.Fatal("the migration accepted sys_api rows that already hold a duplicate (app_code, path, action)")
}
if !contains(err.Error(), "order") || !contains(err.Error(), "/api/v1/order") {
t.Errorf("the error does not name the offending row: %v", err)
}
if db.Migrator().HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was built despite the migration refusing")
}
// sys_menu's column and index are independent of sys_api's outcome and
// should already be in place - a partial failure here still leaves a
// record of what succeeded, same as any other non-transactional DDL
// migration in this package.
if !db.Migrator().HasColumn(&adminmodels.SysMenu{}, "SeedCode") {
t.Error("sys_menu.seed_code was not added even though only the sys_api step failed")
}
}
// Only live rows count towards the duplicate check: a row a prior,
// unrelated soft-delete already retired does not block the index it will
// never occupy a slot in.
func TestSeedNaturalKeysIgnoresSoftDeletedApiDuplicates(t *testing.T) {
db := openSeedNaturalKeysDB(t)
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed live row: %v", err)
}
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET", DeletedAt: time.Now().UnixMilli()}).Error; err != nil {
t.Fatalf("seed soft-deleted row: %v", err)
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate: %v", err)
}
if !db.Migrator().HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was not built")
}
}
// The point of the sys_api index, mirroring
// TestSeedNaturalKeysMenuUniqueIndexBindsLiveRowsOnly above: a second live
// row is rejected, and the key is free again once the row is
// soft-deleted.
func TestSeedNaturalKeysApiUniqueIndexBindsLiveRowsOnly(t *testing.T) {
db := openSeedNaturalKeysDB(t)
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed: %v", err)
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate: %v", err)
}
t.Run("a second live row with the same natural key is rejected", func(t *testing.T) {
err := db.Exec(
"INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', '/api/v1/order', 'GET', 0)",
).Error
if err == nil {
t.Fatal("a duplicate (app_code, path, action) was accepted while both rows were live")
}
})
t.Run("the key is free again once the row is soft-deleted", func(t *testing.T) {
if err := db.Exec(
"UPDATE sys_api SET deleted_at = ? WHERE path = '/api/v1/order'", time.Now().UnixMilli(),
).Error; err != nil {
t.Fatalf("soft-delete: %v", err)
}
if err := db.Exec(
"INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', '/api/v1/order', 'GET', 0)",
).Error; err != nil {
t.Errorf("the key stayed taken after its row was soft-deleted: %v", err)
}
})
}
// Running it twice must be safe: DDL does not roll back on MySQL, so an
// operator whose first attempt failed partway through (say, sys_menu's step
// succeeded and sys_api's refused) has nothing to do but run the whole
// migration again once the duplicates are reconciled.
func TestSeedNaturalKeysIsRepeatable(t *testing.T) {
db := openSeedNaturalKeysDB(t)
if err := db.Create(&oldSeedApi{AppCode: "order", Path: "/api/v1/order", Action: "GET"}).Error; err != nil {
t.Fatalf("seed: %v", err)
}
for i := 0; i < 3; i++ {
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("migrate %d: %v", i, err)
}
}
}
// The wrapper's contract with Migrate.run(): the version is only recorded
// once the whole thing - both columns, both indexes - succeeded.
func TestSeedNaturalKeysWrapperRecordsTheVersion(t *testing.T) {
db := openSeedNaturalKeysDB(t)
if err := _1786700008000SeedNaturalKeys(db, "1786700008000"); err != nil {
t.Fatalf("migrate: %v", err)
}
var applied common.Migration
if err := db.Where("version = ?", "1786700008000").First(&applied).Error; err != nil {
t.Fatalf("sys_migration was not recorded: %v", err)
}
}
// GROUP BY treats two NULLs as equal for grouping purposes; a UNIQUE INDEX
// treats every NULL as distinct from every other value, including another
// NULL - both are standard SQL semantics, not a quirk of one dialect (see
// the postgres-only test file next to this one for the same check against
// a real server). A duplicate check that groups on the raw columns without
// accounting for that difference refuses an install the index itself would
// accept without complaint, on data there is nothing to "reconcile" -
// worse than the index simply failing to build, because it stops a library
// that has nothing wrong with it.
//
// sys_api.path/action carry no NOT NULL constraint - see the design doc's
// note on this migration for why that stays true in this batch, changing
// it is an independent, backward-incompatible migration of its own - so
// this state is reachable in a real database even though seedApis's own
// Create call, which always writes the Go zero value "" rather than NULL,
// never produces it itself. Inserted via raw SQL for exactly that reason:
// models.SysApi's Path/Action are plain (non-pointer) Go strings, which
// cannot represent NULL through a normal Create call.
func TestSeedNaturalKeysDoesNotFlagWhatTheIndexWouldAccept(t *testing.T) {
db := openSeedNaturalKeysDB(t)
for i := 0; i < 2; i++ {
if err := db.Exec(
"INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', NULL, NULL, 0)",
).Error; err != nil {
t.Fatalf("seed NULL row %d: %v", i, err)
}
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("seedNaturalKeys refused a library the unique index itself accepts: %v", err)
}
if !db.Migrator().HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was not built even though seedNaturalKeys reported success")
}
}
// The case above has both path and action NULL on every row, which both
// of the query's two NULL-exclusion conditions independently catch - it
// cannot tell "only path IS NOT NULL is doing anything here" apart from
// "both conditions are doing something". A row missing only one of the
// two is exactly as real (an api registered with a path but no method,
// or vice versa) and exercises only one condition at a time: two rows
// sharing a real path but both NULL in action, or two rows sharing a real
// action but both NULL in path. GROUP BY treats each pair's shared NULL
// the same way it treats a shared (NULL, NULL) - as equal - and the
// unique index accepts both pairs for the same reason it accepts the
// (NULL, NULL) case, so neither belongs in the count either.
func TestSeedNaturalKeysDoesNotFlagPartiallyNullRows(t *testing.T) {
cases := []struct {
name string
insert string // two rows, sharing a value in exactly one of path/action
}{
{
name: "path is null, action repeats",
insert: "INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', NULL, 'GET', 0)",
},
{
name: "action is null, path repeats",
insert: "INSERT INTO sys_api (app_code, path, action, deleted_at) VALUES ('order', '/api/v1/order', NULL, 0)",
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
db := openSeedNaturalKeysDB(t)
for i := 0; i < 2; i++ {
if err := db.Exec(tc.insert).Error; err != nil {
t.Fatalf("seed row %d: %v", i, err)
}
}
if err := seedNaturalKeys(db); err != nil {
t.Fatalf("seedNaturalKeys refused a library the unique index itself accepts: %v", err)
}
if !db.Migrator().HasIndex(&adminmodels.SysApi{}, "uk_sys_api_app_path_action_del") {
t.Error("the unique index was not built even though seedNaturalKeys reported success")
}
})
}
}
+77 -3
View File
@@ -11,8 +11,28 @@
// - /ready is readiness: should this instance receive requests now. It fails
// while the dependencies are unreachable, and - the part that only exists
// because of the life-cycle phases - it fails as soon as shutdown begins,
// before the server stops accepting, so a load balancer has a chance to
// take the instance out before connections are cut.
// before the server stops accepting.
//
// # What the draining answer is worth
//
// Order alone does not produce a window. Answering before the server stops
// accepting is the right order - the reverse reports the state after the
// connections are already cut - but with nothing between the two they are
// microseconds apart, and a poller on a multi-second interval never sees the
// 503.
//
// The delay between them is extend.shutdown.drain, which is zero unless it is
// configured. On the shipped defaults this is therefore still an answer that
// can be read rather than one anything acts on; a deployment that sets a drain
// window is the one that gets a window to act in.
//
// What acts on it depends on who does the removing. A load balancer that polls
// /ready takes this instance out when it reads the 503, and the window has to
// cover its check interval times its failure threshold, plus however long the
// removal takes to apply. On Kubernetes the endpoint is withdrawn when the Pod
// receives a deletionTimestamp, concurrently with SIGTERM and regardless of
// what the probe returns - there the window covers the delay in that removal
// reaching every node, and the 503 is what makes the state observable.
package health
import (
@@ -21,6 +41,7 @@ import (
"encoding/hex"
"errors"
"fmt"
"sync"
"sync/atomic"
"time"
@@ -48,6 +69,55 @@ type Check struct {
Err string `json:"error,omitempty"`
}
// extra are checks a host registers that this package cannot make itself.
//
// The direction is why this exists. Whether the schema matches what the binary
// expects is answered by the migration registry, which lives under cmd/ - and
// common/ has never imported cmd/. Rather than start, the host registers the
// check from where both are already in scope.
var (
extraMu sync.RWMutex
extra []namedCheck
)
type namedCheck struct {
name string
fn func(context.Context) error
}
// Register adds a check to what Ready asks.
//
// It panics on a duplicate name rather than replacing or appending: two checks
// under one name make the failing one impossible to identify from the response,
// and registering the same one twice is a wiring mistake worth hearing about at
// start-up rather than never.
func Register(name string, fn func(context.Context) error) {
if name == "" {
panic("health: a registered check needs a name")
}
if fn == nil {
panic("health: check " + name + " is nil")
}
extraMu.Lock()
defer extraMu.Unlock()
for _, c := range extra {
if c.name == name {
panic("health: check " + name + " is already registered")
}
}
extra = append(extra, namedCheck{name: name, fn: fn})
}
// registered returns the checks a host has added, copied so that Ready is not
// iterating the slice while another goroutine appends to it.
func registered() []namedCheck {
extraMu.RLock()
defer extraMu.RUnlock()
out := make([]namedCheck, len(extra))
copy(out, extra)
return out
}
// Ready asks every dependency this process cannot serve a request without.
//
// The queue is deliberately absent. Nothing on AdapterQueue answers "are you
@@ -55,10 +125,14 @@ type Check struct {
// a queue that is down degrades logging rather than stopping requests - which
// is a reason to alert, not a reason to leave the load balancer pool.
func Ready(ctx context.Context) []Check {
return []Check{
checks := []Check{
safely("database", func() error { return pingDB(ctx) }),
safely("cache", probeCache),
}
for _, c := range registered() {
checks = append(checks, safely(c.name, func() error { return c.fn(ctx) }))
}
return checks
}
// safely turns a panic into a failed check.
+6 -3
View File
@@ -187,9 +187,12 @@ func TestEveryDependencyIsReportedEvenWithNothingConfigured(t *testing.T) {
}
}
// Draining is what makes the shutdown graceful from the outside: it has to be
// observable before the server stops accepting, or the load balancer learns
// about the shutdown by having its connections cut.
// BeginDraining sets the flag and Draining reports it, before anything else is
// taken apart. That is the whole of what can be checked from inside the
// process: whether anyone outside gets to read it depends on
// extend.shutdown.drain, which is zero unless it is configured, and on who is
// routing traffic here - the package comment has both. The subprocess tests in
// cmd/api are where a reader on the other end of a socket sees the 503.
func TestDrainingIsObservableOnceItBegins(t *testing.T) {
previous := draining.Load()
t.Cleanup(func() { draining.Store(previous) })
+125
View File
@@ -0,0 +1,125 @@
package health
import (
"context"
"errors"
"testing"
)
// isolate empties the registered checks and puts them back, so one test in
// this package cannot decide what the next one sees.
func isolate(t *testing.T) {
t.Helper()
extraMu.Lock()
previous := extra
extra = nil
extraMu.Unlock()
t.Cleanup(func() {
extraMu.Lock()
extra = previous
extraMu.Unlock()
})
}
func findCheck(checks []Check, name string) (Check, bool) {
for _, c := range checks {
if c.Name == name {
return c, true
}
}
return Check{}, false
}
// A registered check has to reach Ready's answer, or the host has wired
// something that never gets asked.
func TestARegisteredCheckIsAsked(t *testing.T) {
isolate(t)
Register("schema", func(context.Context) error { return errors.New("two behind") })
checks := Ready(context.Background())
c, ok := findCheck(checks, "schema")
if !ok {
t.Fatal("Ready did not ask the registered check")
}
if c.OK {
t.Error("the check returned an error and was still reported OK")
}
if c.Err != "two behind" {
t.Errorf("Err = %q, want the check's own message", c.Err)
}
if Healthy(checks) {
t.Error("Healthy said yes while a registered check was failing")
}
}
// The context Ready is given has to reach the check: it carries the probe's
// deadline, and a check that ignores it can hold the handler past it.
func TestTheRegisteredCheckIsGivenReadysContext(t *testing.T) {
isolate(t)
type key struct{}
Register("ctx", func(ctx context.Context) error {
if ctx.Value(key{}) != "carried" {
return errors.New("the check was handed a different context")
}
return nil
})
checks := Ready(context.WithValue(context.Background(), key{}, "carried"))
c, ok := findCheck(checks, "ctx")
if !ok {
t.Fatal("the registered check was not asked")
}
if !c.OK {
t.Errorf("check failed: %s", c.Err)
}
}
// A check that panics must not take the process down through the probe, the
// same guarantee the built-in checks have.
func TestARegisteredCheckThatPanicsFailsRatherThanCrashes(t *testing.T) {
isolate(t)
Register("boom", func(context.Context) error { panic("registry unreachable") })
checks := Ready(context.Background())
c, ok := findCheck(checks, "boom")
if !ok {
t.Fatal("the registered check was not asked")
}
if c.OK {
t.Error("a panicking check was reported OK")
}
}
func TestRegisteringTheSameNameTwicePanics(t *testing.T) {
isolate(t)
Register("dup", func(context.Context) error { return nil })
defer func() {
if recover() == nil {
t.Error("registering a duplicate name did not panic; two checks under " +
"one name make the failing one impossible to identify")
}
}()
Register("dup", func(context.Context) error { return nil })
}
func TestRegisterRefusesAnEmptyNameOrNilCheck(t *testing.T) {
isolate(t)
for _, tc := range []struct {
name string
fn func(context.Context) error
why string
}{
{"", func(context.Context) error { return nil }, "empty name"},
{"nilfn", nil, "nil function"},
} {
func() {
defer func() {
if recover() == nil {
t.Errorf("%s did not panic", tc.why)
}
}()
Register(tc.name, tc.fn)
}()
}
}
+91 -17
View File
@@ -1,29 +1,103 @@
package middleware
import (
"net/http"
"github.com/gin-gonic/gin"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
"net/http"
)
// defaultDemoMsg is what a refused request is told when nothing is configured.
//
// It is the string this middleware used to carry hard-coded, kept verbatim so
// that a deployment which never set application.demomsg is answered exactly as
// it was before.
const defaultDemoMsg = "谢谢您的参与,但为了大家更好的体验,所以本次提交就算了吧!\U0001F600\U0001F600\U0001F600"
// demoWriteRoutes are routes that change something despite being registered as
// GET, so the method alone does not say whether they are safe to serve.
//
// All three belong to the code generator: two write Go source files onto the
// server's filesystem and the third inserts menus, APIs and casbin rules into
// the database. They are registered under a group whose own name says it does
// no role check, and a demo deployment lets anybody log in - so on a demo host
// they were reachable by any visitor, and the menus one had in fact been used.
//
// Spelled as gin route patterns, which is what Context.FullPath returns, so a
// path parameter matches whatever value it is given.
//
// This list cannot be checked from here: common/ may not import app/, so this
// package cannot see which routes exist. What keeps it honest is a test beside
// the routes themselves - see app/other/router - which registers them and
// fails if any entry here has stopped being a real route.
//
// It also does not close the general hole. Nothing stops the next GET handler
// that writes something from being added without an entry here, and no static
// check can tell a handler that writes from one that reads. Demo mode refuses
// the routes it has been told about; that is the whole of the guarantee.
var demoWriteRoutes = map[string]bool{
"/api/v1/gen/toproject/:tableId": true,
"/api/v1/gen/apitofile/:tableId": true,
"/api/v1/gen/todb/:tableId": true,
}
// DemoWriteRoutes returns the routes demo mode refuses despite their method.
//
// Exported only so the test that lives beside the route registrations can
// check every one of them still exists; nothing else should need it.
func DemoWriteRoutes() []string {
out := make([]string, 0, len(demoWriteRoutes))
for route := range demoWriteRoutes {
out = append(out, route)
}
return out
}
// demoAllows reports whether demo mode lets a request through.
//
// route is the matched gin route pattern and uri the raw request target; the
// two are different things and both are needed. The route is what identifies a
// handler regardless of the values in its path parameters, and it is empty for
// a request that matched nothing - which is why the login and logout checks
// still read the raw target, as they always did.
func demoAllows(method, route, uri string) bool {
if demoWriteRoutes[route] {
return false
}
return method == http.MethodGet ||
method == http.MethodOptions ||
uri == "/api/v1/login" ||
uri == "/api/v1/logout"
}
// demoMessage is the answer a refused request gets.
//
// application.demomsg has been in the configuration all along and nothing read
// it: the message was hard-coded here, and the demo host's configured string
// happened to be identical, so the setting looked like it worked. An empty
// value falls back rather than answering with nothing.
func demoMessage() string {
if msg := config.ApplicationConfig.DemoMsg; msg != "" {
return msg
}
return defaultDemoMsg
}
// DemoEvn refuses anything that would change state while mode is demo.
func DemoEvn() gin.HandlerFunc {
return func(c *gin.Context) {
method := c.Request.Method
if config.ApplicationConfig.Mode == "demo" {
if method == "GET" ||
method == "OPTIONS" ||
c.Request.RequestURI == "/api/v1/login" ||
c.Request.RequestURI == "/api/v1/logout" {
c.Next()
} else {
c.JSON(http.StatusOK, gin.H{
"code": 500,
"msg": "谢谢您的参与,但为了大家更好的体验,所以本次提交就算了吧!\U0001F600\U0001F600\U0001F600",
})
c.Abort()
return
}
if config.ApplicationConfig.Mode != "demo" {
c.Next()
return
}
c.Next()
if demoAllows(c.Request.Method, c.FullPath(), c.Request.RequestURI) {
c.Next()
return
}
c.JSON(http.StatusOK, gin.H{
"code": 500,
"msg": demoMessage(),
})
c.Abort()
}
}
+144
View File
@@ -0,0 +1,144 @@
package middleware
import (
"net/http"
"net/http/httptest"
"strings"
"testing"
"github.com/gin-gonic/gin"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
)
// demoMode puts the process in demo mode for one test and puts it back.
func demoMode(t *testing.T, mode, msg string) {
t.Helper()
previousMode, previousMsg := config.ApplicationConfig.Mode, config.ApplicationConfig.DemoMsg
t.Cleanup(func() {
config.ApplicationConfig.Mode = previousMode
config.ApplicationConfig.DemoMsg = previousMsg
})
config.ApplicationConfig.Mode, config.ApplicationConfig.DemoMsg = mode, msg
}
// The method is not enough on its own. Three of the generator's routes are
// registered as GET and write anyway - two of them onto the server's
// filesystem, one into the database - so a guard that reads only the method
// serves them to anybody who can log in, which on a demo host is everybody.
func TestDemoRefusesTheWritesThatAreServedOverGET(t *testing.T) {
const login = "/api/v1/login"
for _, tc := range []struct {
name string
method string
route, uri string
wantThrough bool
}{
{"a plain read", http.MethodGet, "/api/v1/dept", "/api/v1/dept", true},
{"a write, by method", http.MethodPost, "/api/v1/dept", "/api/v1/dept", false},
{"login is how a visitor gets in", http.MethodPost, login, login, true},
{"logout", http.MethodPost, "/api/v1/logout", "/api/v1/logout", true},
{"preflight", http.MethodOptions, "/api/v1/dept", "/api/v1/dept", true},
// A request that matched no route has an empty pattern, and the guard
// still has to refuse it by method - this is what a POST to a path
// that does not exist looks like from in here.
{"a write to nothing at all", http.MethodPost, "", "/api/v1/__probe__", false},
// The three this change is about.
{"generator writes the database", http.MethodGet,
"/api/v1/gen/todb/:tableId", "/api/v1/gen/todb/3", false},
{"generator writes source files", http.MethodGet,
"/api/v1/gen/toproject/:tableId", "/api/v1/gen/toproject/3", false},
{"generator writes an api file", http.MethodGet,
"/api/v1/gen/apitofile/:tableId", "/api/v1/gen/apitofile/3", false},
// The read-only half of the generator has to keep working, or the demo
// host cannot demonstrate the feature at all. Refusing too much is as
// much of a defect as refusing too little.
{"generator preview stays available", http.MethodGet,
"/api/v1/gen/preview/:tableId", "/api/v1/gen/preview/3", true},
{"generator table tree stays available", http.MethodGet,
"/api/v1/gen/tabletree", "/api/v1/gen/tabletree", true},
{"table list stays available", http.MethodGet,
"/api/v1/db/tables/page", "/api/v1/db/tables/page", true},
{"column list stays available", http.MethodGet,
"/api/v1/db/columns/page", "/api/v1/db/columns/page", true},
} {
t.Run(tc.name, func(t *testing.T) {
if got := demoAllows(tc.method, tc.route, tc.uri); got != tc.wantThrough {
t.Errorf("demoAllows(%s %s) = %v, want %v", tc.method, tc.route, got, tc.wantThrough)
}
})
}
}
// Everything above is about demo mode only. A deployment that is not a demo
// runs the generator for real, and a guard that reached it there would have
// taken the feature away from every production install.
func TestOutsideDemoModeNothingIsRefused(t *testing.T) {
demoMode(t, "prod", "")
gin.SetMode(gin.TestMode)
for _, route := range append(DemoWriteRoutes(), "/api/v1/dept") {
t.Run(route, func(t *testing.T) {
r := gin.New()
r.Use(DemoEvn())
r.GET(route, func(c *gin.Context) { c.String(http.StatusOK, "served") })
w := httptest.NewRecorder()
r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, requestFor(route), nil))
if w.Body.String() != "served" {
t.Errorf("answered %q; outside demo mode the handler must run", w.Body.String())
}
})
}
}
// The refusal has to come back as the demo message rather than a 403 or a 404:
// the front end shows it to the visitor, and the point of a demo host is that
// being turned away is explained.
func TestDemoRefusalCarriesTheConfiguredMessage(t *testing.T) {
gin.SetMode(gin.TestMode)
const route = "/api/v1/gen/todb/:tableId"
for _, tc := range []struct {
name, configured, want string
}{
{"configured", "come back tomorrow", "come back tomorrow"},
// A deployment that never set application.demomsg keeps the answer it
// already had; an empty setting must not become an empty message.
{"not configured", "", defaultDemoMsg},
} {
t.Run(tc.name, func(t *testing.T) {
demoMode(t, "demo", tc.configured)
r := gin.New()
r.Use(DemoEvn())
r.GET(route, func(c *gin.Context) { c.String(http.StatusOK, "served") })
w := httptest.NewRecorder()
r.ServeHTTP(w, httptest.NewRequest(http.MethodGet, requestFor(route), nil))
if w.Code != http.StatusOK {
t.Errorf("answered %d, want 200 so the front end reads the body", w.Code)
}
if body := w.Body.String(); !strings.Contains(body, tc.want) {
t.Errorf("body %q does not carry %q", body, tc.want)
}
if strings.Contains(w.Body.String(), "served") {
t.Error("the handler ran; the request was supposed to be refused")
}
})
}
}
// requestFor turns a route pattern into a request target by giving every path
// parameter a value.
func requestFor(route string) string {
segments := strings.Split(route, "/")
for i, segment := range segments {
if strings.HasPrefix(segment, ":") {
segments[i] = "1"
}
}
return strings.Join(segments, "/")
}
+118
View File
@@ -8,10 +8,12 @@
package storage
import (
"context"
"log"
"sync"
"github.com/go-admin-team/go-admin-core/v2/captcha"
corelog "github.com/go-admin-team/go-admin-core/v2/logger"
"github.com/go-admin-team/go-admin-core/v2/sdk"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
)
@@ -21,6 +23,7 @@ func Setup() {
setupCache()
setupCaptcha()
setupQueue()
registerQueueDrain()
}
func setupCache() {
@@ -41,8 +44,123 @@ var (
// shut it down, and counted so a consumer can tell one from the next.
installed interface{ Shutdown() }
installedGen uint64
// drainRegistered records that the BeforeExit callback is on the runtime,
// so that a reload does not add another one.
drainRegistered bool
)
// setShutdown is sdk.Runtime.SetShutdown, indirected so that registering can
// be observed.
//
// It has to be: the runtime does not report how many callbacks a phase holds,
// and shutdownQueue takes the adapter on its first run, so every registration
// after the first returns immediately and changes nothing anybody can see. A
// reload adding one callback per round would therefore be invisible from the
// outside - which is exactly how it would survive.
var setShutdown = func(f func(context.Context)) { sdk.Runtime.SetShutdown(f) }
// registerQueueDrain puts shutdownQueue on the BeforeExit phase, once.
//
// Setup is one of the callbacks bootstrap.SetupConfig re-runs on every
// configuration change, so registering from it without a guard would leave one
// callback per reload - each shutting down the same adapter, each reported
// separately when the budget runs out.
//
// A flag under the existing mutex rather than a sync.Once: the tests in this
// package already save and restore installed and installedGen to keep one test
// from deciding what the next one sees, and a sync.Once cannot be put back.
func registerQueueDrain() {
queueMu.Lock()
first := !drainRegistered
drainRegistered = true
queueMu.Unlock()
if first {
setShutdown(shutdownQueue)
}
}
// drainedInTime shuts q down and reports whether it finished before ctx expired.
func drainedInTime(ctx context.Context, q interface{ Shutdown() }) bool {
done := make(chan struct{})
go func() {
defer close(done)
q.Shutdown()
}()
return finishedBeforeDeadline(ctx, done)
}
// finishedBeforeDeadline waits for done or for ctx, and resolves a tie in
// favour of done.
//
// The tie is the reason this is a function of its own rather than one select
// inline. Both channels can be ready when the select runs, select picks at
// random among ready cases, and so a single look reports an overrun for a
// drain that completed - about half the times it lands there, which is exactly
// often enough to be dismissed as noise. core's own RunShutdown re-checks for
// this reason.
//
// Taking channels rather than a queue is what makes it testable: a closed done
// and an expired ctx can be handed in together, which is the state a race
// would otherwise have to be caught in.
func finishedBeforeDeadline(ctx context.Context, done <-chan struct{}) bool {
select {
case <-done:
return true
case <-ctx.Done():
}
select {
case <-done:
return true
default:
return false
}
}
// shutdownQueue drains the queue this package installed, on the way out.
//
// Nothing used to. core's Memory.Shutdown closes the queue and waits for every
// consumer to finish what it is holding, and the legacy adapter cancels its
// context and closes the underlying queue - but neither ran at exit, so the
// process left with the login log, the operation log and the API sync still
// buffered, and left reporting success.
//
// The adapter is read here rather than captured at registration because a
// reload replaces it. Registration happens once per process; this runs against
// whatever is current when the signal arrives.
//
// Only an adapter this package installed. sdk.Runtime.GetQueueAdapter never
// returns nil - with no queue section configured the runtime wraps its own
// fallback queue - so going through that accessor would shut down a queue this
// package neither built nor started.
//
// The adapter is taken, not read: after this the package owns nothing, so a
// reload arriving mid-shutdown builds a new one instead of being handed a
// closed one to shut down again. Both implementations tolerate a second
// Shutdown, so this is about who owns it rather than about a crash.
func shutdownQueue(ctx context.Context) {
queueMu.Lock()
q := installed
installed = nil
queueMu.Unlock()
if q == nil {
return
}
if drainedInTime(ctx, q) {
corelog.Info("queue: drained")
return
}
// The wait is what the budget bounds, not the work: Shutdown takes no
// context and is still running in that goroutine. Saying so here names what
// is being lost, which the generic overrun message cannot.
corelog.Warnf("queue: the shutdown budget ran out while the queue was still draining - " +
"whatever it had not delivered goes with the process. Raise extend.shutdown.cleanup " +
"if this recurs.")
}
// QueueGeneration reports how many times this package has installed a queue
// adapter. It changes every time setupQueue builds a new one, which is on
// every configuration reload, and stays 0 for as long as the configuration has
+333
View File
@@ -0,0 +1,333 @@
package storage
import (
"context"
"sync"
"sync/atomic"
"testing"
"time"
"github.com/go-admin-team/go-admin-core/v2/sdk"
"github.com/go-admin-team/go-admin-core/v2/sdk/config"
"github.com/go-admin-team/go-admin-core/v2/sdk/runtime"
corestorage "github.com/go-admin-team/go-admin-core/v2/storage"
)
// countingQueue stands in for an installed adapter. Only Shutdown is exercised
// - the drain callback never publishes or consumes - so the rest of
// AdapterQueue is deliberately absent: `installed` is typed on Shutdown alone,
// and widening the fake would only invite it to be used for something else.
type countingQueue struct {
calls atomic.Int32
block chan struct{}
// started is closed on the way into Shutdown, so a test can wait for the
// call rather than assume the goroutine that makes it was scheduled. The
// caller returns on its own deadline while Shutdown is still running, so
// reading calls straight after that return is a race with the increment.
startOnce sync.Once
started chan struct{}
}
func newCountingQueue() *countingQueue {
return &countingQueue{started: make(chan struct{})}
}
func (q *countingQueue) Shutdown() {
q.startOnce.Do(func() { close(q.started) })
q.calls.Add(1)
if q.block != nil {
<-q.block
}
}
// waitStarted blocks until Shutdown has been entered, or fails the test.
func (q *countingQueue) waitStarted(t *testing.T) {
t.Helper()
select {
case <-q.started:
case <-time.After(5 * time.Second):
t.Fatal("Shutdown was never called")
}
}
// isolate gives the test its own runtime and its own view of what this package
// has installed, and puts the process-wide state back afterwards.
//
// Same isolation as TestSetupBumpsTheQueueGenerationOnEveryReload, plus
// drainRegistered: it is what stops a reload registering a second callback, so
// leaving it set would make every later test in this binary see a package that
// has already registered.
func isolate(t *testing.T) {
t.Helper()
prevQ, prevC := config.QueueConfig, config.CacheConfig
prevRuntime := sdk.Runtime
queueMu.Lock()
prevInstalled, prevGen, prevRegistered := installed, installedGen, drainRegistered
queueMu.Unlock()
t.Cleanup(func() {
config.QueueConfig, config.CacheConfig = prevQ, prevC
sdk.Runtime = prevRuntime
queueMu.Lock()
installed, installedGen, drainRegistered = prevInstalled, prevGen, prevRegistered
queueMu.Unlock()
})
sdk.Runtime = runtime.NewConfig()
queueMu.Lock()
installed, drainRegistered = nil, false
queueMu.Unlock()
}
// setInstalled puts a fake where setupQueue would have left the real adapter.
//
// Legitimate because the callback reads `installed` when it runs rather than
// capturing it at registration - that is the property that lets a reload
// replace the adapter and still have the right one drained.
func setInstalled(q interface{ Shutdown() }) {
queueMu.Lock()
installed = q
queueMu.Unlock()
}
func currentInstalled() interface{ Shutdown() } {
queueMu.Lock()
defer queueMu.Unlock()
return installed
}
// Issue #911: nothing shut the queue down at exit, so whatever was buffered
// went with the process.
//
// This is the half that matters most - a callback is on BeforeExit and it
// reaches the adapter this package installed. It says nothing about how many
// times the callback was registered; see the test below for that.
func TestSetupPutsTheQueueDrainOnBeforeExit(t *testing.T) {
isolate(t)
config.CacheConfig = &config.Cache{Memory: struct{}{}}
config.QueueConfig = &config.Queue{Memory: &config.QueueMemory{PoolSize: 10}}
Setup()
Setup()
Setup()
q := newCountingQueue()
setInstalled(q)
if err := sdk.Runtime.RunShutdown(context.Background()); err != nil {
t.Fatalf("RunShutdown: %v", err)
}
if got := q.calls.Load(); got != 1 {
t.Errorf("Shutdown called %d times, want 1 - 0 means nothing registered the drain", got)
}
}
// Setup is re-run on every configuration change, so registering from it has to
// be guarded: a callback per reload would leave the shutdown phase holding a
// row of identical entries, each timed and each eligible to be named as the one
// that overran the budget.
//
// Counted at the seam rather than through the effect. The test above cannot see
// this - shutdownQueue takes the adapter on its first run, so the second and
// third callbacks find nothing and return, and three registrations produce
// exactly the same observable result as one. That is a good property of the
// callback and a blind spot for any test that goes through it.
func TestSetupRegistersTheDrainOncePerProcessHoweverManyReloads(t *testing.T) {
isolate(t)
previous := setShutdown
t.Cleanup(func() { setShutdown = previous })
registrations := 0
setShutdown = func(func(context.Context)) { registrations++ }
config.CacheConfig = &config.Cache{Memory: struct{}{}}
config.QueueConfig = &config.Queue{Memory: &config.QueueMemory{PoolSize: 10}}
Setup()
Setup()
Setup()
if registrations != 1 {
t.Errorf("three reloads registered the drain %d times, want 1", registrations)
}
}
// The drain reaches the adapter that is current when the signal arrives, not
// one captured while wiring up. A reload replaces the adapter, and draining the
// one that was installed at start-up would drain something nobody has published
// to since.
func TestTheDrainRunsAgainstTheAdapterInstalledLast(t *testing.T) {
isolate(t)
config.CacheConfig = &config.Cache{Memory: struct{}{}}
config.QueueConfig = &config.Queue{Memory: &config.QueueMemory{PoolSize: 10}}
Setup()
first, second := newCountingQueue(), newCountingQueue()
setInstalled(first)
setInstalled(second)
if err := sdk.Runtime.RunShutdown(context.Background()); err != nil {
t.Fatalf("RunShutdown: %v", err)
}
if first.calls.Load() != 0 {
t.Error("the adapter that was replaced was shut down; the callback captured it instead of " +
"reading it when it ran")
}
if second.calls.Load() != 1 {
t.Errorf("the current adapter was shut down %d times, want 1", second.calls.Load())
}
}
// Nothing installed is the shipped default: settings.yml has no queue section,
// so setupQueue returns early and the runtime's own fallback queue is what
// callers get. Shutting that down would close a queue this package neither
// built nor started.
func TestTheDrainDoesNothingWhenThisPackageInstalledNothing(t *testing.T) {
isolate(t)
config.CacheConfig = &config.Cache{Memory: struct{}{}}
config.QueueConfig = &config.Queue{}
Setup()
if currentInstalled() != nil {
t.Fatal("an empty queue configuration installed an adapter, so this test asserts nothing")
}
if err := sdk.Runtime.RunShutdown(context.Background()); err != nil {
t.Fatalf("RunShutdown: %v", err)
}
}
// The budget bounds the wait, not the work. A consumer that never finishes must
// not hold the process past its grace period - SIGKILL would arrive mid-write
// instead of at a point of the process's choosing.
func TestTheDrainStopsWaitingWhenTheBudgetIsGone(t *testing.T) {
isolate(t)
blocked := newCountingQueue()
blocked.block = make(chan struct{})
t.Cleanup(func() { close(blocked.block) })
setInstalled(blocked)
ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond)
defer cancel()
returned := make(chan struct{})
go func() {
defer close(returned)
shutdownQueue(ctx)
}()
select {
case <-returned:
case <-time.After(5 * time.Second):
t.Fatal("shutdownQueue did not return after its context expired - it waits on a Shutdown " +
"that takes no context, so the wait has to be bounded here")
}
// Waited for rather than read straight after the return: Shutdown runs on a
// goroutine that the caller does not join, so the increment is not ordered
// against the caller giving up on its deadline.
blocked.waitStarted(t)
if blocked.calls.Load() != 1 {
t.Errorf("Shutdown called %d times, want 1 - the drain has to be attempted even when it "+
"cannot be waited out", blocked.calls.Load())
}
}
// After the drain this package owns nothing. A reload arriving mid-shutdown
// then builds a new adapter rather than being handed a closed one as its
// `previous` to shut down again.
func TestTheDrainGivesUpOwnershipOfTheAdapter(t *testing.T) {
isolate(t)
setInstalled(newCountingQueue())
shutdownQueue(context.Background())
if got := currentInstalled(); got != nil {
t.Errorf("installed is %T after the drain, want nil", got)
}
}
// runtimeQueue is a full AdapterQueue, so it can be handed to the runtime
// rather than only to this package's own record of what it installed.
type runtimeQueue struct {
countingQueue
}
func (q *runtimeQueue) String() string { return "runtime-fake" }
func (q *runtimeQueue) Append(corestorage.Messager) error { return nil }
func (q *runtimeQueue) Register(string, corestorage.ConsumerFunc) {}
func (q *runtimeQueue) Run() {}
// The drain must not reach a queue this package did not install.
//
// sdk.Runtime.GetQueueAdapter never returns nil: with no queue section
// configured it wraps the runtime's own fallback, and the wrapper's Shutdown
// forwards. Reaching for the accessor would therefore look like it worked and
// would close a queue this package neither built nor started - the same shape
// as the `if q := GetQueueAdapter(); q != nil` that setupQueue already had to
// drop.
//
// The previous test's empty-configuration case cannot see this: it only checks
// that RunShutdown returns, which it would either way.
func TestTheDrainNeverReachesTheRuntimesOwnQueue(t *testing.T) {
isolate(t)
onTheRuntime := &runtimeQueue{countingQueue: *newCountingQueue()}
sdk.Runtime.SetQueueAdapter(onTheRuntime)
if currentInstalled() != nil {
t.Fatal("this package installed something, so the distinction under test is not set up")
}
if sdk.Runtime.GetQueueAdapter() == nil {
t.Fatal("the accessor returned nil, so it is no longer the trap this guards")
}
shutdownQueue(context.Background())
if got := onTheRuntime.calls.Load(); got != 0 {
t.Errorf("the runtime's queue was shut down %d times - the drain went through "+
"GetQueueAdapter instead of the adapter this package installed", got)
}
}
// A drain that finishes in the same instant the budget expires counts as
// finished.
//
// Both channels are ready when the select runs, and select picks at random
// among ready cases, so a single look reports an overrun for a drain that
// completed - roughly half the times it lands here. The repetition is what
// makes that visible: one iteration passes either way.
func TestATieBetweenTheDeadlineAndTheDrainGoesToTheDrain(t *testing.T) {
expired, cancel := context.WithCancel(context.Background())
cancel()
<-expired.Done()
done := make(chan struct{})
close(done)
for i := 0; i < 1000; i++ {
if !finishedBeforeDeadline(expired, done) {
t.Fatalf("iteration %d of 1000: both the deadline and the drain were ready and the "+
"deadline won - a drain that completed is being reported as an overrun", i)
}
}
}
// The other side of it. A drain that really has not finished has to be
// reported, or the warning never fires and the tie-break above has quietly
// turned into "always say it drained".
func TestADrainThatHasNotFinishedIsReportedAsAnOverrun(t *testing.T) {
expired, cancel := context.WithCancel(context.Background())
cancel()
<-expired.Done()
stillRunning := make(chan struct{}) // never closed
if finishedBeforeDeadline(expired, stillRunning) {
t.Error("an unfinished drain was reported as having finished in time")
}
}
+136
View File
@@ -1,5 +1,10 @@
package config
import (
"fmt"
"strings"
)
var ExtConfig Extend
// Extend 扩展配置
@@ -13,6 +18,7 @@ type Extend struct {
AMap AMap // 这里配置对应配置文件的结构即可
FileStore FileStore
RateLimit RateLimit
Shutdown Shutdown
}
// DefaultInboundQPS is the limit applied when nothing is configured. It is the
@@ -72,3 +78,133 @@ type ObjectStore struct {
func (o ObjectStore) Configured() bool {
return o.Endpoint != "" && o.AccessKeyID != "" && o.AccessKeySecret != "" && o.BucketName != ""
}
// Default budgets for a graceful shutdown, in seconds. Each applies to the
// matching field of extend.shutdown when that field is absent, and together
// they are what the process spent before the section existed - so a deployment
// that configures nothing keeps the shutdown it already had.
//
// The drain default is zero deliberately. The three budgets are spent one
// after the other, and once their sum reaches the orchestrator's stop grace
// period the process is killed part-way through its cleanup callbacks, which
// is worse than not draining at all. `docker stop` allows ten seconds by
// default and 5+3 already leaves little room, so a non-zero default here would
// slow down every existing shutdown to buy something only a load balancer that
// polls /ready can collect.
const (
DefaultDrainSeconds = 0
DefaultServerSeconds = 5
DefaultCleanupSeconds = 3
)
// Shutdown is how long a graceful shutdown may spend, stage by stage.
//
// extend:
// shutdown:
// drain: 0
// server: 5
// cleanup: 3
// grace: 30
//
// Every field is a pointer for the reason RateLimit.InboundQPS is: nil means
// "not configured" and takes the default, while a value that was written down
// is taken literally, zero included. Without that separation `server: 0` - do
// not wait for in-flight requests, which is a reasonable thing to ask under a
// very short grace period - could not be said at all, and `drain: 0` would
// have to mean something different from `server: 0` in the same section.
type Shutdown struct {
// Drain is how long to keep serving normally after a stop signal arrives.
// Throughout it /ready answers 503 and keep-alive is switched off, which
// is what gives whatever routes traffic here time to stop routing it
// before the listener closes. Zero is no window: the readiness flip and
// the listener closing are then microseconds apart and nothing observes
// the first.
//
// What the window is worth depends on who does the removing and on what
// basis; the package comment in common/health has the two cases, and they
// do not want the same value.
Drain *int
// Server is how long the server waits for in-flight requests once the
// listener is closed.
Server *int
// Cleanup is how long the BeforeExit callbacks get after that.
Cleanup *int
// Grace is the stop grace period the orchestrator gives this process -
// `docker stop --timeout`, or terminationGracePeriodSeconds. Nothing reads
// it during a shutdown; it exists so start-up can say whether the budget
// fits inside it. Absent means no comparison is made, because the
// reference values differ threefold between runtimes and a fixed threshold
// would warn about configurations that are correct.
Grace *int
}
// ShutdownBudget is what a shutdown will actually spend, in seconds, after the
// fallbacks have been applied.
type ShutdownBudget struct {
Drain int
Server int
Cleanup int
// Grace is zero when extend.shutdown.grace was not configured.
Grace int
}
// Budget resolves the configured section into the values that will be spent.
//
// A negative is refused rather than corrected. A wait cannot be negative, so
// there is no reading of one to honour, and quietly turning it into zero would
// be the failure this whole section exists to remove: written down, accepted,
// and not what happens. It is returned as an error rather than reported here
// so that the rule can be checked without ending the process.
func (s Shutdown) Budget() (ShutdownBudget, error) {
var negative []string
for _, f := range []struct {
name string
value *int
}{
{"drain", s.Drain},
{"server", s.Server},
{"cleanup", s.Cleanup},
{"grace", s.Grace},
} {
if f.value != nil && *f.value < 0 {
negative = append(negative, fmt.Sprintf("%s: %d", f.name, *f.value))
}
}
if len(negative) > 0 {
return ShutdownBudget{}, fmt.Errorf(
"extend.shutdown was given a negative number of seconds (%s); "+
"a wait cannot be negative, and 0 is how to say \"do not wait\"",
strings.Join(negative, ", "))
}
return ShutdownBudget{
Drain: budgetSeconds(s.Drain, DefaultDrainSeconds),
Server: budgetSeconds(s.Server, DefaultServerSeconds),
Cleanup: budgetSeconds(s.Cleanup, DefaultCleanupSeconds),
Grace: budgetSeconds(s.Grace, 0),
}, nil
}
func budgetSeconds(configured *int, fallback int) int {
if configured != nil {
return *configured
}
return fallback
}
// Total is the whole of the shutdown, since the three stages run one after the
// other.
func (b ShutdownBudget) Total() int { return b.Drain + b.Server + b.Cleanup }
// Overrun reports how many seconds have to be found for the budget to fit
// inside the configured grace period. It is zero when no grace period was
// configured and when the budget already fits.
//
// Fitting means strictly less: the grace period is when SIGKILL is sent, so a
// budget that ends exactly then leaves the last callback no time to return.
func (b ShutdownBudget) Overrun() int {
if b.Grace <= 0 || b.Total() < b.Grace {
return 0
}
return b.Total() - b.Grace + 1
}
+167 -1
View File
@@ -1,6 +1,9 @@
package config
import "testing"
import (
"strings"
"testing"
)
func TestObjectStoreConfigured(t *testing.T) {
if (ObjectStore{}).Configured() {
@@ -32,3 +35,166 @@ func TestRateLimitThreshold(t *testing.T) {
t.Errorf("configured limit = %v, want %v", got, custom)
}
}
func ptr(v int) *int { return &v }
// The zero-value rule is the same for all four fields, and it is the one the
// section would otherwise need a paragraph of documentation to survive: nil
// takes the default, a number that was written down is spent literally. A
// `server: 0` that quietly became five seconds would be the same class of
// failure this whole batch is about - configuration accepted and not applied.
func TestShutdownBudgetFallbacks(t *testing.T) {
for _, tc := range []struct {
name string
in Shutdown
want ShutdownBudget
}{
{
// What an existing settings.yml hits after an upgrade: no
// extend.shutdown section at all, and therefore the shutdown it
// already had.
name: "nothing configured",
in: Shutdown{},
want: ShutdownBudget{Drain: 0, Server: 5, Cleanup: 3},
},
{
name: "all four configured",
in: Shutdown{Drain: ptr(10), Server: ptr(8), Cleanup: ptr(4), Grace: ptr(30)},
want: ShutdownBudget{Drain: 10, Server: 8, Cleanup: 4, Grace: 30},
},
{
// The case a plain int could not express: do not wait for
// in-flight requests, which is a reasonable thing to ask for when
// the grace period is very short.
name: "explicit zeros are spent, not replaced",
in: Shutdown{Drain: ptr(0), Server: ptr(0), Cleanup: ptr(0)},
want: ShutdownBudget{Drain: 0, Server: 0, Cleanup: 0},
},
{
name: "one field configured, the rest default",
in: Shutdown{Drain: ptr(15)},
want: ShutdownBudget{Drain: 15, Server: 5, Cleanup: 3},
},
} {
t.Run(tc.name, func(t *testing.T) {
got, err := tc.in.Budget()
if err != nil {
t.Fatalf("Budget() = %v", err)
}
if got != tc.want {
t.Errorf("Budget() = %+v, want %+v", got, tc.want)
}
})
}
}
// A negative is refused, not corrected. Turning it into zero would be the
// failure this section exists to remove - written down, accepted, and not what
// happens - and there is no reading of a negative wait to honour.
//
// The last row is what makes the other four mean anything: an implementation
// that refused every value would pass them all.
func TestShutdownBudgetRefusesNegativeSeconds(t *testing.T) {
for _, tc := range []struct {
name string
in Shutdown
wantErr bool
}{
{name: "negative drain", in: Shutdown{Drain: ptr(-1)}, wantErr: true},
{name: "negative server", in: Shutdown{Server: ptr(-1)}, wantErr: true},
{name: "negative cleanup", in: Shutdown{Cleanup: ptr(-1)}, wantErr: true},
{name: "negative grace", in: Shutdown{Grace: ptr(-1)}, wantErr: true},
{name: "explicit zeros are not negative", in: Shutdown{Drain: ptr(0), Server: ptr(0), Cleanup: ptr(0)}},
} {
t.Run(tc.name, func(t *testing.T) {
_, err := tc.in.Budget()
if tc.wantErr && err == nil {
t.Fatal("Budget() accepted a negative number of seconds")
}
if !tc.wantErr && err != nil {
t.Fatalf("Budget() = %v, want the zeros taken literally", err)
}
})
}
}
// The message has to name every field that is wrong, not the first one: a
// caller who fixes one and gets the same error back learns to distrust it.
func TestShutdownBudgetNamesEveryNegativeField(t *testing.T) {
_, err := Shutdown{Drain: ptr(-1), Server: ptr(-30), Cleanup: ptr(-3), Grace: ptr(-9)}.Budget()
if err == nil {
t.Fatal("Budget() accepted four negative values")
}
for _, name := range []string{"drain", "server", "cleanup", "grace"} {
if !strings.Contains(err.Error(), name) {
t.Errorf("%q does not name %s", err, name)
}
}
}
// The sum is what has to fit inside the orchestrator's grace period, and the
// verdict is only reached when a grace period was configured. A fixed
// threshold instead would warn about the manifest this repository ships.
func TestShutdownBudgetOverrun(t *testing.T) {
resolved := func(s Shutdown) ShutdownBudget {
b, err := s.Budget()
if err != nil {
t.Fatalf("Budget() = %v", err)
}
return b
}
for _, tc := range []struct {
name string
budget ShutdownBudget
wantTotal int
wantOverrun int
}{
{
name: "defaults, no grace period to judge against",
budget: resolved(Shutdown{}),
wantTotal: 8,
},
{
name: "fits with room to spare",
budget: resolved(Shutdown{Drain: ptr(10), Grace: ptr(30)}),
wantTotal: 18,
},
{
// Equal is not a fit. The grace period is when SIGKILL is sent, so
// a budget that ends exactly then leaves the last callback no time
// to return.
name: "exactly equal still overruns",
budget: resolved(Shutdown{Drain: ptr(22), Grace: ptr(30)}),
wantTotal: 30,
wantOverrun: 1,
},
{
name: "over by five",
budget: resolved(Shutdown{Drain: ptr(26), Grace: ptr(30)}),
wantTotal: 34,
wantOverrun: 5,
},
{
// The reason the threshold is a configured value rather than a
// constant: the same budget is wrong under `docker stop` and right
// under a Kubernetes default.
name: "the docker default is the tighter one",
budget: resolved(Shutdown{Drain: ptr(10), Grace: ptr(10)}),
wantTotal: 18,
wantOverrun: 9,
},
} {
t.Run(tc.name, func(t *testing.T) {
if got := tc.budget.Total(); got != tc.wantTotal {
t.Errorf("Total() = %d, want %d", got, tc.wantTotal)
}
if got := tc.budget.Overrun(); got != tc.wantOverrun {
t.Errorf("Overrun() = %d, want %d", got, tc.wantOverrun)
}
if over := tc.budget.Overrun(); over > 0 && tc.budget.Total()-over >= tc.budget.Grace {
t.Errorf("Overrun() = %d does not bring %d under the %d grace period",
over, tc.budget.Total(), tc.budget.Grace)
}
})
}
}
+34
View File
@@ -82,6 +82,40 @@ settings:
# 会被负载均衡、监控和压测统计成成功)。
rateLimit:
inboundQPS: 200
# shutdown budgets, in seconds. The three are spent one after the other,
# and their sum has to stay inside the stop grace period the orchestrator
# allows - once it is up, SIGKILL arrives part-way through the cleanup
# callbacks, which is worse than not draining at all.
shutdown:
# How long to keep serving normally after a stop signal arrives. For that
# long /ready answers 503 and keep-alive is switched off, which is what
# gives a load balancer time to take this instance out of rotation before
# the listener closes.
#
# What to set depends on who removes this instance and on what basis. A
# load balancer that polls /ready itself needs at least "check interval x
# failure threshold + however long removal takes to apply". A Kubernetes
# Service removes the endpoint when the Pod is deleted, concurrently with
# SIGTERM and regardless of what the probe returns, so here this covers
# the delay in that removal reaching every node.
#
# 0 by default: a deployment that leaves this alone shuts down exactly as
# it did before this section existed. It also means /ready never reports
# draining - the flip and the closed listener are microseconds apart, and
# no poller reads anything in between.
drain: 0
# How long to wait for in-flight requests once the listener is closed.
server: 5
# How long the BeforeExit cleanup callbacks get after that.
cleanup: 3
# The stop grace period the orchestrator gives this process - `docker stop
# --timeout`, or terminationGracePeriodSeconds. Nothing reads it during a
# shutdown; start-up uses it to say whether the three budgets above fit
# inside it, and warns when they do not. Left out, nothing is compared:
# the reference values are 10s for docker and 30s for Kubernetes, three
# times apart, and a fixed threshold would warn about correct
# configurations.
#grace: 30
# fileStore 对象存储。上传接口的 source 参数决定走哪一家:
# source=1 只存本地,source=2 阿里云 OSS,source=3 七牛 Kodo
# 没有填的那一家在被请求时会返回明确错误,不会静默存到别处。
+34
View File
@@ -66,6 +66,40 @@ settings:
# 会被负载均衡、监控和压测统计成成功)。
rateLimit:
inboundQPS: 200
# shutdown budgets, in seconds. The three are spent one after the other,
# and their sum has to stay inside the stop grace period the orchestrator
# allows - once it is up, SIGKILL arrives part-way through the cleanup
# callbacks, which is worse than not draining at all.
shutdown:
# How long to keep serving normally after a stop signal arrives. For that
# long /ready answers 503 and keep-alive is switched off, which is what
# gives a load balancer time to take this instance out of rotation before
# the listener closes.
#
# What to set depends on who removes this instance and on what basis. A
# load balancer that polls /ready itself needs at least "check interval x
# failure threshold + however long removal takes to apply". A Kubernetes
# Service removes the endpoint when the Pod is deleted, concurrently with
# SIGTERM and regardless of what the probe returns, so here this covers
# the delay in that removal reaching every node.
#
# 0 by default: a deployment that leaves this alone shuts down exactly as
# it did before this section existed. It also means /ready never reports
# draining - the flip and the closed listener are microseconds apart, and
# no poller reads anything in between.
drain: 0
# How long to wait for in-flight requests once the listener is closed.
server: 5
# How long the BeforeExit cleanup callbacks get after that.
cleanup: 3
# The stop grace period the orchestrator gives this process - `docker stop
# --timeout`, or terminationGracePeriodSeconds. Nothing reads it during a
# shutdown; start-up uses it to say whether the three budgets above fit
# inside it, and warns when they do not. Left out, nothing is compared:
# the reference values are 10s for docker and 30s for Kubernetes, three
# times apart, and a fixed threshold would warn about correct
# configurations.
#grace: 30
cache:
# redis:
# addr: 127.0.0.1:6379
+68
View File
@@ -0,0 +1,68 @@
package config
import (
"testing"
coreconfig "github.com/go-admin-team/go-admin-core/v2/config"
"github.com/go-admin-team/go-admin-core/v2/config/source/file"
)
// shippedSettings is the shape the loader fills in, cut down to the part under
// test. The reader is JSON-based, so the keys are matched against field names
// case-insensitively - which is exactly the matching that silently drops a
// section the struct has no field for.
type shippedSettings struct {
Settings struct {
Extend Extend
}
}
func (*shippedSettings) OnChange() {}
// The shutdown section has to arrive where it is read from, and with the
// values the documentation claims.
//
// This is the failure this batch exists to remove, one level up: the loader
// discards keys no field matches, without an error and without a log line, so
// a section put in the wrong place is written, accepted, and never applied.
// Nothing but loading the shipped file through the real loader can tell the
// two apart - the struct compiles either way.
//
// The values are asserted as well as the arrival. A settings.yml that shipped
// a different default from config.Default*Seconds would give two answers to
// "what does an unconfigured deployment spend", and the file is the one people
// read.
func TestTheShippedSettingsReachTheShutdownStruct(t *testing.T) {
for _, name := range []string{"settings.yml", "settings.full.yml"} {
t.Run(name, func(t *testing.T) {
var loaded shippedSettings
c, err := coreconfig.NewConfig(
coreconfig.WithSource(file.NewSource(file.WithPath(name))),
coreconfig.WithEntity(&loaded),
)
if err != nil {
t.Fatalf("load %s: %v", name, err)
}
t.Cleanup(func() { _ = c.Close() })
s := loaded.Settings.Extend.Shutdown
if s.Drain == nil || s.Server == nil || s.Cleanup == nil {
t.Fatalf("%s left extend.shutdown unfilled (%+v); the section is written but nothing reads it",
name, s)
}
want := ShutdownBudget{
Drain: DefaultDrainSeconds,
Server: DefaultServerSeconds,
Cleanup: DefaultCleanupSeconds,
}
got, err := s.Budget()
if err != nil {
t.Fatalf("%s does not resolve: %v", name, err)
}
if got != want {
t.Errorf("%s ships %+v, want the documented defaults %+v", name, got, want)
}
})
}
}
+7
View File
@@ -7,6 +7,13 @@ services:
restart: always
ports:
- 8000:8000
# Compose allows 10 seconds by default, and this process spends
# drain + server + cleanup from extend.shutdown before it exits - 8 out of
# the box, more for anyone who configures a drain window. Past the deadline
# it is sent SIGKILL and the cleanup callbacks are cut off part-way
# through. checksilent's docker-stop-cuts-shutdown-short check compares
# this against config/settings.yml.
stop_grace_period: 30s
volumes:
- ./config/:/go-admin-api/config/
- ./static/:/go-admin-api/static/
+3 -2
View File
@@ -803,5 +803,6 @@ core 里那行注释自己写着「The interface has no way to report this to th
`checksilent` 一个文件都看不到。所以它保的是**这个仓库和它的 fork**,
不是你的应用——你的应用要自己跑自己的检查。
`checksilent` 还检查另外五类"不出声的失败",写模块时值得先看一眼
`go run ./tools/checksilent -h`。
`checksilent` 还检查其他几类"不出声的失败",写模块时值得先看一眼
`AGENTS.md` 的「静默失败校验」一节,或者 `tools/checksilent/checks.go` 里的
`runChecks`(`-h` 只打印命令行参数,不列检查)。
+2 -2
View File
@@ -11,7 +11,7 @@ require (
github.com/casbin/casbin/v3 v3.8.1
github.com/gin-gonic/gin v1.12.0
github.com/glebarez/sqlite v1.11.0
github.com/go-admin-team/go-admin-core/v2 v2.7.0
github.com/go-admin-team/go-admin-core/v2 v2.8.0
github.com/google/uuid v1.6.0
github.com/huaweicloud/huaweicloud-sdk-go-obs v3.26.6+incompatible
github.com/mssola/user_agent v0.6.0
@@ -26,6 +26,7 @@ require (
github.com/swaggo/gin-swagger v1.6.1
github.com/swaggo/swag v1.16.6
github.com/unrolled/secure v1.17.0
go.yaml.in/yaml/v3 v3.0.5
golang.org/x/crypto v0.54.0
gorm.io/driver/mysql v1.6.0
gorm.io/driver/postgres v1.6.2
@@ -126,7 +127,6 @@ require (
go.uber.org/atomic v1.11.0 // indirect
go.uber.org/multierr v1.11.0 // indirect
go.uber.org/zap v1.27.1 // indirect
go.yaml.in/yaml/v3 v3.0.5 // indirect
golang.org/x/arch v0.30.0 // indirect
golang.org/x/exp v0.0.0-20251219203646-944ab1f22d93 // indirect
golang.org/x/image v0.41.0 // indirect
+2 -6
View File
@@ -145,12 +145,8 @@ github.com/glebarez/go-sqlite v1.22.0 h1:uAcMJhaA6r3LHMTFgP0SifzgXg46yJkgxqyuyec
github.com/glebarez/go-sqlite v1.22.0/go.mod h1:PlBIdHe0+aUEFn+r2/uthrWq4FxbzugL0L8Li6yQJbc=
github.com/glebarez/sqlite v1.11.0 h1:wSG0irqzP6VurnMEpFGer5Li19RpIRi2qvQz++w0GMw=
github.com/glebarez/sqlite v1.11.0/go.mod h1:h8/o8j5wiAsqSPoWELDUdJXhjAhsVliSn7bWZjOhrgQ=
github.com/go-admin-team/go-admin-core/v2 v2.5.0 h1:aD1SALklBxizGB9u8cOgm4OT8z656FM83F4fD6dMz9g=
github.com/go-admin-team/go-admin-core/v2 v2.5.0/go.mod h1:LG/XvEfOplbuadKrPTPm0Nu5pN06aQUNZZC3ao4B4gs=
github.com/go-admin-team/go-admin-core/v2 v2.6.0 h1:sRoZaxniTpbe287uR/uWpA14Jl1GTAcfGXLKoBLph2w=
github.com/go-admin-team/go-admin-core/v2 v2.6.0/go.mod h1:LG/XvEfOplbuadKrPTPm0Nu5pN06aQUNZZC3ao4B4gs=
github.com/go-admin-team/go-admin-core/v2 v2.7.0 h1:1qV0/5iFBvkE3BRtm4ip0v0QYG9Fgx4UtOTd8zkQT9c=
github.com/go-admin-team/go-admin-core/v2 v2.7.0/go.mod h1:LG/XvEfOplbuadKrPTPm0Nu5pN06aQUNZZC3ao4B4gs=
github.com/go-admin-team/go-admin-core/v2 v2.8.0 h1:ZTw5Z/UT1/7OltbGPEaEVerRk4z3koB6O8nDbb84tPM=
github.com/go-admin-team/go-admin-core/v2 v2.8.0/go.mod h1:LG/XvEfOplbuadKrPTPm0Nu5pN06aQUNZZC3ao4B4gs=
github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as=
github.com/go-kit/kit v0.10.0/go.mod h1:xUsJbQ/Fp4kEt7AFgCuvyX4a71u8h9jB8tj/ORgOZ7o=
+61
View File
@@ -22,6 +22,21 @@ metadata:
app: go-admin
version: v1
spec:
# One replica, and the drain window below buys nothing at one replica: there
# is nowhere to send the traffic this pod stops taking. Raising it needs two
# changes that are not this number:
#
# The volume below is shared by every replica, and the log path in
# settings.yml lives on it, so a second pod would append to the same
# rotating file.
#
# The job scheduler is per process while its handle on a job is one shared
# column. Startup runs `UPDATE sys_job SET entry_id = 0 WHERE entry_id > 0`
# across the whole table (app/jobs/jobbase.go), so a second pod erases the
# first pod's ids and writes its own, and every pod registers the whole
# enabled list in its own scheduler. Neither symptom logs anything: an
# enabled job fires once per pod, and stopping one from the UI removes an
# entry from the wrong process and still answers 200. See #915.
replicas: 1
selector:
matchLabels:
@@ -39,6 +54,40 @@ spec:
imagePullPolicy: IfNotPresent
ports:
- containerPort: 8000
# Readiness answers "send me requests". It fails while the database or
# the cache is unreachable, so this pod stays out of the Service until
# the datastore settings.yml names is really there - which is a change
# from having no probe at all, where a pod with an unreachable database
# was still sent traffic.
#
# timeoutSeconds is 3 rather than the default 1 because the handler
# allows its checks 2 seconds (readyTimeout in
# app/other/router/monitor.go). At the default, a database that answers
# in 1.2s is recorded as a failed check while the handler is returning
# 200.
readinessProbe:
httpGet:
path: /api/v1/ready
port: 8000
periodSeconds: 5
timeoutSeconds: 3
failureThreshold: 3
# Liveness answers "restart me", which is a different question: a
# process whose database is unreachable does not want restarting, so
# this points at /health, which is a bare 200. Both probes skip the
# rate limiter - see exemptProbes in cmd/api/server.go - because a
# liveness probe that collects 429s under load gets the container
# restarted at the moment the deployment can least afford to lose it.
#
# initialDelaySeconds covers the migrations, which run before the
# listener opens.
livenessProbe:
httpGet:
path: /api/v1/health
port: 8000
initialDelaySeconds: 15
periodSeconds: 10
failureThreshold: 3
volumeMounts:
- name: go-admin
mountPath: /temp
@@ -47,6 +96,18 @@ spec:
- name: go-admin-config
mountPath: /config/
readOnly: true
# SIGKILL arrives when this is up, so it has to be longer than what the
# process spends shutting down: extend.shutdown's drain + server +
# cleanup, which settings.yml ships as 0 + 5 + 3. Raise drain here and
# this number has to follow, or the cleanup callbacks are cut off
# part-way through - checksilent's shutdown-budget-overruns-grace check
# is what notices.
#
# No preStop hook on purpose. A sleep there would be spent before the
# process is told anything, so BeginDraining never runs and /ready
# answers 200 for the whole of it - and it would be added to the budget
# above rather than replacing any of it.
terminationGracePeriodSeconds: 30
volumes:
- name: go-admin
persistentVolumeClaim:
+13
View File
@@ -21,6 +21,8 @@ const (
checkImportBoundary = "contract-import-boundary"
checkShimAlias = "contract-shim-alias"
checkDataScopeRoute = "datascope-route-unguarded"
checkShutdownGrace = "shutdown-budget-overruns-grace"
checkDockerStop = "docker-stop-cuts-shutdown-short"
)
// Package paths, relative to the module. Spelled once so a module rename
@@ -50,6 +52,17 @@ func runChecks(s *snapshot, opt options) ([]Finding, error) {
out = append(out, checkContractShimAlias(s)...)
out = append(out, checkDataScopeRoutes(s)...)
for _, run := range []func(*snapshot) ([]Finding, error){
checkShutdownBudgetFitsGrace,
checkDockerStopGrace,
} {
fs, err := run(s)
if err != nil {
return nil, err
}
out = append(out, fs...)
}
if opt.UIDir != "" {
fs, err := checkMenuNames(s, opt.UIDir)
if err != nil {
+17 -7
View File
@@ -1,13 +1,23 @@
// Command checksilent reports the failures in this repository that do not
// announce themselves: no error, no log line, behaviour quietly wrong.
//
// Seven checks, six of them ERROR and one WARN. An ERROR fails the run; a WARN
// prints and does not. The split is not about how bad the consequence is - all
// seven are bad - but about how certain the detection is. Everything reported as
// an ERROR is decided from this repository's own syntax. The one WARN compares
// against a second repository through a regular expression, and a check that
// can be wrong must not be able to stop a build, or the first response to it
// will be an ignore comment.
// An ERROR fails the run; a WARN prints and does not. The split is not about
// how bad the consequence is - every one of these is bad - but about how much
// room is left to act.
//
// Most of them report only ERROR: each is decided from this repository's own
// files and is either true or not. The menu-name check reports only WARN,
// because it compares against a second repository through a regular
// expression, and a check that can be wrong must not be able to stop a build
// or the first response to it will be an ignore comment. The two
// shutdown-budget checks report at both levels from one arithmetic: a budget
// that already overruns is an ERROR, and one that fits with no headroom left
// is a WARN - it works today, so failing the build on it would be failing a
// correct configuration.
//
// The list of checks is runChecks in checks.go. It is deliberately not
// repeated here as a count: the two places that carried one were both wrong by
// the time anybody looked.
//
// Usage:
//
+651
View File
@@ -0,0 +1,651 @@
package main
import (
"bytes"
"errors"
"fmt"
"io"
"os"
"path/filepath"
"regexp"
"strconv"
"strings"
yaml "go.yaml.in/yaml/v3"
)
// The files this check compares, and the package the fallbacks come from.
const (
settingsFile = "config/settings.yml"
k8sDeployFile = "scripts/k8s/deploy.yml"
pkgHostConfig = "config"
drainConstName = "DefaultDrainSeconds"
serverConstName = "DefaultServerSeconds"
cleanupConstName = "DefaultCleanupSeconds"
)
// graceMarginSeconds is the headroom a shutdown budget needs beyond itself.
//
// Spelled once and used by both checks below, because they fail the same way:
// somebody raises a budget in config/settings.yml and does not go looking for
// the two other places that have to allow room for it. Two margins would
// eventually be two different numbers.
const graceMarginSeconds = 5
// checkShutdownBudgetFitsGrace compares the shutdown budget this repository
// ships against the stop grace period its own Kubernetes manifest allows.
//
// The two are not merely adjacent examples. scripts/k8s/prerun.sh builds the
// settings-admin ConfigMap out of config/settings.yml, and the Deployment
// mounts that ConfigMap - so the manifest deploys that file.
//
// The budgets are spent one after the other, and when their sum reaches
// terminationGracePeriodSeconds the kubelet sends SIGKILL while the cleanup
// callbacks are still running. Nothing reports it: the pod disappears
// mid-shutdown and it reads as a crash rather than as a number that was raised
// in one file and not the other. Which is how it would be raised - drain is
// the interesting knob and the grace period is in a different directory.
//
// Two levels, and an overrun is not also reported as a shortage of headroom:
// every Error satisfies the Warn condition too, and an Error that always drags
// a duplicate Warn behind it teaches people to skip Warns.
//
// A preStop hook counts, even though the shipped manifest has none. It is
// spent before the process is told anything, so it is added to the budget
// rather than overlapping it - and a self-check that cannot see it would
// understate the real cost by however long somebody set it to, which is worse
// than not checking.
//
// It reports nothing when either file is absent and when the manifest sets no
// grace period, because there is then no second number to disagree with.
func checkShutdownBudgetFitsGrace(s *snapshot) ([]Finding, error) {
budget, ok, err := shippedShutdownBudget(s)
if err != nil || !ok {
return nil, err
}
m, ok, err := readManifest(s)
if err != nil || !ok {
return nil, err
}
if m.grace == nil {
return nil, nil
}
var out []Finding
if m.preStopUnreadable {
out = append(out, Finding{
Check: checkShutdownGrace,
Severity: Warn.String(),
File: k8sDeployFile,
Line: m.preStopLine,
Col: 1,
Message: "this preStop hook is not a sleep, so how long it takes cannot be read here " +
"and is not in the sum below; it is spent before the process is told anything, " +
"so whatever it costs has to fit inside terminationGracePeriodSeconds as well.",
severity: Warn,
})
}
total := m.preStop + budget.drain + budget.server + budget.cleanup
grace := *m.grace
spelled := fmt.Sprintf("preStop %d + drain %d + server %d + cleanup %d",
m.preStop, budget.drain, budget.server, budget.cleanup)
switch {
case total >= grace:
out = append(out, Finding{
Check: checkShutdownGrace,
Severity: Error.String(),
File: k8sDeployFile,
Line: m.graceLine,
Col: 1,
Message: fmt.Sprintf(
"terminationGracePeriodSeconds is %d and the shutdown takes %d (%s, from %s); "+
"SIGKILL would arrive while the cleanup callbacks are still running. "+
"Raise it to %d, or take %d off the budget.",
grace, total, spelled, settingsFile,
total+graceMarginSeconds, total+graceMarginSeconds-grace),
severity: Error,
})
case total+graceMarginSeconds > grace:
out = append(out, Finding{
Check: checkShutdownGrace,
Severity: Warn.String(),
File: k8sDeployFile,
Line: m.graceLine,
Col: 1,
Message: fmt.Sprintf(
"terminationGracePeriodSeconds is %d and the shutdown takes %d (%s, from %s), "+
"which leaves under %ds of headroom; a callback that runs slightly long is "+
"cut off. Raise it to %d.",
grace, total, spelled, settingsFile, graceMarginSeconds, total+graceMarginSeconds),
severity: Warn,
})
}
return out, nil
}
// dockerStopArgs matches a stop command in a script or a workflow.
var (
dockerStopArgs = regexp.MustCompile(`\bdocker\s+stop\b`)
// --timeout is the current name, --time its deprecated spelling and -t the
// short form; docker still accepts all three, so all three are read. The
// long name comes first because --time is a prefix of it, and a flag that
// the check cannot read is reported as no deadline at all - which would
// have this tool pressing people towards the deprecated spelling.
dockerStopTime = regexp.MustCompile(`(--timeout|--time|-t)[=\s]*(\d+)`)
)
// checkDockerStopGrace reports a stop path that does not allow this process
// the time it spends shutting down.
//
// docker allows ten seconds unless told otherwise, and that number is nowhere
// near the command - so a budget raised in config/settings.yml passes every
// test, deploys, and then has its cleanup callbacks killed on the next
// release. Same failure as the manifest's grace period, same arithmetic, same
// margin; only the file it lives in is different.
//
// Both ways of stopping this repository's container are covered, because
// covering one of two identical paths is what produces a clean run that means
// nothing: `docker stop` in a workflow or a script, and stop_grace_period in
// the compose file the Makefile's own `make run` uses.
//
// An absent deadline is reported rather than assumed to be ten: the value that
// applies is then invisible at the call site and cannot follow the budget.
func checkDockerStopGrace(s *snapshot) ([]Finding, error) {
budget, ok, err := shippedShutdownBudget(s)
if err != nil || !ok {
return nil, err
}
total := budget.drain + budget.server + budget.cleanup
spelled := fmt.Sprintf("drain %d + server %d + cleanup %d",
budget.drain, budget.server, budget.cleanup)
sites, err := findStopDeadlines(s)
if err != nil {
return nil, err
}
var out []Finding
for _, site := range sites {
finding := Finding{
Check: checkDockerStop,
File: site.file,
Line: site.line,
Col: 1,
}
switch {
case !site.set:
finding.Severity, finding.severity = Error.String(), Error
finding.Message = fmt.Sprintf(
"%s allows the default %d seconds, and this process spends %d shutting down "+
"(%s, from %s). %s.",
site.what, dockerDefaultGraceSeconds, total, spelled, settingsFile,
site.fix(total+graceMarginSeconds))
case site.seconds <= total:
finding.Severity, finding.severity = Error.String(), Error
finding.Message = fmt.Sprintf(
"%s allows %d seconds and this shutdown takes %d (%s, from %s); the cleanup "+
"callbacks are killed part-way through. %s.",
site.what, site.seconds, total, spelled, settingsFile,
site.fix(total+graceMarginSeconds))
case site.seconds < total+graceMarginSeconds:
finding.Severity, finding.severity = Warn.String(), Warn
finding.Message = fmt.Sprintf(
"%s allows %d seconds over a shutdown that takes %d (%s, from %s), which leaves "+
"under %ds of headroom. %s.",
site.what, site.seconds, total, spelled, settingsFile, graceMarginSeconds,
site.fix(total+graceMarginSeconds))
default:
continue
}
out = append(out, finding)
}
return out, nil
}
// dockerDefaultGraceSeconds is what docker allows a container to stop in when
// nothing says otherwise. It applies to `docker stop` and to compose alike.
const dockerDefaultGraceSeconds = 10
// stopSite is one place this repository decides how long a container gets.
type stopSite struct {
file string
line int
// what names the setting in the finding, in the spelling of the file it
// was found in.
what string
// compose says which of the two fixes to suggest.
compose bool
seconds int
set bool
}
func (s stopSite) fix(seconds int) string {
if s.compose {
return fmt.Sprintf("Set stop_grace_period: %ds", seconds)
}
return fmt.Sprintf("Pass --timeout %d", seconds)
}
func findStopDeadlines(s *snapshot) ([]stopSite, error) {
sites, err := findDockerStops(s.Root)
if err != nil {
return nil, err
}
compose, err := findComposeServices(s)
if err != nil {
return nil, err
}
return append(sites, compose...), nil
}
// dockerStopExtensions and dockerStopNames are where a stop command can be
// written in this repository: workflows, shell scripts and the Makefile.
var (
dockerStopExtensions = map[string]bool{".yml": true, ".yaml": true, ".sh": true, ".bash": true}
dockerStopNames = map[string]bool{"Makefile": true, "makefile": true}
)
func findDockerStops(root string) ([]stopSite, error) {
var out []stopSite
err := filepath.Walk(root, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
if info.IsDir() {
if path != root && skippedDirs[info.Name()] {
return filepath.SkipDir
}
return nil
}
if !dockerStopExtensions[filepath.Ext(path)] && !dockerStopNames[info.Name()] {
return nil
}
b, err := os.ReadFile(path)
if err != nil {
return err
}
rel, err := filepath.Rel(root, path)
if err != nil {
return err
}
for i, line := range strings.Split(string(b), "\n") {
// A commented-out command is not one that runs, and the settings
// file describes `docker stop` in prose right beside the budget
// this check reads.
if trimmed := strings.TrimSpace(line); strings.HasPrefix(trimmed, "#") {
continue
}
if !dockerStopArgs.MatchString(line) {
continue
}
site := stopSite{
file: filepath.ToSlash(rel),
line: i + 1,
what: "`docker stop` with no --timeout",
}
if m := dockerStopTime.FindStringSubmatch(line); m != nil {
seconds, err := strconv.Atoi(m[2])
if err != nil {
continue
}
// Quoted back in the spelling it was written in, so the
// message cannot misreport what the line says.
site.what = fmt.Sprintf("`docker stop %s %d`", m[1], seconds)
site.seconds, site.set = seconds, true
}
out = append(out, site)
}
return nil
})
return out, err
}
type shutdownSeconds struct{ drain, server, cleanup int }
// shippedShutdownBudget reads extend.shutdown out of the settings file this
// repository ships, filling in whatever it leaves out from the Go constants
// that do the same at run time.
//
// Taking the fallbacks from the snapshot rather than repeating 0/5/3 here is
// what keeps this honest when the defaults move: a tool that carries its own
// copy of the number it is checking eventually checks the wrong one.
//
// A negative value is left alone. config.Shutdown.Budget refuses it and the
// server does not start, so it is not a failure that passes unnoticed - and
// adding a negative into the sums above would understate them.
func shippedShutdownBudget(s *snapshot) (shutdownSeconds, bool, error) {
raw, ok, err := readRepoFile(s, settingsFile)
if err != nil || !ok {
return shutdownSeconds{}, false, err
}
var doc struct {
Settings struct {
Extend struct {
Shutdown *struct {
Drain *int `yaml:"drain"`
Server *int `yaml:"server"`
Cleanup *int `yaml:"cleanup"`
} `yaml:"shutdown"`
} `yaml:"extend"`
} `yaml:"settings"`
}
if err := yaml.Unmarshal(raw, &doc); err != nil {
return shutdownSeconds{}, false, fmt.Errorf("%s: %w", settingsFile, err)
}
section := doc.Settings.Extend.Shutdown
if section == nil {
return shutdownSeconds{}, false, nil
}
defaults, ok := s.hostConfigDefaults()
if !ok {
// The constants moved or were renamed. Reporting nothing would let the
// check go quiet, which is the failure it exists to catch, so this
// stops the run instead.
return shutdownSeconds{}, false, fmt.Errorf(
"%s has extend.shutdown but package %s declares no %s/%s/%s to fall back on",
settingsFile, pkgHostConfig, drainConstName, serverConstName, cleanupConstName)
}
budget := shutdownSeconds{
drain: orDefault(section.Drain, defaults.drain),
server: orDefault(section.Server, defaults.server),
cleanup: orDefault(section.Cleanup, defaults.cleanup),
}
if budget.drain < 0 || budget.server < 0 || budget.cleanup < 0 {
return shutdownSeconds{}, false, nil
}
return budget, true, nil
}
func orDefault(configured *int, fallback int) int {
if configured != nil {
return *configured
}
return fallback
}
// hostConfigDefaults reads the three fallback constants out of the parsed tree.
func (s *snapshot) hostConfigDefaults() (shutdownSeconds, bool) {
for _, sf := range s.Files {
if sf.Pkg != s.pkg(pkgHostConfig) {
continue
}
drain, okDrain := sf.consts[drainConstName]
server, okServer := sf.consts[serverConstName]
cleanup, okCleanup := sf.consts[cleanupConstName]
if okDrain && okServer && okCleanup {
return shutdownSeconds{int(drain), int(server), int(cleanup)}, true
}
}
return shutdownSeconds{}, false
}
// manifest is what the shipped Deployment says about how long it will wait.
type manifest struct {
grace *int
graceLine int
// preStop is the longest sleep any container's hook performs, since the
// hooks of several containers run at the same time.
preStop int
preStopLine int
preStopUnreadable bool
}
var preStopSleep = regexp.MustCompile(`\bsleep\s+(\d+)s?\b`)
// readManifest finds the grace period and the preStop hooks in the shipped
// manifest, with the lines they are on so a finding can be opened at them.
//
// The file holds several documents and only the Deployment carries a pod
// template, so every document is decoded and the first one with a grace period
// wins.
func readManifest(s *snapshot) (manifest, bool, error) {
raw, ok, err := readRepoFile(s, k8sDeployFile)
if err != nil || !ok {
return manifest{}, false, err
}
dec := yaml.NewDecoder(bytes.NewReader(raw))
for {
var doc struct {
Spec struct {
Template struct {
Spec struct {
Grace *int `yaml:"terminationGracePeriodSeconds"`
Containers []struct {
Lifecycle struct {
// A value, not a pointer: yaml.v3 only hands
// the raw node to a field of type yaml.Node,
// and a *yaml.Node field is allocated and left
// empty - which reads as "the hook is there but
// unreadable" for every manifest that has one.
PreStop yaml.Node `yaml:"preStop"`
} `yaml:"lifecycle"`
} `yaml:"containers"`
} `yaml:"spec"`
} `yaml:"template"`
} `yaml:"spec"`
}
switch err := dec.Decode(&doc); {
case errors.Is(err, io.EOF):
return manifest{}, false, nil
case err != nil:
return manifest{}, false, fmt.Errorf("%s: %w", k8sDeployFile, err)
}
pod := doc.Spec.Template.Spec
if pod.Grace == nil && len(pod.Containers) == 0 {
continue
}
m := manifest{
grace: pod.Grace,
graceLine: lineOf(raw, "terminationGracePeriodSeconds:"),
}
for _, c := range pod.Containers {
hook := c.Lifecycle.PreStop
if hook.Kind == 0 {
continue
}
m.preStopLine = hook.Line
if seconds, ok := preStopSeconds(&hook); ok {
// The longest one, not the sum: the hooks of several
// containers run at the same time.
if seconds > m.preStop {
m.preStop = seconds
}
continue
}
m.preStopUnreadable = true
}
if m.grace == nil {
continue
}
return m, true, nil
}
}
// preStopSeconds reads how long a hook sleeps for.
//
// Every scalar under the hook is joined and searched, because the sleep can be
// written as one argument or as several: ["sh","-c","sleep 10"] and
// ["sleep","10"] both wait ten seconds.
func preStopSeconds(node *yaml.Node) (int, bool) {
var words []string
var walk func(*yaml.Node)
walk = func(n *yaml.Node) {
if n == nil {
return
}
if n.Kind == yaml.ScalarNode {
words = append(words, n.Value)
}
for _, child := range n.Content {
walk(child)
}
}
walk(node)
m := preStopSleep.FindStringSubmatch(strings.Join(words, " "))
if m == nil {
return 0, false
}
seconds, err := strconv.Atoi(m[1])
if err != nil {
return 0, false
}
return seconds, true
}
// lineOf locates a key for a finding's position. A miss reports line 1 rather
// than failing: the position is where to look, and the message is the finding.
func lineOf(content []byte, key string) int {
for i, l := range strings.Split(string(content), "\n") {
if strings.Contains(l, key) && !strings.HasPrefix(strings.TrimSpace(l), "#") {
return i + 1
}
}
return 1
}
// readRepoFile reads a file relative to the scanned root, reporting absence
// rather than failing on it: the checks run over fixtures that carry only what
// the check under test needs.
func readRepoFile(s *snapshot, rel string) ([]byte, bool, error) {
b, err := os.ReadFile(filepath.Join(s.Root, filepath.FromSlash(rel)))
switch {
case errors.Is(err, os.ErrNotExist):
return nil, false, nil
case err != nil:
return nil, false, err
}
return b, true, nil
}
// composeFiles are the names Docker Compose looks for, in its own order of
// preference.
var composeFiles = []string{"compose.yaml", "compose.yml", "docker-compose.yaml", "docker-compose.yml"}
// composeDuration matches the durations compose accepts for
// stop_grace_period: a bare number of seconds, or hours, minutes and seconds
// in that order.
var composeDuration = regexp.MustCompile(`^(?:(\d+)h)?(?:(\d+)m)?(?:(\d+)s?)?$`)
// findComposeServices reports the stop_grace_period of every compose service
// that runs this repository's own image.
//
// Only those services. The grace period of a database or a cache alongside it
// is not this process's shutdown budget, and reporting one against the other
// would be arithmetic about two unrelated things.
func findComposeServices(s *snapshot) ([]stopSite, error) {
var out []stopSite
for _, name := range composeFiles {
raw, ok, err := readRepoFile(s, name)
if err != nil {
return nil, err
}
if !ok {
continue
}
var root yaml.Node
if err := yaml.Unmarshal(raw, &root); err != nil {
return nil, fmt.Errorf("%s: %w", name, err)
}
if len(root.Content) == 0 {
continue
}
services := mapValue(root.Content[0], "services")
if services == nil {
continue
}
for i := 0; i+1 < len(services.Content); i += 2 {
key, service := services.Content[i], services.Content[i+1]
if !runsThisRepo(service, s.ModulePath) {
continue
}
site := stopSite{
file: name,
line: key.Line,
what: fmt.Sprintf("service %s, which sets no stop_grace_period,", key.Value),
compose: true,
}
if grace := mapValue(service, "stop_grace_period"); grace != nil {
seconds, ok := composeSeconds(grace.Value)
if !ok {
// A duration this cannot read is left alone rather than
// guessed at: compose knows what it means, and inventing a
// number here would report against a value nobody wrote.
continue
}
site.line = grace.Line
site.what = fmt.Sprintf("stop_grace_period on service %s", key.Value)
site.seconds, site.set = seconds, true
}
out = append(out, site)
}
}
return out, nil
}
// runsThisRepo reports whether a compose service starts the image this
// repository builds - by building it, or by naming it.
func runsThisRepo(service *yaml.Node, modulePath string) bool {
if mapValue(service, "build") != nil {
return true
}
image := mapValue(service, "image")
if image == nil {
return false
}
repository := image.Value
if i := strings.LastIndex(repository, ":"); i > strings.LastIndex(repository, "/") {
repository = repository[:i]
}
return baseName(repository) == baseName(modulePath)
}
func baseName(path string) string {
if i := strings.LastIndex(path, "/"); i >= 0 {
return path[i+1:]
}
return path
}
func composeSeconds(value string) (int, bool) {
m := composeDuration.FindStringSubmatch(strings.TrimSpace(value))
if m == nil || m[1]+m[2]+m[3] == "" {
return 0, false
}
var total int
for i, unit := range []int{3600, 60, 1} {
if m[i+1] == "" {
continue
}
n, err := strconv.Atoi(m[i+1])
if err != nil {
return 0, false
}
total += n * unit
}
return total, true
}
// mapValue returns the value a mapping node holds for key.
func mapValue(node *yaml.Node, key string) *yaml.Node {
if node == nil || node.Kind != yaml.MappingNode {
return nil
}
for i := 0; i+1 < len(node.Content); i += 2 {
if node.Content[i].Value == key {
return node.Content[i+1]
}
}
return nil
}
+484
View File
@@ -0,0 +1,484 @@
package main
import (
"strings"
"testing"
)
// hostConfigSource is the part of config/extend.go this check reads: the
// fallbacks it applies to whatever the settings file leaves out.
const hostConfigSource = `package config
const (
DefaultDrainSeconds = 0
DefaultServerSeconds = 5
DefaultCleanupSeconds = 3
)
`
// factorySettings is what this repository ships: 0 + 5 + 3.
const factorySettings = "settings:\n extend:\n shutdown:\n drain: 0\n server: 5\n cleanup: 3\n"
func settingsWith(shutdown string) string {
return "settings:\n extend:\n" + shutdown
}
// deployWith builds a manifest with the given container extras and pod-level
// lines, in the shape the shipped one has.
func deployWith(containerExtra, podExtra string) string {
return `---
apiVersion: v1
kind: Service
metadata:
name: go-admin
---
apiVersion: apps/v1
kind: Deployment
metadata:
name: go-admin-v1
spec:
replicas: 1
template:
spec:
containers:
- name: go-admin
image: go-admin
` + containerExtra + podExtra
}
func graceOf(seconds string) string {
return " terminationGracePeriodSeconds: " + seconds + "\n"
}
const preStopSleep25 = ` lifecycle:
preStop:
exec:
command: ["sh", "-c", "sleep 25"]
`
// The six scenarios worked through in the technical plan, plus the one that
// only fails when preStop is left out of the sum.
//
// The values matter. "raise server to 25" gives 28, which is under a grace
// period of 30 and reaches only the WARN level - it would not show that the
// ERROR level works at all.
func TestShutdownBudgetAgainstTheGracePeriod(t *testing.T) {
for _, tc := range []struct {
name string
settings string
deploy string
want Severity
contains string
}{
{
name: "the shipped defaults, with headroom",
settings: factorySettings,
deploy: deployWith("", graceOf("30")),
want: -1,
},
{
// The one this check exists for: drain is the interesting knob and
// the grace period is in another directory, so raising one and not
// the other is the natural mistake.
name: "the budget was raised and the manifest was not",
settings: settingsWith(" shutdown:\n drain: 0\n server: 30\n cleanup: 3\n"),
deploy: deployWith("", graceOf("30")),
want: Error,
contains: "preStop 0 + drain 0 + server 30 + cleanup 3",
},
{
// Equal is not a fit: the grace period is when SIGKILL is sent, so
// a budget that ends exactly then leaves nothing time to return.
name: "the grace period was lowered to the budget",
settings: factorySettings,
deploy: deployWith("", graceOf("8")),
want: Error,
},
{
name: "fits, but with nothing to spare",
settings: factorySettings,
deploy: deployWith("", graceOf("12")),
want: Warn,
contains: "leaves under 5s of headroom",
},
{
// The hook is spent before the process is told anything, so it is
// added to the budget rather than overlapping it.
name: "a preStop hook is part of the budget",
settings: factorySettings,
deploy: deployWith(preStopSleep25, graceOf("30")),
want: Error,
contains: "preStop 25 + drain 0 + server 5 + cleanup 3",
},
{
// The example from the review: 10 + 10 + 5 + 3 against 30.
name: "preStop and a drain window together, just fitting",
settings: settingsWith(" shutdown:\n drain: 10\n server: 5\n cleanup: 3\n"),
deploy: deployWith(` lifecycle:
preStop:
exec:
command: ["sleep", "10"]
`, graceOf("30")),
want: Warn,
},
{
name: "no shutdown section",
settings: settingsWith(" rateLimit:\n inboundQPS: 200\n"),
deploy: deployWith("", graceOf("30")),
want: -1,
},
{
// Nothing to disagree with. A manifest without a grace period gets
// the Kubernetes default, which this file cannot see, and guessing
// at it would make the check wrong rather than quiet.
name: "the manifest sets no grace period",
settings: settingsWith(" shutdown:\n drain: 300\n"),
deploy: deployWith("", ""),
want: -1,
},
{
// config.Shutdown.Budget refuses this and the server does not
// start, so it is not a failure that passes unnoticed - and adding
// a negative into the sum would understate it.
name: "a negative budget is left to the run-time refusal",
settings: settingsWith(" shutdown:\n drain: -100\n server: 5\n cleanup: 3\n"),
deploy: deployWith("", graceOf("5")),
want: -1,
},
} {
t.Run(tc.name, func(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": hostConfigSource,
"config/settings.yml": tc.settings,
"scripts/k8s/deploy.yml": tc.deploy,
})
got := only(t, check(t, root, options{}), checkShutdownGrace)
if tc.want < 0 {
if len(got) != 0 {
t.Fatalf("reported %d findings, want none:\n%v", len(got), got)
}
return
}
if len(got) != 1 {
// An ERROR also satisfies the WARN condition, so a second
// finding here means the two levels were not made exclusive -
// and an ERROR that always drags a duplicate WARN behind it
// teaches people to skip WARNs.
t.Fatalf("reported %d findings, want exactly 1:\n%v", len(got), got)
}
if got[0].severity != tc.want {
t.Errorf("reported %s, want %s: %s", got[0].Severity, tc.want, got[0].Message)
}
if tc.contains != "" && !strings.Contains(got[0].Message, tc.contains) {
t.Errorf("message %q does not contain %q", got[0].Message, tc.contains)
}
if got[0].File != k8sDeployFile {
t.Errorf("reported against %s, want %s", got[0].File, k8sDeployFile)
}
if want := lineOf([]byte(tc.deploy), "terminationGracePeriodSeconds:"); got[0].Line != want {
t.Errorf("reported line %d, want %d", got[0].Line, want)
}
})
}
}
// A hook whose duration cannot be read is said out loud rather than counted as
// nothing. It is still spent inside the grace period, and a self-check that
// silently valued it at zero would be the understatement this check exists to
// prevent.
func TestAnUnreadablePreStopIsReported(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": hostConfigSource,
"config/settings.yml": factorySettings,
"scripts/k8s/deploy.yml": deployWith(` lifecycle:
preStop:
httpGet:
path: /drain
port: 8000
`, graceOf("30")),
})
got := only(t, check(t, root, options{}), checkShutdownGrace)
if len(got) != 1 {
t.Fatalf("reported %d findings, want 1:\n%v", len(got), got)
}
if got[0].severity != Warn {
t.Errorf("reported %s, want WARN", got[0].Severity)
}
if !strings.Contains(got[0].Message, "not a sleep") {
t.Errorf("message %q does not say why the hook could not be read", got[0].Message)
}
}
// Either file missing means there is nothing to compare, which is the state
// every other check's fixture is in.
func TestShutdownBudgetIsSkippedWithoutBothFiles(t *testing.T) {
for _, files := range []map[string]string{
{"config/extend.go": hostConfigSource},
{"config/extend.go": hostConfigSource, "config/settings.yml": settingsWith(" shutdown:\n drain: 300\n")},
{"config/extend.go": hostConfigSource, "scripts/k8s/deploy.yml": deployWith("", graceOf("30"))},
} {
root := fixture(t, files)
if got := only(t, check(t, root, options{}), checkShutdownGrace); len(got) != 0 {
t.Errorf("reported %d findings with only %d file(s):\n%v", len(got), len(files), got)
}
}
}
// A tool that cannot find the defaults it is meant to apply has to say so.
// Reporting nothing would be the failure this whole tool is about: a check
// that stops checking and goes on printing a clean run.
func TestShutdownBudgetStopsWhenTheFallbacksAreGone(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": "package config\n\nconst DefaultDrainSeconds = 0\n",
"config/settings.yml": settingsWith(" shutdown:\n drain: 1\n"),
"scripts/k8s/deploy.yml": deployWith("", graceOf("30")),
})
s, err := load(root)
if err != nil {
t.Fatalf("load: %v", err)
}
if _, err := runChecks(s, options{}); err == nil {
t.Fatal("runChecks succeeded with the fallback constants renamed away")
} else if !strings.Contains(err.Error(), serverConstName) {
t.Errorf("error %q does not name the missing constant", err)
}
}
// The same arithmetic and the same margin as the manifest check, against the
// other place a shutdown gets cut short.
func TestDockerStopAgainstTheShutdownBudget(t *testing.T) {
for _, tc := range []struct {
name string
script string
want Severity
contains string
}{
{
name: "explicit and generous",
script: "sudo docker stop --timeout 30 \"$PREV\"\n",
want: -1,
},
{
// --time is the deprecated spelling of the same flag and docker
// still honours it. A check that could not read it would report a
// deadline that exists as missing, and push whoever fixed that
// towards a flag that is on its way out.
name: "the deprecated spelling still counts",
script: "docker stop --time 30 go-admin\n",
want: -1,
},
{
name: "the short form counts too",
script: "docker stop -t 30 go-admin\n",
want: -1,
},
{
// docker's default is 10 and this process spends 8, so it happens
// to work today - and would stop working the first time anybody
// configures a drain window, without the command changing.
name: "no deadline at all",
script: "sudo docker stop \"$PREV\" >/dev/null\n",
want: Error,
contains: "Pass --timeout 13",
},
{
name: "shorter than the shutdown",
script: "docker stop --timeout 5 go-admin\n",
want: Error,
contains: "allows 5 seconds and this shutdown takes 8",
},
{
// Quoted back in the spelling that was written, so the message
// cannot misreport the line it is pointing at.
name: "the message quotes the flag that was used",
script: "docker stop -t 5 go-admin\n",
want: Error,
contains: "`docker stop -t 5`",
},
{
name: "longer than the shutdown but inside the margin",
script: "docker stop --timeout=10 go-admin\n",
want: Warn,
},
{
name: "exactly the margin",
script: "docker stop --timeout 13 go-admin\n",
want: -1,
},
{
// The settings file describes `docker stop` in prose right beside
// the budget this check reads.
name: "a commented-out command is not one that runs",
script: "# docker stop go-admin\n",
want: -1,
},
} {
t.Run(tc.name, func(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": hostConfigSource,
"config/settings.yml": factorySettings,
"scripts/deploy.sh": "#!/bin/sh\n" + tc.script,
})
got := only(t, check(t, root, options{}), checkDockerStop)
if tc.want < 0 {
if len(got) != 0 {
t.Fatalf("reported %d findings, want none:\n%v", len(got), got)
}
return
}
if len(got) != 1 {
t.Fatalf("reported %d findings, want 1:\n%v", len(got), got)
}
if got[0].severity != tc.want {
t.Errorf("reported %s, want %s: %s", got[0].Severity, tc.want, got[0].Message)
}
if tc.contains != "" && !strings.Contains(got[0].Message, tc.contains) {
t.Errorf("message %q does not contain %q", got[0].Message, tc.contains)
}
if got[0].File != "scripts/deploy.sh" || got[0].Line != 2 {
t.Errorf("reported %s:%d, want scripts/deploy.sh:2", got[0].File, got[0].Line)
}
})
}
}
func composeWith(service string) string {
return "version: '3.8'\nservices:\n" + service
}
// The compose file is the other way this repository's container is stopped -
// `make run` starts it that way - and it fails identically: the default is ten
// seconds and it is nowhere near the budget it has to cover.
func TestComposeStopGraceAgainstTheShutdownBudget(t *testing.T) {
for _, tc := range []struct {
name string
service string
want Severity
contains string
}{
{
name: "generous",
service: " api:\n image: go-admin:latest\n stop_grace_period: 30s\n",
want: -1,
},
{
name: "not set at all",
service: " api:\n image: go-admin:latest\n",
want: Error,
contains: "Set stop_grace_period: 13s",
},
{
name: "shorter than the shutdown",
service: " api:\n image: go-admin:latest\n stop_grace_period: 5s\n",
want: Error,
contains: "stop_grace_period on service api allows 5 seconds",
},
{
name: "longer than the shutdown but inside the margin",
service: " api:\n image: go-admin:latest\n stop_grace_period: 10s\n",
want: Warn,
},
{
// Compose takes hours and minutes as well as seconds, and a check
// that only read the digits would call 1m30s ninety times too
// short.
name: "minutes and seconds",
service: " api:\n image: go-admin:latest\n stop_grace_period: 1m30s\n",
want: -1,
},
{
// A service running something else is not this process, and its
// grace period has nothing to do with this budget.
name: "another image is not this application",
service: " db:\n image: mysql:8\n",
want: -1,
},
{
// Built from this repository, so it is this application whatever
// the image ends up being called.
name: "built here rather than named",
service: " api:\n build: .\n",
want: Error,
contains: "service api, which sets no stop_grace_period",
},
} {
t.Run(tc.name, func(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": hostConfigSource,
"config/settings.yml": factorySettings,
"docker-compose.yml": composeWith(tc.service),
})
got := only(t, check(t, root, options{}), checkDockerStop)
if tc.want < 0 {
if len(got) != 0 {
t.Fatalf("reported %d findings, want none:\n%v", len(got), got)
}
return
}
if len(got) != 1 {
t.Fatalf("reported %d findings, want 1:\n%v", len(got), got)
}
if got[0].severity != tc.want {
t.Errorf("reported %s, want %s: %s", got[0].Severity, tc.want, got[0].Message)
}
if tc.contains != "" && !strings.Contains(got[0].Message, tc.contains) {
t.Errorf("message %q does not contain %q", got[0].Message, tc.contains)
}
if got[0].File != "docker-compose.yml" {
t.Errorf("reported against %s, want docker-compose.yml", got[0].File)
}
})
}
}
func TestComposeDurations(t *testing.T) {
for _, tc := range []struct {
in string
want int
wantOK bool
}{
{in: "30s", want: 30, wantOK: true},
{in: "30", want: 30, wantOK: true},
{in: "1m30s", want: 90, wantOK: true},
{in: "2m", want: 120, wantOK: true},
{in: "1h", want: 3600, wantOK: true},
{in: "1h0m30s", want: 3630, wantOK: true},
{in: "", wantOK: false},
{in: "forever", wantOK: false},
{in: "500ms", wantOK: false},
} {
t.Run(tc.in, func(t *testing.T) {
got, ok := composeSeconds(tc.in)
if ok != tc.wantOK {
t.Fatalf("composeSeconds(%q) ok = %v, want %v", tc.in, ok, tc.wantOK)
}
if ok && got != tc.want {
t.Errorf("composeSeconds(%q) = %d, want %d", tc.in, got, tc.want)
}
})
}
}
// The command can be written in a workflow or in the Makefile as easily as in
// a shell script, and a check that only looked at one of them would be quiet
// about the others.
func TestDockerStopIsFoundInEveryKindOfFile(t *testing.T) {
root := fixture(t, map[string]string{
"config/extend.go": hostConfigSource,
"config/settings.yml": factorySettings,
".github/workflows/ship.yml": "jobs:\n deploy:\n steps:\n - run: docker stop app\n",
"Makefile": "stop:\n\tdocker stop app\n",
"scripts/deploy.sh": "docker stop app\n",
})
got := only(t, check(t, root, options{}), checkDockerStop)
if len(got) != 3 {
t.Fatalf("found %d commands, want 3:\n%v", len(got), got)
}
}
func TestLineOfIgnoresComments(t *testing.T) {
content := []byte("a: 1\n # terminationGracePeriodSeconds: 99\n terminationGracePeriodSeconds: 30\n")
if got := lineOf(content, "terminationGracePeriodSeconds:"); got != 3 {
t.Errorf("lineOf = %d, want 3 - a commented-out key is not the setting", got)
}
}