Support multiple feed-ingress controllers per environment #195

domleb · 2019-01-31T15:57:10Z

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 · 2019-02-01T10:35:01Z

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

peterbale

I've suggested a few changes from the current setup, however, I always wanted to bring up a slight change to the way it's implemented. 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. Additionally we can rename the flag elb-ingress-name-tag-value? I think making it global makes it seem like this is supported across all of the updaters, which it isn't (and the implementation my be very different for those too).

peterbale · 2019-02-13T11:09:39Z

cmd/feed-ingress/main.go

@@ -40,6 +40,8 @@ var (
 	pushgatewayIntervalSeconds     int
 	pushgatewayLabels              cmd.KeyValues
 	controllerConfig               controller.Config
+	mainIngress                    bool


I think it might be clearer to call this variable defaultIngress and have ingressName default to main so that as a user, by default this feed instance can act as the default controller and attach to the main elb pair.

have ingressName default to main so that as a user, by default this feed instance can act as the default controller and attach to the main elb pair.

I thought about this but decided it was safer to not default to main. The reason is that if a test instance is deployed without this configuration it would then attach to the main ELBs and potentially break the main ingress. What do you think?

couldn't we have an ingress without a name? and avoid this main boolean?
I am just unsure as of why do we need it. If we want to maintain backwards compatibility, a nameless feed would be a way to have it, and if you want specific ingresses, you name them.

couldn't we have an ingress without a name? and avoid this main boolean?
I am just unsure as of why do we need it. If we want to maintain backwards compatibility, a nameless feed would be a way to have it, and if you want specific ingresses, you name them

The three of us discussed the two approaches:

The current implementation of a 'main' feed that handles ingresses that don't have an ingress name specified for backwards compatibility

requires the main elbs to be tagged and feed-ingress can only attach if it's given the tag value

this is a safety feature to ensure that test feed-ingress pods that haven't been configured don't attach to the main elbs

requires an extra bool argument so that the main feed-ingress instance know to handle ingresses without a name, for backwards compatibility

An ingress that doesn't have a name that will be the main ingress

don't add the ingress-name tag to the main elb and never add ingress-name to ingresses that are for the main / default elb

doesn't require the main-ingress bool to handle two cases

may need to update the logic that attaches to the elb to handle the no tag case

potential risk that test pods without ingress-name specified could attach to the main elb

could mitigate this by e.g. requiring the ingress-name arg and checking that it's not the same as a default randomly generated string (although, there's still a chance that someone will set a required arg to an empty string if they are just playing around :)

don't add the ingress-name tag to the main elb ...

may need to update the logic that attaches to the elb to handle the no tag case

we also said that we could intentionally add the tag our current elb with an empty name, so that the logic needs no change. I think an empty string is valid (unnamed).

(although, there's still a chance that someone will set a required arg to an empty string if they are just playing around :)

given that the params should be given as either --main-ingress (option 1) or --ingress-name "" (option 2), I'd say that there are more chances of people accidentally using --main-ingress.

--main-ingress defaults to false so this would need to be given as --main-ingress=true (or one of the other variants of true) so you would have to intentionally being setting it. Still not fool proof so not ideal.

I think flags are not really the mechanism we would use to prevent one feed-ingress attaching to an incorrect elb by miss-configuration. It would probably be more on the side of "does this instance of feed has credentials with permissions to attach itself to this elb?"

In our case, test elbs can't actually attach to the main elb because they get a different role. But that's just how we've done it (recently) and not provided by feed.

Ah great! In my opinion this should be that safety mechanism :)

I am of the opinion that the boolean field may not be necessary. This is because we are anyways advertising this as a breaking change, which means we can lose backwards compatibility. I would suggest for the feed ingress to process only those entries with the name annotation matching the name of the ingress and ignore anything which does not contain the name.

It would still be a seamless change for the application teams at least in our case because we can make the change in k8plugin to have a name defaulted to "the main ingress" (which is the equivalent of the current breed of ingress we have). There can be a cut-off date before which all the teams should specify the ingress name.

I am not fully convinced that we need to protect the test app getting processed by the main/default ingress. Such a mechanism does not exist now and also there's nothing stopping from the test ingress attaching to the main ELB by starting with the corresponding name. Such mistakes can be caught via monitoring and alerting and am not convinced we need a boolean flag to protect.

I would simply make the name mandatory for the ingress to start and a corresponding ELB with a tag matching the ingress name, without which the ingress should not get to running state.

peterbale · 2019-02-13T11:14:13Z

elb/elb.go

 	initMetrics()
-	log.Infof("ELB Front end region: %s cluster: %s expected frontends: %d", region, labelValue, expectedNumber)
+	log.Infof("ELB Front end region: %s cluster: %s expected frontends: %d", region, frontendTagValue, expectedNumber)


It would be good to include the ingress name tag here too

peterbale · 2019-02-13T11:25:00Z

elb/elb.go

-					clusterFrontEnds[lb.Scheme] = lb
-				}
+		for _, elbDescription := range output.TagDescriptions {
+


New line can be removed

peterbale · 2019-02-13T11:25:49Z

elb/elb.go

 			}
 		}
 	}
 	return clusterFrontEnds, nil
 }

+func tagsDoMatch(elbTags []*aws_elb.Tag, tagsToMatch map[string]string) bool {
+


New line can be removed

domleb · 2019-02-13T15:08:51Z

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 · 2019-02-15T08:51:28Z

controller/controller.go

+	}
+
+	return isValid
+}


I think this could be simplified as

Suggested change

}

if ingressName, ok := ingress.Annotations[ingressNameAnnotation]; ok {

return ingressName == c.ingressName

}

return c.mainIngress

totahuanocotl · 2019-02-15T08:55:39Z

elb/elb.go

 	initMetrics()
-	log.Infof("ELB Front end region: %s cluster: %s expected frontends: %d", region, labelValue, expectedNumber)
+	log.Infof("ELB Front end region: %s, cluster: %s, expected frontends: %d, ingress name: %s", region, frontendTagValue, expectedNumber, ingressNameTagValue)


is cluster the right label for frontendTagValue?

totahuanocotl · 2019-02-15T09:09:07Z

cmd/feed-ingress/main.go

@@ -40,6 +40,8 @@ var (
 	pushgatewayIntervalSeconds     int
 	pushgatewayLabels              cmd.KeyValues
 	controllerConfig               controller.Config
+	mainIngress                    bool


couldn't we have an ingress without a name? and avoid this main boolean?
I am just unsure as of why do we need it. If we want to maintain backwards compatibility, a nameless feed would be a way to have it, and if you want specific ingresses, you name them.

totahuanocotl · 2019-02-15T09:13:20Z

controller/controller.go

-				if address := serviceMap[serviceName]; address != "" {
+				if address := serviceMap[serviceName]; address == "" {
+					skipped = append(skipped, fmt.Sprintf("%s/%s (service doesn't exist)", ingress.Namespace, ingress.Name))
+				} else if !c.validIngressName(ingress) {


based on this context, I am not sure validIngressName is quite appropriate.
Is it not something more of the sorts of "can this feed handle this name"?

Simplifies this logic as per my above comment as it is mandatory for the ingress entries to have a name, else they will be ignored. No need to have an additional check on the boolean flag to allow these anonymous entries.

totahuanocotl · 2019-02-15T10:00:18Z

elb/elb.go

+	}
+
+	return matches == len(tagsToMatch)
+}


I think we should approach this differently, so that we don't loop over the elbTags for each tag to Match:

convert the elbTags to a map first (1 loop),

iterate over the tagsToMatch (1 loop)

if the tag to match key and value are not on the elb tags map, return false.

return true.

totahuanocotl · 2019-02-15T10:05:55Z

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

supreethrao · 2019-02-15T10:48:18Z

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

FoMO :D

supreethrao · 2019-02-16T12:05:43Z

controller/controller.go

@@ -15,6 +15,7 @@ import (
 	"github.com/sky-uk/feed/k8s"
 	"github.com/sky-uk/feed/util"
 	v1 "k8s.io/client-go/pkg/api/v1"
+	"k8s.io/client-go/pkg/apis/extensions/v1beta1"


Might need to use apps/v1 package for deployments now that we are on 1.10 k8s.

supreethrao

+1 for the simplicity of the code for such a fundamental change and also for the test coverage. But, I am in favour of losing the boolean flag and making the change in the k8-plugin. This would mean that we will have to defer the rollout of this version of feed ingress till the time people have upgraded to the newer version of k8plugin. Most application teams just use the major version with a wildcard in their grade dependency eg. 22.+ This would mean a seamless change for them. But we will need to give some time for teams which don't do this and having a need to use feed ingress to upgrade to the version of k8s plugin which provides default ingress names.

peterbale · 2019-02-18T09:12:57Z

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 · 2019-06-06T10:39:06Z

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

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

Also instruct go to update the packages in setup to ensure local and travis build are run with the latest version.

totahuanocotl

lgtM!

We aren't using a default ingress-controller name now so Pete's review is no longer applicable

instructs feed-ingress to process ingress resources without the sky.uk/ingress-controller-name annotation

howardburgess · 2019-08-20T10:26:59Z

@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.

on ingress resources instead of our custom sky one

howardburgess · 2019-08-21T18:47:37Z

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

Use a consistent term for ELB tag and feed-ingress command-line flag

totahuanocotl

One occurrence missed 🙈

totahuanocotl · 2019-08-27T08:31:54Z

elb/elb.go

@@ -45,7 +45,7 @@ func New(region string, frontendTagValue string, ingressControllerNameTagValue s
 		metadata:                      ec2metadata.New(session),
 		awsElb:                        aws_elb.New(session),
 		frontendTagValue:              frontendTagValue,
-		ingressControllerNameTagValue: ingressControllerNameTagValue,
+		ingressControllerNameTagValue: ingressClassTagValue,


elb.ingressControllerNameTagValue -> el.ingressClassTagValue

totahuanocotl

lgtM!

domleb added the review label Jan 31, 2019

domleb self-assigned this Feb 1, 2019

domleb force-pushed the named-ingresses branch from b05cd5d to 98f3fb8 Compare February 8, 2019 15:00

domleb removed their assignment Feb 8, 2019

peterbale self-assigned this Feb 12, 2019

peterbale previously requested changes Feb 13, 2019

View reviewed changes

peterbale assigned domleb and unassigned peterbale Feb 13, 2019

domleb assigned peterbale and unassigned domleb Feb 13, 2019

totahuanocotl reviewed Feb 15, 2019

View reviewed changes

totahuanocotl suggested changes Feb 15, 2019

View reviewed changes

supreethrao self-assigned this Feb 15, 2019

supreethrao reviewed Feb 16, 2019

View reviewed changes

supreethrao suggested changes Feb 16, 2019

View reviewed changes

peterbale assigned domleb and unassigned peterbale Feb 18, 2019

supreethrao removed their assignment Feb 19, 2019

domleb changed the title ~~Support multiple ingresses per environment~~ [WIP] Support multiple ingresses per environment Jun 6, 2019

domleb removed the review label Jun 17, 2019

joannenolan-sky added the review label Jul 2, 2019

Dominic Le Bredonchel and others added 3 commits July 3, 2019 11:54

Support multiple ingresses per enviroment

dc63b19

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

Format changes as the travis build failed

89aba21

Also instruct go to update the packages in setup to ensure local and travis build are run with the latest version.

Update readme and changelog

3438564

Review comment

027a3f1

howardburgess assigned totahuanocotl and unassigned howardburgess Aug 13, 2019

totahuanocotl previously approved these changes Aug 13, 2019

View reviewed changes

totahuanocotl assigned howardburgess and unassigned totahuanocotl Aug 13, 2019

howardburgess assigned supreethrao Aug 14, 2019

Add flag --include-unnamed-ingresses which

63491c6

instructs feed-ingress to process ingress resources without the sky.uk/ingress-controller-name annotation

howardburgess dismissed totahuanocotl’s stale review via 63491c6 August 19, 2019 17:15

Update README for new --include-unnamed-ingresses flag

d3a111e

howardburgess assigned totahuanocotl and unassigned howardburgess and domleb Aug 20, 2019

totahuanocotl previously approved these changes Aug 21, 2019

View reviewed changes

totahuanocotl removed their assignment Aug 21, 2019

Use the standard kubernetes.io/ingress.class annotation

fdb06aa

on ingress resources instead of our custom sky one

howardburgess dismissed totahuanocotl’s stale review via fdb06aa August 21, 2019 18:34

howardburgess assigned totahuanocotl Aug 21, 2019

Use "ingress class" everywhere instead of "controller name"

1aae0ff

Use a consistent term for ELB tag and feed-ingress command-line flag

totahuanocotl suggested changes Aug 27, 2019

View reviewed changes

howardburgess added 2 commits August 27, 2019 09:51

Review comment - missing rename

7ae4658

Simplify imports

ad6efc2

totahuanocotl approved these changes Aug 27, 2019

View reviewed changes

supreethrao approved these changes Aug 27, 2019

View reviewed changes

supreethrao removed their assignment Aug 27, 2019

howardburgess merged commit abdda38 into master Aug 28, 2019

howardburgess deleted the named-ingresses branch August 28, 2019 09:28

-}
+        if ingressName, ok := ingress.Annotations[ingressNameAnnotation]; ok {
+		return ingressName == c.ingressName
+	}
+        return c.mainIngress

Support multiple feed-ingress controllers per environment #195

Support multiple feed-ingress controllers per environment #195

Conversation

domleb commented Jan 31, 2019 • edited by howardburgess Loading

domleb commented Feb 1, 2019

peterbale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totahuanocotl Feb 15, 2019 • 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

supreethrao Feb 16, 2019 • 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

domleb commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totahuanocotl Feb 15, 2019 • 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

totahuanocotl commented Feb 15, 2019

supreethrao commented Feb 15, 2019

Choose a reason for hiding this comment

supreethrao left a comment

Choose a reason for hiding this comment

peterbale commented Feb 18, 2019

domleb commented Jun 6, 2019

totahuanocotl left a comment

Choose a reason for hiding this comment

howardburgess commented Aug 20, 2019

howardburgess commented Aug 21, 2019 • edited Loading

totahuanocotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

totahuanocotl left a comment

Choose a reason for hiding this comment

domleb commented Jan 31, 2019 •

edited by howardburgess

Loading

totahuanocotl Feb 15, 2019 •

edited

Loading

supreethrao Feb 16, 2019 •

edited

Loading

totahuanocotl Feb 15, 2019 •

edited

Loading

howardburgess commented Aug 21, 2019 •

edited

Loading