Skip to content

test(e2e): tighten the install-key and access-policy cases - #7084

Open
otavio wants to merge 12 commits into
masterfrom
test/e2e-harness-refinements
Open

test(e2e): tighten the install-key and access-policy cases#7084
otavio wants to merge 12 commits into
masterfrom
test/e2e-harness-refinements

Conversation

@otavio

@otavio otavio commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

What

Twelve commits that tighten what the install-key and access-policy cases assert, how they
fail, and where the verbs they use live. No case is added or removed — the coverage is the
one #7079 landed.

Why

The cases were written against a harness that only knew how to create users and namespaces,
so each one carried its own request-and-Eventually for install keys, access policies,
device statuses and service logs. Several assertions were also softer than they look: a
refusal was matched against a hand-written string, a dial that died before an approval was
offered sat out the full deadline instead of reporting its error, a helper failing inside a
subtest called FailNow on the parent, and a wait for a key charged nothing returned on its
first poll.

Follows #7079, now merged.

Changes

The history is ordered so the mechanical work is separable from the work that changes what
the suite proves: five small fixes, then the harness, then one migration per test file, then
the two assertions that got stronger.

  • harness helpers fail on the caller's test: runAdminCommand failed on dc.t, the T
    the environment was created with, and AuthUser asserted on it directly. When a stack is
    shared across subtests that T is the parent, so a helper failing inside a subtest called
    FailNow off the goroutine running the test — the subtest surfaced as a bare Goexit
    instead of its message, and the parent was marked failed with its other subtests still
    running. Both take the caller's T now, and the field is renamed setupT so the next
    helper reaching for it has to notice whose it is.
  • stack verbs move onto the harness: CreateInstallKey, AwaitInstallKeyUses,
    RequireInstallKeyUnused, CreateAccessPolicy, AwaitDeviceWithStatus, AwaitServerLog,
    AwaitLogContains and EnrollIdentity/EnrollIdentityAs join NewUser and NewMember
    rather than living as a second set of helpers beside the cases that happened to need them
    first. AwaitLogContains takes any LogSource: the agent is a container the test starts
    itself, not a compose service.
  • names for the two expressions the suite repeated: deviceSSHID for
    <login>@<namespace>.<device> and SSHAddress for localhost: plus the published port,
    39 call sites each.
  • denial reasons matched through their model constants, so a rename moves the tests with
    it.
  • the no-policy case asserts why the login is refused. It previously required only that
    the dial fail. It cannot wait for a reason= line the way the other two rows do, because
    none is logged on that path: Session.Evaluate returns ErrAccessDenied as soon as
    NamespaceHasAccessPolicies reports false, so Session.authorize — the only place that
    logs a reason — is never reached. It waits for the evaluation failure and the error carried
    with it instead. That is knowingly weaker than a reason, and the commit message says so:
    the message matches any evaluation failure that is neither billing nor firewall, and
    session.ErrAccessDenied cannot be pinned because server/ is a module tests/go.mod
    does not require.
  • a dial that dies before an approval is offered fails with its own error instead of
    waiting out the 60s deadline, and the failure is raised on the test goroutine:
    require.FailNow inside an Eventually condition would exit testify's goroutine rather
    than fail the test.
  • ssh.ClientConfig.Timeout dropped from dialSSH, which never applied it — only
    ssh.Dial reads it, and only for the TCP connect. The handshake stays unbounded, which the
    approval case needs, with the package timeout as the backstop.
  • startAgent shared by the install-key cases, and the container lifecycle split out of
    startAcceptedAgent so a case whose device is not accepted by hand can ask for the first
    half.
  • the mode cases become a table in both suites, and the status assertions the ?status=
    filter already guarantees are dropped — the filter is a real WHERE clause, so a device in
    the wrong state leaves the list empty and the wait fails on its own.
  • the uncharged install key gets its own assertion. AwaitInstallKeyUses asserted a
    different proposition depending on the value of an int, and on the zero path it was not a
    wait: EventuallyWithT returns as soon as one tick passes, and a freshly created key
    already reports no uses and no stamp. A server that charged a manual key a tick later would
    have left the case green. It now only waits for a count to rise; RequireInstallKeyUnused
    reads the key once for the negative, after AwaitDeviceWithStatus has waited for the
    enrollment that must not have charged it.

Testing

tests/ builds and vets clean against current master, and so does every commit
individually — git bisect over this branch will not hit a build error. The suite itself is
the QA job's tests project; the eight new stacks are what pushed test_timeout to 60m in
#7079.

@otavio
otavio requested a review from a team as a code owner September 12, 2026 22:01
Base automatically changed from test/e2e-install-key-access-policy to master September 12, 2026 22:01
@otavio
otavio force-pushed the test/e2e-harness-refinements branch from 86bb939 to 3f1585f Compare September 12, 2026 22:03
@otavio

otavio commented Sep 12, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Claude finished @otavio's task in 15m 55s —— View job

Code Review Complete

Reviewed 6 files. 2 inline issues posted — Standards: 0 violations, 2 judgement calls. Spec: 0. Correctness: 0.

Standards

The worst is at tests/identity_access_test.go:200-203: 3f1585f replaced this row's reason=no_grant assertion (added in 1148499) with two free-text log strings. The change is correct — Session.Evaluate short-circuits with ErrAccessDenied before any policy decision, so 1148499's premise was wrong — but the correction is recorded nowhere. git blame lands on a commit about moving harness verbs, and 1148499's message still asserts the false claim. Second: AwaitInstallKeyUses (tests/environment/docker_compose.go:195-201) asserts a different proposition depending on its uses argument, and on the uses == 0 path the EventuallyWithT returns on its first poll.

Correctness

No defects. The setupT rename is complete (it survives only in configurator.go, on the T that built the stack); no require/FailNow remains inside an Eventually condition — the branch removes the one that was there; every model constant the new assertions read exists with the value the old literals had; both new log substrings can actually match; no races, no leaked contexts, no route changes.

Spec

The Spec axis did not run: the PR references no issue. #7079 is a merged PR, and no commit on the branch carries a Fixes: trailer.

Additional notes

No cap dropped anything (2 of 5 judgement-call slots used). Four candidates were dropped because this branch did not introduce them, in case they're wanted as follow-ups: the SSH handshake in dialSSH has no client-side bound (dropping ClientConfig.Timeout was a no-op — ssh.NewClientConn never read it, so it was already unbounded, and the server's own HandshakeTimeout is the real backstop); the approval dial goroutine still gets context.Background(); startAgent registers t.Cleanup after Start, so a failed Start leaks the container (same ordering as the code it was extracted from); and newSSHEnvironment still hand-rolls the POST /api/login that AuthUser now covers — the last verb the branch consolidated everywhere else.

I could not verify compilation: go vet on the tests module was denied in this sandbox, so the build claim in the PR description is unchecked here. Every changed call site was read by hand instead.

To request another review round, comment /review.
· branch test/e2e-harness-refinements

github-actions[bot]
github-actions Bot reviewed Sep 12, 2026
View reviewed changes
Comment thread tests/identity_access_test.go
github-actions[bot]
github-actions Bot reviewed Sep 12, 2026
View reviewed changes
Comment thread tests/environment/docker_compose.go Outdated
@otavio
otavio force-pushed the test/e2e-harness-refinements branch from 3f1585f to 8d10641 Compare September 12, 2026 22:51
otavio added 12 commits September 12, 2026 19:57
@otavio
test(e2e): match denial reasons through their model constants
d92e34b 
DenialReason is the value the server logs and queries rely on, so the
test now builds the expected "reason=" line from models.ReasonNoGrant and
models.ReasonDeniedByPolicy. A rename moves the test with it instead of
leaving it waiting on a string the gateway no longer writes.
@otavio
test(e2e): fail fast when the login dies before an approval is offered
f028169 
The wait for the approval code watched only the banners, so a dial that
failed before an approval was minted (device offline, the connection
dropped by AuthenticableSessionOrDrop, openApproval erroring before the
banner) sat out the full 60s and was reported as "the gateway never
offered an approval code" while the real error was discarded.

The loop now selects on the banners, the dial result and one deadline on
the test goroutine. The failure has to be raised there: testify runs an
Eventually condition in a goroutine of its own, where require's FailNow
would exit that goroutine rather than fail the test. A dial that returns
nil without a code is a failure too, since an unknown key must not get in
unapproved.

This also drops the 10ms tick, which was never the real cadence: testify
re-arms its ticker only after the condition returns, and the condition
blocked for up to a second.
@otavio
test(e2e): drop the ssh client timeout dialSSH never applies
b1a18d5 
ssh.ClientConfig.Timeout is read only by ssh.Dial, which hands it to
net.DialTimeout for the TCP connect. dialSSH connects with DialContext
and passes the conn to ssh.NewClientConn, which neither consults Timeout
nor sets a deadline, so the 15s claimed a bound the handshake never had.

Removing it changes no behaviour. A real cap would be a conn deadline
around NewClientConn with a budget chosen per caller, since the approval
case has to outlast the server's 90s approval window.
@otavio
test(e2e): stop reading the agent state after a failed inspect
5bbf336 
assert.NoError records a failure without leaving the condition, so a
failed agent.State still reached state.Running. A nil state panics inside
the goroutine testify spawns for the condition, taking down the whole test
binary (and, with Ryuk disabled, leaving every compose stack unreaped); a
zero-value state has Running false and makes the subtest pass because
Docker failed. Return from the condition on the error instead, as
awaitServerLogContains already does.
@otavio
test(e2e): drop status assertions the device filter already guarantees
b23a607 
awaitDevicesWithStatus lists /api/devices?status=<status>, and a non-empty
status becomes a real WHERE status = ? in the store, so every device it
returns already carries the queried status and the follow-up equality
could not fail. The signal is the poll's assert.Len: a device enrolled in
the wrong state leaves the filtered list empty and the wait times out.
With nothing left reading the devices, the helper no longer returns them.
@otavio
test(e2e): fail harness helpers on the test that calls them
be1d51d 
The helpers reported failures against the T that built the stack, so a
failing subtest blamed the parent and the run carried on past it. They
now take the caller's T, and the field holding the setup one is named
setupT to say which is which.

AuthUser takes it in place of the context it used to take, so its callers
move with it.
@otavio
test(e2e): add the stack verbs to the compose harness
ec074d1 
Creating an install key or an access policy, waiting for a device to
reach a status or a key to reach a use count, enrolling an identity and
reading a service's log were each spelled out inside whichever case
needed them first, as a request and an Eventually the next case then
repeated. They are things a test does to a running stack, so they belong
beside NewUser and NewMember.

AwaitLogContains takes any log source, because the agent is a container
the test starts itself rather than a compose service. It polls every 2s
rather than the 1s the API waits use, since each tick reads the whole log
from the start. AwaitDeviceWithStatus asserts through the ?status= filter
instead of reading the device back: the filter is a real WHERE clause, so
a device in the wrong state leaves the list empty and the wait fails.

The cases move onto these in the commits that follow.
@otavio
test(e2e): move the ssh cases onto the harness
967dcf1 
The ssh cases built their own stack, logged their own user in and spelled
out their own SSHID, so every case repeated the four lines before the one
it was actually about. They now go through newSSHEnvironment, startAgent
and startAcceptedAgent, and name the gateway through SSHAddress and the
device through deviceSSHID rather than concatenating either in place.

startAcceptedAgent keeps only the accepting: the container lifecycle it
used to own is startAgent's, so a case that wants a pending device can
reach for one without getting the acceptance too.

newSigner and NewAgentContainerWithInstallKey move here from the cases
that happened to need them first, next to the other agent options.
@otavio
test(e2e): move the install-key cases onto the harness
8da707d 
The two mode cases differed only in the mode they created the key with,
the status they expected and the uses they expected charged, so they
become a table over those three. Creating the key, starting the agent and
waiting for either now go through the harness, which leaves the case with
the three lines that say what it is about.

The unissued-key case waits for the agent to log the refusal before
watching it exit, so a container that dies for an unrelated reason no
longer reads as the server having turned the key down.
@otavio
test(e2e): move the access-policy cases onto the harness
540a723 
The three refusal cases differed only in what they set up and which
server log proved the refusal, so they become a table over those two,
with requireLoginRefused doing the dial and the waiting. Enrolling an
identity, creating a policy and reading the server log now go through the
harness rather than being spelled out per case.

The doc comment names the approval case alongside the others, since it is
the one subtest the table does not cover.
@otavio
test(e2e): assert why a namespace without policies refuses the login
918c36b 
The case only required the dial to fail, so a tunnel that was not up yet
or a transport error passed it just as well as the refusal it is named
for, and default-deny regressing into default-allow needed a second
failure to go red.

It cannot wait for a reason= line the way the other two rows do, because
none is logged on this path: Session.Evaluate returns ErrAccessDenied as
soon as NamespaceHasAccessPolicies reports false, and Session.authorize,
the only place that logs a reason, is never reached. What it waits for
instead is the default arm of the switch that reports the failure, plus
the error carried with it.

That is weaker than a reason, and knowingly so. The message matches any
evaluation failure that is neither billing nor firewall, and the two
substrings are waited for independently against the whole log, so nothing
ties them to one decision. Pinning session.ErrAccessDenied is not on
offer: server/ is a separate module, and tests/go.mod requires only the
root one.
@otavio
test(e2e): give the uncharged install key its own assertion
cc33d58 
AwaitInstallKeyUses asserted a different thing depending on the value of
an int, and on the zero path it was not a wait at all: EventuallyWithT
returns as soon as one tick passes, and a key that has just been created
already reports no uses and no stamp, so it returned on its first poll. A
server that charged a manual key a tick later would have left the case
green, which is the half of "is charged nothing yet" the case exists to
prove.

AwaitInstallKeyUses now only waits for a count to rise, and requires it.
The negative gets RequireInstallKeyUnused, which reads the key once and
says so in its name; it is safe there because AwaitDeviceWithStatus has
already waited for the enrollment that must not have charged it.

The table drops the uses field for a closure per row, the shape the
refusal cases already use, so a row names the assertion it wants rather
than leaving the call site to read it off an integer.
@otavio
otavio force-pushed the test/e2e-harness-refinements branch from 8d10641 to cc33d58 Compare September 12, 2026 23:03
@otavio

otavio commented Sep 12, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Sep 12, 2026 •
edited
Loading

Copy link
Copy Markdown

Code Review Complete

The automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment /review.

View job

@otavio otavio mentioned this pull request Sep 12, 2026
Merged
otavio added a commit that referenced this pull request Sep 12, 2026
@otavio
ci(review): review a PR retargeted onto master or marked ready
4e4f911 
A stacked PR never got an automatic review. #7084 was opened against the branch of #7079 at
22:01:33, #7079 merged two seconds later, and GitHub retargeted #7084 onto master at 22:01:37.
The opened event carried the old base, so branches: [master] filtered it out and no run was ever
queued; the retarget arrives as edited, which nothing listened for; and the force-push that
followed is synchronize, which stays excluded so the author drives re-review. The PR fell through
every trigger and only got a review when its author asked for one by hand.

edited covers the retarget, gated on changes.base so editing a title or a description does not
spend a review. ready_for_review closes the sibling hole, where a PR opened as a draft is filtered
by the draft check at open and has nothing to re-trigger it once it is ready.

The same guard has to sit on cancel-in-progress, not only on the job. A run joins the concurrency
group and cancels the one in flight before any job-level if: is evaluated, so gating the job alone
would let a title edit cancel a running review and then skip its own, leaving no review at all —
and it would defeat this change's own case, where a retarget starts a review that the author's
next title edit kills. The other triggers cannot produce an edited action, so their runs keep
cancelling as they do today.

The comment saying the review runs only on open went with the change that made it false.
otavio added a commit that referenced this pull request Sep 12, 2026
@otavio
ci(review): review a PR retargeted onto master or marked ready
f54a2de 
A stacked PR never got an automatic review. #7084 was opened against the branch of #7079 at
22:01:33, #7079 merged two seconds later, and GitHub retargeted #7084 onto master at 22:01:37.
The opened event carried the old base, so branches: [master] filtered it out and no run was ever
queued; the retarget arrives as edited, which nothing listened for; and the force-push that
followed is synchronize, which stays excluded so the author drives re-review. The PR fell through
every trigger and only got a review when its author asked for one by hand.

edited covers the retarget, gated on changes.base so editing a title or a description does not
spend a review. ready_for_review closes the sibling hole, where a PR opened as a draft is filtered
by the draft check at open and has nothing to re-trigger it once it is ready.

The same guard has to sit on cancel-in-progress, not only on the job. A run joins the concurrency
group and cancels the one in flight before any job-level if: is evaluated, so gating the job alone
would let a title edit cancel a running review and then skip its own, leaving no review at all —
and it would defeat this change's own case, where a retarget starts a review that the author's
next title edit kills. The other triggers cannot produce an edited action, so their runs keep
cancelling as they do today.

The comment saying the review runs only on open went with the change that made it false.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@otavio