Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add health check location #100

Merged

Conversation

jmastr
Copy link

@jmastr jmastr commented Dec 13, 2016

Ideally, this feature should be a part of a bigger feature (the default
server) that solves #52.

For now, we add the "-health-status" parameter to the controller. If
it is present, the default server listening on port 80 with the health
check location "/nginx-health" gets added to the main nginx
configuration.

Closes #90

@pleshakov
Copy link
Contributor

@jmastr Thanks!

Any reason you chose the default type as default_type text/html;? For the response that NGINX is sending, text/plain suits better.

@jmastr jmastr force-pushed the jmastr/add_health_check_location branch from 2279a91 to 811e684 Compare December 14, 2016 08:19
@jmastr
Copy link
Author

jmastr commented Dec 14, 2016

@pleshakov It was for debugging only. I wanted to stop my browser downloading "healthy\n" all the time. I removed those lines.

@pleshakov
Copy link
Contributor

@jmastr Could you add it again as the text/plain?

Ideally, this feature should be a part of a bigger feature (the default
server) that solves nginxinc#52.

For now, we add the "-health-status" parameter to the controller. If
it is present, the default server listening on port 80 with the health
check location "/nginx-health" gets added to the main nginx
configuration.

Closes nginxinc#90
@jmastr jmastr force-pushed the jmastr/add_health_check_location branch from 811e684 to 4e5661c Compare December 14, 2016 14:51
@jmastr
Copy link
Author

jmastr commented Dec 14, 2016

@pleshakov Sure. Pushed it. But it is the default anyways, isn't it?

@pleshakov
Copy link
Contributor

@jmastr Great! The default is default_type application/octet-stream;

@jmastr
Copy link
Author

jmastr commented Dec 14, 2016

@pleshakov I am just curious, am I looking in the wrong place: http://nginx.org/en/docs/http/ngx_http_core_module.html#default_type?

@pleshakov
Copy link
Contributor

@jmastr That's correct. Sorry for the confusion. However, this default is overwritten by this line https://github.com/nginxinc/kubernetes-ingress/blob/master/nginx-controller/nginx/nginx.conf.tmpl#L16, which becomes the default :)

@jmastr
Copy link
Author

jmastr commented Dec 14, 2016

@pleshakov Thanks for the explanation. Everything looks good to me now.

@jmastr
Copy link
Author

jmastr commented Dec 14, 2016

@pleshakov btw. I am just preparing my talk about Ingress and NGINX Ingress Controller for tonight's Kubernetes meetup. Told you about it: https://www.meetup.com/Berlin-Kubernetes-Meetup/events/235968196/

@pleshakov
Copy link
Contributor

@jmastr Awesome! Good luck! :)

@pleshakov pleshakov merged commit d5c43bf into nginxinc:master Dec 14, 2016
@pleshakov
Copy link
Contributor

@jmastr How did your talk go?

@jmastr
Copy link
Author

jmastr commented Dec 19, 2016

@pleshakov It went very well. The people were pretty interested. I could have talked a lot longer about that topic. There was one guy already, who manages 100+ vhosts with the NGINX Ingress Controller. Keep up the good work!

@pleshakov
Copy link
Contributor

@jmastr Glad it went very well! thx!

@georgecrawford
Copy link

@jmastr I know this isn't yet ready for public consumption, but I did try it. I've tried adding -health-status as a command param to the container, and I see that the expected block is added to /etc/nginx/nginx.conf. But at this point, I don't see my custom settings as defined in -nginx-configmaps. However, moments, later, I see that the main config file is re-written, with my custom settings from -nginx-configmaps, but without the -health-status block. It only therefore persists if I don't set -nginx-configmaps at all. Do you agree this is a bug? Want a new ticket?

@jmastr
Copy link
Author

jmastr commented Feb 20, 2017

@georgecrawford I did not test it with configmaps indeed, so the probability that it is a bug is quite high.

@maticko maticko mentioned this pull request Sep 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants