gep/api/test: refine ListenerSet status semantics and add Conflicted reason

[snorwin]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:
This PR implements the suggestions I made in #4692 (comment). I decided to bundle the GEP updates, the API changes, and the conformance test changes into a single PR. The main reason is to make the effects of the GEP changes more visible and tangible.

Summary of the GEP changes:

• The Programmed condition MUST be set to False with an appropriate reason when the Accepted condition is set to False.
• The Accepted condition has a new reason Conflicted, which MUST be set when the Conflicted condition is set to True.
• Clarified the semantics of the ParentNotAccepted reason, as well as the effect of a ListenerSet on Gateway acceptance.

Disclaimer: This PR reflects how I would personally resolve the current situation. I'm looking for feedback and happy to adapt it accordingly, my main goal is to create a concrete basis for a more detailed, less high-level discussion.

Which issue(s) this PR fixes:

Fixes #4760

Does this PR introduce a user-facing change?:

TODO

[snorwin]

/cc @davidjumani @zirain @rikatz

FYI: @JackStromberg @zhaohuabing

[snorwin]

/cc @youngnick

[davidjumani]

Should we mandate the reason for it to be accepted ?

[snorwin]

@davidjumani are there other reasons, that you have in mind, which would be applicable when this condition is true?

[davidjumani]

None comes to mind. Was just curious if that was required

[zirain]

why do we need to restrict the reason?

[snorwin]

@zirain I think conformance tests should be as strict as possible. Since the spec doesn't mention any reason for the ResolvedRefs=true condition other than ResolvedRefs itself, why not assert it directly? However, if your implementation uses other valid reasons, I'd be happy to relax the assertion here.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: davidjumani, snorwin
Once this PR has been reviewed and has the lgtm label, please assign danwinship for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[davidjumani]

/hold

[zhaohuabing]

Is it intentional for this conformance test to require Accepted=False, Reason=Conflicted while allowing any reason for Programmed=False?

Should this use Reason: "" // any reason for Accepted too? The specific conflict is already asserted by Conflicted=True, Reason=HostnameConflict, and there may be multiple valid reasons for Accepted=False.

[zhaohuabing]

For example, a ListenerSet listener could be rejected for a reference issue while also having a hostname conflict:

conditions:
- type: Accepted
  status: "False"
  reason: Invalid 
- type: ResolvedRefs
  status: "False"
  reason: RefNotPermitted
- type: Conflicted
  status: "True"
  reason: HostnameConflict

[snorwin]

That's a different topic, you're talking about condition reason precedence, which I don't think is defined anywhere, and this test isn't checking for it either.

We always try to test invalid cases in isolation, so you shouldn't see both RefNotPermitted and Conflicted in the same conformance test. Otherwise, it may indicate that your implementation isn't meeting the prerequisites of the test, there should always be only one specific reason applicable to a given test.

[zhaohuabing]

That makes sense, thanks. I agree conformance tests should isolate invalid cases and avoid testing condition reason precedence.

My concern is that the GEP does not seem to give implementations clear guidance on the Accepted reason here. If the API/GEP text does not define Accepted=False, Reason=Conflicted as the required mapping whenever Conflicted=True, I would suggest not strictly requiring the Accepted reason in this test. The specific conflict is already asserted by Conflicted=True, Reason=HostnameConflict.

If that mapping is intended to be required, should we also state it explicitly in the GEP/API text?

[snorwin]

Agreed that the GEP isn't precise about this, but rather than relaxing conformance, I'd prefer to update the GEP itself.