A better story for sub-routes.

[mattmoor]

In what area(s)?

/area API
/area networking

Describe the feature

This is an offshoot of the v1beta1 task force, which has been considering our v1beta1 shape holistically. While that discussion is ongoing, this chunk of work (at the Route level) felt worthy of surfacing for broader discussion. Thanks to @evankanderson for highlighting this issue, and the task force for helping shape this proposal for consideration by the broader group.

The Problem

@evankanderson presented (recording, doc) several weeks back about the problem with the current method of handling name: in Route.

The Proposal

This proposal is fairly close to what was proposed there, but with a small change. Instead of disallowing name: in spec.traffic for Route and instantiating multiple Routes from Service, this proposes modifying Route to accomplish what it does through a new sub-resource that for the sake of focus I'll call "Kevin" (meet kevin).

Kevin is a new internal API between Route / ClusterIngress that encapsulates basic traffic splits.

Kevin is effectively exactly what @evankanderson proposed as the future of Route, and the net effect of this is an identical "shape" for Service in release mode, however, we preserve the ability to program the Route in the same intuitive / powerful way we have today.

Detailed Proposal

1. We will introduce status.traffic[*].url that MUST be used in conjunction with status.traffic[*].name to determine the URL by which a particular name: will be accessed (a URL path MUST not be returned). The spec will no longer be prescriptive about how this URL is formed, and (similar to Allow for the format of Routes of a Service to be configurable #3306) Operators MAY allow this to be configured, but these names MUST(*) be stable.

2. Create the Kevin resource. Kevin is analogous to Route, but disallows name: on splits and requires revisionName: (for consistency across Kevins).

3. Have Route start to create Kevin resources in parallel to ClusterIngress (**), make the switch to the Kevin URLs.

4. Deprecate the {name}.{route}... (to be deleted when we release v1beta1).

---

(*) - This is the goal state, but we will intentional violate this as part of rolling this out to go from {name}.{route} to (configurable) {route}-{name}.

(**) - At least Istio seems to be cool with redundant, but not-conflicting VirtualService definitions. We should talk to ClusterIngress implementers about this. We might also want to consider using this transition to make a switch to non-cluster scoped resources in anticipation of #1997 which will eliminate our need for cross-namespace ingress (cc @bbrowning since this angle wasn't discussed, but the cluster-scoped resources came up recently).

cc @evankanderson @vaikas-google

[mattmoor]

During the discussion in the API WG Wednesday, we kicked around the prospects of:

1. Just thinking of Kevin as a virtual resource.
2. Just using a namespaced Ingress resource in place of (or as) Kevin.

One problem with the first is that there isn't anything to decorate with a label if you want a particular Kevin to become internal-only, which was a nice property when I was thinking about this...

One problem with the second is that the names are independent of the URLs, and we enter into the same problematic territory that K8s Ingress has around possible conflicting URL definitions. This also doesn't handle the internal-only label problem because Ingress is "lower-level" than where we'd make that decision.

[mattmoor]

/milestone Serving 0.6
Dan broke out the first item into a separate issue, so moving the rest out.

[tcnghia]

/assign

[mattmoor]

@tcnghia and I discussed this a bit a week or so ago, and I wanted to surface the current thinking here.

I'll start with the broad strokes, so folks can tl;dr the rest.

• Resources-wise, we will stick with Route -> ClusterIngress
• We will use N+1 "placeholder services" which are 1:1 with "Kevin"s above.

---

Sticking with a single ClusterIngress punts on a variety of migration issues that we'd have had sharding the programming under N Kevins. In principle, as structured above, the s/ClusterIngress/Ingress/ work is now fully independent of this work (cc @bbrowning @markusthoemmes @wtam2018 ).

The placeholder services being 1:1 with Kevin give us all of the nice properties we want without the need for our own Kevin resource.

• API Server will guarantee name uniqueness
• For cluster-local subroutes, we can default to copying labels from the Route, but support folks mutating these labels post-facto to hide the subroutes.

[mattmoor]

I think this work should start with some cleanup of the Route controller, which is currently a bit messy.

Currently the Route controller has traffic.Config and traffic.RevisionTarget abstractions, which if you squint (at the latter) is kind of the predecessor to Kevin. Let's start by cleaning this abstraction up to encapsulate all of the information we need to program ClusterIngress.

Currently the traffic.Config holds a map[string]traffic.RevisionTarget, which has a strange key-space:

"": {{
   // The main traffic split.
}},
"v1": {{
   // The "v1" kevin
}},
"v2": {{
  // The "v2" kevin
}},

I feel like we should replace the key-space with the names of kevins:

"foo": {{
  // The main traffic split
}},
"foo-v1": {{
  // The "v1" kevin
}},
"foo-v2": {{
  // The "v2" kevin
}}

[tcnghia]

One more thing about the N+1 place holder K8 Service. In our current reconciliation loop we create them after the ClusterIngress are ready. In order to better make use of API Server to detect unavailable sub route name earlier we will need to change to.

For each Route:

• Create N+1 place holder Services corresponding to the main Route and its N sub-routes. This step is to "reserve" these domain names.
• Create ClusterIngress and wait for it be Ready.
• Update the place holder services to point to the Load Balancer IPs/Domains returned in ClusterIngress Status.

[tcnghia]

So I believe the work items for this issue are:

Part 1: Make use of API Server to avoid subRoute conflicts

1. Refactor traffic.Config to use full name of Route instead of Revision name (see A better story for sub-routes. #3419 (comment)).
2. Change Route reconciler to create N+1 placeholder services before creating ClusterIngress, and fail if one already has an owning Route (using ownerref). After ClusterIngress is Ready, update these N+1 placeholder services to ExternalName pointing to the ClusterIngress Load Balancer (see A better story for sub-routes. #3419 (comment)).
3. Improve Route Status with indicative error messages. (instead of "cannot create K8s Service" we should say "subRoute name blah is being used", may be could even use owner reference to tell which Route used it). This work could be folded into previous step too.

Part 2: Allow users to set visibility setting per subRoute by adding label to the subRoute's placeholder Service.

1. Change ClusterIngress Spec to allow specifying visibility per ClusterIngressRule, and its Status to return the LoadBalancer address per hostname.
2. Change placeholder Service reconciliation logic to avoid overriding visibility label.
3. Change placeholder Service reconciliation to consume LoadBalancer address on a per-hostname basis.
4. Plumb visibility labels on the placeholder Services to the visibility setting on ClusterIngressRule.

@mattmoor @andrew-su

[tcnghia]

/assign @andrew-su

Thanks! 🙏

[andrew-su]

As part of these changes, I started with the refactor as suggested but that quickly went down horribly as there were many places relying on the map referring to "defaultName" for the default route, which led to lots of different places failing. After fighting with that for a long while, I decided to just do the refactor later.. Going with as minimal change as I need to get the functionality working.

The current state of this change, a placeholder service for each tagged service + 1 service for the aggregated traffic. The externalName for these placeholders are routename-tag.namespace.domain (in the default case). The services at least from manually testing is about to route correctly. However, there are a handful of e2e tests failing and the table_test for route package.

Some undesirable behaviour I noticed is that updating the route did not update the clusteringress resource to reflect the new/deleted hosts, the services created do not get cleaned up (this is likely because there's no cleanup logic at the moment.)

[andrew-su]

route did not update the clusteringress resource to reflect the new/deleted hosts

It was a typo... which caused it to attempt to create the same service twice thus failing early before updating cluster ingress. Thank you controller logs.

[tcnghia]

@andrew-su I think missing CI update would fail a bunch of e2e. so hopefully fixing the typo will fix all those e2e failures?

[andrew-su]

The typo fixed a bunch of them but there's some issues with cluster-local visibility. Will take a look more tomorrow.

[andrew-su]

Let's say an operator changed the service's visibility label to ClusterLocal. Is ClusterIngressRule.Hosts supposed to refer to the local address only or is it adding the Visibility key?

Change placeholder Service reconciliation logic to avoid overriding visibility label.

That should already be done, as the only time we modify the metadata is during creating.

[andrew-su]

Since we're being more specific about the visibility within each rule. ClusterIngress.Visibility no longer seems relevant.

[tcnghia]

@andrew-su ClusterIngress.Visibility could be a convenient fallback when a rule is missing its own Visibility setting.

[andrew-su]

For reference:
#4558
#4559
#4560

[tcnghia]

@andrew-su thanks for landing all the PRs.

generally the rite of passage for such a large feature like this to close it with an integration test PR or else @mattmoor will reopen it right away :D. thanks !