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

Document how the NGINX Ingress controller build nginx.conf #2479

Merged
merged 1 commit into from
Jun 4, 2018

Conversation

aledbf
Copy link
Member

@aledbf aledbf commented May 7, 2018

Which issue this PR fixes:

fixes #1943

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 7, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 7, 2018
Copy link

@diazjf diazjf left a comment

Choose a reason for hiding this comment

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

Looks Really Good! Just a few comments!


## NGINX configuration

The goal of this Ingress controller is the assembly of a configuration file (nginx.conf). This is something we need to do, at least until [this PR is merged in OpenResty](0). The main implication of this requirement is the need to reload NGINX after a change in the configuration file.
Copy link

Choose a reason for hiding this comment

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

We can probably exclude:

This is something we need to do, at least until [this PR is merged in OpenResty](0). The main implication of this requirement is the need to reload NGINX after a change in the configuration file.

since, the Ingress Controller is always going to assemble a config file, in any case. We can add that PR to another section detailing what it does.

## NGINX model

Usually, a Kubernetes Controllers utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster, in particular (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
To get this object from the cluster we use [Kubernetes Informers](2), in particular `FilteredSharedInformer`. This informers allows reacting to changes in using [callbacks](3) to individual changes when a new object is added, modified or removed. Unfortunately, there is no way to know if a particular change is going to affect or not the final configuration file. This is one of the uses of the model is avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.
Copy link

Choose a reason for hiding this comment

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

The or not can be removed.

Usually, a Kubernetes Controllers utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster, in particular (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
To get this object from the cluster we use [Kubernetes Informers](2), in particular `FilteredSharedInformer`. This informers allows reacting to changes in using [callbacks](3) to individual changes when a new object is added, modified or removed. Unfortunately, there is no way to know if a particular change is going to affect or not the final configuration file. This is one of the uses of the model is avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.

The final representation of the model is generated from a Go template and the model that reflect the NGINX configuration.
Copy link

Choose a reason for hiding this comment

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

We can provide a link to the template. We can add some information about template functions as well.

Copy link

Choose a reason for hiding this comment

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

Also it would be good to add how annotations and cm changes alter the template.


Building a model is an expensive operation, for this reason, the use of the synchronization loop is a must. By using a [work queue](4) it is possible to not lose changes and remove the use of [sync.Mutex](5) to force a single execution of the sync loop and additionally it is possible to create a time window between the start and end of the sync loop that allows us to discard unnecessary updates. It is important to understand that any change in the cluster could generate events that the informer will send to the controller and one of the reasons for the [work queue](4).

Operations to build the model:
Copy link

Choose a reason for hiding this comment

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

Awesome 👍

Building a model is an expensive operation, for this reason, the use of the synchronization loop is a must. By using a [work queue](4) it is possible to not lose changes and remove the use of [sync.Mutex](5) to force a single execution of the sync loop and additionally it is possible to create a time window between the start and end of the sync loop that allows us to discard unnecessary updates. It is important to understand that any change in the cluster could generate events that the informer will send to the controller and one of the reasons for the [work queue](4).

Operations to build the model:

Copy link

Choose a reason for hiding this comment

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

If the same annotations already existed for a particular path, old ones win.


The next list describe the scenarios when a reload is required:

- New Ingress rule.
Copy link

Choose a reason for hiding this comment

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

New Ingress Resource Created*

@aledbf aledbf removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2018
@aledbf aledbf changed the title WIP: Document how the NGINX Ingress controller build nginx.conf Document how the NGINX Ingress controller build nginx.conf May 24, 2018
@codecov-io
Copy link

codecov-io commented May 24, 2018

Codecov Report

Merging #2479 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2479   +/-   ##
=======================================
  Coverage   40.63%   40.63%           
=======================================
  Files          74       74           
  Lines        5094     5094           
=======================================
  Hits         2070     2070           
  Misses       2743     2743           
  Partials      281      281

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5958c0...360e609. Read the comment docs.


## NGINX model

Usually, a Kubernetes Controllers utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster, in particular (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: extra s in "Kubernetes Controllers"

Copy link
Member

Choose a reason for hiding this comment

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

in particular (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster

this is the complete list of objects ingress-nginx watches, no? What about

To this purpose, we need to build a model using different objects from the cluster. ingress-nginx controller in particular watches (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.

## NGINX model

Usually, a Kubernetes Controllers utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster, in particular (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
To get this object from the cluster we use [Kubernetes Informers](2), in particular `FilteredSharedInformer`. This informers allows reacting to changes in using [callbacks](3) to individual changes when a new object is added, modified or removed. Unfortunately, there is no way to know if a particular change is going to affect the final configuration file. This is one of the uses of the model is avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.
Copy link
Member

Choose a reason for hiding this comment

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

s/allows/allow

Copy link
Member

Choose a reason for hiding this comment

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

reacting to changes in using callbacks to individual changes when a new

to changes in using seems to be redundant

Copy link
Member

@ElvinEfendi ElvinEfendi May 25, 2018

Choose a reason for hiding this comment

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

after

Unfortunately, there is no way to know if a particular change is going to affect the final configuration file.

maybe make it explicit that we have to rebuild a new model from scratch? something like

Therefore on every change we have to rebuild a new model from scratch based on the state of cluster and compare it to the current model. If the new model equals to the current one then we avoid generating a new Nginx configuration and reloading workers. Otherwise we generate a new Nginx configuration based on the new model, replace the current model and reload Nginx workers.

## NGINX model

Usually, a Kubernetes Controller utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster. This Ingress controller in particular watches (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
To get this object from the cluster we use [Kubernetes Informers](2), in particular `FilteredSharedInformer`. This informers allow reacting [callbacks](3) to individual changes when a new object is added, modified or removed. Unfortunately, there is no way to know if a particular change is going to affect the final configuration file. This is one of the uses of the model is avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.
Copy link
Member

Choose a reason for hiding this comment

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

Please check my other comment at #2479 (comment). It was lost after your commit.

Usually, a Kubernetes Controller utilizes the [synchronization loop pattern](1) to check if the desired state in the controller is updated or a change is required. To this purpose, we need to build a model using different objects from the cluster. This Ingress controller in particular watches (in no special order) Ingresses, Services, Endpoints, Secrets, and Configmaps to generate a point in time configuration file that reflects the state of the cluster.
To get this object from the cluster we use [Kubernetes Informers](2), in particular `FilteredSharedInformer`. This informers allow reacting [callbacks](3) to individual changes when a new object is added, modified or removed. Unfortunately, there is no way to know if a particular change is going to affect the final configuration file. This is one of the uses of the model is avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.

The final representation of the model is generated from a [Go template](6) and the model that reflect the NGINX configuration.
Copy link
Member

@ElvinEfendi ElvinEfendi May 25, 2018

Choose a reason for hiding this comment

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

The final representation of the model

representation of model or the final Nginx configuration? I presumed by model you refer to the data structure that holds all the objects to render the Nginx config template.

--

s/reflect/reflects


Operations to build the model:

- List Ingress rules using the field ResourceVersion, i.e. old rules first.
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean "Order Ingress rules by ResourceVersion field"?

- If more than one Ingress defines a TLS section for the same host, the oldest rule wins.
- If multiple Ingresses define an annotation that affects configuration of the Server block, the oldest rule wins.

- Create a list of NGINX Servers (hostname)
Copy link
Member

Choose a reason for hiding this comment

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

hostname

"per hostname" would be clearer IMO


## When a reload is required

The next list describe the scenarios when a reload is required:
Copy link
Member

Choose a reason for hiding this comment

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

s/describe/describes


## Avoiding reloads

In some cases, it is possible to avoid reloads, in particular when there is a change in the endpoints, i.e. a pod is started or replaced. It is out of the scope of this Ingress controller to remove reloads completely. This would require an incredible amount of work and at some point makes no sense. This can change only if NGINX changes the way new configurations are read, basically, new changes do not replace worker processes.
Copy link
Member

Choose a reason for hiding this comment

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

should we reference to enable-dynamic-confioguration docs here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, maybe when we enable enable-dynamic-configuration as the default?

Copy link
Member

Choose a reason for hiding this comment

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

OK,

In some cases, it is possible to avoid reloads, in particular when there is a change in the endpoints, i.e. a pod is started or replaced

is a bit confusing then. It sounds like we are saying ingress-nginx already avoids reload in those cases by default - but it does not.


One of the uses of the model is to avoid unnecessary reloads when there's no change in the state and to detect conflicts in definitions.

The final representation of the model is generated from a [Go template](6) and the model that reflects the NGINX configuration.
Copy link
Member

Choose a reason for hiding this comment

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

should not this be final representation of Nginx configuration? I commented at #2479 (comment)

@aledbf aledbf merged commit 3816a66 into kubernetes:master Jun 4, 2018
@aledbf aledbf deleted the how-it-works branch June 14, 2018 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explain what happens with ingress in different namespaces
6 participants