Implement merging strategy #77

thetechnick · 2016-11-20T17:34:00Z

I have implemented a first version to fix issue #76, some cleanup ist still needed

This pull request introduces a new component, the intermixer ("suggestions for a better name are welcome :P "), this component will act as a final layer for the generated IngressNginxConfig.
The core of this strategy is to generate a file in the conf.d/ folder for each server and not for each ingress.

After receiving the IngressNginxConfig of a new/updated ingress object, the intermixer performs the following steps:

Check if there are configs generated by other ingress objects that contain a server with the same name of one of the new/updated servers.
Get all configs that contain a server with one of those names
Sort these configs ascending after the creation time of the ingress object, so the oldest config is the first index
Use the first server as a base and add the locations of every other server in the list, if there is a conflict (same path) the newer location will be used
Get a list of upstreams used in the newly composed server
Return a list of new IngressNginxConfig objects each containing one server and its upstreams

Notes:

I am using a feature of golang 1.7 to setup sequential tests using t.Run()
The unittests may not cover all edge cases

pleshakov · 2016-11-21T15:30:36Z

@thetechnick
Thx!

I see two options on how to proceed with this pull request:

Put in on hold until the merging rules are defined by the API. Later we can reuse it.
Make it an optional experimental feature of the controller turned off by default, as it might be valuable for people.

Let me know your thoughts

thetechnick · 2016-11-21T17:09:29Z

@pleshakov
I think enabling the user to specify other merging behaviors, like:

data:
  merging-strategy: NeverMerge #(emit an error on colliding ingress definitions)

would be quite easy to add to the implementation, but as stated in #76 I would prefer the merging as the default setting.

pleshakov · 2016-11-23T15:25:48Z

@thetechnick
Let's add it as an optional feature. I'll add documentation and mention cases when it is useful, such as support for kube-lego.

Rather than to enable/disable it via the ConfigMap, I suggest to do it via the controller command-line arguments. I'm not sure that letting users to change it on-the-fly is a good idea: what will happen to the existing Ingress resources when merging is enabled? This will require to add more logic to the controller to regenerate the whole configuration.

The related feature is to emit errors when this option is disabled and an Ingress with the existing hostname is added. I assume that something in Configurator must track all the existing hostnames.

Also, I think it's better to not change the NginxController and to do all the merging when enabled in the Configurator.

thetechnick · 2016-11-23T17:00:12Z

@pleshakov
Ok sounds good, I will change the PR accordingly.
So something like --enable-merging?

Is there any downside to generate one file for each server object than a file for each ingress object?
If you have no objections, I would refactor the IngressNginxConfig to only contain only one server record, because if the merging is enabled you cannot map one file to one ingress object, so I would rather put one server entry per file (I already do this the hacky way in this PR).

pleshakov · 2016-11-23T19:25:45Z

@thetechnick
great!

So something like --enable-merging?

looks good

Is there any downside to generate one file for each server object than a file for each ingress object?

I don't see any downside

thetechnick · 2016-11-26T21:05:41Z

@pleshakov
It took some effort, but I have rewritten this PR from scratch to work as agreed upon.
I have tested this PR on my own environment and it works fine, the unittests should cover everything quite well.
The newly added NeverMerger is the default implementation and should address the issues with conflicting ingress definitions like multiple default servers in #79.
As a next step, we should add the errors of Mergers as events to the ingress objects.

pleshakov

Please see my comments

pleshakov · 2016-11-30T15:59:09Z

nginx-controller/nginx/oldestfirstmerger.go

+		locationMap[location.Path] = location
+	}
+
+	if merge.SSL {


Every additional Ingress replaces the settings of the previous ones. I think we should keep the settings of the oldest Ingress object if they exist.

I am not happy with this but it is necessary for kube-lego to work properly and is quite close to the behavior of the contrib/nginx controller.
For the first merge of the kube-lego managed ingress object this works, but not for every following one.
When you first create your ingress object, the ingress object of kube-lego will be created afterwards.
So every thing works as expected.

But if you create a new ingress object after this, it does not work anymore, because the ingress object of kube-lego is only updated to include a rule to newly created hosts. This leads to the situation that the kube-lego ingress is actually older than the "original" one. If we use the oldest ingress as a base here, all settings that you have made to your own ingress object are lost (even to enable SSL...).
This is why I have implemented this merging behavoir of "adding" settings together, so the settings are never removed in the merge, only added.

The settings of new Ingress objects replace the settings of the previous objects. Can we add new settings only if the old ones don't exist?

pleshakov · 2016-11-30T16:04:07Z

nginx-controller/nginx/configurator.go

@@ -74,7 +78,8 @@ func (cnf *Configurator) updateCertificates(ingEx *IngressEx) map[string]string

 	return pems
 }
-func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]string) IngressNginxConfig {
+
+func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]string) []IngressNginxConfig {


Since we're moving to a single NGINX config per hostname, we can drop the IngressNginxConfig

type IngressNginxConfig struct { Upstreams []Upstream Servers []Server }

and use Server instead, provided that we moved the array of upstreams to the Server.

pleshakov · 2016-11-30T16:06:18Z

nginx-controller/nginx/configurator.go

@@ -171,7 +176,18 @@ func (cnf *Configurator) generateNginxCfg(ingEx *IngressEx, pems map[string]stri
 		servers = append(servers, server)
 	}

-	return IngressNginxConfig{Upstreams: upstreamMapToSlice(upstreams), Servers: servers}
+	result := []IngressNginxConfig{}


We can eliminate this steps. We generate Servers that include upstreams and then return those Servers

pleshakov · 2016-11-30T16:13:42Z

nginx-controller/nginx/nevermerger.go

+	"k8s.io/kubernetes/pkg/apis/extensions"
+)
+
+type NeverMerger struct {


We shouldn't have the NeverMerger, because it doesn't do merging, but keeps track of hostnames and Ingress resources. This doesn't fit into merging. I suggest to add its functionality to the Configurator.

Its something that implements the Merger interface and is used the same way, only the behavior is different.
I would like to leave it like this because we can switch out different implementations of the merging engine if things change.
I do not like the name, though.

The fact that we have the Merger interface is great! We can easily replace one Merger with another one. However, I'd prefer the hostname checking not to be a part of it, because it's a different thing, though you can make it fit into the Merger interface.

Ok so you want to check for duplicated hostnames in the generated Servers in any case?
My understanding was that the output of the Mergers Merge() method cannot contain hostname/server name collisions anymore, because that is the whole purpose of this component.

Adding a additional collision check afterwards will only be redundant self validation, which should be put into the unittests of the Merger implementation.

Edit: We could also rename this component to something like Deduplicator, if this matches the behavior better ;)

Adding a additional collision check afterwards will only be redundant self validation, which should be put into the unittests of the Merger implementation.

I think a conditional statement in the configurator is what we need:
if merging is enabled then do the merging, using one of the implementations
else do the hostname checking, if any error -- ignore the Ingress and report the error

Edit: We could also rename this component to something like Deduplicator, if this matches the behavior better ;)

but it will still have the interface of the meger

I understand what you want to do, but I do not see any good reason to do this...

if cnf.merger == nil { // use the logic in NeverMerger, but implemented in the Configurator }

Switching out the implementation of the interface in this case seems the most clean solution to me.
Maybe you can add a small example or explain why you want the logic to reside in the Configurator.

Correct, something like this:

if cnf.merger == nil { // use the logic in NeverMerger, but implemented in the Configurator } else { // use merger } // write config files

The logic doesn't have to be implemented in the Configurator, it can be implemented in another Component.

Yes, when using only Merger, it looks more clean and it's less code, I agree. However, the functionality of NeverMerger doesn't fit into the Merger interface. The interface is different, something like this :

AddConfigs(ingName string , configs []IngressNginxConfig) error GetConfigNames(ingName string) ([]string, error)

If it was another Merging algorithm, then yes. But NeverMerger simple checks duplicated hostnames and keeps tract of Ingresses and corresponding configuration files.

AddConfigs(ingName string , configs []IngressNginxConfig) error
GetConfigNames(ingName string) ([]string, error)

But this is as the Merger interface of:

Merge(*extensions.Ingress, []IngressNginxConfig) []IngressNginxConfig Separate(string) ([]IngressNginxConfig, []string)

So this is essentially about the right semantic for the component(s) that are responsible for "merging". I still think that using a single component that is responsible for the "collision prevention" (however the component called), is the right way to do ...

I see two strategies for "collision prevention" here:

Throw an error and reject colliding objects (NeverMerger)

Merge colliding objects by rules compatible with the contrib ingress controller (OldestFirstMerger)

Those strategies and the concrete implementations need the exact same interface:
After we removed the IngressNginxConfig object: s/IngressNginxConfig/Server/
(the ingress object in RemoveConfigs can be replaced by its name "namespace/name")

// A new ingress is added, do something to prevent collisions and return the "collision free" configs I can use AddConfigs(ing *extensions.Ingress, []IngressNginxConfig) (changed []IngressNginxConfig) // A ingress object is removed, so colliding objects are no longer blocked. // Return the configs that need to be rewritten (changed) or are no longer blocked by the removed ingress and // the configs we can safely remove RemoveConfigs(ing *extensions.Ingress) (changed []IngressNginxConfig, deleted []IngressNginxConfig)

I would like to rename the Interface and the Components to:
Merger > CollisionHandler
NeverMerger > DenyCollisionHandler
OldestFirstMerger > MergingCollisionHandler

The interface of CollisionHandler will change to the above example.

I would like to rename the Interface and the Components to:
Merger > CollisionHandler
NeverMerger > DenyCollisionHandler
OldestFirstMerger > MergingCollisionHandler
The interface of CollisionHandler will change to the above example.

Looks good to me!
The methods should also return an error, so that the collision error can be returned, for example

pleshakov · 2016-11-30T16:21:50Z

nginx-controller/nginx/configurator.go

@@ -35,8 +37,10 @@ func (cnf *Configurator) AddOrUpdateIngress(name string, ingEx *IngressEx) {
 	defer cnf.lock.Unlock()

 	pems := cnf.updateCertificates(ingEx)
-	nginxCfg := cnf.generateNginxCfg(ingEx, pems)
-	cnf.nginx.AddOrUpdateIngress(name, nginxCfg)
+	generatedConfigs := cnf.generateNginxCfg(ingEx, pems)


After we generate Configs we should check if any of the hostnames already exists in NGINX, if merging is disabled. If it does, the AddOrUpdateIngress should return an error and the controller should return the Ingress resource. Please see the comment about the NeverMerger.

thetechnick · 2016-12-01T09:20:27Z

@pleshakov
Because this PR is getting quite complicated, I would like to first implement the switch to one server per ingress file and after that is merged it will be easier to merge the rest in.

thetechnick · 2016-12-07T11:17:35Z

@pleshakov
I think I have added everything we have discussed.
My own testing on my k8s environment did not reveal any issues.
I have also seperated the "single server per file" thingy to its own branch:
thetechnick@f210ac0

pleshakov · 2016-12-09T07:08:32Z

@thetechnick

I tried to run it, but immediately got the panic: Recovered from panic: "assignment to entry in nil map" (assignment to entry in nil map) This happens because var upstreams map[string]Upstream in the generateNginxCfg is nil.

Then, the ingress.tmpl is invalid. listen 80{{if .Server.ProxyProtocol}} proxy_protocol{{end}}; must be changed to listen 80{{if .ProxyProtocol}} proxy_protocol{{end}}; and so on.

Then, once the map and the template are fixed, the following Ingress resources are causing troubles:

The one with default backend:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: cafe-ingress
spec:
  backend:
    serviceName: tea-svc
    servicePort: 80
  rules:
    - host: cafe1.example.com
      http:
        paths:
        - path: /tea
          backend:
            serviceName: tea-svc
            servicePort: 80
        - path: /coffee
          backend:
            serviceName: coffee-svc
            servicePort: 80
    - host: cafe2.example.com
      http:
        paths:
        - path: /tea
          backend:
            serviceName: tea-svc
            servicePort: 80
        - path: /coffee
          backend:
            serviceName: coffee-svc
            servicePort: 80

The resulting NGINX config has the following:

location / {
                proxy_http_version 1.1;

                proxy_connect_timeout 60s;
                proxy_read_timeout 60s;
                client_max_body_size 1m;
                proxy_set_header Host $host;
                proxy_set_header X-Real-IP $remote_addr;
                proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
                proxy_set_header X-Forwarded-Host $host;
                proxy_set_header X-Forwarded-Port $server_port;
                proxy_set_header X-Forwarded-Proto $scheme;

                proxy_buffering on;

                proxy_pass http://;

The one with empty host:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: test-default-backend
spec:
  backend:
    serviceName: testsvc
    servicePort: 80

For this one, the name of the generated config file is .conf, which isn't recognized by NGINX. We could use some name that is invalid hostname (an ingress with such name can’t be created).

DenyCollisionHandler

The approach with sorting Ingress objects by timestamp is interesting. However, I'm wondering if it is necessary. Why not to simply Ignore ingresses that contain duplicated hostnames? Also, currently you can accept some hostnames of an Ingress but reject the duplicated ones. Why not to reject such Ingresses completely with all their hostnames?

thetechnick · 2016-12-09T10:39:35Z

I am so sorry, I accidentally tested a older version on my cluster, because I forgot to make clean the binary...

The nil pointer, template and the default server filename are now fixed my latest commits.
I will look into the issues with the cafe-ingress, but it will take some time.

pleshakov · 2016-12-12T14:13:11Z

@thetechnick
Let me know if you need any help finishing implementing this feature or how I can help you.

If it helps you, we can have multiple pull requests: one for refactoring the config generation per server, one for the deny handler, one for the merging handler.

thetechnick · 2016-12-18T14:37:20Z

@pleshakov
I can split up this PR into multiple ones, but I do not have access to a kubernetes environment anymore, so it is very difficult for me to test changes.

Preferably there are unittests for the main components in place, but I cannot add them due to my own time constraints.

pleshakov · 2016-12-19T13:20:44Z

@thetechnick
If you could split this PR into two -- the config generation refactoring and the Handlers -- that'd be great! Thanks!

thetechnick · 2017-01-22T09:54:12Z

Closing because of time constraints

hzxuzhonghu · 2022-01-14T03:43:28Z

Is this feature implemented?

brianehlert · 2022-01-14T16:24:38Z

mergeable Ingress has been implemented if that is what you are referring to: https://github.com/nginxinc/kubernetes-ingress/tree/v2.1.0/examples/mergeable-ingress-types
However, more flexibility and capability is possible with our CRDs: https://github.com/nginxinc/kubernetes-ingress/tree/v2.1.0/examples/custom-resources
https://docs.nginx.com/nginx-ingress-controller/configuration/virtualserver-and-virtualserverroute-resources/

thetechnick mentioned this pull request Nov 21, 2016

Same host rule in multiple ingress objects #76

Closed

thetechnick force-pushed the ingress-merging branch from 6c710f1 to ea702c5 Compare November 26, 2016 20:49

thetechnick mentioned this pull request Nov 26, 2016

Secret retry #84

Merged

pleshakov added this to the v0.7.0 milestone Nov 28, 2016

pleshakov reviewed Nov 30, 2016

View reviewed changes

pleshakov mentioned this pull request Dec 1, 2016

Add something like /status or /health to NGINX config #90

Closed

Nico Schieder added 4 commits December 3, 2016 16:34

Refactored IngressNginxConfig to contain only one server

11a157a

Removed IngressNginxConfig

f210ac0

Rebased to include changes of branch single-server-per-file

d303109

Rewrite of mergers to collision handlers

ff40883

thetechnick force-pushed the ingress-merging branch from cb9cfd2 to ff40883 Compare December 7, 2016 10:38

thetechnick mentioned this pull request Dec 8, 2016

Nginx should bind to tcp6 as well as tcp4 #96

Closed

Nico Schieder added 2 commits December 9, 2016 11:27

Fix map nil pointer error and wrong keys in template

997d27f

Fixed default backend filename

ec1d0cb

thetechnick closed this Jan 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement merging strategy #77

Implement merging strategy #77

thetechnick commented Nov 20, 2016

pleshakov commented Nov 21, 2016

thetechnick commented Nov 21, 2016

pleshakov commented Nov 23, 2016

thetechnick commented Nov 23, 2016

pleshakov commented Nov 23, 2016

thetechnick commented Nov 26, 2016

pleshakov left a comment

pleshakov Nov 30, 2016

thetechnick Dec 1, 2016

pleshakov Dec 1, 2016 •

edited

Loading

pleshakov Nov 30, 2016

pleshakov Nov 30, 2016

pleshakov Nov 30, 2016

thetechnick Dec 1, 2016

pleshakov Dec 1, 2016

thetechnick Dec 2, 2016 •

edited

Loading

pleshakov Dec 2, 2016

thetechnick Dec 2, 2016

pleshakov Dec 2, 2016

thetechnick Dec 2, 2016

pleshakov Dec 2, 2016

pleshakov Nov 30, 2016

thetechnick commented Dec 1, 2016

thetechnick commented Dec 7, 2016

pleshakov commented Dec 9, 2016

thetechnick commented Dec 9, 2016

pleshakov commented Dec 12, 2016

thetechnick commented Dec 18, 2016

pleshakov commented Dec 19, 2016

thetechnick commented Jan 22, 2017

hzxuzhonghu commented Jan 14, 2022

brianehlert commented Jan 14, 2022

Implement merging strategy #77

Implement merging strategy #77

Conversation

thetechnick commented Nov 20, 2016

pleshakov commented Nov 21, 2016

thetechnick commented Nov 21, 2016

pleshakov commented Nov 23, 2016

thetechnick commented Nov 23, 2016

pleshakov commented Nov 23, 2016

thetechnick commented Nov 26, 2016

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

pleshakov Dec 1, 2016 • edited Loading

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

thetechnick Dec 2, 2016 • edited Loading

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

thetechnick commented Dec 1, 2016

thetechnick commented Dec 7, 2016

pleshakov commented Dec 9, 2016

thetechnick commented Dec 9, 2016

pleshakov commented Dec 12, 2016

thetechnick commented Dec 18, 2016

pleshakov commented Dec 19, 2016

thetechnick commented Jan 22, 2017

hzxuzhonghu commented Jan 14, 2022

brianehlert commented Jan 14, 2022

pleshakov Dec 1, 2016 •

edited

Loading

thetechnick Dec 2, 2016 •

edited

Loading