👷 Deploy request for nginx-kubernetes-ingress pending review.

Visit the deploys page to approve it

Hi @hafe , could you please update the conflicting files?

@hafe , could you please gofumpt files that are failing validation

Thanks for the pr @hafe, @vepatel is looking at this pr now.

@hafe could you please update the branch? I am reviewing changes atm.

@hafe could you please update the branch? I am reviewing changes atm.

You mean just rebase it from main?

@hafe could you please update the branch? I am reviewing changes atm.

You mean just rebase it from main?

Yes, please rebase.

@hafe thank you again for contributing. I've tested the feature and confirm it works as expected.

input data:

kind: ConfigMap
apiVersion: v1
metadata:
  name: nginx-config
  namespace: nginx-ingress
data:
  access-log: "yslog:server=localhost:514"
  access-log-off: "false"

will results with: access_log /dev/stdout main;

kind: ConfigMap
apiVersion: v1
metadata:
  name: nginx-config
  namespace: nginx-ingress
data:
  access-log: "syslog:server=localhost:514"
  access-log-off: "false"

will results with: access_log syslog:server=localhost:514;

kind: ConfigMap
apiVersion: v1
metadata:
  name: nginx-config
  namespace: nginx-ingress
data:
  access-log: "syslog:server=localhost:514"
  access-log-off: "true"

will result with: access_log off;

If a user tries to set a value that is not valid, the log will be set to default one. NIC logs will show the error:

E20240809 10:59:06.679467       1 configmaps.go:212] Configmap nginx-ingress/nginx-config: Invalid value for key access-log: "ssyslog:server=localhost:514"

@hafe since the NIC does not handle warnings in case of invalid access_log value, could you please add information to the error log to inform user that the access_log value will be set to default (/dev/stdout main)?

Handling the error event will likely be a separate PR.

cc / @vepatel @shaun-nx

I can do that. It should really be a warning log also

@jjngx @hafe lets add it to the docs as well for clarity and since we can't report it through configmap event:

access_log value will be set to /dev/stdout main in-case user tries to configure it with location other than a syslog.

@hafe could you please also update template tests?

Not sure what you mean by that

Not sure what you mean by that

could you run make test from the root dir of the project and check if the internal/configs/version1/__snapshots__/template_test.snap changes? When you look in the file there are new lines: access_log ; that indicates invalid (from NGINX perspective) lines.

No nothing changes.

No nothing changes.

Ok, I will check what is going on with the templates and snapshots, and get back to you ASAP.

No nothing changes.

Ok, I will check what is going on with the templates and snapshots, and get back to you ASAP.

@hafe there is a missing filed AccessLog in multiple input data (config structs) in the template_test.go file in pkg internal/configs/version1. Please note that after the access_log change the MainConfig struct requires the AccessLog value to be defined, for example: AccessLog: "/dev/stdout main", as you added on line 2041.
You need to update all structs (MainConfig) that are used as input test data for all template tests. After that please run make test and check if the test template is updated.

After that please update the branch. Once again thank you for contributing! I am hoping to get your PR merged next week.

Seems to be OK now after latest updates. Thanks for the help!