/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.
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.
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.
/health returned 200 without asking anything. Whatever it was meant to say, an
orchestrator reading it learned only that a process was accepting connections.
The two questions are not the same one, and the answers differ:
- /health stays a bare 200. It answers "should I restart you", and a process
whose database is unreachable does not want restarting - that turns one
outage into a crash loop and discards the connection pool, the cache and
every request in flight on the way.
- /ready is new. It answers "should I send you requests", fails while a
dependency is unreachable, and fails from the moment shutdown begins.
That last part is what the life-cycle phases bought. BeginDraining sits next to
BeginShutdown, before the server stops accepting, so a load balancer is told to
stop sending while this instance can still finish what it holds. Reversed - and
that is where it was - the connections are cut first and the probe reports it
afterwards.
The queue is deliberately not checked. Nothing on AdapterQueue answers "are you
reachable" without publishing something, the memory backend cannot fail, and a
queue that is down degrades logging rather than stopping requests: a reason to
alert, not a reason to leave the pool.
The cache probe writes and reads back rather than only reading. A cache that
answers "miss" for every key - a client pointed at the wrong server - is
indistinguishable from a healthy one on a read alone.
Every check runs behind a recover, and that is not defensive habit. The test for
"nothing configured" found the reason: GetCacheAdapter builds a wrapper around
whatever is configured and returns it even when nothing is, so the value is not
nil, the cache inside it is, and Set dereferences it. A nil check cannot see
that, and GetQueueAdapter behaves the same way. Whatever the cause, a probe is
the last thing that should be able to take the process down - the caller is
asking whether this instance is well, and killing it is the wrong reply.
The counter-proof compiles and fails: without the recover, the unconfigured case
panics rather than reporting two failed checks.
The per-tenant setup ended with `defer crontab.Stop()` on the line above
`select {}`. The select never returned, so the deferred call was unreachable
for the life of the process: the scheduler had never once been stopped. And
because setup never returned, the `for k, db := range dbs` loop in Setup never
reached its second iteration - with several tenant databases configured, only
whichever one came first out of the map ever got a scheduler at all.
Both fall out of deleting the select, which was blocking for nothing: cron.Start
is `go c.run()` and has never needed anything to hold the caller.
The stop becomes a BeforeExit callback. cron.Stop returns a context that closes
once the jobs already running have finished, so the shutdown budget has
something real to bound - and giving up on that wait leaves those jobs running
until the process exits, which is better than holding the whole shutdown open
for one job that will not end.
Startup moves from a bare goroutine in run() onto AfterListen. Two reasons: the
phase runs behind core's panic guard, which does not reach across a goroutine
boundary, so a panic while loading jobs used to take the process down; and the
jobs it starts can call the API, which is only true once the socket is
accepting. It can be synchronous now precisely because setup returns.
Tested where it can be: startCrontab is split out so a scheduler can be started
with no database in sight. The job runs every second; after RunShutdown, two
and a half seconds of silence is the assertion. The counter-proof - registering
no callback, which is what this commit replaces - compiles and reports "the job
fired 2 more times after shutdown".
There is one test, not several, because BeforeExit closes to further
registration once it has run; a second RunShutdown in the same binary would
find an empty registry and pass while proving nothing.
**The multi-tenant half has no test.** setup needs a *gorm.DB per tenant before
it reaches the line that was blocking, and this repository's CI has no database
- `make build` is CGO_ENABLED=0 with no sqlite tag. It is the same defect
though: the loop could not advance past a call that never returned.
Logging in on a deployment with enabledp: true ends on the login page. The
login itself succeeds - sys_login_log records it - and then /api/v1/getinfo
answers 401 "登录失败", which sends the browser straight back.
The query behind it reads:
SELECT * FROM sys_user WHERE sys_user.user_id = 1 AND 1 = 0 AND deleted_at = 0
The 1 = 0 comes from the data-permission scope. GetInfo asked for a permission
with GetPermissionFromContext, but the group this route sits in installs only
the JWT middleware - no PermissionAction - so nothing ever put one in the
context and what came back was the zero value. An unset scope is not one of the
five recognised ones, and since unknown scopes began failing closed rather than
silently matching every row, that zero value now means "match nothing".
The route was working by accident before, and only on deployments that enable
data permissions: the repository default is enabledp: false, where Permission
returns the query untouched. That is why the local suite and CI are both green
and the demo site is not.
Two different faults, so two different fixes:
/getinfo reads the caller's own row - the id comes from the token. A data
scope answers "whose rows may this user see", so there is nothing left for it
to restrict, and applying one is not a stricter version of the query but a
broken one: DataScopeSelf matches on create_by, and an account is created by
whoever added it, so a scoped self-read would 401 every user who did not create
their own account. It now goes through GetSelf, which does no scoping at all -
which is how GetProfile has always read the same row.
/sys-api is the opposite case. Its three handlers do read the permission, and
they are listing and updating other people's rows, so the middleware belongs
there and was simply missing. Added.
Those four endpoints were found by checking every handler that reads the
permission against the group it is registered on. The check reports four before
this commit and none after.
No test. Both paths need a *gorm.DB with sys_user and sys_role rows before they
reach the line that matters, and this repository's CI has no database - `make
build` is CGO_ENABLED=0 with no sqlite tag. What can be tested is the shape of
the mistake rather than its effect, and that belongs in tools/checksilent as a
rule of its own; it is not in this commit because a site that cannot be logged
into should not wait for it.
The comment called Visible "0" "hidden by default" and then said an
administrator should not have to unhide the menu - which cannot both be
true. "0" is shown; every menu this repository seeds, including the demo
product menu that is visible on the demo site, uses it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
grantToAdminRole does two independent things - it grants the menus to the
admin role and writes a casbin rule per api - and SeedMenus skipped the
whole call whenever the menu list came back empty.
An application is free to register apis with no menus: endpoints another
service calls, a webhook, a UI mounted somewhere else. Those installs wrote
their sys_api rows and then no casbin rule for any of them, so every one of
those endpoints was denied to everyone, admin included - from a migration
that reported success and left rows in the table to prove it had run. There
is nothing to look at afterwards that says what went wrong.
Guard on both lists instead, so nothing registered stays a no-op and apis
alone still get granted.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
core's seed package defines what an application may ask for and leaves the
writing to the host, which is the only side that knows its own tables. No
host implemented it, so SeedMenus returned ErrNoSeeder and an application's
menus never appeared.
adminSeeder writes all four kinds of row, not the two an obvious reading
would stop at: without sys_menu_api_rule and the sys_role_menu / casbin_rule
grants, the menu exists and no role can reach it.
Ids are always autoincrement, never caller-assigned - checksilent's
menu-id-collision check reads literals in this repository's tree and cannot
see an application in the module cache, so the collision is removed by
construction instead of guarded. The runtime validation covers what a static
scan cannot reach for a third-party spec: duplicate codes, unresolved parents
and api references, an unknown kind, and a sort outside sys_menu.sort's
tinyint range.
MenuSpec carries no menu name, so one is synthesised from the app code and
the spec code - two applications both choosing "list" would otherwise collide
on the frontend's keep-alive key.
It lives in app/admin/service because cmd links both subcommands into one
binary, so its init runs whichever one is invoked, and cmd/migrate never has
to import app/admin to reach it.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
sys_migration already carries app_code; sys_menu and sys_api did not, so
nothing said which application seeded a row - which is what an uninstall or
an audit would have to ask.
The migration adds the columns through the runtime models rather than
cmd/migrate/migration/models, whose frozen ModelTime is wrong for anything
ordered after the soft-delete conversion.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Four modules each called AuthInit and built their own instance, so which
one Runtime handed back was decided by whichever module initialised last.
The JwtToken key was also registered as an unbound method expression,
which GetHandlerFunc's type assertion can never match - the key was
registered and unusable at the same time.
The instance is now built once here and registered as a bound closure.
Modules read it back through GetAuthMiddleware, which is fatal rather
than nil when called before InitMiddleware has run: a process without a
JWT middleware should not reach the point of serving a request.
Only one call site needs the instance itself rather than the handler
(admin's /login, for LoginHandler); the thirty-odd MiddlewareFunc() call
sites are unchanged.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The privilege-escalation tests skipped themselves when opening the in-memory
database or running AutoMigrate failed. Both depend on nothing outside the
process, so a failure there means the environment is genuinely broken - and a
security regression that quietly does not run is worse than one that is
missing, because CI stays green either way.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
app/admin/models/datascope.go carried a second copy of the scope logic with no
callers. Its department-tree pattern was written as "%" + id + "%" instead of
"%/" + id + "/%", so dept_id 1 also matched /11/, /21/ and /100/ - visibility
into unrelated subtrees.
It sat where someone looking for a data permission example would find it. The
copy that is actually wired up stays in common/actions.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Nothing checked what went into sys_role.data_scope, so creating a role without
a dataScope stored an empty string - the value that used to be indistinguishable
from "see everything".
All three DTOs that write the column are validated, not just the insert path:
they target the same column, and guarding one entrance while leaving two open
would not be a guard.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
Two directions, because the fix has to hold both: an attacker with no policy on
this route cannot raise another user's role, and a self-edit cannot raise its
own. The second one is what keeps the fix from being "just remove the route
from CasbinExclude", which would break the profile page.
The tests drive the handler directly rather than through the router, because
the middleware is exactly what does not run for this route - the defence lives
in the handler, so that is where it has to be proven.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
The profile page posts the whole user object back, including roleId, deptId and
status, because it renders from a full SysUser it fetched earlier. A caller
editing their own record can therefore hand back a tampered roleId.
Self-edits now reload those three fields from the database and ignore whatever
the request carried. For an honest client this is a no-op - the values it sends
are already its own - so the profile page keeps working unchanged.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
PUT /api/v1/sys-user sits in CasbinExclude so the profile page can reach it,
which means AuthCheckRole never runs for this route. The handler took the
target user id from the request body, so any authenticated caller could edit
another user's record - including their roleId.
The route has to stay excluded: the profile page and the admin user list share
this one endpoint, so removing the exclusion would break self-service editing
for every non-admin role. The check therefore moves into the handler: when the
target is not the caller, the request is put through Casbin explicitly.
EnforceRoleFor carries the same admin short-circuit and enforcement AuthCheckRole
uses, so a route that opts out of the middleware can still ask the same question.
Claude-Session: https://claude.ai/code/session_01HPTAw8b8tAdFNFn8rKdPYx
common/middleware imported app/admin/service/dto for two string constants,
which made a package apps are told to build on depend on one particular app.
The previous commit added a data-permission scope to SysApi.Update and
returned early on db.Error, which left the RowsAffected check below it
unreachable: First reports a row the scope excluded as ErrRecordNotFound,
so the caller got "record not found" where the code meant to say
"无权更新该数据".
Map that one error to the permission message and drop the check it made
dead. The two cases - the row does not exist, and the row exists but is
not yours - have to look the same from outside, and now do.
Found by Copilot's review of #889.
SysApi.Update took a DataPermission and never used it, so with data
permission enabled the update reached rows the caller could not read
through GetPage, Get or Remove, which all scope the query. It also
reported "无权更新该数据" for a row that simply did not exist, a message
that only becomes true once the scope is applied.
Drops the Debug() left on the query, which logged the statement for
every call.
BeforeCreate and BeforeUpdate run Encrypt on whatever is in the struct,
and a user read from the database carries the stored hash in Password.
Hashing it again produces a hash of a hash: the password that user knows
stops matching, they cannot log in, and nothing reports an error.
Only the Omit("password") on SysUser.Update stood between that and the
stored credential. Any other write to this model - a profile update
written the way every other model here is written - destroys the
password, permanently and silently.
Encrypt now returns early when Password already parses as a bcrypt hash.
That also removes the round SysUser.Update was paying and discarding:
306ns where it was 54.7ms, on a route reachable without the permission
check, since PUT /api/v1/sys-user is in CasbinExclude.
The cost of deciding from the value is that a password which is itself a
well-formed bcrypt hash would be stored unchanged. That is a
60-character string beginning "$2a$", and it grants whoever set it no
access they did not already have.
The answer was written at info level on every captcha request, so a currently
valid answer sat in the application log. Anyone able to read the log - an
operator, a log aggregator, anything that ships logs off the host - could
bypass the check the captcha exists to enforce.
The default log level records it, so this was not limited to debug builds.
thirdUpload dispatched on the source parameter and then built the same
zero-value ALiYunOSS in both branches, so source=3 could not have
reached qiniu even with credentials.
Neither branch had credentials to use. OXS.Setup is the initialisation
path and nothing in the repository called it, and no configuration field
existed to fill. The store is now taken from extend.fileStore, and a
provider that was not configured says so rather than producing the
provider's own complaint about an empty bucket name.
The two handlers passed errors.New("") to e.Error, discarding what
actually went wrong; they now pass the error.
Every import of the module changes, not only the seven packages that
moved out of sdk/pkg: Go requires the major version in the path from v2
on. Both happen in one pass —
go run github.com/go-admin-team/go-admin-core/tools/coreupgrade@v2.0.0 -w -v2 .
go mod tidy
— which is the command the release notes give, run here as a consumer
would run it. 210 imports across 95 files.
The compatibility shims this used are gone in v2, so the paths that
moved had to move: sdk/pkg/captcha, sdk/pkg/jwtauth and its user
package, sdk/pkg/response and sdk/pkg/casbin.
The count of unformatted files is unchanged at 34, none of them touched
by this: the tool reformats a file only if it was already gofmt clean,
so a migration cannot disappear into whitespace.
The exclusion list was a subquery against `$GenConfig.DBName`.sys_tables,
so it named the schema by hand. Generating from a schema that is not the
one holding sys_tables made the whole query fail, and because the
subquery read the table directly it also counted soft-deleted entries:
deleting a generator entry never handed its table back.
Read the registrations through the model on this connection instead. An
empty list skips the clause - NOT IN (NULL) is unknown for every row,
which would leave a fresh install with nothing to generate from.
pkg.Assert panics when its condition is false, so pkg.Assert(true,
"目前只支持mysql数据库") is a no-op. On postgres or sqlserver the code
generator did not report that it needs MySQL: DBTables returned an empty
list with a nil error, and DBColumns ran its query on the zero-value
*gorm.DB left over from the branch that never assigned, which is a nil
dereference rather than a message.
Assert the driver up front instead of asserting a constant in an else,
which also removes the placeholder *gorm.DB the fall-through relied on.
DBColumns.GetPage had no guard at all and gets the same one.
pkg.Assert panics when its condition is false, so
Assert(TableName == "", "table name cannot be empty") rejected every
request that carried a table name and let the empty one through. The
model layer repeated the inversion with if TableName != "" { return
error }, so either one alone was enough to break the endpoint.
Flip both, and hoist the model guard out of the mysql branch so it
matches GetList ten lines below, which had it right all along.
Review caught that this reissued getByRoleName's query instead of
calling it, so it passed whether or not the production line still said
what it was supposed to — a test named for a change it did not touch.
It calls getByRoleName now, and restoring the hand-written clause fails
it for exactly the reason this PR exists: with the marker non-null,
"deleted_at is null" matches nothing and the query returns an empty
list.
Two things in front of the unique-index work, both safe on their own.
getSysMenuByRoleName carried "deleted_at is null" in its where clause.
GORM adds that condition itself for a model with a DeletedAt field, so
it was a duplicate — and one phrased as a column being null, which stops
being true the moment the column stops being nullable. A schema that
moves to a non-null delete marker would have turned this query into one
that matches nothing, silently, for admin users only.
SysDictType.Insert dropped the error from its duplicate check: a query
that failed left the count at zero and the insert went ahead as though
the name were free.
The test pins what the removed clause was there for. Its counter-proof
is Unscoped rather than deleting the field — taking ModelTime off the
model fails to compile, which proves nothing.
The pinned core dated from April, before sdk stopped being a separate module,
so the build resolved sdk packages from the old module and core packages from
the new one. Dropping the separate requirement is what makes the two agree
again.
Most of the diff is renames that came with that: the tenant accessors gained a
ByTenant suffix, GetDb now returns one database and GetAllDb the map, and
casbin moved to v3.
The change that matters is four call sites moving from GetMemoryQueue to
GetQueuePrefix. GetMemoryQueue returns a queue fixed at construction, so the
login log, the operate log and the api check ran in process no matter what the
settings file selected — a second instance saw none of it. GetQueuePrefix
returns whatever the configuration built, which is the point of being able to
configure a queue at all.
Verified against core at main: build and vet clean. The two file_store failures
are unchanged from before this branch; they need cloud credentials.
zhangwenjian