[nginx-ingress-controller]: Check for errors in nginx template #1450
Conversation
| @@ -67,7 +67,9 @@ func NewManager(kubeClient *client.Client) *Manager { | |||
|
|
|||
| ngx.sslDHParam = ngx.SearchDHParamFile(config.SSLDirectory) | |||
|
|
|||
| ngx.loadTemplate() | |||
| if err := ngx.loadTemplate(); err != nil { | |||
| glog.Fatalf("invalid NGINX template: %v", err) | |||
There was a problem hiding this comment.
do you really want to fatal when a load fails? isn't this different from what nginx usually does? meanign if you have a working config and you overwrite it with some invalid config, nginx will keep working with the old config right?
There was a problem hiding this comment.
I'm specifically asking about what we should do if someone modifies the template to have a syntax error. I think the answer is we should do whatever nginx would do if someone modified the nginx.conf to have a syntax error.
There was a problem hiding this comment.
do you really want to fatal when a load fails?
Only the first time is loaded (in case someone uses a custom template)
I think the answer is we should do whatever nginx would do if someone modified the nginx.conf to have a syntax error.
If the template cannot be loaded the pod will fail the health checks (crashloop) and the log doesn't help (check the output of 1448).
There was a problem hiding this comment.
so we don't watch the custom template? failing on first failure to load is fine, I'm more concerned with what happens if i:
- kubectl exec and overwrite template
- endpoints change
do we crashloop, or keep using the old template (sounds like we keep using the old template, meaning we ignore updates to the config map. that's fine as long as it's clear in the doc)?
There was a problem hiding this comment.
so we don't watch the custom template?
nop, we keep using the old template.
I'll add a watch to the template file to cover:
- kubectl exec and overwrite template
- update in the config map (mounted template)
if there's an error log the error and keep using the working version of the template
|
LGTM assume you're going to make proposed changes in a follow up pr |
…te-errors [nginx-ingress-controller]: Check for errors in nginx template
fixes #1448