Document nginx controller configuration tweaks #132

pedrosland · 2017-01-15T23:44:59Z

This PR contains many small tweaks to the nginx controller configuration documentation.

Some of these changes are perhaps personal taste. I am happy to change or to delete entirely.

I have another PR to follow for config map additions but I think this is un-reviewable enough.

k8s-reviewable · 2017-01-15T23:45:05Z

This change is

coveralls · 2017-01-16T00:02:50Z

Coverage increased (+0.04%) to 42.563% when pulling b81a49c on pedrosland:docs/nginx-controller-config-1 into 2eb9ab5 on kubernetes:master.

cmluciano · 2017-01-16T19:00:14Z

controllers/nginx/configuration.md

-**Please note the template is tied to the go code. Be sure to no change names in the variable `$cfg`**
+**Please note the template is tied to the Go code. Do not change names in the variable `$cfg`.**
+
+To know more about the template syntax please check the [Go template package](https://golang.org/pkg/text/template/).


"For more information about"

cmluciano · 2017-01-16T19:01:36Z

controllers/nginx/configuration.md


-To use custom values in an Ingress rule define this annotations:
+**With the default values NGINX will not health check your backends, and whenever the endpoints controller notices a readiness probe failure that pod's IP will be removed from the list of endpoints, causing nginx to also remove it from the upstreams.**


This sentence should probably be split up

cmluciano · 2017-01-16T19:03:32Z

controllers/nginx/configuration.md


+**Important:**
+The upstreams are shared. All Ingress rules using the same service will use the same upstream. This means only one of the rules should define annotations to configure the upstream servers.


I know that this existed before, but it would be helpful to expand on "the upstreams".

cmluciano · 2017-01-16T19:06:13Z

controllers/nginx/configuration.md

-Name of the secret that contains the usernames and passwords with access to the `path/s` defined in the Ingress Rule.
-The secret must be created in the same namespace than the Ingress rule
+The name of the secret that contains the usernames and passwords with access to the `path`'s defined in the Ingress Rule.
+The secret must be created in the same namespace than the Ingress rule.


Is this meant to say "as the" instead of "than the" ? Or is this implying ordering?

cmluciano · 2017-01-16T19:07:23Z

controllers/nginx/configuration.md



 ### Rate limiting

-The annotations `ingress.kubernetes.io/limit-connections` and `ingress.kubernetes.io/limit-rps` allows the creation of a limit in the connections that can be opened by a single client IP address. This can be use to mitigate [DDoS Attacks](https://www.nginx.com/blog/mitigating-ddos-attacks-with-nginx-and-nginx-plus)
+The annotations `ingress.kubernetes.io/limit-connections` and `ingress.kubernetes.io/limit-rps` allows the creation of a limit in the connections that can be opened by a single client IP address. This can be used to mitigate [DDoS Attacks](https://www.nginx.com/blog/mitigating-ddos-attacks-with-nginx-and-nginx-plus).


"allow the creation"

cmluciano · 2017-01-16T19:07:57Z

controllers/nginx/configuration.md


-Please check the [whitelist](examples/whitelist/README.md) example
+*Note:* adding an annotation overrides any global restriction.


Capitalize Adding

cmluciano · 2017-01-16T19:10:07Z

controllers/nginx/configuration.md


-#### Retries in no idempotent methods
+#### Retries in non idempotent methods


pedrosland · 2017-01-17T14:52:01Z

controllers/nginx/configuration.md

-The upstreams are shared. All Ingress rules using the same service will use the same upstream. This means only one of the rules should define annotations to configure the upstream servers.
+In NGINX, backend server pools are called "[upstreams](http://nginx.org/en/docs/http/ngx_http_upstream_module.html)". Each upstream contains the endpoints for a service. An upstream is created for each service that has Ingress rules defined.
+
+**Important:** All Ingress rules using the same service will use the same upstream. Only one of the Ingress rules should define annotations to configure the upstream servers.


Is this the right idea?

pedrosland · 2017-01-17T14:52:18Z

Thanks for the feedback.

coveralls · 2017-01-17T15:04:48Z

Coverage increased (+0.02%) to 42.543% when pulling 9e94863 on pedrosland:docs/nginx-controller-config-1 into 2eb9ab5 on kubernetes:master.

euank · 2017-01-17T17:28:43Z

controllers/nginx/configuration.md

@@ -9,27 +9,27 @@
 * [Rate limiting](#rate-limiting)
 * [Secure backends](#secure-backends)
 * [Whitelist source range](#whitelist-source-range)
-* [Allowed parameters in configuration config map](#allowed-parameters-in-configuration-configmap)
+* [Allowed parameters in configuration config map](#allowed-parameters-in-configuration-config-map)


nit, the text could read "ConfigMap" which is what the k8s docs typically call it.

euank · 2017-01-17T17:28:58Z

controllers/nginx/configuration.md


-1. config map: create a stand alone config map, use this if you want a different global configuration
-2. annotations: [annotate the ingress](#annotations), use this if you want a specific configuration for the site defined in the Ingress rule
+1. [config map](#allowed-parameters-in-configuration-config-map): create a stand alone config map, use this if you want a different global configuration


Same nit as above about ConfigMap.

euank · 2017-01-17T23:20:33Z

controllers/nginx/configuration.md

@@ -110,49 +110,47 @@ Indicates the [HTTP Authentication Type: Basic or Digest Access Authentication](
 ingress.kubernetes.io/auth-secret:secretName
 ```

-Name of the secret that contains the usernames and passwords with access to the `path/s` defined in the Ingress Rule.
-The secret must be created in the same namespace than the Ingress rule
+The name of the secret that contains the usernames and passwords with access to the `path`'s defined in the Ingress Rule.


nit, I prefer paths to path's.

euank · 2017-01-17T23:21:26Z

controllers/nginx/configuration.md

@@ -110,49 +110,47 @@ Indicates the [HTTP Authentication Type: Basic or Digest Access Authentication](
 ingress.kubernetes.io/auth-secret:secretName


nit, space after :

euank · 2017-01-17T23:22:08Z

controllers/nginx/configuration.md



 ### External Authentication

 To use an existing service that provides authentication the Ingress rule can be annotated with `ingress.kubernetes.io/auth-url` to indicate the URL where the HTTP request should be sent.
-Additionally is possible to set `ingress.kubernetes.io/auth-method` to specify the HTTP method to use (GET or POST) and `ingress.kubernetes.io/auth-send-body` to true or false (default).
+Additionally it is possible to set `ingress.kubernetes.io/auth-method` to specify the HTTP method to use (GET or POST) and `ingress.kubernetes.io/auth-send-body` to true or false (default).

 ```
 ingress.kubernetes.io/auth-url:"URL to the authentication service"


nit, space after :

euank · 2017-01-17T23:34:26Z

controllers/nginx/configuration.md


 TLSv1 is enabled to allow old clients like:
 - [IE 8-10 / Win 7](https://www.ssllabs.com/ssltest/viewClient.html?name=IE&version=8-10&platform=Win%207&key=113)
 - [Java 7u25](https://www.ssllabs.com/ssltest/viewClient.html?name=Java&version=7u25&key=26)

-If you dont need to support this clients please remove TLSv1
+If you dont need to support this clients please remove TLSv1.


nit, 'this clients'

euank · 2017-01-17T23:36:15Z

controllers/nginx/configuration.md


-**use-http2:** Enables or disables the [HTTP/2](http://nginx.org/en/docs/http/ngx_http_v2_module.html) support in secure connections 
+**use-http2:** Enables or disables the [HTTP/2](http://nginx.org/en/docs/http/ngx_http_v2_module.html) support in secure connections.


euank · 2017-01-17T23:36:50Z

controllers/nginx/configuration.md



-**gzip-types:** Sets the MIME types in addition to "text/html" to compress. The special value "*"" matches any MIME type.
-Responses with the "text/html" type are always compressed if `use-gzip` is enabled
+**use-proxy-protocol:** Enables or disables the use of the [PROXY protocol](https://www.nginx.com/resources/admin-guide/proxy-protocol/) to receive client connection (real IP address) information passed through proxy servers and load balancers such as HAproxy and Amazon Elastic Load Balancer (ELB).


s/the use of the/use of the/

euank · 2017-01-17T23:37:16Z

controllers/nginx/configuration.md



-**worker-processes:** Sets the number of [worker processes](http://nginx.org/en/docs/ngx_core_module.html#worker_processes). By default "auto" means number of available CPU cores
+**worker-processes:** Sets the number of [worker processes](http://nginx.org/en/docs/ngx_core_module.html#worker_processes). By default "auto" means number of available CPU cores.


/By default/The default of

euank · 2017-01-17T23:38:46Z

controllers/nginx/configuration.md

@@ -340,22 +342,23 @@ The next table shows the options, the default value and a description
 |use-gzip|"true"|
 |use-http2|"true"|
 |vts-status-zone-size|10m|
-|worker-processes|<number of CPUs>|
+|worker-processes|number of CPUs|


maybe something like "$(nproc)" would be clearer, since it isn't literally being set to 'number of CPUs'.

I'm not sure the best option.

I'm not sure about these either. It was on my list to ask about for later.

I have a couple more changes in the pipeline and I don't see a clear way to express:

|ssl-dh-param|value provided by OpenSSL|

Your suggestion would fix this case though and perhaps that is good enough for now? Another idea is to simply write < instead of <.

Some of these values are quoted but others are not. Perhaps we could quote the items to be taken literally or </> the values that are not?

@pedrosland @euank I think we can leave this change for a different PR

euank · 2017-01-17T23:41:42Z

I left some nits, but overall looks much cleaner, thanks a ton! ✏️ ✏️ ✏️

pedrosland · 2017-01-19T18:06:10Z

@euank Thanks for the feedback. I've changed all of these except for this which I'm not sure about. Perhaps we should discuss this and leave it for another PR.

aledbf · 2017-01-19T18:07:32Z

/lgtm

aledbf · 2017-01-19T18:07:50Z

@pedrosland thanks!

coveralls · 2017-01-19T18:21:38Z

Coverage increased (+0.8%) to 43.285% when pulling 292375e on pedrosland:docs/nginx-controller-config-1 into 2eb9ab5 on kubernetes:master.

pedrosland added 2 commits January 15, 2017 23:38

Many mostly small tweaks to the documentation

727eb2c

Sort config map params alphabetically

b81a49c

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

cmluciano reviewed Jan 16, 2017

View reviewed changes

Incorporate feedback

9e94863

pedrosland commented Jan 17, 2017

View reviewed changes

euank reviewed Jan 17, 2017

View reviewed changes

chentao1596 mentioned this pull request Jan 18, 2017

Add content and descriptions about nginx's configuration #136

Merged

Incorporate more feedback

292375e

aledbf self-assigned this Jan 19, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2017

aledbf merged commit fbcedc0 into kubernetes:master Jan 19, 2017

chentao1596 mentioned this pull request Jan 20, 2017

fix wrong links and punctuations #145

Merged

haoqing0110 pushed a commit to stolostron/management-ingress that referenced this pull request Mar 5, 2021

update copyright statement (kubernetes#132)

7bcb737

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document nginx controller configuration tweaks #132

Document nginx controller configuration tweaks #132

pedrosland commented Jan 15, 2017 •

edited

Loading

k8s-reviewable commented Jan 15, 2017

coveralls commented Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

cmluciano Jan 16, 2017

pedrosland Jan 17, 2017

pedrosland commented Jan 17, 2017

coveralls commented Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

euank Jan 17, 2017

pedrosland Jan 18, 2017

aledbf Jan 19, 2017

euank commented Jan 17, 2017 •

edited

Loading

pedrosland commented Jan 19, 2017

aledbf commented Jan 19, 2017

aledbf commented Jan 19, 2017

coveralls commented Jan 19, 2017


		To use custom values in an Ingress rule define this annotations:
		With the default values NGINX will not health check your backends, and whenever the endpoints controller notices a readiness probe failure that pod's IP will be removed from the list of endpoints, causing nginx to also remove it from the upstreams.


		Important:
		The upstreams are shared. All Ingress rules using the same service will use the same upstream. This means only one of the rules should define annotations to configure the upstream servers.


		Please check the [whitelist](examples/whitelist/README.md) example
		Note: adding an annotation overrides any global restriction.


		#### Retries in no idempotent methods
		#### Retries in non idempotent methods

		@@ -110,49 +110,47 @@ Indicates the [HTTP Authentication Type: Basic or Digest Access Authentication](
		ingress.kubernetes.io/auth-secret:secretName


		use-http2: Enables or disables the [HTTP/2](http://nginx.org/en/docs/http/ngx_http_v2_module.html) support in secure connections
		use-http2: Enables or disables the [HTTP/2](http://nginx.org/en/docs/http/ngx_http_v2_module.html) support in secure connections.



		worker-processes: Sets the number of [worker processes](http://nginx.org/en/docs/ngx_core_module.html#worker_processes). By default "auto" means number of available CPU cores
		worker-processes: Sets the number of [worker processes](http://nginx.org/en/docs/ngx_core_module.html#worker_processes). By default "auto" means number of available CPU cores.

Document nginx controller configuration tweaks #132

Document nginx controller configuration tweaks #132

Conversation

pedrosland commented Jan 15, 2017 • edited Loading

k8s-reviewable commented Jan 15, 2017

coveralls commented Jan 16, 2017

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

pedrosland commented Jan 17, 2017

coveralls commented Jan 17, 2017

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

Choose a reason for hiding this comment

euank commented Jan 17, 2017 • edited Loading

pedrosland commented Jan 19, 2017

aledbf commented Jan 19, 2017

aledbf commented Jan 19, 2017

coveralls commented Jan 19, 2017

pedrosland commented Jan 15, 2017 •

edited

Loading

euank commented Jan 17, 2017 •

edited

Loading