Add error-log-level configmap key #342
Conversation
There was a problem hiding this comment.
Looks great, thanks. Just a few small changes.
Please add this config map key to the table in the customization example readme.
You've changed the default log level for NGINX OSS - I think that's fine, we'll need to add it to the release notes.
A small naming issue, I've commented.
Thanks!
nginx-controller/nginx/config.go
Outdated
| @@ -49,6 +49,7 @@ type Config struct { | |||
| HealthCheckMandatory bool | |||
| HealthCheckMandatoryQueue int64 | |||
| SlowStart string | |||
| LogLevel string | |||
There was a problem hiding this comment.
Please rename to MainLogLevel or MainErrorLogLevel -- settings that go in the main nginx config file have a "Main" prefix.
pleshakov
added
the
enhancement
|
You may also want to rename the config map key to Text for the customization readme: Thanks |
|
I renamed the config map key to |
examples/customization/README.md
Outdated
| @@ -23,6 +23,7 @@ The table below summarizes all of the options. For some of them, there are examp | |||
| | N/A | `http2` | Enables HTTP/2 in servers with SSL enabled. | `False` | | |||
| | `nginx.org/redirect-to-https` | `redirect-to-https` | Sets the 301 redirect rule based on the value of the `http_x_forwarded_proto` header on the server block to force incoming traffic to be over HTTPS. Useful when terminating SSL in a load balancer in front of the Ingress controller — see [115](https://github.com/nginxinc/kubernetes-ingress/issues/115) | `False` | | | |||
| | `ingress.kubernetes.io/ssl-redirect` | `ssl-redirect` | Sets an unconditional 301 redirect rule for all incoming HTTP traffic to force incoming traffic over HTTPS. | `True` | | | |||
| | N/A | `error-log-level` | Sets the global error log level for NGINX. [error-log-level](http://nginx.org/en/docs/ngx_core_module.html#error_log). | See the [template file](../../nginx-controller/nginx/nginx.conf.tmpl). | | | |||
There was a problem hiding this comment.
Could you possible change the description to Sets the global [error log level](http://nginx.org/en/docs/ngx_core_module.html#error_log) for NGINX. ?
Could you put notice in the default column instead of a link to the template?
nginx-controller/nginx/config.go
Outdated
| @@ -257,7 +259,9 @@ func ParseConfigMap(cfgm *api_v1.ConfigMap, nginxPlus bool) *Config { | |||
| sslDHParamFile = strings.Trim(sslDHParamFile, "\n") | |||
| cfg.MainServerSSLDHParamFileContent = &sslDHParamFile | |||
| } | |||
|
|
|||
| if MainErrorLogLevel, exists := cfgm.Data["error-log-level"]; exists { | |||
There was a problem hiding this comment.
MainErrorLogLevel -> errorLogLevel
nginx-controller/nginx/nginx.go
Outdated
| @@ -142,6 +142,7 @@ type NginxMainConfig struct { | |||
| ServerNamesHashBucketSize string | |||
| ServerNamesHashMaxSize string | |||
| LogFormat string | |||
| MainErrorLogLevel string | |||
There was a problem hiding this comment.
MainErrorLogLevel -> ErrorLogLevel
…om kubernetes config map
|
Sorry for bothering you, could you please check it again? |
|
@boranx thanks for the PR!! |
pleshakov
changed the title
Proposed changes
The log level in NGINX templates was hard coded. This PR allows you to inject the log level from K8s config map. The default value will be "notice". I also updated the example config. Tested and works well in K8s.
Checklist
Before creating a PR, run through this checklist and mark each as complete.