Navigation Menu

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

Extend usability of nginx-ingress chart #177

Merged
merged 3 commits into from Oct 1, 2018
Merged

Conversation

MoonMoon1919
Copy link
Contributor

@MoonMoon1919 MoonMoon1919 commented Oct 1, 2018

What it is

  • Extends the usability of the chart by adding in conditionals for some of the more modern nginx-ingress features

Why

  • The big why here is making this chart usable when terminating SSL at an ELB. The example on the main repo shows that when terminating SSL at the ELB level, the target port for https rules should be the http port (specified as http, not 80).
  • When terminating SSL at the ELB level, proxy protocol should be disabled, previously this chart had service.beta.kubernetes.io/aws-load-balancer-proxy-protocol: '*' hard coded as a value, I implemented a minor change in the design of the chart to include this annotation in the service so long as use-proxy-protocol is set to true (it is by default still). This annotation should not be added while terminating SSL at an ELB.
  • Upgrading the default version to 0.15.0 brings some general bug fixes to the nginx image as well as the option to change the whitelist ips for the nginx status check which comes in handy.
  • Making the port rules optional in the service (ie: no http rule) allows users to only have open what they want open at the ELB layer, if no http traffic is going through, then it shouldn't be there.

Backwards compatability

  • Outside of the upgrade to the container default version, none. I have been using 0.15.0 in multiple environments for some time now without issue.

@osterman
Copy link
Member

osterman commented Oct 1, 2018

@MoonMoon1919 just a heads up, in case you missed it (or are interested) we've updated our official helmfile to use the official nginx-ingress + our fancy 404s etc.

https://github.com/cloudposse/helmfiles/blob/master/helmfile.d/0320.nginx-ingress.yaml

Copy link
Member

@osterman osterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - defer to @goruha and @alebabai for final +2

@MoonMoon1919
Copy link
Contributor Author

Thanks @osterman, will check that out

@osterman osterman merged commit 6b928d8 into master Oct 1, 2018
@osterman osterman deleted the mmoon-update-nginx branch October 1, 2018 22:02
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.

None yet

3 participants