[NEW] nginx-ingress #399

mgoodness · 2017-01-14T22:39:59Z

This is a stand-alone chart for the nginx ingress controller that, along with the kube-lego chart (#397), replaces nginx-lego. As mentioned and (briefly) discussed in #385.

@jackzampolin

k8s-ci-robot · 2017-01-14T22:40:00Z

Hi @mgoodness. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jackzampolin · 2017-01-15T23:46:48Z

@mgoodness This looks great! I'd be happy to stay on as the maintainer! 😉 It's much better as two charts. I'll pull this down and give it a test.

viglesiasce · 2017-01-23T19:41:33Z

Tested this over the weekend without any issues.

Will wait for @jackzampolin to give the LGTM

bacongobbler · 2017-01-30T21:45:48Z

ping @jackzampolin, does this have your LGTM?

jackzampolin · 2017-01-30T23:26:25Z

@bacongobbler Got to this today. LGTM. Sorry for the delay there.

prydonius · 2017-01-30T23:46:44Z

This is the first time we're changing the name/deprecating a chart, since nginx-lego will stay in the repo, is there something we can do to let people know it's deprecated? e.g. we could push a final version that explains it's moved to nginx-ingress in the NOTES.txt?

jackzampolin · 2017-01-31T00:19:19Z

@prydonius I don't know how many users it had. In this particular case there was an issue (my b) with the other chart that prevented it from deploying successfully.

It would be nice to do in the future but I think it might be overkill for this particular instance.

jason-riddle · 2017-02-01T00:23:40Z

I ran into a small hiccup in regards to the instructions.

export NODE_PORT=$(kubectl --namespace default get services -o jsonpath="{.spec.ports[0].nodePort}" silly-aardvark-nginx-ingress-controller)

export NODE_IP=$(kubectl --namespace default get nodes -o jsonpath="{.items[0].status.addresses[0].address}")

echo "Visit http://$NODE_IP:$NODE_PORT to access your application."

```
  services:
    - name: controller
      # annotations:
      type: NodePort
```

~~So after I created my ingress, I queried it and it appear to has the correct external facing ip.~~

Update below.

jason-riddle · 2017-02-01T00:27:11Z

Update: I mispoke. The problem for me was

kubectl --namespace default get nodes -o jsonpath="{.items[0].status.addresses[0].address}"

should be

kubectl --namespace default get nodes -o jsonpath="{.items[0].status.addresses[1].address}"

so

status.addresses[1]
# instead of status.addresses[0]

prydonius · 2017-02-01T02:41:35Z

@jason-riddle it appears that a lot of charts use this in their NOTES, can you create a separate issue for it?

jason-riddle · 2017-02-01T02:57:42Z

sure

munnerz · 2017-02-03T11:32:44Z

Hey @mgoodness - I've just opened #540, and then gone on to spot this PR :)

You seem to have put more into documenting so think it's best we stick with your PR, but may I request you add support for TCP & UDP configmaps into yours too? You can see an example of how I've done it in the above linked PR. I'm happy to do it myself, but will need to wait until after this has merged if so!

viglesiasce · 2017-02-03T20:07:16Z

@k8s-bot e2e test this

viglesiasce · 2017-02-03T20:18:32Z

stable/nginx-ingress/templates/controller-service.yaml

@@ -0,0 +1,29 @@
+{{- $root := . -}}
+{{- range .Values.controller.services }}
+---


Tests are failing because of this. Do we have to loop in this file? When would I have multiple? Can we start with this hardcoded to 1 service?

My specific use-case is to have the same nginx-ingress controllers exposed via two different ELBs (one internet-facing and one VPC-internal). But now that I think about it, that shouldn't be strictly necessary. I'll play with it over the weekend.

mgoodness · 2017-02-15T07:23:30Z

After further discussion with @prydonius, we've decided not to delete nginx-lego as part of this PR. Instead, I'll create another in which the README and NOTES.txt make it clear that it's been deprecated. There's also work planned in the Helm project to support/enforce deprecation.

prydonius · 2017-02-16T21:22:20Z

stable/nginx-ingress/Chart.yaml

+name: nginx-ingress
+version: 0.3.0
+description: An nginx Ingress controller that uses ConfigMap to store the nginx configuration.
+keywords:


Can we add an icon here? e.g. pointing to https://cdn.wp.nginx.com/wp-content/themes/nginx-theme/assets/img/logo.png?

prydonius · 2017-02-23T01:31:37Z

stable/nginx-ingress/README.md

+`controller.stats.service.externalIPs` | controller service stats external IP addresses | `[]`
+`controller.stats.service.loadBalancerIP` | IP address to assign to load balancer (if supported) | `""`
+`controller.stats.service.loadBalancerSourceRanges` | list of IP CIDRs allowed access to load balancer (if supported) | `[]`
+`controller.service.type` | type of controller stats service to create | `ClusterIP`


should this be controller.stats.service.type?

prydonius · 2017-02-23T01:33:06Z

stable/nginx-ingress/README.md

+`defaultBackend.service.externalIPs` | default backend service external IP addresses | `[]`
+`defaultBackend.service.loadBalancerIP` | IP address to assign to load balancer (if supported) | `""`
+`defaultBackend.service.loadBalancerSourceRanges` | list of IP CIDRs allowed access to load balancer (if supported) | `[]`
+`controller.service.type` | type of default backend service to create | `ClusterIP`


defaultBackend.service.type?

prydonius · 2017-02-23T01:39:57Z

@mgoodness just some small fixes in the README, otherwise this lgtm!

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 14, 2017

lachie83 added the awaiting review label Jan 16, 2017

jackzampolin mentioned this pull request Jan 23, 2017

[nginx-lego] Trim '-' suffix from resource names #339

Closed

viglesiasce added code reviewed UX reviewed labels Jan 23, 2017

mgoodness mentioned this pull request Jan 25, 2017

Added loadBalancerIP as a configuration option in nginx-lego #513

Closed

jackzampolin mentioned this pull request Jan 30, 2017

[NEW] kube-lego #397

Merged

prydonius added lgtm Indicates that a PR is ready to be merged. and removed awaiting review labels Jan 30, 2017

jason-riddle mentioned this pull request Feb 1, 2017

NODE_IP in certain charts is set to the internal ip address #531

Closed

munnerz mentioned this pull request Feb 3, 2017

Add nginx-ingress chart #540

Closed

viglesiasce modified the milestone: Milestone 1 Feb 3, 2017

viglesiasce previously requested changes Feb 3, 2017

View reviewed changes

viglesiasce added changes needed and removed lgtm Indicates that a PR is ready to be merged. labels Feb 3, 2017

Michael Goodness added 4 commits February 14, 2017 12:47

Fixed metrics servicePort

a434b19

Use templates for fullnames

61497b9

Renamed enableStats -> stats.enabled

920205e

Deprecate nginx-lego, not delete

ad2a9fb

lachie83 modified the milestones: Milestone 2, Milestone 3 Feb 15, 2017

andrewhowdencom mentioned this pull request Feb 16, 2017

Deploying applications with Prometheus sidecars #663

Closed

mgoodness mentioned this pull request Feb 16, 2017

stable/nginx-lego doesn't use its own resource configuration for the kube-lego container #664

Closed

prydonius reviewed Feb 16, 2017

View reviewed changes

Michael Goodness added 3 commits February 16, 2017 16:43

Added icon

1b8aa51

Use nginx.org logo

d11422d

No default resources

a12e4ce

prydonius reviewed Feb 23, 2017

View reviewed changes

prydonius added changes needed UX reviewed and removed awaiting review labels Feb 23, 2017

PR feedback

5089534

prydonius added code reviewed lgtm Indicates that a PR is ready to be merged. and removed changes needed labels Feb 23, 2017

mgoodness merged commit eae290a into helm:master Feb 23, 2017

mgoodness deleted the nginx-ingress branch February 23, 2017 02:25

mgoodness mentioned this pull request Feb 23, 2017

feature(nginx-lego): allow customizing annotations for nginx service #721

Closed

yanns pushed a commit to yanns/charts that referenced this pull request Jul 28, 2017

[NEW] nginx-ingress (helm#399)

d06015f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] nginx-ingress #399

[NEW] nginx-ingress #399

mgoodness commented Jan 14, 2017

k8s-ci-robot commented Jan 14, 2017

jackzampolin commented Jan 15, 2017

viglesiasce commented Jan 23, 2017

bacongobbler commented Jan 30, 2017

jackzampolin commented Jan 30, 2017

prydonius commented Jan 30, 2017

jackzampolin commented Jan 31, 2017

jason-riddle commented Feb 1, 2017 •

edited

Loading

jason-riddle commented Feb 1, 2017

prydonius commented Feb 1, 2017

jason-riddle commented Feb 1, 2017

munnerz commented Feb 3, 2017

viglesiasce commented Feb 3, 2017

viglesiasce Feb 3, 2017

mgoodness Feb 3, 2017

mgoodness commented Feb 15, 2017

prydonius Feb 16, 2017

prydonius Feb 23, 2017

prydonius Feb 23, 2017

prydonius commented Feb 23, 2017

[NEW] nginx-ingress #399

[NEW] nginx-ingress #399

Conversation

mgoodness commented Jan 14, 2017

k8s-ci-robot commented Jan 14, 2017

jackzampolin commented Jan 15, 2017

viglesiasce commented Jan 23, 2017

bacongobbler commented Jan 30, 2017

jackzampolin commented Jan 30, 2017

prydonius commented Jan 30, 2017

jackzampolin commented Jan 31, 2017

jason-riddle commented Feb 1, 2017 • edited Loading

jason-riddle commented Feb 1, 2017

prydonius commented Feb 1, 2017

jason-riddle commented Feb 1, 2017

munnerz commented Feb 3, 2017

viglesiasce commented Feb 3, 2017

viglesiasce Feb 3, 2017

Choose a reason for hiding this comment

mgoodness Feb 3, 2017

Choose a reason for hiding this comment

mgoodness commented Feb 15, 2017

prydonius Feb 16, 2017

Choose a reason for hiding this comment

prydonius Feb 23, 2017

Choose a reason for hiding this comment

prydonius Feb 23, 2017

Choose a reason for hiding this comment

prydonius commented Feb 23, 2017

jason-riddle commented Feb 1, 2017 •

edited

Loading