Route naming is not stable

mflendrich commented 4 years ago

The route naming scheme we use currently (for v1beta1, v1, tcp and knative ingress) is not stable:

Route: kong.Route{                                                                                                                                                                                                                                                      
  // TODO (#834) Figure out a way to name the routes                                                                                                                                                                                                                           
  // This is not a stable scheme                                                                                                                                                                                                                                        
  // 1. If a user adds a route in the middle,                                                                                                                                                                                                                           
  // due to a shift, all the following routes will                                                                                                                                                                                                                      
  // be PATCHED                                                                                                                                                                                                                                                         
  // 2. Is it guaranteed that the order is stable?                                                                                                                                                                                                                      
  // Meaning, the routes will always appear in the same                                                                                                                                                                                                                 
  // order?                                                                                                                                                                                                                                                             
  Name:          kong.String(ingress.Namespace + "." + ingress.Name + "." + strconv.Itoa(i) + strconv.Itoa(j)),                                                                                                                                                         
  ...
}

The scope of this issue is to fix the issue identified in the comment above.

hbagdi commented 4 years ago

This can be potentially a big breaking change so we have to be careful when designing a solution.

rainest commented 3 years ago

At present, we loop over ingress rules, and then after loop over paths within rules. The controller composes route names from the namespace, Ingress name, rule index, and path index, in that order. As such, this ruleset:

rules:
- host: first.example
  http:
    paths:
    - backend:
        serviceName: first
        servicePort: 80
      path: /first
      pathType: ImplementationSpecific
- host: second.example
  http:
    paths:
    - backend:
        serviceName: secondA
        servicePort: 80
      path: /secondA
      pathType: ImplementationSpecific
    - backend:
        serviceName: secondB
        servicePort: 80
      path: /secondB
      pathType: ImplementationSpecific

results in 3 routes, namespace.ingress.00 (first), namespace.ingress.10 (secondA), and namespace.ingress.11 (secondB).

Prefix-type paths further split into multiple paths: a single /example/ path becomes /example$ and /example/. This isn't particularly important since the order of those paths is consistent: a Prefix path will just result in two path indexes rather than one, preserving relative order within other paths. For example, if secondA was a Prefix path, we'd get 10 and 11 for it and 12 for secondB.

Hashing components, rather than indexing them, seems the only viable option if we want to make these consistent. Sorting doesn't get us anything, and using the actual values, while consistent, could potentially result in very long route names. adler32 or crc32 should be fine for short, albeit meaningless consistent IDs from the combos. I don't think we have any reason to preserve separation between rule and path without meaningful IDs, so it makes sense to use the second, combined string input. CRC32 is probably the better choice, as our data are short and adler32 deals poorly with that.

Though not directly related to static naming, it's worth noting that this does not match neatly with Kong routes. Kong routes can contain multiple hostnames and paths that all resolve to a single Kong service, whereas Ingresses only map a single hostname and path to a given Service: backends are set at the path level rather than at the Ingress level. Because of this, although we provide a single plugin annotation at the Ingress level, the resulting Kong configuration is a separate plugin configuration for each rule+path combination. This often doesn't matter, but does for some plugins (e.g. rate-limiting-advanced will not share counters between rule+path combinations unless you intentionally set namespace, and oauth2 won't share sessions unless you set global_credentials). Unfortunately there's no way to avoid this while maintaining Ingress rule+path->Service mapping.

rainest commented 3 years ago

From discussion of #1003 there are some challenges generating consistent names:

Hash identifiers carry some risk of collision. To avoid that, we'd need to preserve existing logic as a fallback, making a name change merely unlikely.
Using resource components (e.g. matching criteria for routes) ensures we do not encounter collisions, but can require very long names. Furthermore, some components can include characters not allowed in resource names on Kong <2.3, and mapping them to allowed characters would be annoying.

Harry noted that Service APIs will introduce more matching criteria (e.g. methods) in K8S rules down the road. This is partially already true for us, since although such criteria aren't in Ingress rules, konghq.com/methods and similar add them to all rules in an Ingress. Two Ingress resources with an identical ruleset and different konghq.com/methods annotations is a valid configuration, though it cannot result in conflicts as long as we somehow include Ingress namespaces and names (we do so already).

As an alternative to deterministically generating names, we can instead deterministically generate IDs, using the same guaranteed unique combination (Ingress namespace and name plus Ingress rule criteria) as input to a UUID5 generator. This strategy actually already exists in Kong's declarative config system, and (to my surprise) resource IDs persist across restarts, even though the config POST does not include them. However, this does rely on the resource primary key (for routes, the name), and if that changes, the ID will also, or rather, an existing ID will point to a new route, since we reuse names for a different set of configuration.

Postgres-backed instances have no such mechanism, and IDs randomize if the database is blown away. If the database persists, IDs persistence depends on deck's lookup strategy.

The PoC for consistent IDs reuses the #1003 inputs, with the addition of an initial input to generate a seed UUID from the namespace+Ingress name combo (recall that we may add further Ingress-wide match criteria via annotations, so we cannot generate the UUID from the Ingress rules alone). This works fine for DB-less mode but presents a challenge for Postgres mode: the deck library lookup and sync will retrieve existing routes whose config satisfies what we want but not overwrite their ID. To use the new scheme, you'll need to perform a database reset.

mflendrich commented 3 years ago

Closed "won't fix" - we'll implement stable UUIDv5s instead.

shaneutt commented 3 years ago

It would appear we still need to do some work on this issue, relevant to some conversations in PRs that came up recently:

https://github.com/Kong/kubernetes-ingress-controller/pull/1400#discussion_r649369884

UDPIngress has opted for resource based prefixing of route names, but we may need to complete the UUID efforts described here before KIC 2.0 Beta.1 in order to avoid adding more technical debt around how route naming is done.

shaneutt commented 3 years ago

KIC 2.0 works around this for the new UDPIngress type as described above, and we don't have any directly linked bug reports of issues with this in the wild. Since this is fundamentally not a new issue in 2.0 so as such moving this to the POST 2.0 scope.

shaneutt commented 2 years ago

With the advent of Gateway APIs, we decided to add support for this for the HTTPRoute API and #2222 adds that plus retroactive support for all others.

shaneutt commented 2 years ago

This is now declined due to a lack of end-user or customer request and the complexity in implementation.

Kong / kubernetes-ingress-controller

Route naming is not stable #834