CLDSRV-780: ServerAccessLogs json schema and functional tests #6020

leif-scality · 2025-12-02T17:23:29Z

Defined a json schema for the server access logs
Updated unit and logger output to match the schema
Implemented functional tests, to check that cloudserver writes logs correctly to the output file

bert-e · 2025-12-02T17:23:33Z

Hello leif-scality,

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.

bert-e · 2025-12-02T17:23:45Z

Incorrect fix version

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

None

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

9.2.5

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

codecov · 2025-12-02T17:26:35Z

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
8316	1	8315	0

View the top 1 failed test(s) by shortest run time

"after each" hook for "should create a bunch of objects and their versions"::put and get object with versioning With default signature "after each" hook for "should create a bunch of objects and their versions"

Stack Traces | 0.876s run time

The bucket you tried to delete is not empty.

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

dvasilas · 2025-12-03T11:22:49Z

schema/server_access_log.schema.json

+            "type": "string"
+        },
+        "startTime": {
+            "description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type because to be compatible with clickhouse DateTime64(3) type.",


Suggested change

"description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type because to be compatible with clickhouse DateTime64(3) type.",

"description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type to be compatible with clickhouse DateTime64(3) type.",

dvasilas · 2025-12-03T11:24:04Z

schema/server_access_log.schema.json

+            "const": "0"
+        },
+        "loggingEnabled": {
+            "description": "If true it means that the target bucket has bucket logging configured and enabled.",


Suggested change

"description": "If true it means that the target bucket has bucket logging configured and enabled.",

"description": "True if the target bucket has bucket logging configured and enabled.",

dvasilas · 2025-12-03T11:25:59Z

schema/server_access_log.schema.json

+            "minimum": 0
+        },
+        "requester": {
+            "description": "Represents the AWS server access log 'Requester' field.",


Suggested change

"description": "Represents the AWS server access log 'Requester' field.",

"description": "Represents the AWS server access log 'Requester' field. https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html",

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

dvasilas · 2025-12-03T12:29:57Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+                    const result = tv4.validateResult(logEntries[logEntryIdx], schema);
+                    assert.strictEqual(result.valid, true,
+                        `Log entry should match schema: ${JSON.stringify(result.error)}`);
+                    assert.strictEqual(logEntries[logEntryIdx].operation, operation.expectedOperations[operationIdx],


Do you think we can add checks for more fields (maybe not this PR) ?
I am thinking at least bytesReceived, bytesDeleted, objectSize, versionID.

There are some checks that you can probably add easily in this PR: bucketName, bucketOwner (?), objectKey, loggingEnabled, loggingTargetBucket, loggingTargetPrefix, awsAccessKeyID, httpMethod, httpCode.

My point is: let's try to have as much test coverage as possible for the log record.

I now check more properties.

Time properties are not tested because we can't control them, the type is checked by the schema

Bytes*/contentLength are not tested because I think they may change so better do it after. The schema checks their types

VersionID we don't know what version value is going to be used

TLS is not tested because we don't have an HTTPS setup

// List of all the log properties. // Commented properties are set by the test or not tested. // UNKNOWN: The property is not accessible by the test or is non determistic. // DYNAMIC: The propery is set depending on the operation. // STATIC: Shared among all tests. // TODO: The property need to be tested. const commonProperties = { // 'time': '', // UNKNOWN // 'hostname': '', // UNKNOWN // 'pid': '', // UNKNOWN 'action': 'REQUIRED', // DYNAMIC 'accountName': 'Bart', // STATIC 'accountDisplayName': 'Bart', // STATIC 'userName': null, // TODO: Add test with IAM user to get a non null userName. // 'clientPort': '', // UNKNOWN 'httpMethod': 'REQUIRED', // DYNAMIC // 'bytesDeleted': '', // TODO // 'bytesReceived': '', // TODO // 'bodyLength': '', // TODO // 'contentLength': '', // TODO // 'elapsed_ms': '', // UNKNOWN // 'httpURL': '', // TODO // 'startTime': '', // UNKNOWN 'requester': '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', // STATIC 'operation': 'REQUIRED', // DYNAMIC // 'requestURI': '', // TODO 'errorCode': null, // DYNAMIC // 'objectSize': '', // TODO // 'totalTime': '', // UNKNOWN // 'turnAroundTime': '', // UNKNOWN 'referer': null, // TODO: Add test that sets the referer. // 'userAgent': // UNKNOWN // 'versionID': '', // UNKNOWN 'signatureVersion': 'SigV4', // STATIC 'cipherSuite': null, // TODO: Add https tests. 'authenticationType': 'AuthHeader', // STATIC // 'hostHeader': '', // UNKNOWN 'tlsVersion': null, // TODO: Add https tests. 'aclRequired': null, // TODO: Add https tests. 'bucketOwner': '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', // DYNAMIC bucketName, // DYNAMIC // 'req_id': '', // UNKNOWN // 'bytesSent': '', // TODO // 'clientIP': '', // UNKNOWN 'httpCode': 200, // DYNAMIC 'objectKey': null, // DYNAMIC 'logFormatVersion': '0', // STATIC 'loggingEnabled': false, // DYNAMIC 'loggingTargetBucket': null, // DYNAMIC 'loggingTargetPrefix': null, // DYNAMIC 'awsAccessKeyID': 'accessKey1', // STATIC 'raftSessionID': null, // UNKNOWN };

bert-e · 2025-12-03T12:30:17Z

Incorrect fix version

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

None

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

9.2.6

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

dvasilas

Don't you need an Arsenal bump?

dvasilas · 2025-12-04T16:22:10Z

schema/server_access_log.schema.json

+        "startTime": {
+            "description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type to be compatible with clickhouse DateTime64(3) type.",
+            "type": "string",
+            "minimum": 0


Minimum is probably not needed here.

The startTime cannot be negative, so it is just an extra sanity check

Yes, I mean that it may not apply because statTime is a string. Not sure.

Forgot it was a string, you are right

.github/docker/docker-compose.yaml

jonathan-gramain · 2025-12-04T23:00:22Z

schema/server_access_log.schema.json

@@ -0,0 +1,248 @@
+{
+    "$id": "https://scality.com/cloudserver/server_access_log.schema.json",


I would rather see a github.com link pointing to the actual file in a given branch of the Cloudserver repository here

replaced with "https://github.com/scality/cloudserver/tree/development/9.2/schema/lib/server_access_log.schema.json"

jonathan-gramain · 2025-12-04T23:02:07Z

schema/server_access_log.schema.json

+    "type": "object",
+    "properties": {
+        "time": {
+            "description": "Epoch timestamp in seconds, recorded when the log record is created.",


nitpick: some descriptions end with a period, some not, they should be consistent. I would think no period is better if they are not complete sentences.

Added periods to all the lines

jonathan-gramain · 2025-12-04T23:02:56Z

schema/server_access_log.schema.json

+            "type": "string"
+        },
+        "accountName": {
+            "description": "Requester account display name.",


Suggested change

"description": "Requester account display name.",

"description": "Requester account name",

Changed to the correct description

jonathan-gramain · 2025-12-04T23:04:36Z

schema/server_access_log.schema.json

+            "enum": ["GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE", "POST", "PATCH", "CONNECT"]
+        },
+        "bytesDeleted": {
+            "description": "For DeleteObject bytesDeleted is the size in bytes of the deleted object, for DeleteObjects it is the sum of all the deleted objects.",


suggestion for a bit less verbosity and more accuracy:

Suggested change

"description": "For DeleteObject bytesDeleted is the size in bytes of the deleted object, for DeleteObjects it is the sum of all the deleted objects.",

"description": "For DeleteObject: size in bytes of the deleted object, for DeleteObjects: byte size sum of all the deleted objects",

jonathan-gramain · 2025-12-04T23:06:34Z

schema/server_access_log.schema.json

+            "minimum": 0
+        },
+        "bytesReceived": {
+            "description": "Size of the object in bytes of PutObject and UploadPart",


Suggested change

"description": "Size of the object in bytes of PutObject and UploadPart",

"description": "For PutObject and UploadPart: size of the object in bytes",

jonathan-gramain · 2025-12-04T23:12:39Z

schema/server_access_log.schema.json

+            "type": "string"
+        },
+        "req_id": {
+            "description": "Represents the AWS server access log 'Request ID' field. Matches the req_id of the stdout logs.",


I think "stdout" is an implementation detail (and it ends up in log files on S3C transparently) so I'd rather just say "matches the req_id field in logs"

jonathan-gramain · 2025-12-04T23:15:28Z

lib/utilities/serverAccessLogger.js

-        'listParts': 'OBJECT',
+        'completeMultipartUpload': 'UPLOAD',
+        'initiateMultipartUpload': 'UPLOAD',
+        'listMultipartUploads': 'UPLOADS',


Checking if it's not a typo and you mean plural here, since others are all singular.

It is upload in this case, but I fixed other issues in the map

https://github.com/awslabs/glue-extensions-for-iceberg/blob/52bdb2908216a85859fd76a45981d0326d016a2f/spark/src/main/scala/software/amazon/glue/s3a/audit/S3LogVerbs.java#L36

jonathan-gramain · 2025-12-04T23:48:51Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

Nitpick: generally speaking, it should be doable to merge tests for groups of API functions that put, get and delete something, in a single test that would put, then get, then delete the API object. Just to reduce the number of tests since I believe the test coverage would be identical.

Not blocking for me, just a suggestion.

jonathan-gramain · 2025-12-04T23:57:50Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+                    const properties = operation.expected[operationIdx];
+                    for (const [key, val] of Object.entries(properties)) {
+                        assert.strictEqual(logEntries[logEntryIdx][key], val,
+                            `Invalid value for ${key}, action ${properties.action}`);
+                    }


Could this be replaced by:

assert.deepStrictEqual(logEntries[logEntryIdx], operation.expected[operationIdx])

A failed assertion would show a diff, so I think it would give the same amount of debug info, if not more.

No, because some fields we don't control like timestamps.

Output with the suggested change

+ actual - expected { accountDisplayName: 'Bart', accountName: 'Bart', aclRequired: null, action: 'DeleteBucket', authenticationType: 'AuthHeader', awsAccessKeyID: 'accessKey1', + bodyLength: 0, bucketName: 'xxx', bucketOwner: null, + bytesDeleted: null, + bytesReceived: 0, + bytesSent: 197, cipherSuite: null, + clientIP: '::ffff:127.0.0.1', + clientPort: 52912, + contentLength: 0, + elapsed_ms: 0.32016, errorCode: 'NoSuchBucket', + hostHeader: '127.0.0.1:8000', + hostname: 'xps', httpCode: 404, httpMethod: 'DELETE', + httpURL: '/xxx', logFormatVersion: '0', loggingEnabled: false, loggingTargetBucket: null, loggingTargetPrefix: null, objectKey: null, + objectSize: null, operation: 'REST.DELETE.BUCKET', + pid: 10496, raftSessionID: null, referer: null, + req_id: '5061ab0fdc4b9cf4db78', + requestURI: 'DELETE /xxx HTTP/1.1', requester: '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be', signatureVersion: 'SigV4', + startTime: '1764951661.826', + time: 1764951661, tlsVersion: null, + totalTime: '0', + turnAroundTime: '0', + userAgent: 'aws-sdk-nodejs/2.1692.0 linux/v22.14.0 promise', userName: null, + versionID: null } + expected - actual "aclRequired": [null] "action": "DeleteBucket" "authenticationType": "AuthHeader" "awsAccessKeyID": "accessKey1" - "bodyLength": 0 "bucketName": "xxx" "bucketOwner": [null] - "bytesDeleted": [null] - "bytesReceived": 0 - "bytesSent": 197 "cipherSuite": [null] - "clientIP": "::ffff:127.0.0.1" - "clientPort": 52912 - "contentLength": 0 - "elapsed_ms": 0.32016 "errorCode": "NoSuchBucket" - "hostHeader": "127.0.0.1:8000" - "hostname": "xps" "httpCode": 404 "httpMethod": "DELETE" - "httpURL": "/xxx" "logFormatVersion": "0" "loggingEnabled": false "loggingTargetBucket": [null] "loggingTargetPrefix": [null] "objectKey": [null] - "objectSize": [null] "operation": "REST.DELETE.BUCKET" - "pid": 10496 "raftSessionID": [null] "referer": [null] - "req_id": "5061ab0fdc4b9cf4db78" - "requestURI": "DELETE /xxx HTTP/1.1" "requester": "79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be" "signatureVersion": "SigV4" - "startTime": "1764951661.826" - "time": 1764951661 "tlsVersion": [null] - "totalTime": "0" - "turnAroundTime": "0" - "userAgent": "aws-sdk-nodejs/2.1692.0 linux/v22.14.0 promise" "userName": [null] - "versionID": [null] }

jonathan-gramain · 2025-12-04T23:59:26Z

lib/server.js


-           try {
+            try {
+                logger.info('ServerAccessLogger config', { 'config': _config.serverAccessLogs });


Suggested change

logger.info('ServerAccessLogger config', { 'config': _config.serverAccessLogs });

logger.info('ServerAccessLogger config', { config: _config.serverAccessLogs });

nicolas2bert · 2025-12-05T10:59:36Z

schema/server_access_log.schema.json

+            "description": "Represents the AWS server access log 'ACL Required' field.",
+            "type": ["null"]
+        },
+        "bucketOwner": {


bucketOwner and bucketName should never be null.
This is what is breaking integration tests.

For some operations like CreateBucket, deleteObjects, uploadPart, the bucket

here are the server access logs for:

deleteobjects:

{"time":1764928092,"hostname":"nicolas2be-952-store-4","pid":81,"action":"DeleteObjects","accountName":"jul","accountDisplayName":"jul","userName":null,"clientPort":51762,"httpMethod":"POST","bytesDeleted":null,"bytesReceived":129,"bodyLength":129,"contentLength":129,"elapsed_ms":183.005601,"httpURL":"/bucket0?delete","startTime":"1764928092.696","requester":"b4e50bcfa5a588f74c82becb4476a0d7442892ba27d05baa847f0528b1076f72","operation":"REST.POST.OBJECT","requestURI":"POST /bucket0?delete HTTP/1.1","errorCode":null,"objectSize":null,"totalTime":"182","turnAroundTime":"100","referer":null,"userAgent":"aws-cli/1.38.10 md/Botocore#1.37.10 ua/2.1 os/macos#24.5.0 md/arch#arm64 lang/python#3.13.2 md/pyimpl#CPython m/N cfg/retry-mode#legacy botocore/1.37.10","versionID":null,"signatureVersion":"SigV4","cipherSuite":null,"authenticationType":"AuthHeader","hostHeader":"51.21.225.168:8080","tlsVersion":null,"aclRequired":null,"bucketOwner":null,"bucketName":null,"req_id":"151a943d0741b0e6144c","bytesSent":183,"clientIP":"10.160.120.218","httpCode":200,"objectKey":null,"logFormatVersion":"0","loggingEnabled":false,"loggingTargetBucket":null,"loggingTargetPrefix":null,"awsAccessKeyID":"GRRX6ENBPIXH0XXUPZ1P","raftSessionID":null}

CreateBucket:

{"time":1764841467,"hostname":"nicolas2be-952-store-1","pid":109,"action":"CreateBucket","accountName":"jul","accountDisplayName":"jul","userName":null,"clientPort":31794,"httpMethod":"PUT","bytesDeleted":null,"bytesReceived":0,"bodyLength":0,"contentLength":0,"elapsed_ms":268.683911,"httpURL":"/bucket0","startTime":"1764841467.173","requester":"b4e50bcfa5a588f74c82becb4476a0d7442892ba27d05baa847f0528b1076f72","operation":"REST.PUT.BUCKET","requestURI":"PUT /bucket0 HTTP/1.1","errorCode":null,"objectSize":null,"totalTime":"268","turnAroundTime":"224","referer":null,"userAgent":"aws-cli/1.38.10 md/Botocore#1.37.10 ua/2.1 os/macos#24.5.0 md/arch#arm64 lang/python#3.13.2 md/pyimpl#CPython m/N cfg/retry-mode#legacy botocore/1.37.10","versionID":null,"signatureVersion":"SigV4","cipherSuite":null,"authenticationType":"AuthHeader","hostHeader":"51.21.225.168:8080","tlsVersion":null,"aclRequired":null,"bucketOwner":null,"bucketName":null,"req_id":"cbfa47daf640b6b6ec00","bytesSent":null,"clientIP":"10.160.120.218","httpCode":200,"objectKey":null,"logFormatVersion":"0","loggingEnabled":false,"loggingTargetBucket":null,"loggingTargetPrefix":null,"awsAccessKeyID":"GRRX6ENBPIXH0XXUPZ1P","raftSessionID":null}

Fixed by b361882

bucketOwner and bucketName should never be null.

For ListBuckets they are going to be null

dvasilas

We should also include tests about error cases (AccessDenied, NoSuchBucket, ..).

leif-scality · 2025-12-05T16:26:54Z

We should also include tests about error cases (AccessDenied, NoSuchBucket, ..).

I added one test with NoSuchBucket to check the errorCode is set

leif-scality · 2025-12-05T17:00:56Z

ping

bert-e · 2025-12-05T17:01:05Z

Incorrect fix version

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

9.2.5

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

9.2.6

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

leif-scality · 2025-12-05T17:26:08Z

We should also include tests about error cases (AccessDenied, NoSuchBucket, ..).

I added one test with NoSuchBucket to check the errorCode is set

Also added error test for PutObject and GetObject

anurag4DSB

As this is a new standard which will be used by future updates to S3 analytics, we should also write a document to guide engineers on whats the right way to add new fields to server access logs. This will be helpful for everyone to adhere to the standards thought by the creators of server access logs.

lib/api/api.js

anurag4DSB · 2025-12-08T06:51:39Z

schema/server_access_log.schema.json

+        "operation": {
+            "description": "AWS server access log 'Operation' field.",
+            "type": "string"
+        },
+        "requestURI": {
+            "description": "AWS server access log 'Request URI' field.",
+            "type": "string"
+        },
+        "errorCode": {
+            "description": "AWS server access log 'Error Code' field.",
+            "type": ["string", "null"]
+        },
+        "objectSize": {
+            "description": "AWS server access log 'Object Size' field.",
+            "type": ["integer", "null"]
+        },
+        "totalTime": {
+            "description": "AWS server access log 'Total Time' field.",
+            "type": "string"
+        },
+        "turnAroundTime": {
+            "description": "AWS server access log 'Turn Around Time' field.",
+            "type": ["string", "null"]
+        },
+        "referer": {
+            "description": "AWS server access log 'Referer' field.",
+            "type": ["string", "null"]
+        },
+        "userAgent": {
+            "description": "AWS server access log 'User Agent' field.",
+            "type": ["string", "null"]
+        },
+        "versionID": {
+            "description": "AWS server access log 'Version ID' field.",
+            "type": ["string", "null"]
+        },
+        "signatureVersion": {
+            "description": "AWS server access log 'Signature Version' field.",
+            "type": ["string", "null"]
+        },
+        "cipherSuite": {
+            "description": "AWS server access log 'Cipher Suite' field.",
+            "type": ["string", "null"]
+        },
+        "authenticationType": {
+            "description": "AWS server access log 'Authentication Type' field.",
+            "type": ["string", "null"]
+        },
+        "hostHeader": {
+            "description": "AWS server access log 'Host Header' field.",
+            "type": "string"
+        },
+        "tlsVersion": {
+            "description": "AWS server access log 'TLS Version' field.",
+            "type": ["string", "null"]
+        },
+        "aclRequired": {
+            "description": "AWS server access log 'ACL Required' field.",
+            "type": ["null"]


For all the "AWS server access logs 'XXX' field." descriptions, have we considered using actual descriptions as AWS docs change without version control? Or is it intentional that we want engineers to refer to https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html for what the fields actually represent?
It's worth adding the actual description, and we can add a code comment that these are AWS server access log fields.
Example for aclRequired, AWS server access log 'aclRequired' field indicating whether ACL authorization was required. would be more helpful than "AWS server access log 'ACL Required' field."

Also if we want to be AWS compatible, the proper description will also help us check if we are actually compatible. For example for aclRequired allows null, but the actual AWS schema should accept "Yes" or "-".

Example correction:

"aclRequired": { "description": "AWS server access log 'aclRequired' field indicating whether ACL authorization was required.", "type": "string", "enum": ["Yes", "-"] }

if we want to allow nulls in our own internal representation, we could explicitly combine them with something like this

"aclRequired": { "description": "AWS server access log 'aclRequired' field indicating whether ACL authorization was required.", "type": ["string", "null"], "enum": ["Yes", "-"] }

I will let you check and change for others as well.

inlined descriptions

anurag4DSB · 2025-12-08T07:04:26Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+            try {
+                return lines.map(line => JSON.parse(line));
+            } catch (err) {
+                // FIXME: readFileSync may read partial lines making JSON.parse fail, so we need to retry.


The FIXME comments acknowledge that readFileSync may read partial lines, causing JSON.parse to fail. This is a real race condition that will cause flaky tests.

We could use a more robust approach with file position tracking

We prevent flaky test by retrying, cloudserver should not take more than 5 seconds to write the logs
I already create a ticket to track this CLDSRV-800

Could we add the ticket number to the comment please?
(for people just looking at the code)

anurag4DSB · 2025-12-08T07:06:19Z

schema/server_access_log.schema.json

+            "type": "string",
+            "enum": ["GET", "HEAD", "OPTIONS", "TRACE", "PUT", "DELETE", "POST", "PATCH", "CONNECT"]
+        },
+        "bytesDeleted": {


bytesDeleted, bytesReceived, bodyLength, contentLength all have type: ["integer", "null"] with minimum: 0, wouldn't null values fail the minimum constraint?
We could do something like this":

{ "bytesDeleted": { "description": "For DeleteObject: size in bytes...", "oneOf": [ {"type": "integer", "minimum": 0}, {"type": "null"} ] } }

Or better, make these fields optional and omit them when null.

The minimum is only applied when the value is an integer

anurag4DSB · 2025-12-08T07:10:32Z

schema/server_access_log.schema.json

@@ -0,0 +1,247 @@
+{


For future iterations, we could also add a pattern for these fields.

anurag4DSB · 2025-12-08T07:20:25Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+                        {
+                            ...commonProperties,
+                            operation: 'REST.PUT.BUCKET',
+                            bucketOwner: null,
+                            action: 'CreateBucket',
+                            httpMethod: 'PUT',
+                        },
+                        {
+                            ...commonProperties,
+                            operation: 'REST.PUT.CORS',
+                            action: 'PutBucketCors',
+                            httpMethod: 'PUT',
+                        },
+                        {
+                            ...commonProperties,
+                            operation: 'REST.DELETE.CORS',
+                            action: 'DeleteBucketCors',
+                            httpCode: 204,
+                            httpMethod: 'DELETE',


NIT: There is a lot of code duplication, worth extracting into a helper function.
Something like this

function expectedCreateBucket(overrides = {}) { return { ...commonProperties, operation: 'REST.PUT.BUCKET', bucketOwner: null, action: 'CreateBucket', httpMethod: 'PUT', ...overrides, }; } // Usage: expected: [ expectedCreateBucket(), expectedPutObject({ objectKey }), expectedDeleteObject({ objectKey, httpCode: 204 }), ]

Yeah looking back we can do better, the log format should change so I can add the improvements when I update the tests in the next iteration

anurag4DSB · 2025-12-08T07:21:27Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+        });
+
+        for (const operation of operations) {
+            it(`check ${operation.methodName} log entry`, async () => {


Worth being more descriptive here

Suggested change

it(`check ${operation.methodName} log entry`, async () => {

it(`should log correct ${operation.methodName} operation with all required fields`, async () => {

anurag4DSB · 2025-12-08T07:22:25Z

schema/server_access_log.schema.json

+        "awsAccessKeyID",
+        "raftSessionID"
+    ]
+}


The schema doesn't specify "additionalProperties": false, allowing undocumented fields to be added.
This ensures strict validation and prevents accidental field additions.

{ "properties": { /* ... */ }, "required": [ /* ... */ ], "additionalProperties": false }

anurag4DSB · 2025-12-08T07:23:09Z

tests/functional/aws-node-sdk/test/serverAccessLogs/testServerAccessLogFile.js

+        bu = bucketUtil;
+        const s3 = bucketUtil.s3;
+        const logFilePath = config.serverAccessLogs.outputFile;
+        const bucketName = 'test-server-access-log-bucket';


If tests run in parallel or are interrupted, could this cause conflicts?

The tests expect the server-access.log file to be empty, if tests are run in parallel this is no longer true so they will break.

anurag4DSB · 2025-12-08T07:25:55Z

lib/utilities/serverAccessLogger.js

+        httpMethod: req.method === undefined ? null : req.method,
+        bytesDeleted: params.analyticsBytesDeleted === undefined ? null : params.analyticsBytesDeleted,
+        bytesReceived: req.parsedContentLength === undefined ? null : req.parsedContentLength,
+        bodyLength: req.headers['content-length'] === undefined ? null : parseInt(req.headers['content-length'], 10),


Very nitpicking here: There are some hard-to-read and sometimes inconsistent complex null coalescing here.
Could we standardize them?
example:

accountDisplayName: authInfo?.getAccountDisplayName() ?? null, clientPort: req.socket?.remotePort ?? null, bytesDeleted: params.analyticsBytesDeleted ?? null,

bert-e · 2025-12-08T07:26:50Z

Waiting for approval

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

the author
2 peers

The following reviewers are expecting changes from the author, or must review again:

@anurag4DSB

Removing blocker

bert-e · 2025-12-08T08:50:03Z

Waiting for approval

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

the author
2 peers

anurag4DSB · 2025-12-08T08:51:06Z

Removing blocker, as per discussion in daily, might be addressed later in other tickets.

BourgoisMickael · 2025-12-08T14:04:21Z

lib/utilities/serverAccessLogger.js

        }

-        return len;
+        return Number(len);


It's not very nice to encapsulate a primitive into its object constructor instance.

Besides the constructor might convert '' to 0 instead of NaN. Is it the expected behavior to default to 0 for empty string ?

Changed to

if (request && response && objectSizeGetMethods[request.apiMethod]) { const len = Number(response.getHeader('Content-Length')); if (len === undefined || len === null || len === '') { return null; } const numLen = Number(len); if (isNaN(numLen)) { return null; } return numLen; }

That doesn't change the use of Number, this double the usage. And Number cannot return null or undefined. It returns a number

lib/utilities/serverAccessLogger.js

BourgoisMickael

getBytesSent could be simplfied to

function getBytesSent(res, bytesSent) {
    if (bytesSent !== undefined && bytesSent !== null) {
        return bytesSent;
    }
    return res?.getHeader('Content-Length') || null;
}

leif-scality · 2025-12-08T18:12:43Z

ping

leif-scality · 2025-12-08T18:14:54Z

/approve

bert-e · 2025-12-08T18:15:06Z

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

✔️ development/9.2

The following branches have NOT changed:

development/7.10
development/7.4
development/7.70
development/8.8
development/9.0
development/9.1

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

Goodbye leif-scality.

The following options are set: approve

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch 3 times, most recently from 47d0be4 to 51d233b Compare December 2, 2025 20:59

dvasilas reviewed Dec 3, 2025

View reviewed changes

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch 6 times, most recently from b7b0eff to e19d4bc Compare December 3, 2025 20:20

dvasilas approved these changes Dec 4, 2025

View reviewed changes

tcarmet reviewed Dec 4, 2025

View reviewed changes

.github/docker/docker-compose.yaml Show resolved Hide resolved

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch from e19d4bc to acb14ed Compare December 4, 2025 17:55

jonathan-gramain reviewed Dec 4, 2025

View reviewed changes

nicolas2bert reviewed Dec 5, 2025

View reviewed changes

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch from acb14ed to 1d89a88 Compare December 5, 2025 16:16

dvasilas reviewed Dec 5, 2025

View reviewed changes

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch from 1d89a88 to a7fbdf4 Compare December 5, 2025 16:23

CLDSRV-780: store serverAccessLog.bucketName in api.js

d1e284d

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch 4 times, most recently from 204cfdf to 90dbbab Compare December 5, 2025 17:00

leif-scality requested review from jonathan-gramain, nicolas2bert and tcarmet December 5, 2025 19:07

tcarmet approved these changes Dec 6, 2025

View reviewed changes

anurag4DSB previously requested changes Dec 8, 2025

View reviewed changes

BourgoisMickael reviewed Dec 8, 2025

View reviewed changes

lib/utilities/serverAccessLogger.js Show resolved Hide resolved

BourgoisMickael reviewed Dec 8, 2025

View reviewed changes

lib/utilities/serverAccessLogger.js Outdated Show resolved Hide resolved

BourgoisMickael reviewed Dec 8, 2025

View reviewed changes

CLDSRV-780: json schema for server access logs

f1ce650

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch 2 times, most recently from b21fd94 to a20fa05 Compare December 8, 2025 16:21

leif-scality added 7 commits December 8, 2025 17:43

CLDSRV-780: update ServerAccessLogger to follow json schema

2aa771f

CLDSRV-780: add server access logs functional tests

7181d53

CLDSRV-780: log access logs config at startup

8b0b141

CLDSRV-780: update CI for server access logs functional tests

03c50d2

CLDSRV-780: bump arsenal to 8.2.42

5699a75

CLDSRV-780: add bucket owner to server access log in multiObjectDelete

7959f76

CLDSRV-780: Bump version to 9.2.6

f756789

leif-scality force-pushed the feature/CLDSRV-780-test-server-access-logs-file branch from a20fa05 to f756789 Compare December 8, 2025 16:43

bert-e merged commit f756789 into development/9.2 Dec 8, 2025
98 of 99 checks passed

bert-e deleted the feature/CLDSRV-780-test-server-access-logs-file branch December 8, 2025 18:15

	"description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type because to be compatible with clickhouse DateTime64(3) type.",
	"description": "Timestamp in seconds.milliseconds, recorded when the server first routes the request. Represents the AWS server access log 'Time' field. String type to be compatible with clickhouse DateTime64(3) type.",

	"description": "If true it means that the target bucket has bucket logging configured and enabled.",
	"description": "True if the target bucket has bucket logging configured and enabled.",

	"description": "Represents the AWS server access log 'Requester' field.",
	"description": "Represents the AWS server access log 'Requester' field. https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html",

		@@ -0,0 +1,248 @@
		{
		"$id": "https://scality.com/cloudserver/server_access_log.schema.json",

	"description": "Requester account display name.",
	"description": "Requester account name",

	"description": "For DeleteObject bytesDeleted is the size in bytes of the deleted object, for DeleteObjects it is the sum of all the deleted objects.",
	"description": "For DeleteObject: size in bytes of the deleted object, for DeleteObjects: byte size sum of all the deleted objects",

	"description": "Size of the object in bytes of PutObject and UploadPart",
	"description": "For PutObject and UploadPart: size of the object in bytes",

	logger.info('ServerAccessLogger config', { 'config': _config.serverAccessLogs });
	logger.info('ServerAccessLogger config', { config: _config.serverAccessLogs });

	it(`check ${operation.methodName} log entry`, async () => {
	it(`should log correct ${operation.methodName} operation with all required fields`, async () => {

CLDSRV-780: ServerAccessLogs json schema and functional tests #6020

CLDSRV-780: ServerAccessLogs json schema and functional tests #6020

Uh oh!

Conversation

leif-scality commented Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bert-e commented Dec 2, 2025

Hello leif-scality,

Uh oh!

bert-e commented Dec 2, 2025

Incorrect fix version

Uh oh!

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

Uh oh!

❌ 1 Tests Failed:

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!

Choose a reason for hiding this comment

Uh oh!

leif-scality Dec 3, 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!

bert-e commented Dec 3, 2025

Incorrect fix version

Uh oh!

dvasilas left a comment

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!

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!

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

leif-scality commented Dec 2, 2025 •

edited

Loading

codecov bot commented Dec 2, 2025 •

edited

Loading

leif-scality Dec 3, 2025 •

edited

Loading