EXP: XBackend Implementation

[keithmattix]

What type of PR is this?

/kind gep

What this PR does / why we need it:
Adds implementation for XBackend resource. The implementation is limited to ExternalHostname for now since EndpointSelector is still provisional.

Which issue(s) this PR fixes:

NONE

[keithmattix]

Part of #4894

[snorwin]

As the Backend resource doesn't have a parentRef in the spec, what is supposed to be the parent here? Maybe ancestorRef would be a better fit, especially if it is referencing a Gateway or ListenerSet

[keithmattix]

I was mainly going off of InferencePool: https://github.com/kubernetes-sigs/gateway-api-inference-extension/blob/main/api/v1/inferencepool_types.go#L231. Happy to change the name (ancestorRef does sound a bit better here), but was going for consistency first

[snorwin]

@kl52752 wouldn't we run into the same issues as with BackendTLSPolicy if we add a status per Gateway here?

@keithmattix might be worth revisiting the discussion we had on this at EGADS: https://docs.google.com/document/d/16hBNghI_J4Rbesk_hSJZhT0iBRsQwDNZspWb2UCYeM8/edit?tab=t.euigbuw01pdp#heading=h.h6cyawqw9mcg

[snorwin]

Just thinking out loud, it could be quite a burden for implementations to add a status on the Backend resource for every referrer. Today, performance-aware implementations usually reconcile Services independently from HTTPRoutes and Gateways, and adding a status per HTTPRoute or Gateway wouldn't prevent that.

@howardjohn, curious if you share these concerns?

[keithmattix]

I think the main difference with Backend is that it's always ref'd by something else that has a direct link to the Gateway (e.g. xRoute or even InferencePool via extensionRef). Because of that, calculating the ancestor becomes a lot easier. IIUC, the complexity with BackendTLSPolicy is that it can target multiple things.

I'm confusing myself writing this so let's try an example. Let's say I've got Backend foo that is referenced in HTTPRoutes A, B, and C that have parentRefs X, Y, and Z respectively. For the sake of simplicity, let's say we only have one controller (I'll just use istio). The status for that Backend would look like this:

apiVersion: gateway.networking.x-k8s.io/v1alpha1
kind: XBackend
metadata:
  name: foo
spec:
...
status:
- controllerName: istio
  ancestorRef: # or parentRef
    group: gateway.networking.k8s.io
    kind: Gateway
    name: X
  conditions: []
- controllerName: istio
  ancestorRef: # or parentRef
    group: gateway.networking.k8s.io
    kind: Gateway
    name: Y
  conditions: []
- controllerName: istio
  ancestorRef: # or parentRef
    group: gateway.networking.k8s.io
    kind: Gateway
    name: Y
  conditions: []

[keithmattix]

@snorwin wouldn't the status be a resolvedref on the route describing the error?

[snorwin]

@keithmattix yes, that makes sense. The question is whether it would also be visible on the Backend status, or if it would be just Accepted=true as long as at least one xRoute of a Gateway successfully references the Backend.

[keithmattix]

Yeah I would think the latter. The alternative is to make the route the parent (different from policy and InferencePool), but the status could get absolutely massive from that with little benefit IMO since it's duplicated info from the route in most cases

[kflynn]

I tend to be a little skeptical that this per-referent status field is actually going to be useful enough to outweigh the complexity of implementation – but that said, this is experimental, so at this point I'd vote to leave it in (with ancestorRef rather than parentRef) with a note to the tune of "if implementing this turns out to be awful, we'll drop it".

[keithmattix]

Fair enough; done!

[rikatz]

given this is optional, what is the default behavior when empty? I can see some phrasing on when this is useful above, but making it more clear: if user sets, what happens? if it is not set, what happens?

[keithmattix]

The proxy will use whatever protocol was declared via routes or filters. Added some language on this

[rikatz]

same here, you can use https://kubernetes.io/docs/reference/using-api/cel/#kubernetes-ip-address-library to validate if this is an IP, and if it is a valid dnsLabel (unless you wanna go with PreciseHostname validations directly but I think it accepts IPv4 from what I can see)

[keithmattix]

Looks like PreciseHostname actually prevents IP addresses:

gateway-api/apis/v1/shared_types.go

Line 631 in b05a78a

// matches the RFC 1123 definition of a hostname with 1 notable exception that

. And it validates a DNS label as well.

[kl52752]

so client certificate will be the same for ALL parents (aka Gateways)? If backend is going to be used from Egres don't we want to divers client certificate based on Gateway the traffic is comming from?

[keithmattix]

I think the client cert depends on the destination so I'd rephrase that statement: "Any gateway using this backend will use the same client certificate because they're talking to the same destination". The egress gateway is an implementation detail of how you get there

[kflynn]

I agree with @keithmattix: to me, part of the point of Backend is that by definition it will always be used to group endpoints that are equivalent and that must be spoken to the same way. If you need two different TLS setups, you'd set up a Route with multiple backendRefs spanning multiple Backends.

[kl52752]

do you have some proposal what kind of conditions should be set? We don't have references to resolved, we have same CEL matching. What should be the scenarios that this Backend can be rejected by the parent?

[keithmattix]

Good question; I can add these to the type comments. The main one that come to mind is Accepted just to validate that the controller reads backend resources. This would also be the place where a gateway that doesn't allow external hostname backends would say "no I don't accept this"

[rikatz]

/approve

leaving lgtm for @kflynn and @mikemorris

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: keithmattix, rikatz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kflynn]

/lgtm
/cherry-pick release-1.6

[k8s-infra-cherrypick-robot]

@kflynn: once the present PR merges, I will cherry-pick it on top of release-1.6 in a new PR and assign it to you.

Details

In response to this:

/lgtm
/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[kflynn]

Going to go ahead and

/unhold

since I think all the outstanding feedback has been addressed.

[k8s-infra-cherrypick-robot]

@kflynn: new pull request created: #4924

Details

In response to this:

/lgtm
/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.