Adding nginx_proxy access_log format ability #4102

tataruty · 2025-10-17T12:02:10Z

Proposed changes

Problem: As a user of NGF
I want to have the ability to configure the log format of NGINX's access and error logs
So that I can easily collect logs from NGINX in my logging platform.

Solution: Adding log_format and access_log fields to nginx-proxy CRD, also adding default values for them

Testing: TODO

Closes #1200

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

codecov · 2025-10-17T12:04:53Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.01%. Comparing base (1b34946) to head (3d18022).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4102      +/-   ##
==========================================
+ Coverage   85.99%   86.01%   +0.01%     
==========================================
  Files         131      131              
  Lines       14063    14123      +60     
  Branches       35       35              
==========================================
+ Hits        12094    12148      +54     
- Misses       1769     1773       +4     
- Partials      200      202       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

tataruty · 2025-10-17T12:03:05Z

apis/v1alpha2/nginxproxy_types.go

+	Format    string `json:"format"`
+	Buffer    string `json:"buffer,omitempty"`
+	Condition string `json:"condition,omitempty"`
+	Gzip      bool   `json:"gzip,omitempty"`


I'm really not sure if we need those Buffer, Condition and Gzip or they are part of format itself

I'm not convinced we should support writing logs to a file right now, especially due to our read-only filesystem structure. But I'll defer to others who can spend a bit more time reviewing and researching than I can right now :)

sidenote: if you add any optional fields to a CRD spec, they should be pointer types (except for slices and maps, those are pointers already). The reason for this is that if it's not a pointer type, then if we see that field as its zero value, we have no idea if a user set it to that, or if they just omitted it. By having it be a pointer, it's more obvious because if it's a zero value, we know that a user set it to that, otherwise it would be nil if a user didn't set it.

I'll do a full review next week as the release work is taking up a lot of my time today, but I'll address your questions as best I can now.

I agree with Saylor, we should not support writing to a file. It would require a volume mount, which is not easily configurable, and anyway we don't support log rotation so it would fill up quickly and be an operational nightmare.

The scope of the ticket is just to support configuring the format, however we could also support syslog as a destination, but the default should be /dev/stdout.

Following on from that, we don't need to support the Buffer, and Gzip fields as they only really make sense when writing to a file. Condition requires map directives, we don't need it right now.

We should support the ability to disable the access logs completely too.

Here's what NIC supports for reference: https://docs.nginx.com/nginx-ingress-controller/configuration/global-configuration/configmap-resource/#logging

tataruty · 2025-10-17T12:03:57Z

apis/v1alpha2/nginxproxy_types.go

+	Buffer    string `json:"buffer,omitempty"`
+	Condition string `json:"condition,omitempty"`
+	Gzip      bool   `json:"gzip,omitempty"`
+}


Also not sure if combined option should be included:

map $status $loggable { ~^[23] 0; default 1; } access_log /path/to/access.log combined if=$loggable;

tataruty · 2025-10-17T12:04:38Z

internal/controller/provisioner/objects.go

+	if len(logFormats) == 0 {
+		logFormats = append(logFormats, ngfAPIv1alpha2.LogFormat{
+			Name:   "default",
+			Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent",


if we have a default format, then what it should be?

sjberman · 2025-10-17T14:15:19Z

apis/v1alpha2/nginxproxy_types.go

+	// +optional
+	LogFormats []LogFormat `json:"logFormats,omitempty"`
+
+	// AccessLogs defines the access log settings, including the log file path, format, and optional parameters.


While log_format can only be specified in the http context, access_logcan be specified at pretty much any context. This can be useful for users who want to add logs for a specificlocation, for example, instead of the entire http` block.

Has there been any discussion or thought around how we could let a user specify at that level to be more granular? I think it would probably end up being a policy or filter to do that, which would obviously be a lot more work than this. Since this work is just targeting logging at the http context, let's be clear in this comment that this only applies to that context. If we document this feature, we should also be clear about that.

sjberman · 2025-10-17T14:22:07Z

apis/v1alpha2/nginxproxy_types.go

+	Format    string `json:"format"`
+	Buffer    string `json:"buffer,omitempty"`
+	Condition string `json:"condition,omitempty"`
+	Gzip      bool   `json:"gzip,omitempty"`


I'm not convinced we should support writing logs to a file right now, especially due to our read-only filesystem structure. But I'll defer to others who can spend a bit more time reviewing and researching than I can right now :)

sidenote: if you add any optional fields to a CRD spec, they should be pointer types (except for slices and maps, those are pointers already). The reason for this is that if it's not a pointer type, then if we see that field as its zero value, we have no idea if a user set it to that, or if they just omitted it. By having it be a pointer, it's more obvious because if it's a zero value, we know that a user set it to that, otherwise it would be nil if a user didn't set it.

tataruty · 2025-10-24T18:33:01Z

internal/controller/state/dataplane/types.go


 // Logging defines logging related settings for NGINX.
 type Logging struct {
-	// ErrorLevel defines the error log level.


again lost comments

Yes, it's an annoying bug in the linter tool when it automatically optimizes structs for the compiler, so comments have to be manually added back in.

sjberman · 2025-10-24T18:52:22Z

apis/v1alpha2/nginxproxy_types.go

+	Name   *string `json:"name"`
+	Format *string `json:"format"`


Be sure to add omitempty for all pointer fields.

sjberman · 2025-10-24T18:54:18Z

internal/controller/nginx/config/base_http_config_template.go

+{{- if .LoggingSettings }}
+  {{- /* Define custom log format */ -}}
+  {{- if .LoggingSettings.LogFormat.Name }}
+  log_format {{ .LoggingSettings.LogFormat.Name }} '{{ .LoggingSettings.LogFormat.Format }}';


We don't necessarily want to indent the actual directives that get rendered in the conf, otherwise it'll look strange with a random indent. The templating statements can be indented, but the directives should match the indentation of the context of the conf that they're in (in this case, no indent at all).

Same comment for the access_log directives below.

sjberman · 2025-10-24T18:56:02Z

internal/controller/state/dataplane/configuration.go

+// buildLogging converts the API logging spec (currently singular LogFormat / AccessLog fields
+// in v1alpha2) into internal slice-based representation used by templates.
 func buildLogging(gateway *graph.Gateway) Logging {
 	logSettings := Logging{ErrorLevel: defaultErrorLogLevel}


Can we not just use this Logging struct instead of defining a new LoggingSettings struct that contains similar information?

github-project-automation bot added this to NGINX Gateway Fabric Oct 17, 2025

github-project-automation bot moved this to 🆕 New in NGINX Gateway Fabric Oct 17, 2025

github-actions bot added the enhancement New feature or request label Oct 17, 2025

tataruty commented Oct 17, 2025

View reviewed changes

sjberman reviewed Oct 17, 2025

View reviewed changes

tataruty added 2 commits October 24, 2025 19:23

Adding nginx_proxy access_log format ability

3131263

Make logFormat singular and add ability to turn logs off

3d18022

tataruty force-pushed the enhancement/configurabe_dataplane_log_format branch from 0923130 to 3d18022 Compare October 24, 2025 18:28

tataruty marked this pull request as ready for review October 24, 2025 18:30

tataruty requested a review from a team as a code owner October 24, 2025 18:30

tataruty commented Oct 24, 2025

View reviewed changes

sjberman reviewed Oct 24, 2025

View reviewed changes

Adding nginx_proxy access_log format ability #4102

Are you sure you want to change the base?

Adding nginx_proxy access_log format ability #4102

Uh oh!

Conversation

tataruty commented Oct 17, 2025

Proposed changes

Checklist

Release notes

Uh oh!

codecov bot commented Oct 17, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 17, 2025 •

edited

Loading