HTTPRoute Status Reasons Questions

[pleshakov]

Hi,

I wonder what people think about how to report the following edge cases in the HTTPRoute status.

(1)
The sectionName in a parentRef references a non-existing listener.

HTTPRoute:

spec:
  parentRefs:
  - name: gateway
    sectionName: some-listener

Gateway:

  listeners:
  - name: my-listener
    port: 80
    protocol: HTTP

(2)
The sectionName in a parentRef references a listener that is invalid for some reason.

HTTPRoute:

spec:
  parentRefs:
  - name: gateway
    sectionName: my-listener

Gateway:

  listeners:
  - name: my-listener
    port: 80
    protocol: HTTP
    // For some reason, this listener is invalid
    // Its condition is Detached

(3)
The sectionName in a parentRef is empty and there is no compatible listener found in the Gateway resource.

HTTPRoute:

spec:
  parentRefs:
  - name: gateway

Gateway:

  listeners:
  - name: tcp-listener
    port: 1234
    protocol: TCP

---

For all those cases, an appropriate condition seems to be Accepted with the status False. However, it is not clear what should be an appropriate reason(s). I wonder what people think or if somebody has already implemented handling of those edge cases.

Thanks

[hzxuzhonghu]

Your business controller should be able to know the reason,

[robscott]

This is a good question. I think it's closely related to what @mikemorris and @youngnick are working on with GEP #1364. I agree that all of these should be "Accepted: False", but that we don't have good reasons defined to handle these cases. You've described three cases:

1. ParentRef.SectionName does not match any Listener Names
2. ParentRef references one or more Listeners that are invalid
3. ParentRef doesn't match any listeners

I think 1 and 3 could likely be merged into one reason, but 2 feels distinct to me. Will defer to @youngnick or @mikemorris on this one, but it seems like it should be pretty straightforward to add reasons here, and I'd love to get this into v0.6.0.

[youngnick]

I think adding an InvalidListener reason is fine, but we need to be careful to remember that a Route can be Accepted as long as there is at least one valid Listener. So an InvalidListener Reason should be clearly written to indicate that it can be used with either status True or False.

A couple of examples I can think of:

• The ParentRef includes a SectionName that exists but is an invalid listener; in that case the Accepted Condition would be status: false, with a Reason of InvalidListener, and no config should be generated for the Route (since it's not Accepted).
• The ParentRef does not include a SectionName, and the targeted Gateway has two Listeners, one of which is invalid. The Accepted Condition would be status: true, with a reason of InvalidListener, and config should be generated for the Route (as parented by the valid Listener). In this case, the InvalidListener Reason is essentially a warning.

[mikemorris]

The ParentRef does not include a SectionName, and the targeted Gateway has two Listeners, one of which is invalid. The Accepted Condition would be status: true, with a reason of InvalidListener, and config should be generated for the Route (as parented by the valid Listener). In this case, the InvalidListener Reason is essentially a warning.

This feels a bit awkward - the discoverability of the warning isn't great if a user starts by looking for "bad" statuses. I wonder if it could make more sense to use an InvalidListener reason with the ResolvedRefs condition type instead to express partial validity as we do in cases of partially invalid backendRefs?

Accepted { status: True, reason: Accepted }
ResolvedRefs {
    status: False,
    reason: InvalidListener,
    message: "...did not specify a SectionName, attempt to bind to Listener(s) {listener_names} on {gateway} in {namespace} failed",
}

I think the following could reasonably accurately capture status for the other cases. The constraint on not setting multiple conditions of the same type limits the detail we can provide to the message field, particularly when multiple causes may exist for the same type/reason, or multiple reasons may apply for a given condition/status (I forget if we make any explicit recommendation how to communicate this).

• "ParentRef.SectionName does not match any Listener Names"
• "All (one or more) ParentRef Listener references are invalid"

Accepted {
    status: False,
    reason: InvalidListener,
    message: "Attempt to bind to Listener(s) {listener_names} on {gateway} in {namespace} failed. [{listener_name} is invalid. | {listener} does not exist.]+",
}

• "ParentRef doesn't match any listeners"

This could be expressed by ^ too, but is slightly more awkward within the existing context of the other more verbose listener reasons, I almost wish that NotAllowedByListeners and NoMatchingListenerHostname didn't exist and would instead be subsumed under InvalidListener with details provided in the message field, or the latter reason was tweaked to be slightly less explicit as just NoMatchingListener (which could be equally applicable as an alternative expression of the first case as @robscott mentioned) rather than adding something like NoMatchingListenerProtocol and any potential future more verbose reasons.

[youngnick]

I guess this comes down to how specific we want Reasons (as opposed to Types) to be. To me, the gain in specificity offsets the cost of having more Reasons, but I could be convinced to collate Reasons into less-specific ones.

Also, we can deprecated reasons, so we could conceivably change NotAllowedByListeners and NoMatchingListenerHostname to InvalidListener if that's what we agree on, it would be a conformance-breaking change, but before we go GA is the time to make that sort of change. It's not enough for an API revision, so we could do it in a minor release. But we'd need to be sure that we're not going to want more specific Reasons later.

[pleshakov]

The ParentRef does not include a SectionName, and the targeted Gateway has two Listeners, one of which is invalid. The Accepted Condition would be status: true, with a reason of InvalidListener, and config should be generated for the Route (as parented by the valid Listener). In this case, the InvalidListener Reason is essentially a warning.

Would it make sense in the HTTPRoute to transform a parentRef with an empty sectionName to multiple parentRefs with non-empty sectionNames reporting in the status, where sectionNames will correspond to the listeners they got attached to?

For example:

In HTTPRoute:

spec:
  parentRefs:
  - name: gateway

In its reported status:

status:
  parents:
  - conditions:
    - lastTransitionTime: "2023-03-08T01:13:34Z"
       . . .
      status: "True"
      type: Accepted
    . . .
    parentRef:
       . . .
      sectionName: listener-1 # got attached to a valid listener
  - conditions:
    - lastTransitionTime: "2023-03-08T01:13:34Z"
       . . .
      status: "False"
      type: Accepted
    . . .
    parentRef:
       . . .
      sectionName: listener-2 # got attached to invalid listener

this way it will be possible to report conditions per listener. Also, the HTTPRoute owner will clearly see how many listeners it got attached too.

However, I wonder, in case of an empty section name, if it is really possible to determine if the parentRef can attach to an invalid listener. The implementation will have to match on a partially or completely invalid listener... which I think makes things complicated. So perhaps can we could we report only successful attachments? And in the case when a backendRef with empty sectionName does not get attached to any listener, we can report the status for the empty sectionName with a reason that could describe that no listener was found.

[youngnick]

The section name can't be empty at the Gateway level, it's a required field.

So I think this expansion idea is reasonable, although the downside is that we run the risk of significantly expanding the size of the Route object if there are a lot of Listeners

The maximum is 64 listeners per Gateway, and we allow 32 ParentRefs, so that's a maximum of 1024 sets of Conditions in the worst case, which would be on the order of ~10000 extra lines of YAML, assuming about 10 lines per Condition. If we assume an average of 20 characters per line (which is a guess based on the lines that are in the example above), that's another ~200kB of status that could need to be stored, and the normal maximum etcd record size is 1.5MB). So we might be eating away at the allowable spec size. But it's not obviously enough to break things, so it's probably not a hard blocker.