[stable/nginx-ingress]: add option to specify the scope #775

sdomula · 2017-03-10T13:54:54Z

option to enable scoped ingress contoller (defaults to the namespace of the helm release)
option to specify a namespace

* option to enable scoped ingress contoller (defaults to the namespace of the helm release * option to specify a namespace

k8s-ci-robot · 2017-03-10T13:54:55Z

Hi @sdomula. 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.

k8s-ci-robot · 2017-03-10T13:54:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

mgoodness

Just a couple style requests. Otherwise LGTM.

mgoodness · 2017-03-10T14:45:09Z

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

@@ -31,6 +31,9 @@ spec:
            - --nginx-configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }}
            - --tcp-services-configmap={{ .Release.Namespace }}/{{ template "fullname" . }}-tcp
            - --udp-services-configmap={{ .Release.Namespace }}/{{ template "fullname" . }}-udp
+          {{- if eq .Values.controller.scope.enabled true }}


Just checking a bool here, so {{- if .Values.controller.scope.enabled }} will suffice.

mgoodness · 2017-03-10T14:45:45Z

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

@@ -32,6 +32,9 @@ spec:
            - --nginx-configmap={{ .Release.Namespace }}/{{ template "controller.fullname" . }}
            - --tcp-services-configmap={{ .Release.Namespace }}/{{ template "fullname" . }}-tcp
            - --udp-services-configmap={{ .Release.Namespace }}/{{ template "fullname" . }}-udp
+          {{- if eq .Values.controller.scope.enabled true }}


As above, this conditional can be simplified.

mgoodness · 2017-03-10T15:08:09Z

@sdomula We just need the CLA and it'll get merged. Thanks for the contribution!

sdomula · 2017-03-10T15:17:14Z

I need to wait until the CLA page is up again. I posted the issue here: kubernetes/kubernetes#27796 (comment)

sdomula · 2017-03-10T17:23:36Z

I signed it!

chancez · 2017-03-10T20:30:36Z

Why not just use controller.extraArgs? This seems reasonable, but seems a bit unnecessary perhaps.

chancez · 2017-03-10T20:31:08Z

stable/nginx-ingress/README.md

@@ -51,6 +51,8 @@ Parameter | Description | Default
 `controller.image.pullPolicy` | controller container image pull policy | `IfNotPresent`
 `controller.config` | nginx ConfigMap entries | none
 `controller.defaultBackendService` | default 404 backend service; required only if `defaultBackend.enabled = false` | `""`
+`controller.scope.enabled` | limit the scope of the ingress controller | `false` (watch all namespaces)


This is unnecessary, just set the default for controller.scope.namespace to all. This is the default behavior for the controller already, no need to have conditionals.

My goal was to have all as the default (same behavior as current master) and add the option to set it to {{ .Release.Namespace }}. Maybe you can point me to a better solution. I have not figured out a better way to achieve that.

Is there a way to override variables of a chart dependency with templates?

chancez · 2017-03-10T20:31:29Z

stable/nginx-ingress/README.md

@@ -51,6 +51,8 @@ Parameter | Description | Default
 `controller.image.pullPolicy` | controller container image pull policy | `IfNotPresent`
 `controller.config` | nginx ConfigMap entries | none
 `controller.defaultBackendService` | default 404 backend service; required only if `defaultBackend.enabled = false` | `""`
+`controller.scope.enabled` | limit the scope of the ingress controller | `false` (watch all namespaces)
+`controller.scope.namespace` | namespace to watch for ingress | `""` (use the release namespace)


Why not just controller.watchNamespace?

My intention was to add the possibility to limit the scope of the ingress controller to the namespace of the helm chart release, to avoid hard coding the namespace, e.g, in a wrapper helm chart.

chancez · 2017-03-14T22:22:45Z

As a note: nginx-ingress got another pre-release 0.9.0-beta.3. However, it has an issue, as described here: kubernetes/ingress-nginx#443. I believe the existing chart behavior of specifying the configmaps I probably correct (or at least, its not wrong), but it will cause usage of this chart with 0.9.0-beta.3 to result in a crashing looping ingress controller, as it will error when it cannot find the tcp/udp service configmaps.

This PR need not be updated for the above issue, but just a heads up if you try the new image, you'll run into this.

seanknox · 2017-03-17T05:27:24Z

Thank you @sdomula!

* stable/nginx-ingress: add option to specify the scope * option to enable scoped ingress contoller (defaults to the namespace of the helm release * option to specify a namespace * simplify bool check * Version bump

stable/nginx-ingress: add option to sepcify the scope

08b163e

* option to enable scoped ingress contoller (defaults to the namespace of the helm release * option to specify a namespace

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Mar 10, 2017

mgoodness approved these changes Mar 10, 2017

View reviewed changes

mgoodness added the changes needed label Mar 10, 2017

mgoodness self-assigned this Mar 10, 2017

mgoodness changed the title ~~stable/nginx-ingress: add option to sepcify the scope~~ [stable/nginx-ingress]: add option to specify the scope Mar 10, 2017

simplify bool check

a3996a7

mgoodness added code reviewed UX reviewed lgtm Indicates that a PR is ready to be merged. and removed changes needed labels Mar 10, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 10, 2017

mgoodness removed their assignment Mar 10, 2017

Version bump

a809132

chancez reviewed Mar 10, 2017

View reviewed changes

mgoodness removed the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2017

mgoodness self-assigned this Mar 10, 2017

seanknox merged commit d120384 into helm:master Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stable/nginx-ingress]: add option to specify the scope #775

[stable/nginx-ingress]: add option to specify the scope #775

sdomula commented Mar 10, 2017

k8s-ci-robot commented Mar 10, 2017

k8s-ci-robot commented Mar 10, 2017

mgoodness left a comment

mgoodness Mar 10, 2017

mgoodness Mar 10, 2017

mgoodness commented Mar 10, 2017

sdomula commented Mar 10, 2017

sdomula commented Mar 10, 2017

chancez commented Mar 10, 2017 •

edited

Loading

chancez Mar 10, 2017 •

edited

Loading

sdomula Mar 11, 2017 •

edited

Loading

chancez Mar 10, 2017

sdomula Mar 10, 2017 •

edited

Loading

chancez commented Mar 14, 2017 •

edited

Loading

seanknox commented Mar 17, 2017

[stable/nginx-ingress]: add option to specify the scope #775

[stable/nginx-ingress]: add option to specify the scope #775

Conversation

sdomula commented Mar 10, 2017

k8s-ci-robot commented Mar 10, 2017

k8s-ci-robot commented Mar 10, 2017

mgoodness left a comment

Choose a reason for hiding this comment

mgoodness Mar 10, 2017

Choose a reason for hiding this comment

mgoodness Mar 10, 2017

Choose a reason for hiding this comment

mgoodness commented Mar 10, 2017

sdomula commented Mar 10, 2017

sdomula commented Mar 10, 2017

chancez commented Mar 10, 2017 • edited Loading

chancez Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

sdomula Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

chancez Mar 10, 2017

Choose a reason for hiding this comment

sdomula Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

chancez commented Mar 14, 2017 • edited Loading

seanknox commented Mar 17, 2017

chancez commented Mar 10, 2017 •

edited

Loading

chancez Mar 10, 2017 •

edited

Loading

sdomula Mar 11, 2017 •

edited

Loading

sdomula Mar 10, 2017 •

edited

Loading

chancez commented Mar 14, 2017 •

edited

Loading