Test configurations before apply

[brianehlert]

As an additional measure of safety this implementation should validate new nginx configurations before applying them.

The theory is that a temporary nginx.conf should be written and validated similar to nginx -t -c <filename>.conf
The intention of doing this is to intercept any errors that customers may introduce through the use of snippets or custom templates or other ways to extend NGINX configuration.

When customers apply configurations using a CRD we are able to provide input validation and safety through the API itself. When a customer takes advantage of the ways that we allow the capabilities to be extended beyond the limitations of the CRDs or Ingress object they can put themselves in a situation where a missing ';' at the end of a line can result in a non-operational nginx configuration and thus cause a momentary outage while they revert the changes.

Acceptance Criteria:

• write the new configuration to an isolated temporary path(?)
• use nginx to validate the configuration
• if the new configuration passes, "apply" the configuration to the running configuration
• if the new configuration fails, do nothing and report the errors back as error events against the pod (at this point the configuration has passed all validation logic, so individual resources cannot be informed)

This can potentially leave the customer in a state where configurations are not being applied and thus the pod logs need be inspected for errors. But that is a better result than allowing the customer in inadvertently breaking their applications and causing an outage due to a missing ';'

[shaun-nx]

Acceptance Criteria:

• write the new configuration to an isolated temporary path

I don't believe this is necessary.
If we write a configuration to disk in /etc/nginx/conf.d for example, NGINX will still use the old config until we reload.

Here's what I mean (Using a VirtualServer config in this example).
If we run nginx -t -c /etc/ngxin/conf.d/vs_cafe_default.conf and the config check fails, we should avoid reloading.
So same as what you said: if the new configuration fails, do nothing and report the errors back as error events against the configuration
NGINX will keep using the old valid config even though the invalid config was written.

This just means we should be able to save on writing additional temporary files.

[shaun-nx]

I think we might also be able to simplify the logic and just run nginx -t instead of nginx -t -c <filename>.conf
This will make NGINX validate all configs instead of individual configs.

[brianehlert]

The problem comes if a customer manually restarts NGINX in the pod because they didn't check the API and thus they force the new configuration to be loaded. Yes, this is a real situation in a dev/test environment. But maybe we can't accommodate that concern.

There is nothing we could do for a NIC scaling event that happens while a bad configuration is present. The new pod would have a bad configuration. In theory this new pod would not become ready. But that is not a situation we were specifically attempting to accommodate.

[jasonwilliams14]

I did some digging and testing on this with @lucacome

We currently do a nginx -s to validate the syntax. If the syntax is correct, we do a reload to load the. new conf file.

The specific lines:
https://github.com/nginxinc/kubernetes-ingress/blob/main/internal/nginx/manager.go#L302-L318

We do output the errors in the resources:

  Message:  Configuration for default/cafe was added or updated ; but was not applied: error when updating config from ConfigMap: nginx reload failed: command /usr/sbin/nginx -s reload -e stderr stdout: ""
stderr: "2023/10/02 18:57:22 [emerg] 28#28: directive \"proxy_request_buffering\" is not terminated by \";\" in /etc/nginx/conf.d/vs_default_webapp.conf:117\n"
finished with error: exit status 1

In the case above, I had two resources and manually broke one with a invalid snippet.
See below:

NAMESPACE   NAME     STATE     HOST                 IP           PORTS      AGE
default     cafe     Valid    cafe.example.com     172.18.0.3   [80,443]   11m
default     webapp   Invalid   webapp.example.com   172.18.0.3   [80,443]   14m

I was able to still send traffic to both resources because NIC did not reload the bad .conf.
If I manually kill a pod, when kubernetes respawns that pod as part of the deployment, that will break all resources.

NAMESPACE   NAME     STATE     HOST                 IP           PORTS      AGE
default     cafe     Invalid   cafe.example.com     172.18.0.3   [80,443]   11m
default     webapp   Invalid   webapp.example.com   172.18.0.3   [80,443]   14m

[brianehlert]

You are leading me to believe that this is not work that we need to spend any work on, as we already achieve this with the nginx -s which tells me that maybe the real problem here is that customers don't understand that they need to do a describe on a resource to check for events that describe configuration problems.
As well that we also emit the error in the pod logs.
But a running ingress controller would continue running with the known good configuration.

Now, as Jason outlines above, if a new pod starts up - there is no previous good configuration, so that new replica only knows the bad configuration that is from the Resource YAML. But it still emits the error in the log as it attempts to startup.

What we cannot protect customers from is a valid snippet that does not work as expected.

So maybe this issue is a "nothing burger" and we need to do a blog and video about how to troubleshoot resource and configuration problems with the project.

[brianehlert]

Turns out, this is a non-issue and there is a last attempt fail safe when applying configuration to NGINX.