From bae1e33bb5f1bf744c288fcabbfa41c02ea23f86 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Fri, 17 Oct 2025 12:55:36 +0100 Subject: [PATCH 1/8] Adding nginx_proxy access_log format ability --- apis/v1alpha2/nginxproxy_types.go | 26 ++++ apis/v1alpha2/zz_generated.deepcopy.go | 40 ++++++ .../bases/gateway.nginx.org_nginxproxies.yaml | 38 +++++ deploy/crds.yaml | 38 +++++ internal/controller/provisioner/objects.go | 52 +++++++ .../controller/provisioner/objects_test.go | 132 ++++++++++++++++++ internal/controller/provisioner/templates.go | 12 +- .../state/graph/multiple_gateways_test.go | 24 ++++ 8 files changed, 361 insertions(+), 1 deletion(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index c5d9dfcef1..5142619383 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -297,6 +297,17 @@ type NginxLogging struct { // +optional // +kubebuilder:default=info AgentLevel *AgentLogLevel `json:"agentLevel,omitempty"` + + // LogFormats defines custom log formats that can be used in access logs. + // Each log format must have a unique name. + // + // +optional + LogFormats []LogFormat `json:"logFormats,omitempty"` + + // AccessLogs defines the access log settings, including the log file path, format, and optional parameters. + // + // +optional + AccessLogs []AccessLog `json:"accessLogs,omitempty"` } // NginxErrorLogLevel type defines the log level of error logs for NGINX. @@ -352,6 +363,21 @@ const ( AgentLogLevelFatal AgentLogLevel = "fatal" ) +// LogFormat defines a custom log format for NGINX. +type LogFormat struct { + Name string `json:"name"` + Format string `json:"format"` +} + +// AccessLog defines the configuration for an NGINX access log. +type AccessLog struct { + Path string `json:"path"` + Format string `json:"format"` + Buffer string `json:"buffer,omitempty"` + Condition string `json:"condition,omitempty"` + Gzip bool `json:"gzip,omitempty"` +} + // NginxPlus specifies NGINX Plus additional settings. These will only be applied if NGINX Plus is being used. type NginxPlus struct { // AllowedAddresses specifies IPAddresses or CIDR blocks to the allow list for accessing the NGINX Plus API. diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index 7033aabb3a..558dd0b628 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -13,6 +13,21 @@ import ( apisv1 "sigs.k8s.io/gateway-api/apis/v1" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *AccessLog) DeepCopyInto(out *AccessLog) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AccessLog. +func (in *AccessLog) DeepCopy() *AccessLog { + if in == nil { + return nil + } + out := new(AccessLog) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AutoscalingSpec) DeepCopyInto(out *AutoscalingSpec) { *out = *in @@ -290,6 +305,21 @@ func (in *KubernetesSpec) DeepCopy() *KubernetesSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LogFormat) DeepCopyInto(out *LogFormat) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LogFormat. +func (in *LogFormat) DeepCopy() *LogFormat { + if in == nil { + return nil + } + out := new(LogFormat) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Metrics) DeepCopyInto(out *Metrics) { *out = *in @@ -328,6 +358,16 @@ func (in *NginxLogging) DeepCopyInto(out *NginxLogging) { *out = new(AgentLogLevel) **out = **in } + if in.LogFormats != nil { + in, out := &in.LogFormats, &out.LogFormats + *out = make([]LogFormat, len(*in)) + copy(*out, *in) + } + if in.AccessLogs != nil { + in, out := &in.AccessLogs, &out.AccessLogs + *out = make([]AccessLog, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NginxLogging. diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index 89de827bd4..88138e69ea 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -8016,6 +8016,28 @@ spec: logging: description: Logging defines logging related settings for NGINX. properties: + accessLogs: + description: AccessLogs defines the access log settings, including + the log file path, format, and optional parameters. + items: + description: AccessLog defines the configuration for an NGINX + access log. + properties: + buffer: + type: string + condition: + type: string + format: + type: string + gzip: + type: boolean + path: + type: string + required: + - format + - path + type: object + type: array agentLevel: default: info description: |- @@ -8045,6 +8067,22 @@ spec: - alert - emerg type: string + logFormats: + description: |- + LogFormats defines custom log formats that can be used in access logs. + Each log format must have a unique name. + items: + description: LogFormat defines a custom log format for NGINX. + properties: + format: + type: string + name: + type: string + required: + - format + - name + type: object + type: array type: object metrics: description: |- diff --git a/deploy/crds.yaml b/deploy/crds.yaml index a96a495103..700e4aa194 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -8603,6 +8603,28 @@ spec: logging: description: Logging defines logging related settings for NGINX. properties: + accessLogs: + description: AccessLogs defines the access log settings, including + the log file path, format, and optional parameters. + items: + description: AccessLog defines the configuration for an NGINX + access log. + properties: + buffer: + type: string + condition: + type: string + format: + type: string + gzip: + type: boolean + path: + type: string + required: + - format + - path + type: object + type: array agentLevel: default: info description: |- @@ -8632,6 +8654,22 @@ spec: - alert - emerg type: string + logFormats: + description: |- + LogFormats defines custom log formats that can be used in access logs. + Each log format must have a unique name. + items: + description: LogFormat defines a custom log format for NGINX. + properties: + format: + type: string + name: + type: string + required: + - format + - name + type: object + type: array type: object metrics: description: |- diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 8ad2e1b72b..f6771eb111 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -406,9 +406,15 @@ func (p *NginxProvisioner) buildNginxConfigMaps( workerConnections = *nProxyCfg.WorkerConnections } + // Add LogFormats and AccessLogs to mainFields + logFormats := addLogFormatToNginxConfig(logging) + accessLogs := addAccessLogsToNginxConfig(logging) + mainFields := map[string]interface{}{ "ErrorLevel": logLevel, "WorkerConnections": workerConnections, + "LogFormats": logFormats, + "AccessLogs": accessLogs, } // Create events ConfigMap data using template @@ -1436,3 +1442,49 @@ func DetermineNginxImageName( return fmt.Sprintf("%s:%s", image, tag), pullPolicy } + +func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) []ngfAPIv1alpha2.LogFormat { + logFormats := []ngfAPIv1alpha2.LogFormat{} + if logging == nil { + return logFormats + } + + for _, lf := range logging.LogFormats { + logFormats = append(logFormats, ngfAPIv1alpha2.LogFormat{ + Name: lf.Name, + Format: lf.Format, + }) + } + + if len(logFormats) == 0 { + logFormats = append(logFormats, ngfAPIv1alpha2.LogFormat{ + Name: "default", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }) + } + + return logFormats +} + +func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) []ngfAPIv1alpha2.AccessLog { + accessLogs := []ngfAPIv1alpha2.AccessLog{} + if logging == nil { + return accessLogs + } + + for _, al := range logging.AccessLogs { + accessLogs = append(accessLogs, ngfAPIv1alpha2.AccessLog{ + Path: al.Path, + Format: al.Format, + }) + } + + if len(accessLogs) == 0 { + accessLogs = append(accessLogs, ngfAPIv1alpha2.AccessLog{ + Path: "/var/log/nginx/access.log", + Format: "default", + }) + } + + return accessLogs +} diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go index cb5f4f4fd7..c9a913b6fe 100644 --- a/internal/controller/provisioner/objects_test.go +++ b/internal/controller/provisioner/objects_test.go @@ -334,6 +334,10 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) { g.Expect(ok).To(BeTrue()) g.Expect(cm.Data).To(HaveKey("main.conf")) g.Expect(cm.Data["main.conf"]).To(ContainSubstring("debug")) + // check default log_format and access_log are added + g.Expect(cm.Data["main.conf"]). + To(ContainSubstring("log_format default '$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent';")) + g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/access.log default;")) cmObj = objects[2] cm, ok = cmObj.(*corev1.ConfigMap) @@ -1830,3 +1834,131 @@ func TestBuildNginxResourceObjects_InferenceExtension(t *testing.T) { g.Expect(containers[1].Name).To(Equal("endpoint-picker-shim")) g.Expect(containers[1].Command).To(Equal(expectedCommands)) } + +func TestBuildNginxResourceObjects_NginxProxyConfigWithAccesslog(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + agentTLSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: agentTLSTestSecretName, + Namespace: ngfNamespace, + }, + Data: map[string][]byte{"tls.crt": []byte("tls")}, + } + fakeClient := fake.NewFakeClient(agentTLSSecret) + + provisioner := &NginxProvisioner{ + cfg: Config{ + GatewayPodConfig: &config.GatewayPodConfig{ + Namespace: ngfNamespace, + Version: "1.0.0", + }, + AgentTLSSecretName: agentTLSTestSecretName, + }, + baseLabelSelector: metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "nginx", + }, + }, + k8sClient: fakeClient, + } + + gateway := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gw", + Namespace: "default", + }, + Spec: gatewayv1.GatewaySpec{ + Listeners: []gatewayv1.Listener{ + {Name: "port-8443", Port: 8443, Protocol: "tcp"}, + }, + }, + } + + resourceName := "gw-nginx" + nProxyCfg := &graph.EffectiveNginxProxy{ + IPFamily: helpers.GetPointer(ngfAPIv1alpha2.IPv4), + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), + AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), + LogFormats: []ngfAPIv1alpha2.LogFormat{ + { + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }, + { + Name: "minimal", + Format: "$remote_addr $status $body_bytes_sent", + }, + }, + AccessLogs: []ngfAPIv1alpha2.AccessLog{ + { + Path: "/var/log/nginx/access.log", + Format: "custom_format", + }, + { + Path: "/var/log/nginx/minimal.log", + Format: "minimal", + }, + }, + }, + Metrics: &ngfAPIv1alpha2.Metrics{ + Port: helpers.GetPointer[int32](8080), + }, + Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{ + Service: &ngfAPIv1alpha2.ServiceSpec{ + ServiceType: helpers.GetPointer(ngfAPIv1alpha2.ServiceTypeNodePort), + ExternalTrafficPolicy: helpers.GetPointer(ngfAPIv1alpha2.ExternalTrafficPolicyCluster), + LoadBalancerIP: helpers.GetPointer("1.2.3.4"), + LoadBalancerClass: helpers.GetPointer("myLoadBalancerClass"), + LoadBalancerSourceRanges: []string{"5.6.7.8"}, + }, + Deployment: &ngfAPIv1alpha2.DeploymentSpec{ + Replicas: helpers.GetPointer[int32](3), + Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{ + Enable: true, + MinReplicas: helpers.GetPointer[int32](1), + MaxReplicas: 5, + TargetMemoryUtilizationPercentage: helpers.GetPointer[int32](60), + }, + Pod: ngfAPIv1alpha2.PodSpec{ + TerminationGracePeriodSeconds: helpers.GetPointer[int64](25), + }, + Container: ngfAPIv1alpha2.ContainerSpec{ + Image: &ngfAPIv1alpha2.Image{ + Repository: helpers.GetPointer("nginx-repo"), + Tag: helpers.GetPointer("1.1.1"), + PullPolicy: helpers.GetPointer(ngfAPIv1alpha2.PullAlways), + }, + Resources: &corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.Quantity{Format: "100m"}, + }, + }, + ReadinessProbe: &ngfAPIv1alpha2.ReadinessProbeSpec{ + Port: helpers.GetPointer[int32](9091), + InitialDelaySeconds: helpers.GetPointer[int32](5), + }, + HostPorts: []ngfAPIv1alpha2.HostPort{{ContainerPort: int32(8443), Port: int32(8443)}}, + }, + }, + }, + } + + objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(objects).To(HaveLen(7)) + + cmObj := objects[1] + cm, ok := cmObj.(*corev1.ConfigMap) + g.Expect(ok).To(BeTrue()) + g.Expect(cm.Data).To(HaveKey("main.conf")) + g.Expect(cm.Data["main.conf"]).To(ContainSubstring("debug")) + g.Expect(cm.Data["main.conf"]). + To(ContainSubstring("log_format custom_format '$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent';")) + g.Expect(cm.Data["main.conf"]).To(ContainSubstring("log_format minimal '$remote_addr $status $body_bytes_sent';")) + g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/access.log custom_format;")) + g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/minimal.log minimal;")) +} diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index cc436ec93e..327c9ef3a3 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -10,7 +10,17 @@ var ( ) const mainTemplateText = ` -error_log stderr {{ .ErrorLevel }};` +error_log stderr {{ .ErrorLevel }}; +{{- if .LogFormats }} +{{- range .LogFormats }} +log_format {{ .Name }} '{{ .Format }}'; +{{- end }} +{{- end }} +{{- if .AccessLogs }} +{{- range .AccessLogs }} +access_log {{ .Path }} {{ .Format }}; +{{- end }} +{{- end }}` const eventsTemplateText = ` worker_connections {{ .WorkerConnections }};` diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index 0ae796eb38..c7c0466c51 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -224,6 +224,18 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), + LogFormats: []ngfAPIv1alpha2.LogFormat{ + { + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }, + }, + AccessLogs: []ngfAPIv1alpha2.AccessLog{ + { + Path: "/var/log/nginx/access.log", + Format: "custom_format", + }, + }, }, }) @@ -335,6 +347,18 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), + LogFormats: []ngfAPIv1alpha2.LogFormat{ + { + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }, + }, + AccessLogs: []ngfAPIv1alpha2.AccessLog{ + { + Path: "/var/log/nginx/access.log", + Format: "custom_format", + }, + }, }, DisableHTTP2: helpers.GetPointer(true), }, From 25eebbab848770bf73c846e25326783536cbb6e1 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Fri, 24 Oct 2025 19:24:14 +0100 Subject: [PATCH 2/8] Make logFormat singular and add ability to turn logs off --- apis/v1alpha2/nginxproxy_types.go | 17 ++- apis/v1alpha2/zz_generated.deepcopy.go | 36 ++++-- .../bases/gateway.nginx.org_nginxproxies.yaml | 53 ++++----- deploy/crds.yaml | 53 ++++----- .../nginx/config/base_http_config.go | 2 + .../nginx/config/base_http_config_template.go | 22 ++++ .../nginx/config/base_http_config_test.go | 83 +++++++++++++ internal/controller/provisioner/objects.go | 63 +++++----- .../controller/provisioner/objects_test.go | 30 +---- internal/controller/provisioner/templates.go | 12 +- .../state/dataplane/configuration.go | 60 ++++++++++ .../state/dataplane/configuration_test.go | 112 +++++++++++++++++- internal/controller/state/dataplane/types.go | 28 ++++- .../state/graph/multiple_gateways_test.go | 32 ++--- 14 files changed, 427 insertions(+), 176 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index 5142619383..c234dd2ed2 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -302,12 +302,12 @@ type NginxLogging struct { // Each log format must have a unique name. // // +optional - LogFormats []LogFormat `json:"logFormats,omitempty"` + LogFormat *LogFormat `json:"logFormat,omitempty"` // AccessLogs defines the access log settings, including the log file path, format, and optional parameters. // // +optional - AccessLogs []AccessLog `json:"accessLogs,omitempty"` + AccessLog *AccessLog `json:"accessLog,omitempty"` } // NginxErrorLogLevel type defines the log level of error logs for NGINX. @@ -365,17 +365,14 @@ const ( // LogFormat defines a custom log format for NGINX. type LogFormat struct { - Name string `json:"name"` - Format string `json:"format"` + Name *string `json:"name"` + Format *string `json:"format"` } -// AccessLog defines the configuration for an NGINX access log. +// AccessLog defines the configuration for an NGINX access log. For now only path dev/stdout is used. type AccessLog struct { - Path string `json:"path"` - Format string `json:"format"` - Buffer string `json:"buffer,omitempty"` - Condition string `json:"condition,omitempty"` - Gzip bool `json:"gzip,omitempty"` + Path *string `json:"path"` + Format *string `json:"format"` } // NginxPlus specifies NGINX Plus additional settings. These will only be applied if NGINX Plus is being used. diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index 558dd0b628..fbee1afc76 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -16,6 +16,16 @@ import ( // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AccessLog) DeepCopyInto(out *AccessLog) { *out = *in + if in.Path != nil { + in, out := &in.Path, &out.Path + *out = new(string) + **out = **in + } + if in.Format != nil { + in, out := &in.Format, &out.Format + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AccessLog. @@ -308,6 +318,16 @@ func (in *KubernetesSpec) DeepCopy() *KubernetesSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *LogFormat) DeepCopyInto(out *LogFormat) { *out = *in + if in.Name != nil { + in, out := &in.Name, &out.Name + *out = new(string) + **out = **in + } + if in.Format != nil { + in, out := &in.Format, &out.Format + *out = new(string) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LogFormat. @@ -358,15 +378,15 @@ func (in *NginxLogging) DeepCopyInto(out *NginxLogging) { *out = new(AgentLogLevel) **out = **in } - if in.LogFormats != nil { - in, out := &in.LogFormats, &out.LogFormats - *out = make([]LogFormat, len(*in)) - copy(*out, *in) + if in.LogFormat != nil { + in, out := &in.LogFormat, &out.LogFormat + *out = new(LogFormat) + (*in).DeepCopyInto(*out) } - if in.AccessLogs != nil { - in, out := &in.AccessLogs, &out.AccessLogs - *out = make([]AccessLog, len(*in)) - copy(*out, *in) + if in.AccessLog != nil { + in, out := &in.AccessLog, &out.AccessLog + *out = new(AccessLog) + (*in).DeepCopyInto(*out) } } diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index 88138e69ea..56862b6391 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -8016,28 +8016,18 @@ spec: logging: description: Logging defines logging related settings for NGINX. properties: - accessLogs: + accessLog: description: AccessLogs defines the access log settings, including the log file path, format, and optional parameters. - items: - description: AccessLog defines the configuration for an NGINX - access log. - properties: - buffer: - type: string - condition: - type: string - format: - type: string - gzip: - type: boolean - path: - type: string - required: - - format - - path - type: object - type: array + properties: + format: + type: string + path: + type: string + required: + - format + - path + type: object agentLevel: default: info description: |- @@ -8067,22 +8057,19 @@ spec: - alert - emerg type: string - logFormats: + logFormat: description: |- LogFormats defines custom log formats that can be used in access logs. Each log format must have a unique name. - items: - description: LogFormat defines a custom log format for NGINX. - properties: - format: - type: string - name: - type: string - required: - - format - - name - type: object - type: array + properties: + format: + type: string + name: + type: string + required: + - format + - name + type: object type: object metrics: description: |- diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 700e4aa194..0918795643 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -8603,28 +8603,18 @@ spec: logging: description: Logging defines logging related settings for NGINX. properties: - accessLogs: + accessLog: description: AccessLogs defines the access log settings, including the log file path, format, and optional parameters. - items: - description: AccessLog defines the configuration for an NGINX - access log. - properties: - buffer: - type: string - condition: - type: string - format: - type: string - gzip: - type: boolean - path: - type: string - required: - - format - - path - type: object - type: array + properties: + format: + type: string + path: + type: string + required: + - format + - path + type: object agentLevel: default: info description: |- @@ -8654,22 +8644,19 @@ spec: - alert - emerg type: string - logFormats: + logFormat: description: |- LogFormats defines custom log formats that can be used in access logs. Each log format must have a unique name. - items: - description: LogFormat defines a custom log format for NGINX. - properties: - format: - type: string - name: - type: string - required: - - format - - name - type: object - type: array + properties: + format: + type: string + name: + type: string + required: + - format + - name + type: object type: object metrics: description: |- diff --git a/internal/controller/nginx/config/base_http_config.go b/internal/controller/nginx/config/base_http_config.go index 4bce95a0bc..5817f94acf 100644 --- a/internal/controller/nginx/config/base_http_config.go +++ b/internal/controller/nginx/config/base_http_config.go @@ -12,6 +12,7 @@ var baseHTTPTemplate = gotemplate.Must(gotemplate.New("baseHttp").Parse(baseHTTP type httpConfig struct { DNSResolver *dataplane.DNSResolverConfig + LoggingSettings *dataplane.LoggingSettings Includes []shared.Include NginxReadinessProbePort int32 IPFamily shared.IPFamily @@ -27,6 +28,7 @@ func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { NginxReadinessProbePort: conf.BaseHTTPConfig.NginxReadinessProbePort, IPFamily: getIPFamily(conf.BaseHTTPConfig), DNSResolver: conf.BaseHTTPConfig.DNSResolver, + LoggingSettings: conf.Logging.LoggingSettings, } results := make([]executeResult, 0, len(includes)+1) diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index e50a83a65f..30ee7206b4 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -48,6 +48,28 @@ server { } } +{{- if .LoggingSettings }} + {{- /* Define custom log format */ -}} + {{- if .LoggingSettings.LogFormat.Name }} + log_format {{ .LoggingSettings.LogFormat.Name }} '{{ .LoggingSettings.LogFormat.Format }}'; + {{- end }} + + {{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use dev/stdout with that format. Otherwise use the given path. */ -}} + {{- $disable := false -}} + {{- if .LoggingSettings.AccessLog.Path }} + {{- if eq .LoggingSettings.AccessLog.Path "off" -}} + {{- $disable = true -}} + {{- end -}} + {{- end }} + {{- if $disable }} + access_log off; + {{- else }} + {{- if and .LoggingSettings.AccessLog.Path .LoggingSettings.AccessLog.Format }} + access_log dev/stdout {{ .LoggingSettings.AccessLog.Format }}; + {{- end }} + {{- end }} +{{- end }} + {{ range $i := .Includes -}} include {{ $i.Name }}; {{ end -}} diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index fe7ad3a58d..45ff3c5c28 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -10,6 +10,89 @@ import ( "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/state/dataplane" ) +func TestLoggingSettingsTemplate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + loggingSettings *dataplane.LoggingSettings + expectedOutputs []string + unexpectedOutputs []string + }{ + { + name: "Log format and access log with custom path", + loggingSettings: &dataplane.LoggingSettings{ + LogFormat: dataplane.LogFormat{ + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }, + AccessLog: dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, + }, + expectedOutputs: []string{ + `log_format custom_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, + `access_log dev/stdout custom_format;`, + }, + }, + { + name: "Empty log format name and format", + loggingSettings: &dataplane.LoggingSettings{ + LogFormat: dataplane.LogFormat{ + Name: "", + Format: "", + }, + AccessLog: dataplane.AccessLog{Path: "", Format: ""}, + }, + unexpectedOutputs: []string{ + `log_format custom_format`, + `access_log dev/stdout`, + }, + }, + { + name: "Access log off while format presented", + loggingSettings: &dataplane.LoggingSettings{ + LogFormat: dataplane.LogFormat{ + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", + }, + AccessLog: dataplane.AccessLog{Path: "off"}, + }, + expectedOutputs: []string{ + `access_log off;`, + }, + }, + { + name: "Access log off", + loggingSettings: &dataplane.LoggingSettings{ + AccessLog: dataplane.AccessLog{Path: "off"}, + }, + expectedOutputs: []string{ + `access_log off;`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + conf := dataplane.Configuration{ + Logging: dataplane.Logging{LoggingSettings: tt.loggingSettings}, + } + + res := executeBaseHTTPConfig(conf) + g.Expect(res).To(HaveLen(1)) + httpConfig := string(res[0].data) + for _, expectedOutput := range tt.expectedOutputs { + g.Expect(httpConfig).To(ContainSubstring(expectedOutput)) + } + for _, unexpectedOutput := range tt.unexpectedOutputs { + g.Expect(httpConfig).ToNot(ContainSubstring(unexpectedOutput)) + } + }) + } +} + func TestExecuteBaseHttp_HTTP2(t *testing.T) { t.Parallel() confOn := dataplane.Configuration{ diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index f6771eb111..54274b3e4c 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -407,14 +407,19 @@ func (p *NginxProvisioner) buildNginxConfigMaps( } // Add LogFormats and AccessLogs to mainFields - logFormats := addLogFormatToNginxConfig(logging) - accessLogs := addAccessLogsToNginxConfig(logging) + logFormat := addLogFormatToNginxConfig(logging) + accessLog := addAccessLogsToNginxConfig(logging) mainFields := map[string]interface{}{ "ErrorLevel": logLevel, "WorkerConnections": workerConnections, - "LogFormats": logFormats, - "AccessLogs": accessLogs, + } + + if logFormat != nil { + mainFields["LogFormat"] = logFormat + } + if accessLog != nil { + mainFields["AccessLog"] = accessLog } // Create events ConfigMap data using template @@ -1443,48 +1448,34 @@ func DetermineNginxImageName( return fmt.Sprintf("%s:%s", image, tag), pullPolicy } -func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) []ngfAPIv1alpha2.LogFormat { - logFormats := []ngfAPIv1alpha2.LogFormat{} +func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.LogFormat { + logFormat := &ngfAPIv1alpha2.LogFormat{} if logging == nil { - return logFormats + return logFormat } - for _, lf := range logging.LogFormats { - logFormats = append(logFormats, ngfAPIv1alpha2.LogFormat{ - Name: lf.Name, - Format: lf.Format, - }) - } - - if len(logFormats) == 0 { - logFormats = append(logFormats, ngfAPIv1alpha2.LogFormat{ - Name: "default", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }) + if logging.LogFormat != nil { + logFormat = &ngfAPIv1alpha2.LogFormat{ + Name: logging.LogFormat.Name, + Format: logging.LogFormat.Format, + } } - return logFormats + return logFormat } -func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) []ngfAPIv1alpha2.AccessLog { - accessLogs := []ngfAPIv1alpha2.AccessLog{} +func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.AccessLog { + accessLog := &ngfAPIv1alpha2.AccessLog{} if logging == nil { - return accessLogs - } - - for _, al := range logging.AccessLogs { - accessLogs = append(accessLogs, ngfAPIv1alpha2.AccessLog{ - Path: al.Path, - Format: al.Format, - }) + return accessLog } - if len(accessLogs) == 0 { - accessLogs = append(accessLogs, ngfAPIv1alpha2.AccessLog{ - Path: "/var/log/nginx/access.log", - Format: "default", - }) + if logging.AccessLog != nil { + accessLog = &ngfAPIv1alpha2.AccessLog{ + Path: logging.AccessLog.Path, + Format: logging.AccessLog.Format, + } } - return accessLogs + return accessLog } diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go index c9a913b6fe..027d5be4f3 100644 --- a/internal/controller/provisioner/objects_test.go +++ b/internal/controller/provisioner/objects_test.go @@ -334,10 +334,6 @@ func TestBuildNginxResourceObjects_NginxProxyConfig(t *testing.T) { g.Expect(ok).To(BeTrue()) g.Expect(cm.Data).To(HaveKey("main.conf")) g.Expect(cm.Data["main.conf"]).To(ContainSubstring("debug")) - // check default log_format and access_log are added - g.Expect(cm.Data["main.conf"]). - To(ContainSubstring("log_format default '$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent';")) - g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/access.log default;")) cmObj = objects[2] cm, ok = cmObj.(*corev1.ConfigMap) @@ -1882,25 +1878,13 @@ func TestBuildNginxResourceObjects_NginxProxyConfigWithAccesslog(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormats: []ngfAPIv1alpha2.LogFormat{ - { - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, - { - Name: "minimal", - Format: "$remote_addr $status $body_bytes_sent", - }, + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, - AccessLogs: []ngfAPIv1alpha2.AccessLog{ - { - Path: "/var/log/nginx/access.log", - Format: "custom_format", - }, - { - Path: "/var/log/nginx/minimal.log", - Format: "minimal", - }, + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("/var/log/nginx/access.log"), + Format: helpers.GetPointer("custom_format"), }, }, Metrics: &ngfAPIv1alpha2.Metrics{ @@ -1958,7 +1942,5 @@ func TestBuildNginxResourceObjects_NginxProxyConfigWithAccesslog(t *testing.T) { g.Expect(cm.Data["main.conf"]).To(ContainSubstring("debug")) g.Expect(cm.Data["main.conf"]). To(ContainSubstring("log_format custom_format '$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent';")) - g.Expect(cm.Data["main.conf"]).To(ContainSubstring("log_format minimal '$remote_addr $status $body_bytes_sent';")) g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/access.log custom_format;")) - g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/minimal.log minimal;")) } diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index 327c9ef3a3..b48af7ba7b 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -11,15 +11,11 @@ var ( const mainTemplateText = ` error_log stderr {{ .ErrorLevel }}; -{{- if .LogFormats }} -{{- range .LogFormats }} -log_format {{ .Name }} '{{ .Format }}'; -{{- end }} -{{- end }} -{{- if .AccessLogs }} -{{- range .AccessLogs }} -access_log {{ .Path }} {{ .Format }}; +{{- if .LogFormat }} +log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; {{- end }} +{{- if .AccessLog }} +access_log {{ .AccessLog.Path }} {{ .AccessLog.Format }}; {{- end }}` const eventsTemplateText = ` diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 6615fd7036..cdc4f379f6 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -6,6 +6,7 @@ import ( "fmt" "slices" "sort" + "strings" "github.com/go-logr/logr" discoveryV1 "k8s.io/api/discovery/v1" @@ -1206,6 +1207,8 @@ func convertAddresses(addresses []ngfAPIv1alpha2.RewriteClientIPAddress) []strin return trustedAddresses } +// 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} @@ -1218,11 +1221,68 @@ func buildLogging(gateway *graph.Gateway) Logging { if ngfProxy.Logging.ErrorLevel != nil { logSettings.ErrorLevel = string(*ngfProxy.Logging.ErrorLevel) } + + srcLogSettings := ngfProxy.Logging + ls := LoggingSettings{} + ls.LogFormat = buildLogFormat(srcLogSettings) + ls.AccessLog = buildAccessLog(srcLogSettings) + + if ls.AccessLog.Path == "off" { + logSettings.LoggingSettings = &LoggingSettings{AccessLog: AccessLog{Path: "off"}} + + return logSettings + } + + if ls.LogFormat.Format != "" || ls.AccessLog.Path != "" { + // only set LoggingSettings if at least one of LogFormat or AccessLog is configured + logSettings.LoggingSettings = &ls + } } return logSettings } +func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) LogFormat { + var logFormat LogFormat + // Current API exposes a single LogFormat value whose fields are pointers; include only if both set and non-empty. + if srcLogSettings.LogFormat != nil && + srcLogSettings.LogFormat.Name != nil && + srcLogSettings.LogFormat.Format != nil && + *srcLogSettings.LogFormat.Name != "" && + *srcLogSettings.LogFormat.Format != "" { + logFormat = LogFormat{ + Name: *srcLogSettings.LogFormat.Name, + Format: *srcLogSettings.LogFormat.Format, + } + } + + return logFormat +} + +func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) AccessLog { + var accessLog AccessLog + // Current API exposes a singular *string AccessLog path (no format yet) – only dev/stdout or "off". + if srcLogSettings.AccessLog != nil && + srcLogSettings.AccessLog.Path != nil && + *srcLogSettings.AccessLog.Path != "" { + if strings.ToLower(*srcLogSettings.AccessLog.Path) == "off" { + accessLog = AccessLog{Path: "off"} + } else { + // only "dev/stdout" is supported for now + defaultPath := "dev/stdout" + if srcLogSettings.AccessLog.Format != nil && + *srcLogSettings.AccessLog.Format != "" { + accessLog = AccessLog{ + Path: defaultPath, + Format: *srcLogSettings.AccessLog.Format, + } + } + } + } + + return accessLog +} + func buildWorkerConnections(gateway *graph.Gateway) int32 { if gateway == nil || gateway.EffectiveNginxProxy == nil { return DefaultWorkerConnections diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 73f109be7c..ef9024bb12 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4771,9 +4771,9 @@ func TestBuildLogging(t *testing.T) { t.Parallel() tests := []struct { - msg string - gw *graph.Gateway expLoggingSettings Logging + gw *graph.Gateway + msg string }{ { msg: "Gateway is nil", @@ -4884,6 +4884,114 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ErrorLevel: "emerg"}, }, + { + msg: "LogFormat and AccessLog configured", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`), + }, + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("logs/access.log"), + Format: helpers.GetPointer("custom_format"), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LoggingSettings: &LoggingSettings{ + LogFormat: LogFormat{ + Name: "custom_format", + Format: `'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`, + }, + AccessLog: AccessLog{ + Path: "dev/stdout", + Format: "custom_format", + }, + }, + }, + }, + { + msg: "Only AccessLog set if LogFormat is not configured properly (no name given)", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`), + }, + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("dev/stdout"), + Format: helpers.GetPointer("custom_format"), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LoggingSettings: &LoggingSettings{ + AccessLog: AccessLog{ + Path: "dev/stdout", + Format: "custom_format", + }, + }, + }, + }, + { + msg: "AccessLog OFF while LogFormat is configured", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("OFF"), + Format: helpers.GetPointer("custom_format"), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LoggingSettings: &LoggingSettings{ + AccessLog: AccessLog{ + Path: "off", + }, + }, + }, + }, + { + msg: "AccessLog path replaced with dev/stdout", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("logs/access.log"), + Format: helpers.GetPointer("custom_format"), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LoggingSettings: &LoggingSettings{ + AccessLog: AccessLog{ + Path: "dev/stdout", + Format: "custom_format", + }, + }, + }, + }, } for _, tc := range tests { diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 987c021c7f..65918d0f5a 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -474,8 +474,8 @@ type Ratio struct { // Logging defines logging related settings for NGINX. type Logging struct { - // ErrorLevel defines the error log level. - ErrorLevel string + LoggingSettings *LoggingSettings + ErrorLevel string } // NginxPlus specifies NGINX Plus additional settings. @@ -496,3 +496,27 @@ type DeploymentContext struct { // Integration is "ngf". Integration string `json:"integration"` } + +// LoggingSettings defines logging related settings for NGINX. +type LoggingSettings struct { + // LogFormat is the custom log format. + LogFormat LogFormat + // AccessLog is a configuration for access logs + AccessLog AccessLog +} + +// LogFormat represents a custom log format. +type LogFormat struct { + // Name is the name of the log format. + Name string + // Format is the format string. + Format string +} + +// AccessLog defines the configuration for an NGINX access log. For now only path dev/stdout is used. +type AccessLog struct { + // Path is the path of the access log. Currently supported only dev/stdout, or "off" to disable logging. + Path string + // Format is the log_format name + Format string +} diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index c7c0466c51..d8c420f77c 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -224,17 +224,13 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormats: []ngfAPIv1alpha2.LogFormat{ - { - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, - AccessLogs: []ngfAPIv1alpha2.AccessLog{ - { - Path: "/var/log/nginx/access.log", - Format: "custom_format", - }, + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("/var/log/nginx/access.log"), + Format: helpers.GetPointer("custom_format"), }, }, }) @@ -347,17 +343,13 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormats: []ngfAPIv1alpha2.LogFormat{ - { - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, - AccessLogs: []ngfAPIv1alpha2.AccessLog{ - { - Path: "/var/log/nginx/access.log", - Format: "custom_format", - }, + AccessLog: &ngfAPIv1alpha2.AccessLog{ + Path: helpers.GetPointer("/var/log/nginx/access.log"), + Format: helpers.GetPointer("custom_format"), }, }, DisableHTTP2: helpers.GetPointer(true), From 699c08a2880309c6392652610a9453ff6204ebb8 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Mon, 27 Oct 2025 11:17:41 +0000 Subject: [PATCH 3/8] Remove LoggingSettings structure --- apis/v1alpha2/nginxproxy_types.go | 8 +-- .../bases/gateway.nginx.org_nginxproxies.yaml | 6 --- deploy/crds.yaml | 6 --- .../nginx/config/base_http_config.go | 6 ++- .../nginx/config/base_http_config_template.go | 24 ++++----- .../nginx/config/base_http_config_test.go | 45 ++++++++-------- .../state/dataplane/configuration.go | 24 ++++----- .../state/dataplane/configuration_test.go | 52 +++++++++---------- internal/controller/state/dataplane/types.go | 13 ++--- 9 files changed, 80 insertions(+), 104 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index c234dd2ed2..e4acefff30 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -365,14 +365,14 @@ const ( // LogFormat defines a custom log format for NGINX. type LogFormat struct { - Name *string `json:"name"` - Format *string `json:"format"` + Name *string `json:"name,omitempty"` + Format *string `json:"format,omitempty"` } // AccessLog defines the configuration for an NGINX access log. For now only path dev/stdout is used. type AccessLog struct { - Path *string `json:"path"` - Format *string `json:"format"` + Path *string `json:"path,omitempty"` + Format *string `json:"format,omitempty"` } // NginxPlus specifies NGINX Plus additional settings. These will only be applied if NGINX Plus is being used. diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index 56862b6391..b2f387a562 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -8024,9 +8024,6 @@ spec: type: string path: type: string - required: - - format - - path type: object agentLevel: default: info @@ -8066,9 +8063,6 @@ spec: type: string name: type: string - required: - - format - - name type: object type: object metrics: diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 0918795643..fe527e17cb 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -8611,9 +8611,6 @@ spec: type: string path: type: string - required: - - format - - path type: object agentLevel: default: info @@ -8653,9 +8650,6 @@ spec: type: string name: type: string - required: - - format - - name type: object type: object metrics: diff --git a/internal/controller/nginx/config/base_http_config.go b/internal/controller/nginx/config/base_http_config.go index 5817f94acf..de1dcd0ea4 100644 --- a/internal/controller/nginx/config/base_http_config.go +++ b/internal/controller/nginx/config/base_http_config.go @@ -12,7 +12,8 @@ var baseHTTPTemplate = gotemplate.Must(gotemplate.New("baseHttp").Parse(baseHTTP type httpConfig struct { DNSResolver *dataplane.DNSResolverConfig - LoggingSettings *dataplane.LoggingSettings + LogFormat *dataplane.LogFormat + AccessLog *dataplane.AccessLog Includes []shared.Include NginxReadinessProbePort int32 IPFamily shared.IPFamily @@ -28,7 +29,8 @@ func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { NginxReadinessProbePort: conf.BaseHTTPConfig.NginxReadinessProbePort, IPFamily: getIPFamily(conf.BaseHTTPConfig), DNSResolver: conf.BaseHTTPConfig.DNSResolver, - LoggingSettings: conf.Logging.LoggingSettings, + LogFormat: conf.Logging.LogFormat, + AccessLog: conf.Logging.AccessLog, } results := make([]executeResult, 0, len(includes)+1) diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index 30ee7206b4..5d3aa43260 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -48,27 +48,23 @@ server { } } -{{- if .LoggingSettings }} {{- /* Define custom log format */ -}} - {{- if .LoggingSettings.LogFormat.Name }} - log_format {{ .LoggingSettings.LogFormat.Name }} '{{ .LoggingSettings.LogFormat.Format }}'; + {{- if .LogFormat }} + {{- if .LogFormat.Name }} +log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; + {{- end }} {{- end }} {{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use dev/stdout with that format. Otherwise use the given path. */ -}} - {{- $disable := false -}} - {{- if .LoggingSettings.AccessLog.Path }} - {{- if eq .LoggingSettings.AccessLog.Path "off" -}} - {{- $disable = true -}} - {{- end -}} - {{- end }} - {{- if $disable }} + {{- if .AccessLog }} + {{- if eq .AccessLog.Path "off" }} access_log off; - {{- else }} - {{- if and .LoggingSettings.AccessLog.Path .LoggingSettings.AccessLog.Format }} - access_log dev/stdout {{ .LoggingSettings.AccessLog.Format }}; + {{- else }} + {{- if .AccessLog.Format }} + access_log dev/stdout {{ .AccessLog.Format }}; + {{- end }} {{- end }} {{- end }} -{{- end }} {{ range $i := .Includes -}} include {{ $i.Name }}; diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index 45ff3c5c28..39dc97a550 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -15,19 +15,18 @@ func TestLoggingSettingsTemplate(t *testing.T) { tests := []struct { name string - loggingSettings *dataplane.LoggingSettings + accessLog *dataplane.AccessLog + logFormat *dataplane.LogFormat expectedOutputs []string unexpectedOutputs []string }{ { name: "Log format and access log with custom path", - loggingSettings: &dataplane.LoggingSettings{ - LogFormat: dataplane.LogFormat{ - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, - AccessLog: dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, + logFormat: &dataplane.LogFormat{ + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", }, + accessLog: &dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, expectedOutputs: []string{ `log_format custom_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, `access_log dev/stdout custom_format;`, @@ -35,13 +34,11 @@ func TestLoggingSettingsTemplate(t *testing.T) { }, { name: "Empty log format name and format", - loggingSettings: &dataplane.LoggingSettings{ - LogFormat: dataplane.LogFormat{ - Name: "", - Format: "", - }, - AccessLog: dataplane.AccessLog{Path: "", Format: ""}, + logFormat: &dataplane.LogFormat{ + Name: "", + Format: "", }, + accessLog: &dataplane.AccessLog{Path: "", Format: ""}, unexpectedOutputs: []string{ `log_format custom_format`, `access_log dev/stdout`, @@ -49,22 +46,22 @@ func TestLoggingSettingsTemplate(t *testing.T) { }, { name: "Access log off while format presented", - loggingSettings: &dataplane.LoggingSettings{ - LogFormat: dataplane.LogFormat{ - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, - AccessLog: dataplane.AccessLog{Path: "off"}, + logFormat: &dataplane.LogFormat{ + Name: "custom_format", + Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", }, + accessLog: &dataplane.AccessLog{Path: "off", Format: "custom_format"}, expectedOutputs: []string{ `access_log off;`, + `log_format custom_format`, + }, + unexpectedOutputs: []string{ + `access_log off custom_format`, }, }, { - name: "Access log off", - loggingSettings: &dataplane.LoggingSettings{ - AccessLog: dataplane.AccessLog{Path: "off"}, - }, + name: "Access log off", + accessLog: &dataplane.AccessLog{Path: "off"}, expectedOutputs: []string{ `access_log off;`, }, @@ -77,7 +74,7 @@ func TestLoggingSettingsTemplate(t *testing.T) { g := NewWithT(t) conf := dataplane.Configuration{ - Logging: dataplane.Logging{LoggingSettings: tt.loggingSettings}, + Logging: dataplane.Logging{AccessLog: tt.accessLog, LogFormat: tt.logFormat}, } res := executeBaseHTTPConfig(conf) diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index cdc4f379f6..615b40f7f5 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -1223,26 +1223,26 @@ func buildLogging(gateway *graph.Gateway) Logging { } srcLogSettings := ngfProxy.Logging - ls := LoggingSettings{} - ls.LogFormat = buildLogFormat(srcLogSettings) - ls.AccessLog = buildAccessLog(srcLogSettings) + logFormat := buildLogFormat(srcLogSettings) + accessLog := buildAccessLog(srcLogSettings) - if ls.AccessLog.Path == "off" { - logSettings.LoggingSettings = &LoggingSettings{AccessLog: AccessLog{Path: "off"}} + if accessLog.Path == "off" { + logSettings.AccessLog = &AccessLog{Path: "off"} return logSettings } - if ls.LogFormat.Format != "" || ls.AccessLog.Path != "" { - // only set LoggingSettings if at least one of LogFormat or AccessLog is configured - logSettings.LoggingSettings = &ls + if logFormat != nil && logFormat.Format != "" && accessLog != nil && accessLog.Path != "" { + // only update logSettings if both LogFormat and AccessLog path are configured + logSettings.LogFormat = logFormat + logSettings.AccessLog = accessLog } } return logSettings } -func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) LogFormat { +func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *LogFormat { var logFormat LogFormat // Current API exposes a single LogFormat value whose fields are pointers; include only if both set and non-empty. if srcLogSettings.LogFormat != nil && @@ -1256,10 +1256,10 @@ func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) LogFormat { } } - return logFormat + return &logFormat } -func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) AccessLog { +func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *AccessLog { var accessLog AccessLog // Current API exposes a singular *string AccessLog path (no format yet) – only dev/stdout or "off". if srcLogSettings.AccessLog != nil && @@ -1280,7 +1280,7 @@ func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) AccessLog { } } - return accessLog + return &accessLog } func buildWorkerConnections(gateway *graph.Gateway) int32 { diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index ef9024bb12..b6fc6688f5 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4897,7 +4897,7 @@ func TestBuildLogging(t *testing.T) { '"$http_referer" "$http_user_agent" '`), }, AccessLog: &ngfAPIv1alpha2.AccessLog{ - Path: helpers.GetPointer("logs/access.log"), + Path: helpers.GetPointer("dev/stdout"), Format: helpers.GetPointer("custom_format"), }, }, @@ -4905,22 +4905,20 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ ErrorLevel: "info", - LoggingSettings: &LoggingSettings{ - LogFormat: LogFormat{ - Name: "custom_format", - Format: `'$remote_addr - $remote_user [$time_local] ' + LogFormat: &LogFormat{ + Name: "custom_format", + Format: `'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`, - }, - AccessLog: AccessLog{ - Path: "dev/stdout", - Format: "custom_format", - }, + }, + AccessLog: &AccessLog{ + Path: "dev/stdout", + Format: "custom_format", }, }, }, { - msg: "Only AccessLog set if LogFormat is not configured properly (no name given)", + msg: "No AccessLog setting if LogFormat is not configured properly (no name given)", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ @@ -4939,12 +4937,6 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ ErrorLevel: "info", - LoggingSettings: &LoggingSettings{ - AccessLog: AccessLog{ - Path: "dev/stdout", - Format: "custom_format", - }, - }, }, }, { @@ -4962,10 +4954,8 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ ErrorLevel: "info", - LoggingSettings: &LoggingSettings{ - AccessLog: AccessLog{ - Path: "off", - }, + AccessLog: &AccessLog{ + Path: "off", }, }, }, @@ -4975,6 +4965,12 @@ func TestBuildLogging(t *testing.T) { EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + LogFormat: &ngfAPIv1alpha2.LogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`), + }, AccessLog: &ngfAPIv1alpha2.AccessLog{ Path: helpers.GetPointer("logs/access.log"), Format: helpers.GetPointer("custom_format"), @@ -4984,11 +4980,15 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ ErrorLevel: "info", - LoggingSettings: &LoggingSettings{ - AccessLog: AccessLog{ - Path: "dev/stdout", - Format: "custom_format", - }, + LogFormat: &LogFormat{ + Name: "custom_format", + Format: `'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`, + }, + AccessLog: &AccessLog{ + Path: "dev/stdout", + Format: "custom_format", }, }, }, diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 65918d0f5a..4e61b1613e 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -474,8 +474,9 @@ type Ratio struct { // Logging defines logging related settings for NGINX. type Logging struct { - LoggingSettings *LoggingSettings - ErrorLevel string + AccessLog *AccessLog + LogFormat *LogFormat + ErrorLevel string } // NginxPlus specifies NGINX Plus additional settings. @@ -497,14 +498,6 @@ type DeploymentContext struct { Integration string `json:"integration"` } -// LoggingSettings defines logging related settings for NGINX. -type LoggingSettings struct { - // LogFormat is the custom log format. - LogFormat LogFormat - // AccessLog is a configuration for access logs - AccessLog AccessLog -} - // LogFormat represents a custom log format. type LogFormat struct { // Name is the name of the log format. From b735098012ea76b79bb354e43cdf30cad62ace25 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Mon, 3 Nov 2025 11:15:08 +0000 Subject: [PATCH 4/8] fix template --- apis/v1alpha2/nginxproxy_types.go | 2 +- .../nginx/config/base_http_config_template.go | 26 ++-- .../nginx/config/base_http_config_test.go | 4 +- .../controller/provisioner/objects_test.go | 114 ------------------ internal/controller/provisioner/templates.go | 8 +- .../state/dataplane/configuration.go | 6 +- .../state/dataplane/configuration_test.go | 10 +- internal/controller/state/dataplane/types.go | 11 +- 8 files changed, 32 insertions(+), 149 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index e4acefff30..25de7e5e17 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -369,7 +369,7 @@ type LogFormat struct { Format *string `json:"format,omitempty"` } -// AccessLog defines the configuration for an NGINX access log. For now only path dev/stdout is used. +// AccessLog defines the configuration for an NGINX access log. For now only path /dev/stdout is used. type AccessLog struct { Path *string `json:"path,omitempty"` Format *string `json:"format,omitempty"` diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index 5d3aa43260..50926f5d63 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -48,23 +48,23 @@ server { } } - {{- /* Define custom log format */ -}} - {{- if .LogFormat }} - {{- if .LogFormat.Name }} +{{- /* Define custom log format */ -}} +{{- if .LogFormat }} + {{- if .LogFormat.Name }} log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; - {{- end }} {{- end }} +{{- end }} - {{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use dev/stdout with that format. Otherwise use the given path. */ -}} - {{- if .AccessLog }} - {{- if eq .AccessLog.Path "off" }} - access_log off; - {{- else }} - {{- if .AccessLog.Format }} - access_log dev/stdout {{ .AccessLog.Format }}; - {{- end }} - {{- end }} +{{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use /dev/stdout with that format. Otherwise use the given path. */ -}} +{{- if .AccessLog }} + {{- if eq .AccessLog.Path "off" }} +access_log off; + {{- else }} + {{- if .AccessLog.Format }} +access_log /dev/stdout {{ .AccessLog.Format }}; + {{- end }} {{- end }} +{{- end }} {{ range $i := .Includes -}} include {{ $i.Name }}; diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index 39dc97a550..c31e82d555 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -29,7 +29,7 @@ func TestLoggingSettingsTemplate(t *testing.T) { accessLog: &dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, expectedOutputs: []string{ `log_format custom_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, - `access_log dev/stdout custom_format;`, + `access_log /dev/stdout custom_format;`, }, }, { @@ -41,7 +41,7 @@ func TestLoggingSettingsTemplate(t *testing.T) { accessLog: &dataplane.AccessLog{Path: "", Format: ""}, unexpectedOutputs: []string{ `log_format custom_format`, - `access_log dev/stdout`, + `access_log /dev/stdout`, }, }, { diff --git a/internal/controller/provisioner/objects_test.go b/internal/controller/provisioner/objects_test.go index 027d5be4f3..cb5f4f4fd7 100644 --- a/internal/controller/provisioner/objects_test.go +++ b/internal/controller/provisioner/objects_test.go @@ -1830,117 +1830,3 @@ func TestBuildNginxResourceObjects_InferenceExtension(t *testing.T) { g.Expect(containers[1].Name).To(Equal("endpoint-picker-shim")) g.Expect(containers[1].Command).To(Equal(expectedCommands)) } - -func TestBuildNginxResourceObjects_NginxProxyConfigWithAccesslog(t *testing.T) { - t.Parallel() - g := NewWithT(t) - - agentTLSSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: agentTLSTestSecretName, - Namespace: ngfNamespace, - }, - Data: map[string][]byte{"tls.crt": []byte("tls")}, - } - fakeClient := fake.NewFakeClient(agentTLSSecret) - - provisioner := &NginxProvisioner{ - cfg: Config{ - GatewayPodConfig: &config.GatewayPodConfig{ - Namespace: ngfNamespace, - Version: "1.0.0", - }, - AgentTLSSecretName: agentTLSTestSecretName, - }, - baseLabelSelector: metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app": "nginx", - }, - }, - k8sClient: fakeClient, - } - - gateway := &gatewayv1.Gateway{ - ObjectMeta: metav1.ObjectMeta{ - Name: "gw", - Namespace: "default", - }, - Spec: gatewayv1.GatewaySpec{ - Listeners: []gatewayv1.Listener{ - {Name: "port-8443", Port: 8443, Protocol: "tcp"}, - }, - }, - } - - resourceName := "gw-nginx" - nProxyCfg := &graph.EffectiveNginxProxy{ - IPFamily: helpers.GetPointer(ngfAPIv1alpha2.IPv4), - Logging: &ngfAPIv1alpha2.NginxLogging{ - ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), - AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormat: &ngfAPIv1alpha2.LogFormat{ - Name: helpers.GetPointer("custom_format"), - Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), - }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ - Path: helpers.GetPointer("/var/log/nginx/access.log"), - Format: helpers.GetPointer("custom_format"), - }, - }, - Metrics: &ngfAPIv1alpha2.Metrics{ - Port: helpers.GetPointer[int32](8080), - }, - Kubernetes: &ngfAPIv1alpha2.KubernetesSpec{ - Service: &ngfAPIv1alpha2.ServiceSpec{ - ServiceType: helpers.GetPointer(ngfAPIv1alpha2.ServiceTypeNodePort), - ExternalTrafficPolicy: helpers.GetPointer(ngfAPIv1alpha2.ExternalTrafficPolicyCluster), - LoadBalancerIP: helpers.GetPointer("1.2.3.4"), - LoadBalancerClass: helpers.GetPointer("myLoadBalancerClass"), - LoadBalancerSourceRanges: []string{"5.6.7.8"}, - }, - Deployment: &ngfAPIv1alpha2.DeploymentSpec{ - Replicas: helpers.GetPointer[int32](3), - Autoscaling: &ngfAPIv1alpha2.AutoscalingSpec{ - Enable: true, - MinReplicas: helpers.GetPointer[int32](1), - MaxReplicas: 5, - TargetMemoryUtilizationPercentage: helpers.GetPointer[int32](60), - }, - Pod: ngfAPIv1alpha2.PodSpec{ - TerminationGracePeriodSeconds: helpers.GetPointer[int64](25), - }, - Container: ngfAPIv1alpha2.ContainerSpec{ - Image: &ngfAPIv1alpha2.Image{ - Repository: helpers.GetPointer("nginx-repo"), - Tag: helpers.GetPointer("1.1.1"), - PullPolicy: helpers.GetPointer(ngfAPIv1alpha2.PullAlways), - }, - Resources: &corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.Quantity{Format: "100m"}, - }, - }, - ReadinessProbe: &ngfAPIv1alpha2.ReadinessProbeSpec{ - Port: helpers.GetPointer[int32](9091), - InitialDelaySeconds: helpers.GetPointer[int32](5), - }, - HostPorts: []ngfAPIv1alpha2.HostPort{{ContainerPort: int32(8443), Port: int32(8443)}}, - }, - }, - }, - } - - objects, err := provisioner.buildNginxResourceObjects(resourceName, gateway, nProxyCfg) - g.Expect(err).ToNot(HaveOccurred()) - - g.Expect(objects).To(HaveLen(7)) - - cmObj := objects[1] - cm, ok := cmObj.(*corev1.ConfigMap) - g.Expect(ok).To(BeTrue()) - g.Expect(cm.Data).To(HaveKey("main.conf")) - g.Expect(cm.Data["main.conf"]).To(ContainSubstring("debug")) - g.Expect(cm.Data["main.conf"]). - To(ContainSubstring("log_format custom_format '$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent';")) - g.Expect(cm.Data["main.conf"]).To(ContainSubstring("access_log /var/log/nginx/access.log custom_format;")) -} diff --git a/internal/controller/provisioner/templates.go b/internal/controller/provisioner/templates.go index b48af7ba7b..cc436ec93e 100644 --- a/internal/controller/provisioner/templates.go +++ b/internal/controller/provisioner/templates.go @@ -10,13 +10,7 @@ var ( ) const mainTemplateText = ` -error_log stderr {{ .ErrorLevel }}; -{{- if .LogFormat }} -log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; -{{- end }} -{{- if .AccessLog }} -access_log {{ .AccessLog.Path }} {{ .AccessLog.Format }}; -{{- end }}` +error_log stderr {{ .ErrorLevel }};` const eventsTemplateText = ` worker_connections {{ .WorkerConnections }};` diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 615b40f7f5..dee5a3b846 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -1261,15 +1261,15 @@ func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *LogFormat { func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *AccessLog { var accessLog AccessLog - // Current API exposes a singular *string AccessLog path (no format yet) – only dev/stdout or "off". + // Current API exposes a singular *string AccessLog path (no format yet) – only /dev/stdout or "off". if srcLogSettings.AccessLog != nil && srcLogSettings.AccessLog.Path != nil && *srcLogSettings.AccessLog.Path != "" { if strings.ToLower(*srcLogSettings.AccessLog.Path) == "off" { accessLog = AccessLog{Path: "off"} } else { - // only "dev/stdout" is supported for now - defaultPath := "dev/stdout" + // only "/dev/stdout" is supported for now + defaultPath := "/dev/stdout" if srcLogSettings.AccessLog.Format != nil && *srcLogSettings.AccessLog.Format != "" { accessLog = AccessLog{ diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index b6fc6688f5..457b799122 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4897,7 +4897,7 @@ func TestBuildLogging(t *testing.T) { '"$http_referer" "$http_user_agent" '`), }, AccessLog: &ngfAPIv1alpha2.AccessLog{ - Path: helpers.GetPointer("dev/stdout"), + Path: helpers.GetPointer("/dev/stdout"), Format: helpers.GetPointer("custom_format"), }, }, @@ -4912,7 +4912,7 @@ func TestBuildLogging(t *testing.T) { '"$http_referer" "$http_user_agent" '`, }, AccessLog: &AccessLog{ - Path: "dev/stdout", + Path: "/dev/stdout", Format: "custom_format", }, }, @@ -4929,7 +4929,7 @@ func TestBuildLogging(t *testing.T) { '"$http_referer" "$http_user_agent" '`), }, AccessLog: &ngfAPIv1alpha2.AccessLog{ - Path: helpers.GetPointer("dev/stdout"), + Path: helpers.GetPointer("/dev/stdout"), Format: helpers.GetPointer("custom_format"), }, }, @@ -4960,7 +4960,7 @@ func TestBuildLogging(t *testing.T) { }, }, { - msg: "AccessLog path replaced with dev/stdout", + msg: "AccessLog path replaced with /dev/stdout", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ @@ -4987,7 +4987,7 @@ func TestBuildLogging(t *testing.T) { '"$http_referer" "$http_user_agent" '`, }, AccessLog: &AccessLog{ - Path: "dev/stdout", + Path: "/dev/stdout", Format: "custom_format", }, }, diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index 4e61b1613e..c361c7f39b 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -474,8 +474,11 @@ type Ratio struct { // Logging defines logging related settings for NGINX. type Logging struct { - AccessLog *AccessLog - LogFormat *LogFormat + // AccessLog defines the configuration for the NGINX access log. For now only path /dev/stdout is used. + AccessLog *AccessLog + // LogFormat represents a custom log format. + LogFormat *LogFormat + // ErrorLevel defines the error log level. ErrorLevel string } @@ -506,9 +509,9 @@ type LogFormat struct { Format string } -// AccessLog defines the configuration for an NGINX access log. For now only path dev/stdout is used. +// AccessLog defines the configuration for an NGINX access log. For now only path /dev/stdout is used. type AccessLog struct { - // Path is the path of the access log. Currently supported only dev/stdout, or "off" to disable logging. + // Path is the path of the access log. Currently supported only /dev/stdout, or "off" to disable logging. Path string // Format is the log_format name Format string From b0ad0df1c642c4a993100b54110e6044194e4fe9 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Tue, 4 Nov 2025 11:13:51 +0000 Subject: [PATCH 5/8] fix review comments --- apis/v1alpha2/nginxproxy_types.go | 42 ++++++--- apis/v1alpha2/zz_generated.deepcopy.go | 86 +++++++++---------- .../bases/gateway.nginx.org_nginxproxies.yaml | 15 +++- deploy/crds.yaml | 15 +++- internal/controller/provisioner/objects.go | 12 +-- .../state/dataplane/configuration.go | 6 +- .../state/dataplane/configuration_test.go | 67 +++++++++++++-- .../state/graph/multiple_gateways_test.go | 8 +- 8 files changed, 171 insertions(+), 80 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index 25de7e5e17..5e35f669f2 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -298,16 +298,20 @@ type NginxLogging struct { // +kubebuilder:default=info AgentLevel *AgentLogLevel `json:"agentLevel,omitempty"` - // LogFormats defines custom log formats that can be used in access logs. + // LogFormat defines custom log format that can be used in access logs. // Each log format must have a unique name. + // https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format // // +optional - LogFormat *LogFormat `json:"logFormat,omitempty"` + LogFormat *NginxLogFormat `json:"logFormat,omitempty"` - // AccessLogs defines the access log settings, including the log file path, format, and optional parameters. + // AccessLog defines the access log settings, including the log file path and format name. + // For now only path /dev/stdout can be used. + // Path can be set to "OFF" to disable logging. + // https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log // // +optional - AccessLog *AccessLog `json:"accessLog,omitempty"` + AccessLog *NginxAccessLog `json:"accessLog,omitempty"` } // NginxErrorLogLevel type defines the log level of error logs for NGINX. @@ -363,15 +367,33 @@ const ( AgentLogLevelFatal AgentLogLevel = "fatal" ) -// LogFormat defines a custom log format for NGINX. -type LogFormat struct { - Name *string `json:"name,omitempty"` +// NginxLogFormat defines a custom log format for NGINX. +// https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format +type NginxLogFormat struct { + // Name specifies the name of the log format. + // + // +optional + Name *string `json:"name,omitempty"` + + // Format specifies the log format string. + // + // +optional Format *string `json:"format,omitempty"` } -// AccessLog defines the configuration for an NGINX access log. For now only path /dev/stdout is used. -type AccessLog struct { - Path *string `json:"path,omitempty"` +// NginxAccessLog defines the configuration for an NGINX access log. +// For now only path /dev/stdout can be used. +// Path can be set to "OFF" to disable logging. +// https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log +type NginxAccessLog struct { + // Path specifies the log file path. Or "OFF" to disable logging. + // + // +optional + Path *string `json:"path,omitempty"` + + // Format specifies the log format name to be used. + // + // +optional Format *string `json:"format,omitempty"` } diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index fbee1afc76..79344462cb 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -13,31 +13,6 @@ import ( apisv1 "sigs.k8s.io/gateway-api/apis/v1" ) -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *AccessLog) DeepCopyInto(out *AccessLog) { - *out = *in - if in.Path != nil { - in, out := &in.Path, &out.Path - *out = new(string) - **out = **in - } - if in.Format != nil { - in, out := &in.Format, &out.Format - *out = new(string) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new AccessLog. -func (in *AccessLog) DeepCopy() *AccessLog { - if in == nil { - return nil - } - out := new(AccessLog) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *AutoscalingSpec) DeepCopyInto(out *AutoscalingSpec) { *out = *in @@ -316,10 +291,35 @@ func (in *KubernetesSpec) DeepCopy() *KubernetesSpec { } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *LogFormat) DeepCopyInto(out *LogFormat) { +func (in *Metrics) DeepCopyInto(out *Metrics) { *out = *in - if in.Name != nil { - in, out := &in.Name, &out.Name + if in.Port != nil { + in, out := &in.Port, &out.Port + *out = new(int32) + **out = **in + } + if in.Disable != nil { + in, out := &in.Disable, &out.Disable + *out = new(bool) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metrics. +func (in *Metrics) DeepCopy() *Metrics { + if in == nil { + return nil + } + out := new(Metrics) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *NginxAccessLog) DeepCopyInto(out *NginxAccessLog) { + *out = *in + if in.Path != nil { + in, out := &in.Path, &out.Path *out = new(string) **out = **in } @@ -330,37 +330,37 @@ func (in *LogFormat) DeepCopyInto(out *LogFormat) { } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LogFormat. -func (in *LogFormat) DeepCopy() *LogFormat { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NginxAccessLog. +func (in *NginxAccessLog) DeepCopy() *NginxAccessLog { if in == nil { return nil } - out := new(LogFormat) + out := new(NginxAccessLog) in.DeepCopyInto(out) return out } // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *Metrics) DeepCopyInto(out *Metrics) { +func (in *NginxLogFormat) DeepCopyInto(out *NginxLogFormat) { *out = *in - if in.Port != nil { - in, out := &in.Port, &out.Port - *out = new(int32) + if in.Name != nil { + in, out := &in.Name, &out.Name + *out = new(string) **out = **in } - if in.Disable != nil { - in, out := &in.Disable, &out.Disable - *out = new(bool) + if in.Format != nil { + in, out := &in.Format, &out.Format + *out = new(string) **out = **in } } -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Metrics. -func (in *Metrics) DeepCopy() *Metrics { +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NginxLogFormat. +func (in *NginxLogFormat) DeepCopy() *NginxLogFormat { if in == nil { return nil } - out := new(Metrics) + out := new(NginxLogFormat) in.DeepCopyInto(out) return out } @@ -380,12 +380,12 @@ func (in *NginxLogging) DeepCopyInto(out *NginxLogging) { } if in.LogFormat != nil { in, out := &in.LogFormat, &out.LogFormat - *out = new(LogFormat) + *out = new(NginxLogFormat) (*in).DeepCopyInto(*out) } if in.AccessLog != nil { in, out := &in.AccessLog, &out.AccessLog - *out = new(AccessLog) + *out = new(NginxAccessLog) (*in).DeepCopyInto(*out) } } diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index b2f387a562..120606c0fb 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -8017,12 +8017,18 @@ spec: description: Logging defines logging related settings for NGINX. properties: accessLog: - description: AccessLogs defines the access log settings, including - the log file path, format, and optional parameters. + description: |- + AccessLog defines the access log settings, including the log file path and format name. + For now only path /dev/stdout can be used. + Path can be set to "OFF" to disable logging. + https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log properties: format: + description: Format specifies the log format name to be used. type: string path: + description: Path specifies the log file path. Or "OFF" to + disable logging. type: string type: object agentLevel: @@ -8056,12 +8062,15 @@ spec: type: string logFormat: description: |- - LogFormats defines custom log formats that can be used in access logs. + LogFormat defines custom log format that can be used in access logs. Each log format must have a unique name. + https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format properties: format: + description: Format specifies the log format string. type: string name: + description: Name specifies the name of the log format. type: string type: object type: object diff --git a/deploy/crds.yaml b/deploy/crds.yaml index fe527e17cb..795185d468 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -8604,12 +8604,18 @@ spec: description: Logging defines logging related settings for NGINX. properties: accessLog: - description: AccessLogs defines the access log settings, including - the log file path, format, and optional parameters. + description: |- + AccessLog defines the access log settings, including the log file path and format name. + For now only path /dev/stdout can be used. + Path can be set to "OFF" to disable logging. + https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log properties: format: + description: Format specifies the log format name to be used. type: string path: + description: Path specifies the log file path. Or "OFF" to + disable logging. type: string type: object agentLevel: @@ -8643,12 +8649,15 @@ spec: type: string logFormat: description: |- - LogFormats defines custom log formats that can be used in access logs. + LogFormat defines custom log format that can be used in access logs. Each log format must have a unique name. + https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format properties: format: + description: Format specifies the log format string. type: string name: + description: Name specifies the name of the log format. type: string type: object type: object diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 54274b3e4c..c018dce100 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -1448,14 +1448,14 @@ func DetermineNginxImageName( return fmt.Sprintf("%s:%s", image, tag), pullPolicy } -func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.LogFormat { - logFormat := &ngfAPIv1alpha2.LogFormat{} +func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.NginxLogFormat { + logFormat := &ngfAPIv1alpha2.NginxLogFormat{} if logging == nil { return logFormat } if logging.LogFormat != nil { - logFormat = &ngfAPIv1alpha2.LogFormat{ + logFormat = &ngfAPIv1alpha2.NginxLogFormat{ Name: logging.LogFormat.Name, Format: logging.LogFormat.Format, } @@ -1464,14 +1464,14 @@ func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1al return logFormat } -func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.AccessLog { - accessLog := &ngfAPIv1alpha2.AccessLog{} +func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.NginxAccessLog { + accessLog := &ngfAPIv1alpha2.NginxAccessLog{} if logging == nil { return accessLog } if logging.AccessLog != nil { - accessLog = &ngfAPIv1alpha2.AccessLog{ + accessLog = &ngfAPIv1alpha2.NginxAccessLog{ Path: logging.AccessLog.Path, Format: logging.AccessLog.Format, } diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index dee5a3b846..0083496563 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -1262,10 +1262,8 @@ func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *LogFormat { func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *AccessLog { var accessLog AccessLog // Current API exposes a singular *string AccessLog path (no format yet) – only /dev/stdout or "off". - if srcLogSettings.AccessLog != nil && - srcLogSettings.AccessLog.Path != nil && - *srcLogSettings.AccessLog.Path != "" { - if strings.ToLower(*srcLogSettings.AccessLog.Path) == "off" { + if srcLogSettings.AccessLog != nil { + if srcLogSettings.AccessLog.Path != nil && strings.ToLower(*srcLogSettings.AccessLog.Path) == "off" { accessLog = AccessLog{Path: "off"} } else { // only "/dev/stdout" is supported for now diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 457b799122..3d78d1e831 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4890,13 +4890,13 @@ func TestBuildLogging(t *testing.T) { EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.LogFormat{ + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ Name: helpers.GetPointer("custom_format"), Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("/dev/stdout"), Format: helpers.GetPointer("custom_format"), }, @@ -4917,18 +4917,71 @@ func TestBuildLogging(t *testing.T) { }, }, }, + { + msg: "AccessLog has no path configured", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`), + }, + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ + Format: helpers.GetPointer("custom_format"), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LogFormat: &LogFormat{ + Name: "custom_format", + Format: `'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`, + }, + AccessLog: &AccessLog{ + Path: "/dev/stdout", + Format: "custom_format", + }, + }, + }, + { + msg: "Nothing configured if AccessLog is missing", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ + Name: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '`), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + LogFormat: nil, + AccessLog: nil, + }, + }, { msg: "No AccessLog setting if LogFormat is not configured properly (no name given)", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.LogFormat{ + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("/dev/stdout"), Format: helpers.GetPointer("custom_format"), }, @@ -4945,7 +4998,7 @@ func TestBuildLogging(t *testing.T) { EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("OFF"), Format: helpers.GetPointer("custom_format"), }, @@ -4965,13 +5018,13 @@ func TestBuildLogging(t *testing.T) { EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.LogFormat{ + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ Name: helpers.GetPointer("custom_format"), Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("logs/access.log"), Format: helpers.GetPointer("custom_format"), }, diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index d8c420f77c..77bb20a492 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -224,11 +224,11 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormat: &ngfAPIv1alpha2.LogFormat{ + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ Name: helpers.GetPointer("custom_format"), Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("/var/log/nginx/access.log"), Format: helpers.GetPointer("custom_format"), }, @@ -343,11 +343,11 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormat: &ngfAPIv1alpha2.LogFormat{ + LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ Name: helpers.GetPointer("custom_format"), Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, - AccessLog: &ngfAPIv1alpha2.AccessLog{ + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Path: helpers.GetPointer("/var/log/nginx/access.log"), Format: helpers.GetPointer("custom_format"), }, From a12534933a29d84ae0012792c772a64353d23a35 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Thu, 6 Nov 2025 11:08:40 +0000 Subject: [PATCH 6/8] Removing NginxLogFormat for usability purpose --- apis/v1alpha2/nginxproxy_types.go | 44 +++------ apis/v1alpha2/zz_generated.deepcopy.go | 36 +------ .../bases/gateway.nginx.org_nginxproxies.yaml | 31 ++---- deploy/crds.yaml | 31 ++---- .../nginx/config/base_http_config_template.go | 9 +- .../nginx/config/base_http_config_test.go | 16 ++-- internal/controller/provisioner/objects.go | 24 +---- .../state/dataplane/configuration.go | 58 +++++------ .../state/dataplane/configuration_test.go | 95 ++++--------------- .../state/graph/multiple_gateways_test.go | 14 +-- 10 files changed, 92 insertions(+), 266 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index 5e35f669f2..55f05a5f3a 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -298,17 +298,8 @@ type NginxLogging struct { // +kubebuilder:default=info AgentLevel *AgentLogLevel `json:"agentLevel,omitempty"` - // LogFormat defines custom log format that can be used in access logs. - // Each log format must have a unique name. - // https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format - // - // +optional - LogFormat *NginxLogFormat `json:"logFormat,omitempty"` - - // AccessLog defines the access log settings, including the log file path and format name. + // AccessLog defines the access log settings, including format itself and disabling option. // For now only path /dev/stdout can be used. - // Path can be set to "OFF" to disable logging. - // https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log // // +optional AccessLog *NginxAccessLog `json:"accessLog,omitempty"` @@ -367,36 +358,29 @@ const ( AgentLogLevelFatal AgentLogLevel = "fatal" ) -// NginxLogFormat defines a custom log format for NGINX. -// https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format -type NginxLogFormat struct { - // Name specifies the name of the log format. - // - // +optional - Name *string `json:"name,omitempty"` - - // Format specifies the log format string. - // - // +optional - Format *string `json:"format,omitempty"` -} - // NginxAccessLog defines the configuration for an NGINX access log. -// For now only path /dev/stdout can be used. -// Path can be set to "OFF" to disable logging. -// https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log type NginxAccessLog struct { - // Path specifies the log file path. Or "OFF" to disable logging. + // Disabled turns off access logging when set to true. // // +optional - Path *string `json:"path,omitempty"` + Disabled *bool `json:"disabled,omitempty"` - // Format specifies the log format name to be used. + // Format specifies the custom log format string. + // If not specified, NGINX default 'combined' format is used. + // For now only path /dev/stdout can be used. + // See https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format // // +optional Format *string `json:"format,omitempty"` } +const ( + // DefaultLogFormatName is used when user provides custom access_log format. + DefaultLogFormatName = "ngf_user_defined_log_format" + // DefaultAccessLogPath is the default path for the access log. + DefaultAccessLogPath = "/dev/stdout" +) + // NginxPlus specifies NGINX Plus additional settings. These will only be applied if NGINX Plus is being used. type NginxPlus struct { // AllowedAddresses specifies IPAddresses or CIDR blocks to the allow list for accessing the NGINX Plus API. diff --git a/apis/v1alpha2/zz_generated.deepcopy.go b/apis/v1alpha2/zz_generated.deepcopy.go index 79344462cb..46037a8c11 100644 --- a/apis/v1alpha2/zz_generated.deepcopy.go +++ b/apis/v1alpha2/zz_generated.deepcopy.go @@ -318,9 +318,9 @@ func (in *Metrics) DeepCopy() *Metrics { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NginxAccessLog) DeepCopyInto(out *NginxAccessLog) { *out = *in - if in.Path != nil { - in, out := &in.Path, &out.Path - *out = new(string) + if in.Disabled != nil { + in, out := &in.Disabled, &out.Disabled + *out = new(bool) **out = **in } if in.Format != nil { @@ -340,31 +340,6 @@ func (in *NginxAccessLog) DeepCopy() *NginxAccessLog { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *NginxLogFormat) DeepCopyInto(out *NginxLogFormat) { - *out = *in - if in.Name != nil { - in, out := &in.Name, &out.Name - *out = new(string) - **out = **in - } - if in.Format != nil { - in, out := &in.Format, &out.Format - *out = new(string) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NginxLogFormat. -func (in *NginxLogFormat) DeepCopy() *NginxLogFormat { - if in == nil { - return nil - } - out := new(NginxLogFormat) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *NginxLogging) DeepCopyInto(out *NginxLogging) { *out = *in @@ -378,11 +353,6 @@ func (in *NginxLogging) DeepCopyInto(out *NginxLogging) { *out = new(AgentLogLevel) **out = **in } - if in.LogFormat != nil { - in, out := &in.LogFormat, &out.LogFormat - *out = new(NginxLogFormat) - (*in).DeepCopyInto(*out) - } if in.AccessLog != nil { in, out := &in.AccessLog, &out.AccessLog *out = new(NginxAccessLog) diff --git a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml index 120606c0fb..07b7911901 100644 --- a/config/crd/bases/gateway.nginx.org_nginxproxies.yaml +++ b/config/crd/bases/gateway.nginx.org_nginxproxies.yaml @@ -8018,17 +8018,19 @@ spec: properties: accessLog: description: |- - AccessLog defines the access log settings, including the log file path and format name. + AccessLog defines the access log settings, including format itself and disabling option. For now only path /dev/stdout can be used. - Path can be set to "OFF" to disable logging. - https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log properties: + disabled: + description: Disabled turns off access logging when set to + true. + type: boolean format: - description: Format specifies the log format name to be used. - type: string - path: - description: Path specifies the log file path. Or "OFF" to - disable logging. + description: |- + Format specifies the custom log format string. + If not specified, NGINX default 'combined' format is used. + For now only path /dev/stdout can be used. + See https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format type: string type: object agentLevel: @@ -8060,19 +8062,6 @@ spec: - alert - emerg type: string - logFormat: - description: |- - LogFormat defines custom log format that can be used in access logs. - Each log format must have a unique name. - https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format - properties: - format: - description: Format specifies the log format string. - type: string - name: - description: Name specifies the name of the log format. - type: string - type: object type: object metrics: description: |- diff --git a/deploy/crds.yaml b/deploy/crds.yaml index 795185d468..046ff44bf3 100644 --- a/deploy/crds.yaml +++ b/deploy/crds.yaml @@ -8605,17 +8605,19 @@ spec: properties: accessLog: description: |- - AccessLog defines the access log settings, including the log file path and format name. + AccessLog defines the access log settings, including format itself and disabling option. For now only path /dev/stdout can be used. - Path can be set to "OFF" to disable logging. - https://nginx.org/en/docs/http/ngx_http_log_module.html#access_log properties: + disabled: + description: Disabled turns off access logging when set to + true. + type: boolean format: - description: Format specifies the log format name to be used. - type: string - path: - description: Path specifies the log file path. Or "OFF" to - disable logging. + description: |- + Format specifies the custom log format string. + If not specified, NGINX default 'combined' format is used. + For now only path /dev/stdout can be used. + See https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format type: string type: object agentLevel: @@ -8647,19 +8649,6 @@ spec: - alert - emerg type: string - logFormat: - description: |- - LogFormat defines custom log format that can be used in access logs. - Each log format must have a unique name. - https://nginx.org/en/docs/http/ngx_http_log_module.html#log_format - properties: - format: - description: Format specifies the log format string. - type: string - name: - description: Name specifies the name of the log format. - type: string - type: object type: object metrics: description: |- diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index 50926f5d63..eef05fd9e2 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -50,9 +50,10 @@ server { {{- /* Define custom log format */ -}} {{- if .LogFormat }} - {{- if .LogFormat.Name }} -log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; - {{- end }} +{{- /* We use a fixed name for user-defined log format to avoid complexity of passing the name around. */ -}} +{{- if .LogFormat.Format }} +log_format ngf_user_defined_log_format '{{ .LogFormat.Format }}'; +{{- end }} {{- end }} {{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use /dev/stdout with that format. Otherwise use the given path. */ -}} @@ -61,7 +62,7 @@ log_format {{ .LogFormat.Name }} '{{ .LogFormat.Format }}'; access_log off; {{- else }} {{- if .AccessLog.Format }} -access_log /dev/stdout {{ .AccessLog.Format }}; +access_log /dev/stdout ngf_user_defined_log_format; {{- end }} {{- end }} {{- end }} diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index c31e82d555..80751b6eab 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -21,15 +21,15 @@ func TestLoggingSettingsTemplate(t *testing.T) { unexpectedOutputs []string }{ { - name: "Log format and access log with custom path", + name: "Log format and access log with custom path and custom format name", logFormat: &dataplane.LogFormat{ Name: "custom_format", Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", }, accessLog: &dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, expectedOutputs: []string{ - `log_format custom_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, - `access_log /dev/stdout custom_format;`, + `log_format ngf_user_defined_log_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, + `access_log /dev/stdout ngf_user_defined_log_format;`, }, }, { @@ -40,23 +40,23 @@ func TestLoggingSettingsTemplate(t *testing.T) { }, accessLog: &dataplane.AccessLog{Path: "", Format: ""}, unexpectedOutputs: []string{ - `log_format custom_format`, + `log_format ngf_user_defined_log_format`, `access_log /dev/stdout`, }, }, { name: "Access log off while format presented", logFormat: &dataplane.LogFormat{ - Name: "custom_format", + Name: "ngf_user_defined_log_format", Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", }, - accessLog: &dataplane.AccessLog{Path: "off", Format: "custom_format"}, + accessLog: &dataplane.AccessLog{Path: "off", Format: "ngf_user_defined_log_format"}, expectedOutputs: []string{ `access_log off;`, - `log_format custom_format`, + `log_format ngf_user_defined_log_format`, }, unexpectedOutputs: []string{ - `access_log off custom_format`, + `access_log off ngf_user_defined_log_format`, }, }, { diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index c018dce100..4f40eab705 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -407,7 +407,6 @@ func (p *NginxProvisioner) buildNginxConfigMaps( } // Add LogFormats and AccessLogs to mainFields - logFormat := addLogFormatToNginxConfig(logging) accessLog := addAccessLogsToNginxConfig(logging) mainFields := map[string]interface{}{ @@ -415,9 +414,6 @@ func (p *NginxProvisioner) buildNginxConfigMaps( "WorkerConnections": workerConnections, } - if logFormat != nil { - mainFields["LogFormat"] = logFormat - } if accessLog != nil { mainFields["AccessLog"] = accessLog } @@ -1448,22 +1444,6 @@ func DetermineNginxImageName( return fmt.Sprintf("%s:%s", image, tag), pullPolicy } -func addLogFormatToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.NginxLogFormat { - logFormat := &ngfAPIv1alpha2.NginxLogFormat{} - if logging == nil { - return logFormat - } - - if logging.LogFormat != nil { - logFormat = &ngfAPIv1alpha2.NginxLogFormat{ - Name: logging.LogFormat.Name, - Format: logging.LogFormat.Format, - } - } - - return logFormat -} - func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.NginxAccessLog { accessLog := &ngfAPIv1alpha2.NginxAccessLog{} if logging == nil { @@ -1472,8 +1452,8 @@ func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1a if logging.AccessLog != nil { accessLog = &ngfAPIv1alpha2.NginxAccessLog{ - Path: logging.AccessLog.Path, - Format: logging.AccessLog.Format, + Disabled: logging.AccessLog.Disabled, + Format: logging.AccessLog.Format, } } diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 0083496563..8de4c3a04b 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -6,7 +6,6 @@ import ( "fmt" "slices" "sort" - "strings" "github.com/go-logr/logr" discoveryV1 "k8s.io/api/discovery/v1" @@ -1223,18 +1222,13 @@ func buildLogging(gateway *graph.Gateway) Logging { } srcLogSettings := ngfProxy.Logging - logFormat := buildLogFormat(srcLogSettings) - accessLog := buildAccessLog(srcLogSettings) + logFormat, accessLog := buildAccessLog(srcLogSettings) - if accessLog.Path == "off" { - logSettings.AccessLog = &AccessLog{Path: "off"} - - return logSettings + if logFormat != nil { + logSettings.LogFormat = logFormat } - if logFormat != nil && logFormat.Format != "" && accessLog != nil && accessLog.Path != "" { - // only update logSettings if both LogFormat and AccessLog path are configured - logSettings.LogFormat = logFormat + if accessLog != nil { logSettings.AccessLog = accessLog } } @@ -1243,42 +1237,34 @@ func buildLogging(gateway *graph.Gateway) Logging { } func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *LogFormat { - var logFormat LogFormat - // Current API exposes a single LogFormat value whose fields are pointers; include only if both set and non-empty. - if srcLogSettings.LogFormat != nil && - srcLogSettings.LogFormat.Name != nil && - srcLogSettings.LogFormat.Format != nil && - *srcLogSettings.LogFormat.Name != "" && - *srcLogSettings.LogFormat.Format != "" { - logFormat = LogFormat{ - Name: *srcLogSettings.LogFormat.Name, - Format: *srcLogSettings.LogFormat.Format, + if srcLogSettings.AccessLog != nil && + srcLogSettings.AccessLog.Format != nil && + *srcLogSettings.AccessLog.Format != "" { + return &LogFormat{ + Name: ngfAPIv1alpha2.DefaultLogFormatName, + Format: *srcLogSettings.AccessLog.Format, } } - return &logFormat + return nil } -func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *AccessLog { - var accessLog AccessLog - // Current API exposes a singular *string AccessLog path (no format yet) – only /dev/stdout or "off". +func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) (*LogFormat, *AccessLog) { + logFormat := buildLogFormat(srcLogSettings) if srcLogSettings.AccessLog != nil { - if srcLogSettings.AccessLog.Path != nil && strings.ToLower(*srcLogSettings.AccessLog.Path) == "off" { - accessLog = AccessLog{Path: "off"} - } else { - // only "/dev/stdout" is supported for now - defaultPath := "/dev/stdout" - if srcLogSettings.AccessLog.Format != nil && - *srcLogSettings.AccessLog.Format != "" { - accessLog = AccessLog{ - Path: defaultPath, - Format: *srcLogSettings.AccessLog.Format, - } + if srcLogSettings.AccessLog.Disabled != nil && *srcLogSettings.AccessLog.Disabled { + return nil, &AccessLog{Path: "off"} + } + + if logFormat != nil { + return logFormat, &AccessLog{ + Path: ngfAPIv1alpha2.DefaultAccessLogPath, + Format: ngfAPIv1alpha2.DefaultLogFormatName, } } } - return &accessLog + return nil, nil } func buildWorkerConnections(gateway *graph.Gateway) int32 { diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 3d78d1e831..1192a7561d 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4890,76 +4890,65 @@ func TestBuildLogging(t *testing.T) { EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("/dev/stdout"), - Format: helpers.GetPointer("custom_format"), - }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", LogFormat: &LogFormat{ - Name: "custom_format", + Name: ngfAPIv1alpha2.DefaultLogFormatName, Format: `'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`, }, AccessLog: &AccessLog{ - Path: "/dev/stdout", - Format: "custom_format", + Path: ngfAPIv1alpha2.DefaultAccessLogPath, + Format: ngfAPIv1alpha2.DefaultLogFormatName, }, }, }, { - msg: "AccessLog has no path configured", + msg: "AccessLog is configured and Disabled = false", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ + Disabled: helpers.GetPointer(false), Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Format: helpers.GetPointer("custom_format"), - }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", LogFormat: &LogFormat{ - Name: "custom_format", + Name: ngfAPIv1alpha2.DefaultLogFormatName, Format: `'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`, }, AccessLog: &AccessLog{ - Path: "/dev/stdout", - Format: "custom_format", + Path: ngfAPIv1alpha2.DefaultAccessLogPath, + Format: ngfAPIv1alpha2.DefaultLogFormatName, }, }, }, { - msg: "Nothing configured if AccessLog is missing", + msg: "Nothing configured if AccessLog Format is missing", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), - Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`), + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ + Disabled: helpers.GetPointer(false), }, }, }, @@ -4970,28 +4959,6 @@ func TestBuildLogging(t *testing.T) { AccessLog: nil, }, }, - { - msg: "No AccessLog setting if LogFormat is not configured properly (no name given)", - gw: &graph.Gateway{ - EffectiveNginxProxy: &graph.EffectiveNginxProxy{ - Logging: &ngfAPIv1alpha2.NginxLogging{ - ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`), - }, - AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("/dev/stdout"), - Format: helpers.GetPointer("custom_format"), - }, - }, - }, - }, - expLoggingSettings: Logging{ - ErrorLevel: "info", - }, - }, { msg: "AccessLog OFF while LogFormat is configured", gw: &graph.Gateway{ @@ -4999,49 +4966,19 @@ func TestBuildLogging(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("OFF"), - Format: helpers.GetPointer("custom_format"), - }, - }, - }, - }, - expLoggingSettings: Logging{ - ErrorLevel: "info", - AccessLog: &AccessLog{ - Path: "off", - }, - }, - }, - { - msg: "AccessLog path replaced with /dev/stdout", - gw: &graph.Gateway{ - EffectiveNginxProxy: &graph.EffectiveNginxProxy{ - Logging: &ngfAPIv1alpha2.NginxLogging{ - ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), + Disabled: helpers.GetPointer(true), Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' '"$request" $status $body_bytes_sent ' '"$http_referer" "$http_user_agent" '`), }, - AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("logs/access.log"), - Format: helpers.GetPointer("custom_format"), - }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", - LogFormat: &LogFormat{ - Name: "custom_format", - Format: `'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`, - }, + LogFormat: nil, AccessLog: &AccessLog{ - Path: "/dev/stdout", - Format: "custom_format", + Path: "off", }, }, }, diff --git a/internal/controller/state/graph/multiple_gateways_test.go b/internal/controller/state/graph/multiple_gateways_test.go index 77bb20a492..7c636d7917 100644 --- a/internal/controller/state/graph/multiple_gateways_test.go +++ b/internal/controller/state/graph/multiple_gateways_test.go @@ -224,13 +224,8 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), - Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), - }, AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("/var/log/nginx/access.log"), - Format: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, }, }) @@ -343,13 +338,8 @@ func Test_MultipleGateways_WithNginxProxy(t *testing.T) { Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelDebug), AgentLevel: helpers.GetPointer(ngfAPIv1alpha2.AgentLogLevelDebug), - LogFormat: &ngfAPIv1alpha2.NginxLogFormat{ - Name: helpers.GetPointer("custom_format"), - Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), - }, AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Path: helpers.GetPointer("/var/log/nginx/access.log"), - Format: helpers.GetPointer("custom_format"), + Format: helpers.GetPointer("$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent"), }, }, DisableHTTP2: helpers.GetPointer(true), From c19544c40649b54516bdd793c106eab9f4c65628 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Thu, 6 Nov 2025 16:24:24 +0000 Subject: [PATCH 7/8] remove logFormat from dataplane as well --- apis/v1alpha2/nginxproxy_types.go | 7 --- .../nginx/config/base_http_config.go | 6 +- .../nginx/config/base_http_config_template.go | 13 ++-- .../nginx/config/base_http_config_test.go | 46 ++++++-------- .../state/dataplane/configuration.go | 38 ++++-------- .../state/dataplane/configuration_test.go | 60 ++++++++++--------- internal/controller/state/dataplane/types.go | 20 ++----- 7 files changed, 74 insertions(+), 116 deletions(-) diff --git a/apis/v1alpha2/nginxproxy_types.go b/apis/v1alpha2/nginxproxy_types.go index 55f05a5f3a..6f9e63066f 100644 --- a/apis/v1alpha2/nginxproxy_types.go +++ b/apis/v1alpha2/nginxproxy_types.go @@ -374,13 +374,6 @@ type NginxAccessLog struct { Format *string `json:"format,omitempty"` } -const ( - // DefaultLogFormatName is used when user provides custom access_log format. - DefaultLogFormatName = "ngf_user_defined_log_format" - // DefaultAccessLogPath is the default path for the access log. - DefaultAccessLogPath = "/dev/stdout" -) - // NginxPlus specifies NGINX Plus additional settings. These will only be applied if NGINX Plus is being used. type NginxPlus struct { // AllowedAddresses specifies IPAddresses or CIDR blocks to the allow list for accessing the NGINX Plus API. diff --git a/internal/controller/nginx/config/base_http_config.go b/internal/controller/nginx/config/base_http_config.go index de1dcd0ea4..536843cb39 100644 --- a/internal/controller/nginx/config/base_http_config.go +++ b/internal/controller/nginx/config/base_http_config.go @@ -12,8 +12,9 @@ var baseHTTPTemplate = gotemplate.Must(gotemplate.New("baseHttp").Parse(baseHTTP type httpConfig struct { DNSResolver *dataplane.DNSResolverConfig - LogFormat *dataplane.LogFormat AccessLog *dataplane.AccessLog + DefaultAccessLogPath string + DefaultLogFormatName string Includes []shared.Include NginxReadinessProbePort int32 IPFamily shared.IPFamily @@ -29,8 +30,9 @@ func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { NginxReadinessProbePort: conf.BaseHTTPConfig.NginxReadinessProbePort, IPFamily: getIPFamily(conf.BaseHTTPConfig), DNSResolver: conf.BaseHTTPConfig.DNSResolver, - LogFormat: conf.Logging.LogFormat, AccessLog: conf.Logging.AccessLog, + DefaultAccessLogPath: dataplane.DefaultAccessLogPath, + DefaultLogFormatName: dataplane.DefaultLogFormatName, } results := make([]executeResult, 0, len(includes)+1) diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index eef05fd9e2..9bea301a89 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -49,20 +49,15 @@ server { } {{- /* Define custom log format */ -}} -{{- if .LogFormat }} +{{- /* Access log directives for AccessLog. If path is "off" we disable logging. */ -}} {{- /* We use a fixed name for user-defined log format to avoid complexity of passing the name around. */ -}} -{{- if .LogFormat.Format }} -log_format ngf_user_defined_log_format '{{ .LogFormat.Format }}'; -{{- end }} -{{- end }} - -{{- /* Access log directives for AccessLog. If path is "off" we disable logging. If Format set, use /dev/stdout with that format. Otherwise use the given path. */ -}} {{- if .AccessLog }} - {{- if eq .AccessLog.Path "off" }} + {{- if .AccessLog.Disabled }} access_log off; {{- else }} {{- if .AccessLog.Format }} -access_log /dev/stdout ngf_user_defined_log_format; +log_format {{ .DefaultLogFormatName }} '{{ .AccessLog.Format }}'; +access_log {{ .DefaultAccessLogPath }} {{ .DefaultLogFormatName }}; {{- end }} {{- end }} {{- end }} diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index 80751b6eab..76dee76c5e 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "sort" "strings" "testing" @@ -13,58 +14,49 @@ import ( func TestLoggingSettingsTemplate(t *testing.T) { t.Parallel() + logFormat := "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent" + tests := []struct { name string accessLog *dataplane.AccessLog - logFormat *dataplane.LogFormat expectedOutputs []string unexpectedOutputs []string }{ { - name: "Log format and access log with custom path and custom format name", - logFormat: &dataplane.LogFormat{ - Name: "custom_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, - accessLog: &dataplane.AccessLog{Path: "/path/to/log.gz", Format: "custom_format"}, + name: "Log format and access log with custom path and custom format name", + accessLog: &dataplane.AccessLog{Format: logFormat}, expectedOutputs: []string{ - `log_format ngf_user_defined_log_format '$remote_addr - [$time_local] "$request" $status $body_bytes_sent';`, - `access_log /dev/stdout ngf_user_defined_log_format;`, + fmt.Sprintf("log_format %s '%s'", dataplane.DefaultLogFormatName, logFormat), + fmt.Sprintf("access_log %s %s", dataplane.DefaultAccessLogPath, dataplane.DefaultLogFormatName), }, }, { - name: "Empty log format name and format", - logFormat: &dataplane.LogFormat{ - Name: "", - Format: "", - }, - accessLog: &dataplane.AccessLog{Path: "", Format: ""}, + name: "Empty format", + accessLog: &dataplane.AccessLog{Format: ""}, unexpectedOutputs: []string{ - `log_format ngf_user_defined_log_format`, - `access_log /dev/stdout`, + fmt.Sprintf("log_format %s '%s'", dataplane.DefaultLogFormatName, logFormat), + fmt.Sprintf("access_log %s %s", dataplane.DefaultAccessLogPath, dataplane.DefaultLogFormatName), }, }, { - name: "Access log off while format presented", - logFormat: &dataplane.LogFormat{ - Name: "ngf_user_defined_log_format", - Format: "$remote_addr - [$time_local] \"$request\" $status $body_bytes_sent", - }, - accessLog: &dataplane.AccessLog{Path: "off", Format: "ngf_user_defined_log_format"}, + name: "Access log off while format presented", + accessLog: &dataplane.AccessLog{Disabled: true, Format: logFormat}, expectedOutputs: []string{ `access_log off;`, - `log_format ngf_user_defined_log_format`, }, unexpectedOutputs: []string{ - `access_log off ngf_user_defined_log_format`, + fmt.Sprintf("access_log off %s", dataplane.DefaultLogFormatName), }, }, { name: "Access log off", - accessLog: &dataplane.AccessLog{Path: "off"}, + accessLog: &dataplane.AccessLog{Disabled: true}, expectedOutputs: []string{ `access_log off;`, }, + unexpectedOutputs: []string{ + `log_format`, + }, }, } @@ -74,7 +66,7 @@ func TestLoggingSettingsTemplate(t *testing.T) { g := NewWithT(t) conf := dataplane.Configuration{ - Logging: dataplane.Logging{AccessLog: tt.accessLog, LogFormat: tt.logFormat}, + Logging: dataplane.Logging{AccessLog: tt.accessLog}, } res := executeBaseHTTPConfig(conf) diff --git a/internal/controller/state/dataplane/configuration.go b/internal/controller/state/dataplane/configuration.go index 8de4c3a04b..5805919fd3 100644 --- a/internal/controller/state/dataplane/configuration.go +++ b/internal/controller/state/dataplane/configuration.go @@ -28,6 +28,10 @@ const ( defaultErrorLogLevel = "info" DefaultWorkerConnections = int32(1024) DefaultNginxReadinessProbePort = int32(8081) + // DefaultLogFormatName is used when user provides custom access_log format. + DefaultLogFormatName = "ngf_user_defined_log_format" + // DefaultAccessLogPath is the default path for the access log. + DefaultAccessLogPath = "/dev/stdout" ) // BuildConfiguration builds the Configuration from the Graph. @@ -1222,13 +1226,8 @@ func buildLogging(gateway *graph.Gateway) Logging { } srcLogSettings := ngfProxy.Logging - logFormat, accessLog := buildAccessLog(srcLogSettings) - if logFormat != nil { - logSettings.LogFormat = logFormat - } - - if accessLog != nil { + if accessLog := buildAccessLog(srcLogSettings); accessLog != nil { logSettings.AccessLog = accessLog } } @@ -1236,35 +1235,20 @@ func buildLogging(gateway *graph.Gateway) Logging { return logSettings } -func buildLogFormat(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *LogFormat { - if srcLogSettings.AccessLog != nil && - srcLogSettings.AccessLog.Format != nil && - *srcLogSettings.AccessLog.Format != "" { - return &LogFormat{ - Name: ngfAPIv1alpha2.DefaultLogFormatName, - Format: *srcLogSettings.AccessLog.Format, - } - } - - return nil -} - -func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) (*LogFormat, *AccessLog) { - logFormat := buildLogFormat(srcLogSettings) +func buildAccessLog(srcLogSettings *ngfAPIv1alpha2.NginxLogging) *AccessLog { if srcLogSettings.AccessLog != nil { if srcLogSettings.AccessLog.Disabled != nil && *srcLogSettings.AccessLog.Disabled { - return nil, &AccessLog{Path: "off"} + return &AccessLog{Disabled: true} } - if logFormat != nil { - return logFormat, &AccessLog{ - Path: ngfAPIv1alpha2.DefaultAccessLogPath, - Format: ngfAPIv1alpha2.DefaultLogFormatName, + if srcLogSettings.AccessLog.Format != nil && *srcLogSettings.AccessLog.Format != "" { + return &AccessLog{ + Format: *srcLogSettings.AccessLog.Format, } } } - return nil, nil + return nil } func buildWorkerConnections(gateway *graph.Gateway) int32 { diff --git a/internal/controller/state/dataplane/configuration_test.go b/internal/controller/state/dataplane/configuration_test.go index 1192a7561d..4f79958156 100644 --- a/internal/controller/state/dataplane/configuration_test.go +++ b/internal/controller/state/dataplane/configuration_test.go @@ -4768,6 +4768,9 @@ func TestBuildRewriteIPSettings(t *testing.T) { func TestBuildLogging(t *testing.T) { defaultLogging := Logging{ErrorLevel: defaultErrorLogLevel} + logFormat := `'$remote_addr - $remote_user [$time_local] ' + '"$request" $status $body_bytes_sent ' + '"$http_referer" "$http_user_agent" '` t.Parallel() tests := []struct { @@ -4885,30 +4888,21 @@ func TestBuildLogging(t *testing.T) { expLoggingSettings: Logging{ErrorLevel: "emerg"}, }, { - msg: "LogFormat and AccessLog configured", + msg: "AccessLog configured", gw: &graph.Gateway{ EffectiveNginxProxy: &graph.EffectiveNginxProxy{ Logging: &ngfAPIv1alpha2.NginxLogging{ ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ - Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`), + Format: helpers.GetPointer(logFormat), }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", - LogFormat: &LogFormat{ - Name: ngfAPIv1alpha2.DefaultLogFormatName, - Format: `'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`, - }, AccessLog: &AccessLog{ - Path: ngfAPIv1alpha2.DefaultAccessLogPath, - Format: ngfAPIv1alpha2.DefaultLogFormatName, + Format: logFormat, }, }, }, @@ -4920,24 +4914,17 @@ func TestBuildLogging(t *testing.T) { ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Disabled: helpers.GetPointer(false), - Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`), + Format: helpers.GetPointer(logFormat), }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", - LogFormat: &LogFormat{ - Name: ngfAPIv1alpha2.DefaultLogFormatName, - Format: `'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`, - }, + AccessLog: &AccessLog{ - Path: ngfAPIv1alpha2.DefaultAccessLogPath, - Format: ngfAPIv1alpha2.DefaultLogFormatName, + Disabled: false, + Format: logFormat, }, }, }, @@ -4955,7 +4942,6 @@ func TestBuildLogging(t *testing.T) { }, expLoggingSettings: Logging{ ErrorLevel: "info", - LogFormat: nil, AccessLog: nil, }, }, @@ -4967,18 +4953,34 @@ func TestBuildLogging(t *testing.T) { ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ Disabled: helpers.GetPointer(true), - Format: helpers.GetPointer(`'$remote_addr - $remote_user [$time_local] ' - '"$request" $status $body_bytes_sent ' - '"$http_referer" "$http_user_agent" '`), + Format: helpers.GetPointer(logFormat), + }, + }, + }, + }, + expLoggingSettings: Logging{ + ErrorLevel: "info", + AccessLog: &AccessLog{ + Disabled: true, + }, + }, + }, + { + msg: "AccessLog OFF", + gw: &graph.Gateway{ + EffectiveNginxProxy: &graph.EffectiveNginxProxy{ + Logging: &ngfAPIv1alpha2.NginxLogging{ + ErrorLevel: helpers.GetPointer(ngfAPIv1alpha2.NginxLogLevelInfo), + AccessLog: &ngfAPIv1alpha2.NginxAccessLog{ + Disabled: helpers.GetPointer(true), }, }, }, }, expLoggingSettings: Logging{ ErrorLevel: "info", - LogFormat: nil, AccessLog: &AccessLog{ - Path: "off", + Disabled: true, }, }, }, diff --git a/internal/controller/state/dataplane/types.go b/internal/controller/state/dataplane/types.go index c361c7f39b..c974a606b0 100644 --- a/internal/controller/state/dataplane/types.go +++ b/internal/controller/state/dataplane/types.go @@ -474,10 +474,8 @@ type Ratio struct { // Logging defines logging related settings for NGINX. type Logging struct { - // AccessLog defines the configuration for the NGINX access log. For now only path /dev/stdout is used. + // AccessLog defines the configuration for the NGINX access log. AccessLog *AccessLog - // LogFormat represents a custom log format. - LogFormat *LogFormat // ErrorLevel defines the error log level. ErrorLevel string } @@ -501,18 +499,10 @@ type DeploymentContext struct { Integration string `json:"integration"` } -// LogFormat represents a custom log format. -type LogFormat struct { - // Name is the name of the log format. - Name string - // Format is the format string. - Format string -} - -// AccessLog defines the configuration for an NGINX access log. For now only path /dev/stdout is used. +// AccessLog defines the configuration for an NGINX access log. type AccessLog struct { - // Path is the path of the access log. Currently supported only /dev/stdout, or "off" to disable logging. - Path string - // Format is the log_format name + // Format is the access log format template. Format string + // Disabled specifies whether the access log is disabled. + Disabled bool } From a0ba554b209405118ced4df90682c96471e8e159 Mon Sep 17 00:00:00 2001 From: Tina Usova Date: Fri, 7 Nov 2025 09:13:44 +0000 Subject: [PATCH 8/8] move template AccessLog to separate structure --- .../nginx/config/base_http_config.go | 31 ++++++++++++++++--- .../nginx/config/base_http_config_template.go | 14 ++++----- .../nginx/config/base_http_config_test.go | 2 +- internal/controller/provisioner/objects.go | 23 -------------- 4 files changed, 35 insertions(+), 35 deletions(-) diff --git a/internal/controller/nginx/config/base_http_config.go b/internal/controller/nginx/config/base_http_config.go index 536843cb39..2b335df347 100644 --- a/internal/controller/nginx/config/base_http_config.go +++ b/internal/controller/nginx/config/base_http_config.go @@ -10,9 +10,15 @@ import ( var baseHTTPTemplate = gotemplate.Must(gotemplate.New("baseHttp").Parse(baseHTTPTemplateText)) +type AccessLog struct { + Format string // User's format string + Path string // Where to write logs (/dev/stdout) + FormatName string // Internal format name (ngf_user_defined_log_format) + Disabled bool // User's disabled flag +} type httpConfig struct { DNSResolver *dataplane.DNSResolverConfig - AccessLog *dataplane.AccessLog + AccessLog *AccessLog DefaultAccessLogPath string DefaultLogFormatName string Includes []shared.Include @@ -30,9 +36,7 @@ func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { NginxReadinessProbePort: conf.BaseHTTPConfig.NginxReadinessProbePort, IPFamily: getIPFamily(conf.BaseHTTPConfig), DNSResolver: conf.BaseHTTPConfig.DNSResolver, - AccessLog: conf.Logging.AccessLog, - DefaultAccessLogPath: dataplane.DefaultAccessLogPath, - DefaultLogFormatName: dataplane.DefaultLogFormatName, + AccessLog: buildAccessLog(conf.Logging.AccessLog), } results := make([]executeResult, 0, len(includes)+1) @@ -44,3 +48,22 @@ func executeBaseHTTPConfig(conf dataplane.Configuration) []executeResult { return results } + +func buildAccessLog(accessLogConfig *dataplane.AccessLog) *AccessLog { + if accessLogConfig != nil { + accessLog := &AccessLog{ + Path: dataplane.DefaultAccessLogPath, + FormatName: dataplane.DefaultLogFormatName, + } + if accessLogConfig.Format != "" { + accessLog.Format = accessLogConfig.Format + } + + if accessLogConfig.Disabled { + accessLog.Disabled = accessLogConfig.Disabled + } + + return accessLog + } + return nil +} diff --git a/internal/controller/nginx/config/base_http_config_template.go b/internal/controller/nginx/config/base_http_config_template.go index 9bea301a89..c96c4e25fa 100644 --- a/internal/controller/nginx/config/base_http_config_template.go +++ b/internal/controller/nginx/config/base_http_config_template.go @@ -52,14 +52,14 @@ server { {{- /* Access log directives for AccessLog. If path is "off" we disable logging. */ -}} {{- /* We use a fixed name for user-defined log format to avoid complexity of passing the name around. */ -}} {{- if .AccessLog }} - {{- if .AccessLog.Disabled }} +{{- if .AccessLog.Disabled }} access_log off; - {{- else }} - {{- if .AccessLog.Format }} -log_format {{ .DefaultLogFormatName }} '{{ .AccessLog.Format }}'; -access_log {{ .DefaultAccessLogPath }} {{ .DefaultLogFormatName }}; - {{- end }} - {{- end }} +{{- else }} +{{- if .AccessLog.Format }} +log_format {{ .AccessLog.FormatName }} '{{ .AccessLog.Format }}'; +access_log {{ .AccessLog.Path }} {{ .AccessLog.FormatName }}; +{{- end }} +{{- end }} {{- end }} {{ range $i := .Includes -}} diff --git a/internal/controller/nginx/config/base_http_config_test.go b/internal/controller/nginx/config/base_http_config_test.go index 76dee76c5e..2eb6322443 100644 --- a/internal/controller/nginx/config/base_http_config_test.go +++ b/internal/controller/nginx/config/base_http_config_test.go @@ -23,7 +23,7 @@ func TestLoggingSettingsTemplate(t *testing.T) { unexpectedOutputs []string }{ { - name: "Log format and access log with custom path and custom format name", + name: "Log format and access log with custom format", accessLog: &dataplane.AccessLog{Format: logFormat}, expectedOutputs: []string{ fmt.Sprintf("log_format %s '%s'", dataplane.DefaultLogFormatName, logFormat), diff --git a/internal/controller/provisioner/objects.go b/internal/controller/provisioner/objects.go index 4f40eab705..8ad2e1b72b 100644 --- a/internal/controller/provisioner/objects.go +++ b/internal/controller/provisioner/objects.go @@ -406,18 +406,11 @@ func (p *NginxProvisioner) buildNginxConfigMaps( workerConnections = *nProxyCfg.WorkerConnections } - // Add LogFormats and AccessLogs to mainFields - accessLog := addAccessLogsToNginxConfig(logging) - mainFields := map[string]interface{}{ "ErrorLevel": logLevel, "WorkerConnections": workerConnections, } - if accessLog != nil { - mainFields["AccessLog"] = accessLog - } - // Create events ConfigMap data using template eventsFields := map[string]interface{}{ "WorkerConnections": workerConnections, @@ -1443,19 +1436,3 @@ func DetermineNginxImageName( return fmt.Sprintf("%s:%s", image, tag), pullPolicy } - -func addAccessLogsToNginxConfig(logging *ngfAPIv1alpha2.NginxLogging) *ngfAPIv1alpha2.NginxAccessLog { - accessLog := &ngfAPIv1alpha2.NginxAccessLog{} - if logging == nil { - return accessLog - } - - if logging.AccessLog != nil { - accessLog = &ngfAPIv1alpha2.NginxAccessLog{ - Disabled: logging.AccessLog.Disabled, - Format: logging.AccessLog.Format, - } - } - - return accessLog -}