Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(pgaudit): support pgaudit.log_rowsoption #4394

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

phisco
Copy link
Contributor

@phisco phisco commented Apr 28, 2024

Fixes #4386.

Turns out that with pgaudit.log_rows enabled an additional field is appended at the end of the CSV row.

In order to properly handle audit logs with log_rows enabled and switching it on and off I've extended the CSVRecordReadWriter to be able to handle more than one possible length and switching between them seamlessly. And added to PgAuditRecord the Rows field.

So now the following Cluster:

apiVersion: postgresql.cnpg.io/v1
kind: Cluster
metadata:
  name: cluster-example
spec:
  instances: 3
  postgresql:
    parameters:
      pgaudit.log: "READ, WRITE, FUNCTION, DDL, ROLE"
      pgaudit.log_catalog: "off"
      pgaudit.log_parameter: "on"
      pgaudit.log_relation: "on"
      pgaudit.log_statement: "on"

      pgaudit.log_rows: "on"

  storage:
    size: 1Gi

Properly results in the following log:

{
	"level": "info",
	"ts": "2024-04-28T11:07:05Z",
	"logger": "pgaudit",
	"msg": "record",
	"logging_pod": "cluster-example-2",
	"record": {
		"log_time": "2024-04-28 11:07:05.168 UTC",
		"user_name": "postgres",
		"database_name": "postgres",
		"process_id": "78",
		"connection_from": "[local]",
		"session_id": "662e2dd9.4e",
		"session_line_num": "1",
		"command_tag": "SELECT",
		"session_start_time": "2024-04-28 11:07:05 UTC",
		"virtual_transaction_id": "2/6",
		"transaction_id": "0",
		"error_severity": "LOG",
		"sql_state_code": "00000",
		"application_name": "cnpg-instance-manager",
		"backend_type": "client backend",
		"query_id": "0",
		"audit": {
			"audit_type": "SESSION",
			"statement_id": "1",
			"substatement_id": "1",
			"class": "READ",
			"command": "SELECT",
			"statement": "SELECT (SELECT timeline_id FROM pg_control_checkpoint()), COALESCE(pg_last_wal_receive_lsn()::varchar, ''), COALESCE(pg_last_wal_replay_lsn()::varchar, ''), pg_is_wal_replay_paused()",
			"parameter": "<none>",
			"rows": "1"
		}
	}
}

And switching to pgaudit.log_rows: "off" results in still logs being parsed correctly:

{
	"level": "info",
	"ts": "2024-04-28T12:02:06Z",
	"logger": "pgaudit",
	"msg": "record",
	"logging_pod": "cluster-example-3",
	"record": {
		"log_time": "2024-04-28 12:02:06.189 UTC",
		"user_name": "postgres",
		"database_name": "postgres",
		"process_id": "436",
		"connection_from": "[local]",
		"session_id": "662e3abe.1b4",
		"session_line_num": "1",
		"command_tag": "BIND",
		"session_start_time": "2024-04-28 12:02:06 UTC",
		"virtual_transaction_id": "2/530",
		"transaction_id": "0",
		"error_severity": "LOG",
		"sql_state_code": "00000",
		"application_name": "cnpg-instance-manager",
		"backend_type": "client backend",
		"query_id": "0",
		"audit": {
			"audit_type": "SESSION",
			"statement_id": "1",
			"substatement_id": "1",
			"class": "READ",
			"command": "SELECT",
			"statement": "SELECT (SELECT timeline_id FROM pg_control_checkpoint()), COALESCE(pg_last_wal_receive_lsn()::varchar, ''), COALESCE(pg_last_wal_replay_lsn()::varchar, ''), pg_is_wal_replay_paused()",
			"parameter": "<none>"
		}
	}
}

@github-actions github-actions bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.21 release-1.22 release-1.23 labels Apr 28, 2024
Copy link
Contributor

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

@gbartolini gbartolini added this to the 1.23.2 milestone Jun 11, 2024
@gbartolini
Copy link
Contributor

/test

Copy link
Contributor

@gbartolini, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/9473112733

@github-actions github-actions bot added the ok to merge 👌 This PR can be merged label Jun 11, 2024
phisco and others added 2 commits June 12, 2024 11:30
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
@mnencia
Copy link
Member

mnencia commented Jun 12, 2024

I propose the following commit message:

This path ensures that the additional `rows` field recorded when
the `pgaudit.log_rows` option is enabled is correctly parsed. 

Previously, the presence of that field resulted in audit logs ending up
in the normal log stream. 

Closes #4386

@mnencia mnencia changed the title feat(pgaudit): correctly parse logs with pgaudit.log_rows on fix(pgaudit): correctly parse logs with pgaudit.log_rows enabled Jun 12, 2024
@phisco
Copy link
Contributor Author

phisco commented Jun 12, 2024

@mnencia sounds good to me!

@gbartolini gbartolini changed the title fix(pgaudit): correctly parse logs with pgaudit.log_rows enabled fix(pgaudit): support pgaudit.log_rowsoption Jun 12, 2024
@gbartolini
Copy link
Contributor

I agree this is a bug

@gbartolini gbartolini merged commit fb7d2f2 into cloudnative-pg:main Jun 12, 2024
26 checks passed
cnpg-bot pushed a commit that referenced this pull request Jun 12, 2024
This patch ensures the correct parsing of the additional `rows` field returned
when the `pgaudit.log_rows` option is enabled.

Previously, the presence of this field caused audit logs to be incorrectly routed
to the normal log stream.

Closes #4386

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit fb7d2f2)
cnpg-bot pushed a commit that referenced this pull request Jun 12, 2024
This patch ensures the correct parsing of the additional `rows` field returned
when the `pgaudit.log_rows` option is enabled.

Previously, the presence of this field caused audit logs to be incorrectly routed
to the normal log stream.

Closes #4386

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit fb7d2f2)
cnpg-bot pushed a commit that referenced this pull request Jun 12, 2024
This patch ensures the correct parsing of the additional `rows` field returned
when the `pgaudit.log_rows` option is enabled.

Previously, the presence of this field caused audit logs to be incorrectly routed
to the normal log stream.

Closes #4386

Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
Co-authored-by: Armando Ruocco <armando.ruocco@enterprisedb.com>
(cherry picked from commit fb7d2f2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-requested ◀️ This pull request should be backported to all supported releases ok to merge 👌 This PR can be merged release-1.21 release-1.22 release-1.23
Development

Successfully merging this pull request may close these issues.

[Bug]: Logger PGAudit not working, postgres logger is used instead
4 participants