ARSN-642: prepare Arsenal to run on Node 24 - #2709
Open
francoisferrand wants to merge 4 commits into
Open
francoisferrand wants to merge 4 commits into
francoisferrand wants to merge 4 commits into
Conversation
Update CI coverage and refresh the Scality dependency set for the Node 24 transition while retaining Node 22 compatibility. Synchronize the lockfile and stabilize the health probe setup on the newer runtime. Issue: ARSN-642
Replace deprecated URL parsing while preserving raw S3 path semantics for routing, metadata, and v2 signature canonicalization. Export the shared parser for consumers that handle client-supplied request targets. Issue: ARSN-642
Contributor
Hello francoisferrand,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
francoisferrand
changed the base branch from
development/8.5
to
development/8.6
September 21, 2026 16:05
Contributor
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
francoisferrand
requested review from
a team,
SylvainSenechal and
benzekrimaha
September 21, 2026 17:11
| matrix: | ||
| # 24 is the target runtime; 22 is what engines.node still allows and | ||
| # what cloudserver/backbeat/scuba run until they move over. | ||
| node-version: ['22', '24'] |
Contributor
There was a problem hiding this comment.
Btw you used this in a lot of repo recently but it means every tests will run twice 🤔 You could've put 24 directly 🤔 , I guess the few remaining repo on node 22 will soon be updated considering you do like one repo per day 🤔
| @@ -0,0 +1,96 @@ | |||
| /** | |||
| * Splitting of request targets (`/bucket/key?acl`) without `url.parse()`, which | |||
Contributor
There was a problem hiding this comment.
A bit heavy to read, while parseRequestTarget could use one or two input->output examples
| }); | ||
|
|
||
| it('should leave a non-ASCII key unencoded', () => { | ||
| assert.strictEqual(resourceFor('/bucket/ключ'), '/bucket/ключ'); |
| const { parseRequestTarget } = require('../../../lib/utils/requestUrl'); | ||
|
|
||
| /* | ||
| * Expected values below were captured from the `url.parse()` this replaces, so |
Contributor
There was a problem hiding this comment.
Its nit and more of a remark, connected to the other new file requestUrl.ts
I notice claude likes to make comment referencing stuff that it just removed, it's okish in the context of the pr, but for the codebase it's weird,
It feels like its outputing its thought context into the comments. I'm talking about the "were captured from the url.parse() this repllaces", not just for this pr but something i notice in general, I don't think this is super good to keep written this way.
SylvainSenechal
approved these changes
Sep 21, 2026
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## development/8.6 #2709 +/- ##
===================================================
- Coverage 74.76% 74.75% -0.01%
===================================================
Files 227 228 +1
Lines 18650 18673 +23
Branches 3864 3873 +9
===================================================
+ Hits 13943 13959 +16
- Misses 4702 4709 +7
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Malformed table row was missing 3 of its 4 cells, tripping markdownlint's MD056 and blocking the test job before any tests ran. Also cleaned up code fence spacing and missing semicolons/commas in the JS examples, which prettier was flagging now that this file is part of the diff. The 'Unified at v17' row was never really a 4-column table row (that's part of why it was malformed): it's a single summary line. Moving it out to a plain sentence below the table removes it as the widest cell in the first column, which is what was pushing prettier's table padding past markdownlint's 80-char line limit. Table data and field names are otherwise untouched, just repadded to prettier's formatting. Issue: ARSN-642
francoisferrand
force-pushed
the
improvement/ARSN-642
branch
from
September 21, 2026 22:15
b114ae4 to
3006f5a
Compare
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
3 participants
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.
Node 22 enters Maintenance in Oct 2026 and reaches end-of-life 30 April 2027. Node 24 is Active LTS until April 2028, so it's time to get Arsenal running there.
CI
tests.yamlnow runs a Node 22/24 matrix (compile/lint/KMIP jobs move fully to 24; Codecov upload stays on the 24 leg). Keeping 22 in the mix a while longer since cloudserver, backbeat and scuba are still on it during this transition.engines.node -> >=22
Was
>=20, which went EOL in April 2026 and wasn't being tested anymore. Deliberately not going to>=24yet — cloudserver, backbeat and scuba need to move first, or Node 22 needs to go EOL (April 2027), whichever comes first.url.parse() removal
url.parse()is runtime-deprecated (DEP0169). Six call sites replaced.Three are straight swaps to
new URL(). Three needed a purpose-built parser (lib/utils/requestUrl.ts) instead, becausenew URL()normalizes in ways that change an S3 request path: it collapses./..segments, reads a leading//as an authority, and percent-encodes non-ASCII./bucket/a/../bbecomes/bucket/b;//bucket/keyloses its bucket. OngetCanonicalizedResource.ts(v2 signature path) that would change what gets signed — a request could end up verified against a different resource than it addresses. Supplying a base doesn't help either: absolute URLs ignore it entirely.parseRequestTarget()reproduces the legacy parser's behavior for these shapes (differentially tested against realurl.parse()output, hundreds of thousands of cases, zero mismatches) without ever resolving a scheme or host. Exported it from Arsenal's index so cloudserver/backbeat can use the same helper for their ownurl.parse()removals, since they have the identical hazard (copy-source header, routeBackbeat).Scality dependency refresh
Bumped once each of them had a Node 24-ready release:
fcntl0.3.1,httpagent1.1.1,sproxydclient8.2.3 (now tagged, previously pinned by commit),werelogs8.2.5,@scality/eslint-config-scality8.3.3. Lockfile refreshed accordingly.Verification
yarn auditbefore/after: prod advisories unchanged (65, none of the bumped Scality packages carried any); dev advisories dropped from 52 to 48 — 4 DoS advisories fixed via the Guidelines 8.3.3 refresh dropping its old markdown-it/linkify-it chain.Out of scope
Deprecated dependency cleanup (
level,ioctl,prom-client,ajv,@types/ioredis) and publishing — tracked separately.Issue: ARSN-642