Add report-status & leader-election to Helm chart #335

Dean-Coakley · 2018-08-13T10:23:28Z

Proposed changes

Add support for report-status to the Helm chart.
- externalService cli arg support
- external-status-address configmap example
Add support for leader-election to the Helm chart.

Checklist

I have read the CONTRIBUTING doc
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto master
I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

Note: I will update documentation once my changes are reviewed

isaachawley · 2018-08-14T09:24:09Z

helm-chart/templates/controller-deployment.yaml

@@ -69,6 +69,12 @@ spec:
 {{- if .Values.controller.healthStatus }}
 - -health-status
 {{- end }}
+{{- if and (.Values.controller.reportIngressStatus.enable) (.Values.controller.reportIngressStatus.externalService) }}
+ - -external-service={{ .Values.controller.reportIngressStatus.externalService }}
+{{- end }}


Here we need to add the flag -report-ingress-status.

Something like:

{{- if .Values.controller.reportIngressStatus.enable }} - -report-ingress-status {{- end }}

Without that flag, status reporting is not actually turned on.

pleshakov

Additionally,
could you make sure the following lines https://github.com/nginxinc/kubernetes-ingress/blob/master/install/rbac/rbac.yaml#L53-L59 are present in the helm RBAC? Also, could you make them depend on controller.reportIngressStatus.enable?

pleshakov · 2018-08-14T13:50:57Z

helm-chart/values-plus.yaml

@@ -14,6 +14,7 @@ controller:
 secret: # <namespace>/<secret_name>
 config:
 entries: {}
+ # external-status-address: "1.2.3.4"


please remove this line

pleshakov · 2018-08-14T13:51:08Z

helm-chart/values.yaml

@@ -9,6 +9,7 @@ controller:
 pullPolicy: IfNotPresent
 config:
 entries: {}
+ # external-status-address: "1.2.3.4"


please remove this line

pleshakov · 2018-08-14T14:04:56Z

helm-chart/README.md

@@ -81,6 +81,9 @@ Parameter | Description | Default
 `controller.useIngressClassOnly` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. | false
 `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | ""
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
+`controller.reportIngressStatus.enable` | Update the address field in the status of Ingresses resources. Requires `controller.reportIngressStatus.externalService`, or `controller.config.entries.external-status-address`. | false


-> Update the address field in the status of Ingresses resources with an external address of the Ingress controller. You must also specify the source of the external address either through an external service via controller.reportIngressStatus.externalService or the external-status-address entry in the ConfigMap via controller.config.entries.

Ok. Adding a note **Note:** controller.reportIngressStatus.externalService takes precedence if both are set.

external-status-address takes precedence

pleshakov · 2018-08-14T14:07:42Z

helm-chart/README.md

@@ -81,6 +81,9 @@ Parameter | Description | Default
 `controller.useIngressClassOnly` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. | false
 `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | ""
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
+`controller.reportIngressStatus.enable` | Update the address field in the status of Ingresses resources. Requires `controller.reportIngressStatus.externalService`, or `controller.config.entries.external-status-address`. | false
+`controller.reportIngressStatus.externalService` | Specifies the name of the service with the type LoadBalancer through which the Ingress controller pods are exposed externally. The external address of the service is used when reporting the status of Ingress resources. Requires `controller.reportIngressStatus.enable`. | nginx-ingress
+`controller.reportIngressStatus.enableLeaderElection` | Enable Leader election to avoid multiple replicas of the controller reporting the status of Ingress resources. | true


--> Enable Leader election to avoid multiple replicas of the controller reporting the status of Ingress resources. Requires controller.reportIngressStatus.enable.

pleshakov · 2018-08-14T14:10:34Z

helm-chart/templates/controller-daemonset.yaml

+{{- if .Values.controller.reportIngressStatus.enable }}
+ - -report-ingress-status
+{{- if .Values.controller.reportIngressStatus.externalService }}
+ - -external-service="{{ .Values.controller.reportIngressStatus.externalService }}"


pleshakov · 2018-08-14T14:10:52Z

helm-chart/templates/controller-deployment.yaml

+{{- if .Values.controller.reportIngressStatus.enable }}
+ - -report-ingress-status
+{{- if .Values.controller.reportIngressStatus.externalService }}
+ - -external-service="{{ .Values.controller.reportIngressStatus.externalService }}"


isaachawley · 2018-08-14T14:59:45Z

helm-chart/README.md

@@ -81,6 +81,9 @@ Parameter | Description | Default
 `controller.useIngressClassOnly` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. | false
 `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | ""
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
+`controller.reportIngressStatus.enable` | Update the address field in the status of Ingresses resources. Requires `controller.reportIngressStatus.externalService`, or `controller.config.entries.external-status-address` to be set. **Note:** `controller.reportIngressStatus.externalService` takes preference if both are set. | false


controller.config.entries.external-status-address takes precedence if both are set.

isaachawley · 2018-08-14T15:02:21Z

helm-chart/README.md

@@ -81,6 +81,9 @@ Parameter | Description | Default
 `controller.useIngressClassOnly` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. | false
 `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | ""
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
+`controller.reportIngressStatus.enable` | Update the address field in the status of Ingresses resources. Requires `controller.reportIngressStatus.externalService`, or `controller.config.entries.external-status-address` to be set. **Note:** `controller.reportIngressStatus.externalService` takes preference if both are set. | false
+`controller.reportIngressStatus.externalService` | Specifies the name of the service with the type LoadBalancer through which the Ingress controller pods are exposed externally. The external address of the service is used when reporting the status of Ingress resources. `controller.reportIngressStatus.enable` must be set to `true`. | nginx-ingress


I would just say "through which the Ingress controller is exposed externally" and not mention pods.

pleshakov

please rebase against the master

2 suggestions

pleshakov · 2018-08-15T17:16:54Z

helm-chart/templates/rbac.yaml

@@ -56,12 +56,14 @@ rules:
 verbs:
 - list
 - watch
+{{- if .Values.controller.reportIngressStatus.enable }}


we need get as well for Ingress resources

thought i added that weeks ago

pleshakov · 2018-08-15T17:26:51Z

helm-chart/README.md

@@ -81,6 +81,9 @@ Parameter | Description | Default
 `controller.useIngressClassOnly` | Ignore Ingress resources without the `"kubernetes.io/ingress.class"` annotation. | false
 `controller.watchNamespace` | Namespace to watch for Ingress resources. By default the Ingress controller watches all namespaces. | ""
 `controller.healthStatus` | Add a location "/nginx-health" to the default server. The location responds with the 200 status code for any request. Useful for external health-checking of the Ingress controller. | false
+`controller.reportIngressStatus.enable` | Update the address field in the status of Ingresses resources with an external address of the Ingress controller. You must also specify the source of the external address either through an external service via `controller.reportIngressStatus.externalService` or the `external-status-address` entry in the ConfigMap via `controller.config.entries`. **Note:** `controller.config.entries.external-status-address` takes precedence if both are set. | false


since by default we set controller.service.create to true, perhaps this should be enabled by default as well?

* Added report-ingress-status to both templates * Fixed leader-election to only be set when report-ingress-status is enabled

* Fix external-status-address to use example ip 1.2.3.4 * Fix externalService default to be nginx-ingress as this will be the most common default * Removed quotes from externalService in values files and moved to templates to follow existing code style

* Added documentation for report-ingress-status related values * Added documentation for leader-election

The IC only requires ingress/status update permissions if report-ingress-status is enabled.

Also add get permissions on ingress resources, needed when the status updater is retrying a status update.

Dean-Coakley added the enhancement Pull requests for new features/feature enhancements label Aug 13, 2018

Dean-Coakley self-assigned this Aug 13, 2018

Dean-Coakley requested review from isaachawley and pleshakov August 13, 2018 10:23

Dean-Coakley force-pushed the update-helm-chart branch from d5b205d to 85749e4 Compare August 13, 2018 10:30

isaachawley suggested changes Aug 14, 2018

View reviewed changes

Dean-Coakley force-pushed the update-helm-chart branch 4 times, most recently from ec60483 to 029f666 Compare August 14, 2018 13:44

pleshakov suggested changes Aug 14, 2018

View reviewed changes

Dean-Coakley force-pushed the update-helm-chart branch from d302211 to 2d6df63 Compare August 14, 2018 14:40

isaachawley suggested changes Aug 14, 2018

View reviewed changes

Dean-Coakley force-pushed the update-helm-chart branch from 0b09b0b to f6245bd Compare August 14, 2018 15:59

isaachawley approved these changes Aug 14, 2018

View reviewed changes

isaachawley requested a review from peterkellydev August 15, 2018 13:36

pleshakov suggested changes Aug 15, 2018

View reviewed changes

Dean-Coakley and others added 7 commits August 16, 2018 09:37

Helm: Add report-status & leader-election support

b3a6f6e

Helm: Fix external-service cli argument templates

65391c1

Helm: Add report-ingress-status cli arg

daa9824

* Added report-ingress-status to both templates * Fixed leader-election to only be set when report-ingress-status is enabled

Helm: Update README with new options

1661f49

* Added documentation for report-ingress-status related values * Added documentation for leader-election

Helm: Fix report-status/leader-election docs

f7fea6b

Selectively enable permissions for status update

c1e723a

The IC only requires ingress/status update permissions if report-ingress-status is enabled.

isaachawley force-pushed the update-helm-chart branch from 9e3767c to 6a568b8 Compare August 16, 2018 08:37

Enable report-ingress-status by default

46500c1

Also add get permissions on ingress resources, needed when the status updater is retrying a status update.

isaachawley force-pushed the update-helm-chart branch from 6a568b8 to 46500c1 Compare August 16, 2018 08:45

pleshakov approved these changes Aug 16, 2018

View reviewed changes

isaachawley merged commit 51a5962 into master Aug 16, 2018

isaachawley deleted the update-helm-chart branch August 17, 2018 08:42

pleshakov changed the title ~~Helm: Add report-status & leader-election support~~ Add report-status & leader-election to Helm chart Nov 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add report-status & leader-election to Helm chart #335

Add report-status & leader-election to Helm chart #335

Dean-Coakley commented Aug 13, 2018 •

edited

Loading

isaachawley Aug 14, 2018

pleshakov left a comment

pleshakov Aug 14, 2018

pleshakov Aug 14, 2018

pleshakov Aug 14, 2018

Dean-Coakley Aug 14, 2018

isaachawley Aug 14, 2018

pleshakov Aug 14, 2018

pleshakov Aug 14, 2018

pleshakov Aug 14, 2018

isaachawley Aug 14, 2018

isaachawley Aug 14, 2018

pleshakov left a comment

pleshakov Aug 15, 2018

isaachawley Aug 16, 2018

pleshakov Aug 15, 2018

Add report-status & leader-election to Helm chart #335

Add report-status & leader-election to Helm chart #335

Conversation

Dean-Coakley commented Aug 13, 2018 • edited Loading

Proposed changes

Checklist

Choose a reason for hiding this comment

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleshakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dean-Coakley commented Aug 13, 2018 •

edited

Loading