adding header handling in routeVeam + unit tests #6022

benzekrimaha · 2025-12-08T14:40:10Z

bert-e · 2025-12-08T14:40:14Z

Hello benzekrimaha,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

codecov · 2025-12-08T14:49:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.39%. Comparing base (60b7aa4) to head (55ea857).
⚠️ Report is 4 commits behind head on development/9.0.
✅ All tests successful. No failed tests found.

Additional details and impacted files

Files with missing lines	Coverage Δ
lib/routes/routeVeeam.js	`97.72% <100.00%> (+0.25%)`	⬆️

@@                 Coverage Diff                 @@
##           development/9.0    #6022      +/-   ##
===================================================
+ Coverage            83.38%   83.39%   +0.01%     
===================================================
  Files                  189      189              
  Lines                12199    12208       +9     
===================================================
+ Hits                 10172    10181       +9     
  Misses                2027     2027

Flag	Coverage Δ
file-ft-tests	`66.78% <18.18%> (-0.04%)`	⬇️
kmip-ft-tests	`26.98% <18.18%> (-0.01%)`	⬇️
mongo-v0-ft-tests	`68.11% <18.18%> (-0.04%)`	⬇️
mongo-v1-ft-tests	`68.12% <18.18%> (-0.01%)`	⬇️
multiple-backend	`35.67% <36.36%> (-0.01%)`	⬇️
quota-tests	`33.06% <18.18%> (-0.01%)`	⬇️
quota-tests-inflights	`34.17% <18.18%> (-0.01%)`	⬇️
unit	`67.61% <100.00%> (+0.02%)`	⬆️
utapi-v2-tests	`33.33% <18.18%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DarkIsDude · 2025-12-08T16:12:11Z

lib/routes/routeVeeam.js

        // Deletion requires that the tags of the object are returned.
-        if (requestQueryParams && Object.keys(requestQueryParams).length > 0
-            && !(method === 'GET' && (requestQueryParams['X-Amz-Credential'] || ('tagging' in requestQueryParams)))) {
+        const queryKeys = Object.keys(requestQueryParams || {});


Here we parse 3 times the requestQueryParams with same logic (lowerCase then compare). We can maybe do eveything in one shot if this route is performance sensitive ?

Possibly this, and also considering an early return based on the length of queryKeys or requestQueryParams ?

lib/routes/routeVeeam.js

DarkIsDude · 2025-12-08T16:15:09Z

lib/routes/routeVeeam.js

+                    if (allowedSdkQueryKeys.has(lowerKey)) {
+                        return false;
+                    }
+                    if (lowerKey.startsWith('x-amz-sdk-')) {


Why this one and not the array at the top ? Why are we more permissive here ?

The startsWith('x-amz-sdk-') check is a defensive fallback: AWS SDKs occasionally adds new telemetry keys under that namespace, and I wanted to avoid blocking a future patch release that introduces, say, x-amz-sdk-language if we’d rather force every key to be enumerated explicitly, I can drop the prefix guard and expand the array whenever we encounter a new one. let me know what you think

.github/pykmip/Dockerfile

SylvainSenechal · 2025-12-09T09:54:21Z

lib/routes/routeVeeam.js

+        const xIdValue = _getQueryValueCaseInsensitive(requestQueryParams, 'x-id');
+        if (xIdValue && xIdValue !== apiToAction[method]) {
            return errorInstances.InvalidRequest
                .customizeDescription('The Veeam SOSAPI folder does not support this action.');


Is veeam as strict as AWS regarding errors messages, or can we customize it some more ? :
"the x-id value you provided in the query parameter: xxx, is unexpected, it should be be xxx for this api"

As nothing was specified I'd rather keep it that way in this PR , we can work on better error logging later on as today we only have this kind of errors

SylvainSenechal · 2025-12-09T10:05:29Z

lib/routes/routeVeeam.js

        // Reject any unsupported request, but allow downloads and deletes from UI
        // Download relies on GETs calls with auth in query parameters, that can be
        // checked if 'X-Amz-Credential' is included.
        // Deletion requires that the tags of the object are returned.


This older comment is mentioning that this logic is used to allow certains apis to go through.

It mentions the Deletion apis requiring tagging. But the logic later checks that the value of method is GET. Will this work when the method is DELETE ? I see in your tests you added 3 tests that are all PutObject, I say let's test it with a DeleteObject too

More general comment and follow up : I find it hard to reason about both the previous logic and the new one, as these logic try to fit all apis at the same time, with long if checking for method type, different headers. For example, the tagging check I only useful for the DeleteObjects if I understood correctly.
What would happen if if later we had another constraint to deal with that only applies to one of the 5 apis

Considering that we have a nice enum list "apiToAction" of all possible apis that this router will handle, I would add a function with an exhaustive switch statement to treat all 5 apis separately.

I also understand that we may not have time to do this

I think the comment about the delete is missleading, i think this comment is still talking about the Get api, and saying that the get method should return tags in order to be able to perform the deletion later. But the way and the place where its written makes it look like we want to allow DELETE apis. anyways

Before we touched it, the route rejected every non-GET request that carried any query parameters.That’s why our PUT started failing as soon as the SDK v3 appended ?x-id=…. The change simply teaches the validator that x-id (and the handful of SDK telemetry keys) are acceptable, while keeping the old rule for everything else.

SylvainSenechal · 2025-12-09T10:40:32Z

lib/routes/routeVeeam.js

+                && (_hasQueryKeyCaseInsensitive(requestQueryParams, 'X-Amz-Credential')
+                    || _hasQueryKeyCaseInsensitive(requestQueryParams, 'tagging')));
+
+        if (!isPresignedGet && queryKeys.length > 0) {


With the previous logic, having !isPresignedGet && queryKeys.length > 0 was enough to return an error.
Can we return an error directly here, and do the check unsupportedKeys right after, because here the 2 are mixed but I think it's not necessary ?

SylvainSenechal

Some comments, I think no need for a bigger refactoring, but some simplifications could be possible ? + adding a test with method different than GET

SylvainSenechal · 2025-12-09T10:52:23Z

lib/routes/routeVeeam.js

+                    }
+                    return true;
+                });
+            if (unsupportedKeys.length > 0) {


Wait so here, if the api ever receive any query different than the ones listed in allowedSdkQueryKeys, we will block the apis ? Isn't it risky because it forces us to be exhaustive in the list of allowed keys, are we sure this list is now complete ?

Before we touched it, the route rejected every non-GET request that carried any query parameters.That’s why our PUT started failing as soon as the SDK v3 appended ?x-id=…. The change simply teaches the validator that x-id (and the handful of SDK telemetry keys) are acceptable, while keeping the old rule for everything else. For the list I tried to make it as exhaustive as possible with everything I could find on official documentations

SylvainSenechal · 2025-12-09T10:53:59Z

tests/unit/internal/routeVeeam.js

+        assert.strictEqual(err, undefined);
+    });
+
+    it('should allow AWS SDK auxiliary x-amz-sdk-* query params on PUT for system.xml', () => {


nit: could add a test with a wrong param x-iamwrong ?

we're already testing a wrong value here => 51810dd#diff-7d9febf34141ec8f0c48580a4e9e899f15f34227d9ecc39fcda6c5392e87f581R108, isn't it over testing to add other headers as we already know that we reject them directly ?

looking at all the branches in the code, more tests are needed : e.g. having "presigned" headers, on GET or otherwise, different case for headers, ...

francoisferrand · 2025-12-09T15:42:34Z

.github/pykmip/Dockerfile

    cd pykmip && \
-    python3 setup.py install && \
+    pip3 install . && \
+    cd / && \


why do we need to cd / ?
everything afterwards uses absolute path...

francoisferrand · 2025-12-09T15:45:06Z

.github/pykmip/Dockerfile

    git clone https://github.com/openkmip/pykmip.git && \
    cd pykmip && \
-    python3 setup.py install && \
+    pip3 install . && \


what is the reason for the build failure? Is there a new release of pykmip?
should the fix not include specifying the version of pykmip that we checkout, so we avoid unexpected behavior changes (or incompatibility when it requires a newer version of python...) ?

The build was failing in the python3 setup.py install step when building pykmip from the GitHub repo on python:3.10-alpine (the setup.py invocation was exiting with status 1). There isn’t a new PyPI release of pykmip; we’re building from the GitHub repo because the latest fixes we need are only there (the last tagged release is still 0.10.0).

if we need a specific SHA1, we can (and should!) specify it anyway; and add a comment explaining why we don't use the release / ...

lib/routes/routeVeeam.js

francoisferrand · 2025-12-09T16:18:33Z

tests/unit/internal/routeVeeam.js

+        assert.strictEqual(err, undefined);
+    });
+
+    it('should allow AWS SDK auxiliary x-amz-sdk-* query params on PUT for system.xml', () => {


looking at all the branches in the code, more tests are needed : e.g. having "presigned" headers, on GET or otherwise, different case for headers, ...

francoisferrand · 2025-12-09T16:38:44Z

branch is not up-to-date with upstream development/9.0 : needs to be rebase and resolve conflicts...

bert-e · 2025-12-10T13:56:12Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-806 contains:

9.1.6

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.34
9.1.7
9.2.9

Please check the Fix Version/s of CLDSRV-806, or the target
branch of this pull request.

bert-e · 2025-12-10T13:59:47Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-806 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.34
9.1.7
9.2.9

Please check the Fix Version/s of CLDSRV-806, or the target
branch of this pull request.

francoisferrand · 2025-12-10T14:22:38Z

.github/pykmip/Dockerfile

    git clone https://github.com/openkmip/pykmip.git && \
    cd pykmip && \
-    python3 setup.py install && \
+    pip3 install . && \


if we need a specific SHA1, we can (and should!) specify it anyway; and add a comment explaining why we don't use the release / ...

bert-e · 2025-12-10T15:11:31Z

Request integration branches

Waiting for integration branch creation to be requested by the user.

To request integration branches, please comment on this pull request with the following command:

/create_integration_branches

Alternatively, the /approve and /create_pull_requests commands will automatically
create the integration branches.

bert-e · 2025-12-12T09:30:15Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-806 contains:

9.0.34
9.1.7
9.2.9

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.0.34
9.1.7
9.2.10

Please check the Fix Version/s of CLDSRV-806, or the target
branch of this pull request.

This commit aims to add the handling of additional headers for Veeam routes in the CloudServer. The change actually enables requests with automatically added headers by the sdk to not be directly rejectded. Issue: CLDSRV-806

The image build was failing because we were trying to install pykmip with python v2 (setup.py install uses python command which points to python v2 by default in alpine image). This change checks out the latest commit and installs pykmip with pip3 as this is the one now supported by pykmip. Issue: CLDSRV-806

Issue: CLDSRV-806

benzekrimaha · 2025-12-12T11:20:30Z

/create_integration_branches

bert-e · 2025-12-12T11:27:07Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_integration_branches

benzekrimaha · 2025-12-12T11:28:00Z

/create_pull_requests

bert-e · 2025-12-12T11:28:16Z

Integration data created

I have created the integration data for the additional destination branches.

this pull request will merge improvement/CLDSRV-806-handling-additionnal-headers into
development/9.0
w/9.1/improvement/CLDSRV-806-handling-additionnal-headers will be merged into development/9.1 (pull request INTEGRATION [PR#6022 > development/9.1] adding header handling in routeVeam + unit tests #6029)
w/9.2/improvement/CLDSRV-806-handling-additionnal-headers will be merged into development/9.2 (pull request INTEGRATION [PR#6022 > development/9.2] adding header handling in routeVeam + unit tests #6030)

The following branches will NOT be impacted:

development/7.10
development/7.4
development/7.70
development/8.8

Follow integration pull requests if you would like to be notified of
build statuses by email.

The following options are set: create_pull_requests, create_integration_branches

bert-e · 2025-12-12T11:28:17Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

The following options are set: create_pull_requests, create_integration_branches

benzekrimaha · 2025-12-12T11:29:16Z

/approve

bert-e · 2025-12-12T11:47:18Z

I have successfully merged the changeset of this pull request
into targetted development branches:

✔️ development/9.0
✔️ development/9.1
✔️ development/9.2

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8

Please check the status of the associated issue CLDSRV-806.

Goodbye benzekrimaha.

The following options are set: approve, create_pull_requests, create_integration_branches

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch 2 times, most recently from f43c0fa to 44da717 Compare December 8, 2025 14:47

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from 44da717 to 51810dd Compare December 8, 2025 15:36

benzekrimaha requested review from a team, DarkIsDude and SylvainSenechal December 8, 2025 15:48

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from 09d2c63 to b6a169d Compare December 8, 2025 15:55

DarkIsDude reviewed Dec 8, 2025

View reviewed changes

francoisferrand reviewed Dec 9, 2025

View reviewed changes

.github/pykmip/Dockerfile Outdated Show resolved Hide resolved

SylvainSenechal reviewed Dec 9, 2025

View reviewed changes

SylvainSenechal requested changes Dec 9, 2025

View reviewed changes

SylvainSenechal reviewed Dec 9, 2025

View reviewed changes

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from a6c3afc to 64e9ffe Compare December 9, 2025 13:23

benzekrimaha requested review from DarkIsDude, SylvainSenechal and francoisferrand December 9, 2025 13:43

scality deleted a comment from bert-e Dec 9, 2025

francoisferrand reviewed Dec 9, 2025

View reviewed changes

DarkIsDude approved these changes Dec 10, 2025

View reviewed changes

benzekrimaha requested a review from francoisferrand December 10, 2025 13:54

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from 792b0a0 to 5c30e2a Compare December 10, 2025 13:56

scality deleted a comment from bert-e Dec 10, 2025

SylvainSenechal approved these changes Dec 10, 2025

View reviewed changes

francoisferrand approved these changes Dec 10, 2025

View reviewed changes

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch 2 times, most recently from 9205b41 to af2fd5f Compare December 12, 2025 09:41

benzekrimaha added 3 commits December 12, 2025 10:41

adding header handling in routeVeam + unit tests

90cb06b

This commit aims to add the handling of additional headers for Veeam routes in the CloudServer. The change actually enables requests with automatically added headers by the sdk to not be directly rejectded. Issue: CLDSRV-806

project version bump

963edb9

Issue: CLDSRV-806

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from af2fd5f to 963edb9 Compare December 12, 2025 09:45

removing ceph tests from ci

55ea857

Issue: CLDSRV-806

benzekrimaha force-pushed the improvement/CLDSRV-806-handling-additionnal-headers branch from fcb0bfe to 55ea857 Compare December 12, 2025 11:19

scality deleted a comment from bert-e Dec 12, 2025

bert-e mentioned this pull request Dec 12, 2025

INTEGRATION [PR#6022 > development/9.1] adding header handling in routeVeam + unit tests #6029

Closed

bert-e mentioned this pull request Dec 12, 2025

INTEGRATION [PR#6022 > development/9.2] adding header handling in routeVeam + unit tests #6030

Merged

bert-e merged commit 55ea857 into development/9.0 Dec 12, 2025
29 checks passed

bert-e deleted the improvement/CLDSRV-806-handling-additionnal-headers branch December 12, 2025 11:47

adding header handling in routeVeam + unit tests #6022

adding header handling in routeVeam + unit tests #6022

Conversation

benzekrimaha commented Dec 8, 2025

Uh oh!

bert-e commented Dec 8, 2025

Hello benzekrimaha,

Uh oh!

codecov bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SylvainSenechal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benzekrimaha Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand commented Dec 9, 2025

Uh oh!

bert-e commented Dec 10, 2025

Incorrect fix version

Uh oh!

bert-e commented Dec 10, 2025

Incorrect fix version

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bert-e commented Dec 10, 2025

Request integration branches

codecov bot commented Dec 8, 2025 •

edited

Loading

SylvainSenechal Dec 9, 2025 •

edited

Loading

benzekrimaha Dec 9, 2025 •

edited

Loading

benzekrimaha Dec 9, 2025 •

edited

Loading