Repository navigation
Conversation
|
Claude finished @otavio's task in 8m 21s —— View job Code Review CompleteReviewed 26 files and posted 2 inline issues:
StandardsThe worst violation is in the doc comment on SpecThe Spec axis did not run. The PR cites shellhub-io/team#243, which is in a repo this run cannot read, and it names no issue in this repo. Additional notes
To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This was referenced Oct 7, 2026
An enterprise stack verified licenses against the production key compiled into cloud's pkg/lic, so a test could only run under the one real license a developer or CI secret supplied. Expired, limited, region-bound or feature-less licenses were out of reach, and so was most of the licensing domain. A run started with IssuingLicenses now owns a test RSA key. The harness builds the server image with -X github.com/shellhub-io/cloud/pkg/lic.PublicKey set to its public half, through a new GO_LDFLAGS build argument that the production build leaves empty. The key lives in tests/.stack/license-issuer.pem and runs reuse it. A fresh key per run would change the build argument and recompile the enterprise server every time. GO_LDFLAGS is declared right before the build step so the module download layers stay cached, and the harness sets it only for enterprise and cloud builds. Each enterprise stack then writes its license to a file of its own and mounts it. The enterprise test overlay maps an unset SHELLHUB_LICENSE_FILE to /dev/null and to an empty ADMIN_API_LICENSE_FILE, which is how WithoutLicense starts a server that loads no license at all. Runs without an issuer, the Playwright suite among them, keep reading SHELLHUB_LICENSE_FILE as before. WithEveryAddressIn mounts GeoLite2 country and city databases, written with mmdbwriter, that place ::/0 in one country. They include reserved networks and turn IPv4 aliasing off because a test connects from a private address, which a stock database leaves unlocated. The locator needs both files to start, and the coordinates do not matter, so they stay at zero. MAXMIND_MIRROR points at an unreachable host only to make the server enable the locator. The fetcher skips every database already present, so it never contacts the host. RestartFailingServer waits for the container's on-failure policy to give up rather than for the API, since a server refusing its license file exits. UseLicense, RestartServer and DeleteDevice outlive the test's context, because Go cancels that context before cleanups run and these helpers restore state in a t.Cleanup. A paid edition now fails early, naming CloudDir's go.mod, when the cloud source is missing, instead of deep inside the image build.
TestEnterpriseLicensing runs one enterprise stack under licenses the run signs, and swaps the installed license per case through the admin API. TestEnterpriseWithoutLicenseFile starts a second stack with no license file. The tests pin two behaviours that read like accidents and are not. - The server logs a refusal for a region the license does not allow as a firewall block. Only the firewall evaluator checks allowed_regions, so the region cases run in a legacy-mode namespace with the firewall feature licensed. - A license that has not started refuses device acceptance but lets a connection through. CanConnectDevice skips IsStarted on purpose, so that a future start date cannot lock an instance out of the devices it already has. Each acceptance case deletes the pending device it enrolled when it ends, because startAcceptedAgent expects exactly one pending device. Their MAC addresses use 02:00:00:00:21:xx, a locally administered prefix numbered after the domain, so they never merge with another case's device. Covers shellhub-io/team#243 Domain 21 Feature Gating, Device Acceptance, Device Connection, License Regions, License Signature and License File Loading, except the billing gating and fail-open cases.
The testcontainers suite ran on the community stack only, so the licensing tests, and the firewall and recording tests that follow them, never ran in CI. A new job checks out the cloud source the way ui-e2e.yml does. It matches the PR's branch name, falls back to master, and runs ^TestEnterprise. It needs no license secret, because the run signs its own licenses. A pull request from a fork cannot mint the cloud token, so the job skips its steps and leaves a notice instead of failing every outside contribution. The community leg skips ^TestEnterprise so a run without the cloud source does not try to build an enterprise stack.
otavio
force-pushed
the
test/e2e-licensing
branch
from
October 8, 2026 14:45
a9c1786 to
dcb8514
Compare
Member
Author
|
/review |
Code Review CompleteThe 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 |
Member
Author
|
Consolidated into #7390, which carries this PR's commits so the whole stack reviews and merges together. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Covers the
[testcontainers]items of shellhub-io/team#243 Domain 21 (Licensing) and runs them in CI. This is the bottom layer of the per-domain e2e stack. The firewall, recording and billing domains that follow reuse its enterprise harness and CI job.An enterprise stack could only run under the one real license
SHELLHUB_LICENSE_FILEnamed, so expired, limited or feature-less licenses were out of reach. A run now signs its own:The production build leaves
GO_LDFLAGSempty. Runs without an issuer, the Playwright suite among them, still readSHELLHUB_LICENSE_FILE.Issue lines covered
TestEnterpriseLicensing/feature gating/firewall routes answer 402 without the featureTestEnterpriseLicensing/feature gating/firewall routes answer with the featureTestEnterpriseLicensing/feature gating/session recording routes answer 402 without the featureTestEnterpriseLicensing/feature gating/session recording routes answer with the featureTestEnterpriseLicensing/feature gating/an expired license answers 402 on every gated routeTestEnterpriseLicensing/feature gating/the license endpoint answers under an expired license,.../the license endpoint installs a license when none is installedTestEnterpriseLicensing/device acceptance/accepting a device within the license limit succeedsTestEnterpriseLicensing/device acceptance/accepting a device with the fleet at the license limit is refusedTestEnterpriseLicensing/device acceptance/accepting a device under an unlimited license succeedsTestEnterpriseLicensing/device acceptance/accepting a device with no license installed is refusedTestEnterpriseLicensing/device acceptance/accepting a device under an expired license is refusedTestEnterpriseLicensing/device acceptance/accepting a device under a license not yet started is refusedTestEnterpriseLicensing/device connection/connecting with the fleet within the license limit succeedsTestEnterpriseLicensing/device connection/connecting with the fleet over the license limit is refusedTestEnterpriseLicensing/device connection/connecting under an unlimited license succeedsTestEnterpriseLicensing/device connection/connecting with no license installed is refusedTestEnterpriseLicensing/device connection/connecting under an expired license is refusedTestEnterpriseLicensing/device connection/connecting under a license not yet started succeedsTestSSHConnectionBehaviour/each cipher the server offers is the one negotiatedTestEnterpriseLicensing/device connection/connecting from a region the license allows succeedsTestEnterpriseLicensing/device connection/connecting from a region the license does not allow is refusedallowed_regionsmeans globalTestEnterpriseLicensing/device connection/connecting under a license that names no region succeeds from anywhereTestEnterpriseLicensing/signature/a license the issuer signed is installedTestEnterpriseLicensing/signature/a license whose body changed after signing is refusedTestEnterpriseLicensing/signature/a license shorter than its signature is refusedTestEnterpriseLicensing/signature/a license that is not base64 is refusedTestEnterpriseLicensing/license file/the server installs the license file on startupTestEnterpriseLicensing/license file/a restart with another license file replaces the installed licenseTestEnterpriseLicensing/license file/a restart with the same license file installs nothingTestEnterpriseWithoutLicenseFileTestEnterpriseLicensing/license file/an invalid license file stops the server from startingNot covered here:
/api/billingonrouter.Group("/api/billing")without the license middlewares, ininternal/billing/routes/routes.go, and has since the monolith conversion in cloud a0681806. The billing branch ofLicenseCheckFeatureis dead code. Billing routes also exist only in the cloud edition.validateDeviceAcceptanceread the license insideUpdateDeviceStatus's transaction, so a failed read aborted the transaction and the accept answered 500. The fix and this case's test are in fix(server): check the license before accepting a device opens its tx #7379, stacked on this PR.Evidence
qa.ymlran the community stack only.testrunner:golangci-lint v2.14.0 reports 0 issues on
tests/with--max-issues-per-linter=0 --max-same-issues=0,go mod tidyfrom an emptygo.sumreproduces the committed one, and actionlint passesqa.yml.The review ran three rounds of
/code-review:Upclean up through one deferred call, madeDownremove files even when network removal fails, madeLicense+Unlicensedan error, stoppedWriteLicenseFilefrom rewriting a real license file, and restructured the refused-connection table. Rejected, with reasons: the SQL row-count helpers, because no API exposes stored rows; the in-package key comparison; the.stackliteral inmain_test.go; and the earlygo.modcheck, which the enterprise leg needs.Merge Danger
Door: two-way
The only production-facing change is the
GO_LDFLAGSbuild argument inserver/Dockerfile, which is empty unless a build passes it.Blast Radius: CI
The new job lengthens QA by one enterprise build and two stacks. It depends on the
CLOUD_DISPATCH_APP_*secrets, the same onesui-e2e.ymluses. The community leg now skips^TestEnterprise.Part of shellhub-io/team#243.