design: Service APIs implementation

[youngnick]

This is a WIP of the Service APIs implementation. It's been slow going, as I've been trying to check as I went that I've got the Service APIs intent correct, and that the way we'll implement is relatively consistent with Contour's current state.

Interested mostly in feedback on the high-level parts, did I miss anything?

There's quite a bit more detail required, particular for HTTPRoute and TLSRoute.

Very interested to hear about any other areas you all think I should cover as part of this design.

[codecov]

Codecov Report

Merging #3213 (9c60cd8) into main (54e2d14) will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3213      +/-   ##
==========================================
+ Coverage   75.30%   75.35%   +0.04%     
==========================================
  Files          98       98              
  Lines        6249     6249              
==========================================
+ Hits         4706     4709       +3     
+ Misses       1436     1433       -3     
  Partials      107      107              

Impacted Files	Coverage Δ
internal/k8s/log.go	69.56% <0.00%> (+6.52%)	⬆️

[github-actions]

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

[stevesloka]

What also do we do with things like:

• web sockets
• timeouts
• wildcard domain names (from api spec notes)
• regex headers
• exact path match type (also needed in ingress v1 support)

[stevesloka]

I'm still confused on the practically of this, you're proposing that we recreate all the same logic to just watch for these two types (e.g. TCP / UDP)?

[youngnick]

I'd guess that as part of this, we'd spin some code out into libraries, probably.

[stevesloka]

Contour only supports two listeners today (e.g. http/https). Does this statement assume that you are permitted to create more dynamically?

[youngnick]

The insecure/secure split definitely needs some thinking as a result of this. The initial thought is that we would replicate the current behavior, which would mean accepting only one http and one https listener, but this will be tricky to do. We may need to do something different with automatic http->https redirect for the Service APIs.

[youngnick]

I've opened #3263 with a new design for this, and will update this document accordingly.

[danehans]

xref: projectcontour/contour-operator#171

[youngnick]

In the latest round of changes, I've updated a few things:

• change the model that Contour only looks at a single Gateway (since it's part of the implementation of that specific Gateway).
• Contour is configured with the full name (name and namespace) of the Gateway.
• Added references to design: Add design for multiple Envoy listeners #3263 and updated the design assuming it's going ahead. If not, this is still doable, just will require extra rules for Listener merging.

[skriss]

I'm OK with either order, as long as we document it.

[youngnick]

To answer @stevesloka's question about additional features, I've also put some more in the documentation about what parts of the API we will target, and how we will build support.

In short, we'll build whatever is in v1alpha1 in the initial cut, then work with the upstream to define the other features (timeouts, websockets, regex headers etc) in the upstream, and support that.

[skriss]

I don't have any other significant feedback here, I think this points us in the right direction. I'd be OK working out the remaining details of processing objects in code, especially given the alpha status of the API.

[stevesloka]

I don't have any additional large comments. There are a lot of behavior details in the API spec which we'll also need to reference during implementation.

There are still quite a few TODO's throughout the doc that will need another review pass from folks if they are expaned upon.

Thanks for this work @youngnick!

[stevesloka]

+1 on removing the flag.

Should we default the name/namespace of a gateway (i.e. contour/projectcontour) so given a generic deployment, someone could use that as a default and not require a config file change?

Contour can still check if the service-apis CRDs exist in the cluster and use that as a key for setting up informers. I just merged #3283 which acts this way, but is different than this design doc, but @skriss asked which is a good point.

[youngnick]

So, the flow if you want to use the service-apis is this:

• install the objects in the cluster
• create a Gateway for Contour to reference
• Configure Contour to use that

If you don't do all those three things, then no service-apis.

That does make sense, although it means that if you install the objects and don't change the default, we should end up logging something saying "hey, you asked me to look for a Gateway called projectcontour/contour, but it doesn't exist". Not a huge deal.

[stevesloka]

Right now I have a default programmed, but we could remove that, just thought it might be nice to have some way of allowing someone to create a Gateway without needing to reconfigure / restart Contour.

This came up in a review with @skriss and thought it was a good point that what I PR'd conflicted with this design and thought we should clear up. =)

[stevesloka]

For a missing serviceName or LocalObjectReference, the docs say:

If the referent cannot be found, the route must be dropped from the Gateway. The controller should raise the “ResolvedRefs” condition on the Gateway with the “DegradedRoutes” reason. The gateway status for this route should be updated with a condition that describes the error more specifically.

Does that mean the entire HTTPRoute is invalid or just the specific HTTPRoute.HTTPRouteRule is invalid?

[sunjayBhatia]

Yeah I wasn't sure about that either (and how it would play with the proposal you had around keeping as much routability as possible even in partially applied resources), I think we chatted about this in office hours or something but I'm not sure there was a conclusion

[youngnick]

You're right, I think we need to implement this per the spec for now, and raise this upstream as a concern. I'll update to reflect this.

[stevesloka]

Chatting in Slack the thought is that only that HTTPRoute.Rule is invalidated so I think the design is correct unless we want to specifically call it out better: https://kubernetes.slack.com/archives/CR0H13KGA/p1612463856030500?thread_ts=1612459180.030400&cid=CR0H13KGA

[youngnick]

Thanks for asking this question in the upstream Slack @stevesloka. I agree with Harry's answer there - we will implement this as "conflicts will invalidate that rule" not "conflicts with invalidate the whole Route" for now, and see if we can push that upstream.

[skriss]

Some aspects of this are not entirely clear to me, e.g. what if there's a service-apis Listener with a wildcard domain name that overlaps with an HTTPProxy with a specific domain name, or what if some other service-apis Listener settings conflict with Contour's listener settings -- but I'm also not sure it's that important right now, and we can figure it out during implementation.

[youngnick]

I think the wildcard domain name thing will be sorted out by implementing wildcard domains everywhere - we should do that before starting with this, since we need to do it for Ingress v1 anyway. I think the limited form of wildcard FQDN defined in the service-apis is probably implementable in HTTPProxy as well.

I think the LIstener settings details should be sorted out during implementation, but I think that the service-apis spec should probably override the Contour config one, since it's more specific (I think). I don't think it matters really, as long as we document it.

[skriss]

no further comments/questions from me, I think we can sort out remaining details during implementation. 🎉

[stevesloka]

I think this is good enough to start implementation and dig into what comes out of that when we hit those bits. 🎉

[youngnick]

Looks like the new ExternalName integration test is failing on main, I'm going to merge this design, and take a look.