bucket logging endpoint changes #7426

aspandey · 2023-08-02T16:30:40Z

Explain the changes

This is a draft of bucket logging feature.
2.Basic idea is to log bucket operation so that replication engine can use it to replicate
that operation like put object.
As soon as operation request return a reply, we get the status and other information for that operation and
create a message with S3 log format and send it to noobaa log bucket configured by the user.
Mainly this task is done in handle_request function of s3_rest.js

aspandey · 2023-08-02T18:30:26Z

aspandey · 2023-08-21T07:18:51Z

@Neon-White @tangledbytes This patch contains creation of log records at the endpoint handler request.
Could you please review this patch? Along with other comments , I need your specific comments on the log format and the information (in log records) which is required by replication engine for the replicating objects.

src/api/bucket_api.js

Neon-White · 2023-08-21T08:18:05Z

src/endpoint/s3/s3_rest.js

@@ -533,6 +551,7 @@ function consume_usage_report() {
 }


+


Seems like this newline can be removed

src/endpoint/s3/s3_bucket_logging.js

dannyzaken · 2023-08-23T11:58:32Z

src/deploy/NVA_build/NooBaa.Dockerfile

@@ -120,6 +120,9 @@ RUN chmod 775 /noobaa_init_files && \
    chgrp -R 0 /noobaa_init_files/ && \
    chmod -R g=u /noobaa_init_files/

+RUN chmod 777 /etc/rsyslog.conf


let's remove rsyslog related changes for now

guymguym · 2023-09-07T15:36:10Z

@aspandey @dannyzaken Can we include in this PR an option for src/cmd/nsfs to enable bucket logging?

guymguym · 2023-09-07T15:39:15Z

@aspandey @dannyzaken Can we include in this PR an option for src/cmd/nsfs to enable bucket logging?

To clarify - I can set up the syslog interface manually - but I do need a flag to enable sending out the logs for nsfs command line.

dannyzaken · 2023-09-10T14:11:01Z

src/endpoint/s3/ops/s3_get_bucket_logging.js

+    return format_logging_response(reply.logging);
+}
+
+function format_logging_response(log_set) {


this function doesn't seem correct to me.

we shouldn't throw an error if there is no bucket logging enabled.

it does not format the response. see the required response here.

Also, we should probably also implement s3_put_bucket_logging. I think both can be done in a different PR together with tests

Right. I will make changes.

Got it. I have removed this change. Will send different PR as suggested by you.

dannyzaken · 2023-09-10T14:13:36Z

src/deploy/NVA_build/NooBaa.Dockerfile

@@ -120,6 +120,7 @@ RUN chmod 775 /noobaa_init_files && \
    chgrp -R 0 /noobaa_init_files/ && \
    chmod -R g=u /noobaa_init_files/

+


dannyzaken · 2023-09-11T08:58:41Z

src/endpoint/s3/s3_rest.js

+    let bucket_info = null;
+    if (req.params && req.params.bucket) {
+        try {
+            bucket_info = await req.object_sdk.internal_rpc_client.bucket.read_bucket_sdk_info({ name: req.params.bucket });


this will perform an RPC call to core on every S3 request. object_sdk should have this data in its cache and we should not perform an RPC call to get it

Got it...will make appropriate changes .

dannyzaken · 2023-09-11T09:00:02Z

src/endpoint/s3/s3_rest.js

+        try {
+            bucket_info = await req.object_sdk.internal_rpc_client.bucket.read_bucket_sdk_info({ name: req.params.bucket });
+            if (bucket_info) {
+                dbg.info("bucket_info =", bucket_info);


there is no dbg.info. use dbg.log0 to dbg.log5 for different log levels.

dannyzaken · 2023-09-11T09:06:20Z

src/endpoint/s3/s3_rest.js

+                dbg.info("bucket_info =", bucket_info);
+            }
+        } catch (err) {
+            dbg.log1("Could not get bucket info from cache", err);


in general, for most errors (unless expected ones) you should use dbg.error and rethrow

dannyzaken · 2023-09-14T17:19:51Z

src/endpoint/s3/s3_rest.js

+    let bucket_info = null;
+    if (req.params && req.params.bucket) {
+        bucket_info = await req.object_sdk.read_bucket_sdk_config_info(req.params.bucket);
+        dbg.log0("read_bucket_sdk_config_info =  ", bucket_info);


This should not be dbg.log0 as it happens on every request and can overload the log. you can change it to log2 or 3

dannyzaken · 2023-09-14T17:20:34Z

src/endpoint/s3/s3_rest.js

+    }
+    // Check if logging is enabled or not and send the S3 logs accordingly.
+    if (s3_logging.is_bucket_logging_enabled(bucket_info, req.params.bucket)) {
+        dbg.log0("Bucket logging is enabled on Bucket : ", req.params.bucket);


dannyzaken · 2023-09-14T17:42:15Z

src/endpoint/s3/s3_rest.js

    collect_bucket_usage(op, req, res);
+
+    let bucket_info = null;


no need to init it to null. it is initialized as undefined

dannyzaken · 2023-09-14T17:45:17Z

src/endpoint/s3/s3_rest.js

+        dbg.log0("read_bucket_sdk_config_info =  ", bucket_info);
+    }
+    // Check if logging is enabled or not and send the S3 logs accordingly.
+    if (s3_logging.is_bucket_logging_enabled(bucket_info, req.params.bucket)) {


If I'm not mistaken, this code should be under the if in line 153.

Also, passing req.params.bucket is redundant, since bucket_info should already contain the name.

dannyzaken · 2023-09-14T17:48:07Z

src/endpoint/s3/s3_bucket_logging.js

+
+function endpoint_bucket_op_logs(op_name, req, res, source_bucket) {
+
+    dbg.info("Sending op logs for op name = ", op_name);


there is no dbg.info. you can change to dbg.log2 or dbg.log3

dannyzaken · 2023-09-14T17:58:04Z

src/endpoint/s3/s3_bucket_logging.js

+    }
+    values = values.slice(0, -2);
+    //Just to log it to endpoint logs. Need to remove it later.
+    dbg.log0("!!!Object operation log!!! :", values);


Even though we plan to replace it with a syslog call eventually, let's change it to dbg.log1 to avoid excessive logging.

src/endpoint/s3/s3_bucket_logging.js

dannyzaken · 2023-09-14T18:11:34Z

src/endpoint/s3/s3_bucket_logging.js

+        request_id: req.request_id
+    };
+
+    const s3_log = new Aws_log_entry();


See my comment in response to Ben. I think we should format it in the (syslog) server side before writing the log object. I think that for now you can just return log as the log record and send it as JSON

dannyzaken · 2023-09-14T18:14:40Z

src/endpoint/s3/s3_bucket_logging.js

+    let values = "";
+    for (const key of Object.keys(s3_log)) {
+        const value = s3_log[key];
+        values += `${value} `;
+    }


I think we should send it as a json (use JSON.stringify()). it is more generic and can be easily parsed later

dannyzaken · 2023-09-14T18:16:21Z

src/endpoint/s3/s3_bucket_logging.js

+    const enable = check_logging_config_perm(source_bucket.bucket_info.logging);
+    return enable;


This check is unnecessary now. we can add it later if we see a reason to.

Suggested change

const enable = check_logging_config_perm(source_bucket.bucket_info.logging);

return enable;

return true;

dannyzaken · 2023-09-14T18:20:16Z

@aspandey also pay attention to the DCO and deepscan tests

alphaprinz · 2023-10-04T12:16:39Z

src/endpoint/s3/s3_rest.js

    collect_bucket_usage(op, req, res);
+
+    let bucket_info;


bucket_info is not used outside the following if scope.
it's better to remove this line and add const inside the scope

src/server/system_services/bucket_server.js

alphaprinz · 2023-10-04T12:18:34Z

src/server/system_services/bucket_server.js

            }]
        }
    });
 }

 async function get_bucket_logging(req) {
-    dbg.log0('get_bucket_logging:', req.rpc_params);
+
+    dbg.log0('delete_bucket_logging:', req.rpc_params);


src/server/system_services/bucket_server.js

alphaprinz · 2023-10-04T12:20:52Z

src/endpoint/s3/s3_bucket_logging.js

+function is_bucket_logging_enabled(source_bucket) {
+
+    if (!source_bucket || !source_bucket.bucket_info.logging) {
+


remove empty line

alphaprinz · 2023-10-04T12:21:36Z

src/endpoint/s3/s3_rest.js

+        dbg.log2("read_bucket_sdk_config_info =  ", bucket_info);
+
+        if (s3_logging.is_bucket_logging_enabled(bucket_info)) {
+            dbg.log2("Bucket logging is enabled on Bucket : ", req.params.bucket);


for bucket:

alphaprinz · 2023-10-04T12:27:48Z

src/endpoint/s3/s3_bucket_logging.js

+    // 2 - Format it and send it to log bucket/syslog.
+
+    const s3_log = get_bucket_log_record(op_name, source_bucket, req, res);
+    dbg.log1("!!!BUCKET Operation Logs!!! ", s3_log);


A bit dramatic...

Suggested change

dbg.log1("!!!BUCKET Operation Logs!!! ", s3_log);

dbg.log1("bucket op log = ", s3_log);

Also, consider prefixing all logs realted to bucket op logs with a common prefix.
It would make it easier to grep for them.
Eg-
dbg.log("bucket op log: sending op logs for op name = ")

I agree with you on prefix part. For now, I am keeping it as it is as we also need to decide on log format.

alphaprinz · 2023-10-04T12:31:12Z

src/endpoint/s3/s3_bucket_logging.js

+
+    const client_ip = http_utils.parse_client_ip(req);
+
+    const log = {


I think some fields are missing?
-time
-requester (if we have such a concept)
-error code
-bytes_send (if available?)
-object size
etc. see https://docs.aws.amazon.com/AmazonS3/latest/userguide/LogFormat.html

Same here. We may not log the exact S3 format. So will decide later.

This version is meeting the current "definition of done". Further changes as warranted.

Signed-off-by: Ashish Pandey <aspandey@redhat.com>

pull-request-size bot added the size/L label Aug 2, 2023

aspandey marked this pull request as draft August 2, 2023 16:35

aspandey requested a review from dannyzaken August 2, 2023 18:30

aspandey force-pushed the logging branch from cbe357a to b0a9610 Compare August 10, 2023 06:05

aspandey force-pushed the logging branch from b0a9610 to 5a4d659 Compare August 21, 2023 07:04

aspandey marked this pull request as ready for review August 21, 2023 07:14

aspandey requested review from Neon-White and tangledbytes August 21, 2023 07:15

Neon-White reviewed Aug 21, 2023

View reviewed changes

dannyzaken reviewed Aug 23, 2023

View reviewed changes

aspandey force-pushed the logging branch from 5a4d659 to be91d8f Compare September 7, 2023 11:50

aspandey force-pushed the logging branch from be91d8f to 88f2104 Compare September 10, 2023 13:40

dannyzaken reviewed Sep 10, 2023

View reviewed changes

dannyzaken reviewed Sep 11, 2023

View reviewed changes

aspandey force-pushed the logging branch from 88f2104 to 10154b3 Compare September 13, 2023 06:55

dannyzaken previously requested changes Sep 14, 2023

View reviewed changes

dannyzaken requested a review from vh05 September 21, 2023 08:23

aspandey force-pushed the logging branch 2 times, most recently from 612b566 to 81a61dc Compare September 25, 2023 06:12

dannyzaken requested review from alphaprinz and dannyzaken October 2, 2023 14:11

alphaprinz reviewed Oct 4, 2023

View reviewed changes

aspandey force-pushed the logging branch from 81a61dc to 16f0154 Compare October 5, 2023 09:24

aspandey force-pushed the logging branch 3 times, most recently from a8d6644 to f6f1a6e Compare October 9, 2023 11:02

alphaprinz approved these changes Oct 9, 2023

View reviewed changes

bucket logging endpoint changes

cd9b4a2

Signed-off-by: Ashish Pandey <aspandey@redhat.com>

aspandey force-pushed the logging branch from f6f1a6e to cd9b4a2 Compare October 10, 2023 06:00

aspandey merged commit a3e960b into noobaa:master Oct 10, 2023
8 checks passed

		@@ -120,6 +120,7 @@ RUN chmod 775 /noobaa_init_files && \
		chgrp -R 0 /noobaa_init_files/ && \
		chmod -R g=u /noobaa_init_files/


		function endpoint_bucket_op_logs(op_name, req, res, source_bucket) {

		dbg.info("Sending op logs for op name = ", op_name);

		const enable = check_logging_config_perm(source_bucket.bucket_info.logging);
		return enable;

	const enable = check_logging_config_perm(source_bucket.bucket_info.logging);
	return enable;
	return true;

		function is_bucket_logging_enabled(source_bucket) {

		if (!source_bucket \|\| !source_bucket.bucket_info.logging) {

	dbg.log1("!!!BUCKET Operation Logs!!! ", s3_log);
	dbg.log1("bucket op log = ", s3_log);


		const client_ip = http_utils.parse_client_ip(req);

		const log = {

bucket logging endpoint changes #7426

bucket logging endpoint changes #7426

Conversation

aspandey commented Aug 2, 2023

Explain the changes

aspandey commented Aug 2, 2023

aspandey commented Aug 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guymguym commented Sep 7, 2023

guymguym commented Sep 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannyzaken commented Sep 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dannyzaken commented Sep 14, 2023 •

edited