Skipper: Weight() predicate - former: https redirect with kubernetes dataclient is radomly bypassed

[ese]

It seems related with #662

Requests to http endpoints are supposed to be responded with a 301 to https but are radomly bypassed directly to the service. This seems only happened in services with several replicas and the loadbalancer feature is creating the routes.

I guess it can be related with how routes are gen and stored in memory and how the predicates are weight.

As much skipper replicas and ingresses you have more probabilities this happens.

example of routes with this problem:

kube_default__nginx_1__test_nginx______nginx_slash_1: Host(/^test-nginx$/) && PathRegexp(/^\//) && LBMember("kube_default__nginx_1__test_nginx______nginx_slash", 1)
  -> dropRequestHeader("X-Load-Balancer-Member")
  -> "http://100.96.3.5:8080";


kube_default__nginx_1__test_nginx______nginx_slash_2: Host(/^test-nginx$/) && PathRegexp(/^\//) && LBMember("kube_default__nginx_1__test_nginx______nginx_slash", 2)
  -> dropRequestHeader("X-Load-Balancer-Member")
  -> "http://100.96.3.8:8080";

kube_default__nginx_1__test_nginx______nginx_slash_0: Host(/^test-nginx$/) && PathRegexp(/^\//) && LBMember("kube_default__nginx_1__test_nginx______nginx_slash", 0)
  -> dropRequestHeader("X-Load-Balancer-Member")
  -> "http://100.96.1.27:8080";

kube_default__nginx_1__test_nginx______nginx_slash__lb_group: Host(/^test-nginx$/) && PathRegexp(/^\//) && LBGroup("kube_default__nginx_1__test_nginx______nginx_slash")
  -> lbDecide("kube_default__nginx_1__test_nginx______nginx_slash", 3)
  -> <loopback>;

kube__redirect: PathRegexp(/.*/) && Header("X-Forwarded-Proto", "http") && HeaderRegexp("X-Forwarded-Port", /.*/)
  -> redirectTo(301, "https:")
  -> <shunt>;

[szuecs]

It looks like we created a race condition in the predicate between the lb group lookup and the redirect.
@aryszka can you check that?
For me it looks like 3 predicates vs. 3 predicates.

[aryszka]

checking

[aryszka]

true, this is a priority race between the redirect route and the loadbalanced routes. My first idea to fix it would be to check explTicitly on the other routes that the X-Forwarded-Proto header is set to https and not http. But I can't say that I really like this approach. What do you think? Or do you have a different suggestion?

Another way can be to put a skipper in a separate container next to the main ingress container, with an inline route that does only the redirect, listens on a different port, and the non-https traffic is directed to that port. In one of our non-kubernetes deployments, we use a similar approach and works for us.

@ese as a temporary workaround, until we fix this you can try either the separate container approach, or you can set a Header("X-Forwarded-Proto", "https") as a custom predicate in an annotation for the ingress specifications.

[aryszka]

as @szuecs pointed out, for the real fix, we could also generate a Prioirty or Weight predicate for the redirect route, once a Weigh(Priority) predicate feature makes it to the master, that is something similar to what suggested here: https://github.com/zalando/skipper/issues/360#issuecomment-393123286

[cg-rsands]

@aryszka can you confirm where this fix should go? We are noticing a lot of these conditions, the browser got a 200 and then a308

[szuecs]

@aryszka in this case we could just have a negative match for example: url.Scheme != "https", not sure if this is set, but we could test it and do not match the redirect route.
This won’t work, because it is not possible to get from a http request on the server side.

[stormmore]

I believe I just ran into this issue but in my case, I have narrowed it down to if you have a trailing / when you put the FQDN in or not, e.g. domain.com vs domain.com/. In the former case, it is bypassing the redirect, the latter is successful.

I am currently running v0.10.92

[cg-rsands]

Hsas there been any update on a fix for this? The suggested one above does not cover it 100% of the time

[szuecs]

@cg-rsands not yet.
We should put more effort into this. Maybe we can enable you to do the change?
Predicates are normally found in predicates/ directory. They implement a simple interface Match(*http.Request) bool.
I am not sure how to do these weights but maybe @aryszka can write more details about that.

[cg-rsands]

@szuecs We could maybe get someone from our operations team to get involved and help out. Keen to help out where possible. If @aryszka could give us some guidance we can try fix.

[aryszka]

Weight() predicate

A new predicate would be required in the predicates/weight package. This predicate would not be a standard one, because its Match() method cannot make the decision independently, but rather its weight value should be used together with the weith values of other routes if they exist, during routing or during generating the runtime routes. The Match() method of this predicate probably can simply return always true, or instances of it don't even need to be created.

The route matching currently works as follows:

1. lookup the route in the path tree
2. if there are multiple matches, select the route with the more predicates
3. if there are multiple matches with the same number of predicates, use one

This should be updated so that it includes a 4th step:

1. lookup the route in the path tree
2. if there are multiple matches, select the route with the more predicates
3. if there are multiple matches with the same number of predicates, use the one with the highest weight
4. if there are routes with equal weights, choose one (the first is fine)

This can be implemented so that sorting of the leaves during generating the runtime routes considers the weight as the last step.

https://github.com/zalando/skipper/blob/master/routing/matcher.go#L61

An alternative way would be where the step 2 and 3 would be merged and the weight value would be calculated together with the number of predicates, but I believe this would lead to more confusion. What do you think?

To be decided:

• should the Weight() predicate count in step 2? Suggesting that yes.
• shall the argument to the Weight() predicate be a float? Suggesting that yes.

Kubernetes ingress

The generated HTTPS redirect route should have an additional Weight(1) predicate.

https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/redirect.go#L83

[szuecs]

• 4 steps sound great to me.
• Weight() should return an int, not float, because do not use a complicated type if a simple is enough. Float is not easy to compare with == for example (you need always to define an epsilon and use <= and >=).
• Weight() should be also counted as predicate, because you could workaround some problems if you just have multiple Weight() defined in your route, if you need

Additionally for the future we could think about having Match() int and Weight(8) would return 8. The number of predicates check could then sum up and match the route with the highest number.

[aryszka]

• "return an int" :+1:
• "Weight() should be also counted as predicate" :+1:
• "Match() int" not sure at the moment, because i believe we can do this more effectively during the route generation phase outside of the request processing instead of the route matching phase.

[ese]

:+1: to Weightcount as predicate and return int

[szuecs]

Thanks! Then I think we just do the proposal from @aryszka as stated in https://github.com/zalando/skipper/issues/689#issuecomment-426021387

[xavivars]

Is there appetite for such a feature? We'd need the weight() predicate, and we're happy to implement it.

[szuecs]

We started to have a public document to collect problems and solution ideas.
Everyone is welcome to suggest paragraphs, changes, ....

[szuecs]

https://opensource.zalando.com/skipper/reference/predicates/#weight-priority was done