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

Design and implement Kong router flavor change plan #5018

Open
1 of 3 tasks
mflendrich opened this issue Oct 30, 2023 · 10 comments
Open
1 of 3 tasks

Design and implement Kong router flavor change plan #5018

mflendrich opened this issue Oct 30, 2023 · 10 comments

Comments

@mflendrich
Copy link
Contributor

mflendrich commented Oct 30, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

If KIC is started with multiple downstream gateways having incompatible router flavors, such configuration has no chance to work and will lead to hard to understand errors.

As per discussion with @mheap, there exists some work on Kong Gateway side which aims to support some (limited?) expressions support in traditional_compatible router flavor. @mheap suggested to punt work in this issue here after 3.1 and wait until the Gateway work has more shape.

Proposed Solution

  • Taken into account Kong Gateway work that @mheap mentioned in the discussion (no reference for this yet)
  • Design a plan for the process of router flavor change. This can potentially look like this but it's pending the effect of Gateway work re: expressions support in traditional_compatible
    • Do we expect 0 down time during the upgrade? Currently not possible, would require implementing multi router flavor support in translator
  • Provide a guide for users at https://docs.konghq.com/kubernetes-ingress-controller/latest/ which they can follow
  • Clearly indicate the "router mismatch among discovered gateways" condition in KIC's logs?

Additional information

post-facto enhancement of #702

Acceptance Criteria

  • A plan for implementing router flavor change is created.
  • As a user I know how to update router flavor (possibly by following docs guide) and I know if down time is expected
@pmalek
Copy link
Member

pmalek commented Oct 31, 2023

@mflendrich Do you intend to work on this? FYI: I've submitted #5040 as initial proposal how to tackle this.

@mflendrich mflendrich removed their assignment Dec 4, 2023
@mflendrich
Copy link
Contributor Author

@pmalek the assignment was meant for the issue definition phase only. Please feel free to go ahead with your attempt at implementation.

@pmalek
Copy link
Member

pmalek commented Jan 3, 2024

Some work has been started in #5040 and put on hold.

Until I get to it, anyone feeling like working on it feel free to pick this up.

@rainest
Copy link
Contributor

rainest commented Jan 5, 2024

tl;dr

  • We need to define our expected and supported user workflows before deciding what we should implement here. Simply logging probably doesn't result in great UX (you'll end up in a broken state despite, and bare minimum we'd need to include "do this to fix the problem" instructions somewhere) and we'd need different approaches depending on whether we expect this inconsistency to be a one-off problem when we change the default or a recurring situation because users are switching back and forth.
  • Proxy instances can be have a consistent router flavor across all replicas and still not come online. The controller doesn't switch the flavor it generates after startup AFAICT. If we expect that this won't be a one-off situation, we probably need to instead rework the controller parser so that it can generate both (traditional_compatible is the same as traditional as far as config is concerned) flavors and send the appropriate one based on the state of an individual proxy replica.
  • If we expect that the change will just be a one-off event for most users, upgrade docs may be a better solution. We'll probably need them anyway since the steps needed won't easily fit into a log message or Event.
  • DB-backed mode introduces a bunch of additional problems because route configuration is shared between all instances. The "generate both flavors of config, send whichever a given replica wants" approach isn't viable there. I didn't explore this one as much, but the notion of "consistent flavor" doesn't quite apply to it because the controller only talks to a single instance and will add config to the DB based on whatever it reports, but the DB is shared between all replicas. I expect that there's no way to handle DB-backed mode without the outage window+scale down to 0 before changing flavor and scaling back up approach.

Do we expect that this is possible in practice? Are there uncontrived configurations where different router configurations are expected? Was there some report elsewhere where we observed this?

In general, we should expect that Kong replicas will all use the same router configuration under normal operation. All replicas in a Deployment will (eventually) have the same environment.

There are two similar exceptions to this rule:

  1. You have a single Deployment of Kong. You edit it and change spec.template.spec.env[name=KONG_ROUTER_FLAVOR] to a different value. During the update rollout, there are a mixture of Pods with the old and new KONG_ROUTER_FLAVOR value.
  2. You have multiple Deployments of Kong and you've kinda tricked Kubernetes into thinking they're a single Deployment. This is essentially what we do for the operator's blue/green deploy feature. The temporary mismatch state is essentially the same, but convergence to a consistent state is up to the operator instead of Kubernetes' native replica handling.

This state deadlocks immediately because the new replicas never become ready. Roughly:

// this has KONG_ROUTER_FLAVOR=expressions
$ kubectl apply -f ~/src/ingress-controller/test/e2e/manifests/all-in-one-dbless.yaml                     
namespace/kong created
...

$ kubectl scale deploy -n kong proxy-kong --replicas=3
deployment.apps/proxy-kong scaled

$ kubectl get po -n kong
NAME                            READY   STATUS    RESTARTS   AGE
ingress-kong-7b59dc9857-gncdp   1/1     Running   0          46s
proxy-kong-6545cc68bf-6hjrc     1/1     Running   0          46s
proxy-kong-6545cc68bf-72vql     1/1     Running   0          27s
proxy-kong-6545cc68bf-pp282     1/1     Running   0          46s

Edit the proxy-kong Deployment to use KONG_ROUTER_FLAVOR=traditional.

This will spawn one new replica (AFAIK disruption budget limits the new replicas until some start working) that never becomes ready. Proxy logs will show repeated

10.244.0.100 - - [05/Jan/2024:00:35:40 +0000] "POST /config?check_hash=1&flatten_errors=1 HTTP/2.0" 400 2365 "-" "Go-http-client/2.0"

The controller will show

2024-01-05T00:37:49Z	error	dataplane-synchronizer	Could not update kong admin	{"error": "performing update for https://10.244.0.102:8444 failed: failed posting new config to /config: got status code 400\nperforming update for https://10.244.0.102:8444 failed: failed posting new config to /config: got status code 400"}

because the route configuration is invalid for its configuration. You can scale the proxy replicas to 0 and then back up to 3 so that all replicas use the same router flavor, but that state being consistent won't actually fix things. I didn't check the code to confirm, but that behavior suggests the controller decides the flavor it'll use at startup and doesn't change it. This just results in a set of all unready replicas even though the router flavor is consistent.

$ kubectl scale deploy -n kong proxy-kong --replicas=0
deployment.apps/proxy-kong scaled

$ kubectl get po -n kong 
NAME                            READY   STATUS    RESTARTS   AGE
ingress-kong-7b59dc9857-gncdp   1/1     Running   0          13m

$ kubectl scale deploy -n kong proxy-kong --replicas=3
deployment.apps/proxy-kong scaled

// these are consistent, but never become ready. the controller is still trying and failing to send a traditional config
$ kubectl get po -n kong                              
NAME                            READY   STATUS    RESTARTS   AGE
ingress-kong-7b59dc9857-gncdp   1/1     Running   0          13m
proxy-kong-65f9b9f779-5xvtx     0/1     Running   0          17s
proxy-kong-65f9b9f779-7tfdz     0/1     Running   0          17s
proxy-kong-65f9b9f779-d87vh     0/1     Running   0          17s

You need to further rollout restart the controller so that it decides a new router flavor after. Warning in logs is probably not that effective here since it's kind of a die roll whether the system will return to a functional state: if the controller ever starts while the proxy replicas are in a mixed state and it (randomly) chooses a replica with the old config value, it'll get stuck on that and the system will remain in deadlock.

IMO we're better off handling this in upgrade docs: for most users, this transition will probably only happen once, when/if we change the chart default flavor (Kong/charts#937 or similar--IDK if there's a relevant open issue at this point since we closed #4719). I wouldn't expect users to switch between the flavors on their own, though it's not out of the question: expressions is new enough that we could encounter bugs and recommend a user switch back to traditional until a bug affecting them is fixed. I haven't encountered such issues myself yet though (we should double-check with the gateway team, they may be aware of stuff we're not), so conservatively we could approach this with upgrade docs saying "do an outage window and scale everything to 0 before modifying this, then modify and scale back up to your usual replica count).

If we do expect that users will need to switch router flavors often going forward, we'd maybe need to consider making the controller able to handle either flavor by generating both expressions and traditional/traditional_compatible config blobs and sending whichever matches an individual proxy replica's config. You don't really want to have to take outage windows often, so if we expect that users will switch between them on an ongoing basis, we probably need to handle it gracefully.

Finally, the above mostly applies to DB-less. DB-backed is, AFAIK, a whole different ballgame because routes live in the DB and aren't magically converted to the other flavor if you switch and restart. Initial exploration suggests that the active router flavor can only "see" routes with its type of config, so switching effectively just makes all routes created on the previous flavor disappear. The controller's diff and update logic may work around that (it should see that the route in the DB doesn't match the expected config for the current flavor and update it), but I didn't actually try to confirm, so take that guess with a generously-sized grain of salt.

@pmalek
Copy link
Member

pmalek commented Jan 15, 2024

You have a single Deployment of Kong. You edit it and change spec.template.spec.env[name=KONG_ROUTER_FLAVOR] to a different value. During the update rollout, there are a mixture of Pods with the old and new KONG_ROUTER_FLAVOR value.

That's the typical upgrade scenario where I'd expect users to want to be covered and have zero down time ideally.

@mheap mentioned that we do not expect users to change the flavor back and forth so I'd consider this as a typical use case for flavor change, one-of basically.


I'd like to propose the following course of action in that case:

  • Add the log as proposed in feat: don't configure Kong Gateways with incompatible router flavor #5040 so at the very least we let users know that the 400s they are getting
  • Consider adding a flavor aware translator that would basically generate config for all flavors detected. I haven't looked at the translator code in detail recently but that seems doable. Certainly not attainable for 3.1 but worth considering after that's released.
  • Document this in docs (until above is implemented). Here I'd like to suggest how we'd like to support this transition for users without the "flavor aware translator".
    • I am tempted to say that DB-backed wouldn't be covered by this guide?
    • DB-less would could basically do the following (as suggested by @rainest)
      • Change the flavor
      • Scale proxy Deployment down to 0 and back up.
      • Perform a rolling restart of the controller. It picks up the new flavor and applies new config
      • NOTE: this implies downtime

Are we OK with this approach or are there any other suggestions to this? @mflendrich @mheap

@pmalek pmalek modified the milestones: KIC v3.1.x, KIC v3.2.x Jan 15, 2024
@pmalek pmalek changed the title Warn if trying to use different router flavors Design and implement Kong router flavor change plan Jan 15, 2024
@pmalek
Copy link
Member

pmalek commented Jan 15, 2024

Updated the title and description and put this tentatively in 3.2 milestone given that there's some pending work on the Gateway side.

@randmonkey
Copy link
Contributor

randmonkey commented Apr 23, 2024

As I got from the gateway team, they have the changes on expression router for accepting fields in the traditional router (but cannot specify both traditional fields like paths, headers and expression in the same route). So the steps of changing router flavors could be:

  • KIC translate to traditional routes and Kong gateway runs traditional/traditional compatible router
  • Upgrade Kong gateway to expression router (needs Kong gateway 3.7+) and KIC still translate to traditional routes
  • Upgrade KIC so that it translate to expression routes.

Following these steps, I think we can implement the upgrade process with no changes to KIC translator. Testing and documentation is required. Maybe we need to add a router flavor upgrade test for this.

@lahabana lahabana modified the milestones: KIC v3.2.x, KIC 3.3.x Jun 3, 2024
@lahabana lahabana modified the milestones: KIC v3.3.x, KIC v3.4.x Jun 28, 2024
@lahabana
Copy link
Contributor

This is a helm chart change only. Let's wait until the Gateway LTS supports "mixed mode expression routing" and we can turn it on for all supported versions at once.

@lahabana
Copy link
Contributor

From @randmonkey:

Kong gateway 2.8LTS will end support in Mar 2025. After that, all supported versions will support expression router.

I'm of the opinion of waiting until Mar 2025 to change the default

@mheap
Copy link
Member

mheap commented Nov 13, 2024

Even then, what’s the downside to keeping the current default? The behaviour is the same

@lahabana lahabana modified the milestones: KIC v3.4.x, KIC v3.5.x Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants