search

sky-uk / feed

Nginx based Kubernetes ingress controller for AWS
BSD 3-Clause "New" or "Revised" License
58 stars 10 forks source link

Support multiple feed-ingress controllers per environment #195

Closed domleb closed 5 years ago

domleb commented 5 years ago

For https://github.com/sky-uk/core-infrastructure/issues/3414

This is a breaking change for feed-ingress to support creating multiple ingresses per environment. This could be used for testing feed-ingress itself and for splitting traffic into different isolated ingresses per environment each with its own set of ELBs.

The change has been applied to feed-ingress but not feed-dns. However the shared modules are backwards compatible and support both.

This change now requires the --ingress-controller-name argument to be passed to the feed-ingress container and the target ELBs must be tagged with sky.uk/KubernetesClusterIngressControllerName. It won't attached to an ELB if it can't find the tag and thus it's a breaking change. This was done to avoid the situation where a test instance could attach to the main ELBs for an environment if they haven't been tagged yet.

The sky.uk/KubernetesClusterIngressControllerName tag must match with --ingress-controller-name for feed-ingress to attach to the ELBs.

Ingress resources should be tagged with the kubernetes.io/ingress.class annotation that should match the name of the feed-ingress instance (specified with the --ingress-controller-name flag). Feed will adopt those ingress resources whose annotations match.

For migration the (new, deprecated) --include-unnamed-ingresses flag may be supplied, in which case feed-ingress will adopt all ingresses without the annotation, in addition to those that match the controller name.

required by https://github.com/sky-uk/core-infrastructure/issues/3414

domleb commented 5 years ago

Connects https://github.com/sky-uk/core-infrastructure/issues/3414

domleb commented 5 years ago

Rather than having it a global option I think this should be to support multiple elb ingresses which means some of the code can come out of the controller and into the elb code.

As discussed the ELB updater, nginx updater and ingress updater all have to apply these changes. Actually the nginx updater doesn't need to do anything special as the controller filters the ingresses but if it didn't then the nginx updater would have to. So I do think it's a global change as all parts have to work together for this to work.

totahuanocotl commented 5 years ago

I added a comment above but think it may get lost: https://github.com/sky-uk/feed/pull/195#discussion_r257153139

supreethrao commented 5 years ago

I added a comment above but think it may get lost: #195 (comment)

FoMO :D

peterbale commented 5 years ago

There seem to be some outstanding questions around the implementation of this and I think we should come to an agreement together before making further changes to the code. I'll assign it back to @domleb so we can discuss and agree a solution.

domleb commented 5 years ago

Removed from review as this is in the backlog at the moment and not being worked on

howardburgess commented 5 years ago

Rebased onto master to resolve conflicts.

jsravn commented 5 years ago

What does "multiple ingresses per environment." mean? Can you give an example? Thanks :).

howardburgess commented 5 years ago

@jsravn the title should more accurately be "Multiple feed-ingress controllers per environment" (EDIT: have updated it).

Currently only one feed-ingress can attach to ELBs and process ingresses for a particular environment.

In a multi-tenanted cluster, certain teams may have a requirement to segregate their traffic from others' using their own ELBs and feed-ingress instances. In this case, after this change, the following would allow traffic to be processed by an independent feed-ingress instance:

In addition to teams with this particular requirement, additional feed-ingress controllers can be used for load testing, for example, without disturbing existing traffic.

howardburgess commented 5 years ago

@totahuanocotl I have added the --include-unnamed-ingresses flag as discussed in yesterday's demo and with Joe afterwards. It's introduced as deprecated since it's for migration only.

howardburgess commented 5 years ago

After our group discussion I have renamed the ingress resource flag to be the standard kubernetes.io/ingress.class instead of a custom one.

Travis is complaining about formatting. Goodness knows why... locally works fine:

$ git status
On branch named-ingresses
Your branch is up-to-date with 'origin/named-ingresses'.

nothing to commit, working tree clean

$ git rev-parse HEAD
fdb06aa7034c521690c8e56f573f66b545455a82

$ go version
go version go1.11.12 linux/amd64

$ make checkformat
== check formatting

$ echo $?
0