Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify ParentRef Uniqueness with Port and SectionName #2326

Closed
robscott opened this issue Aug 18, 2023 · 3 comments · Fixed by #2433
Closed

Clarify ParentRef Uniqueness with Port and SectionName #2326

robscott opened this issue Aug 18, 2023 · 3 comments · Fixed by #2433
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-blocker MUST be completed to complete the milestone
Milestone

Comments

@robscott
Copy link
Member

robscott commented Aug 18, 2023

Currently the webhook and CEL validation consider the following valid:

- name: "foo"
  sectionName: "bar"
- name: "foo"
  port: 80

I personally don't think these should be considered unique and instead we should only consider parentRefs unique if:

  1. They both specify different section names
    OR
  2. They both specify different port numbers

Since the port field in parentRef is still experimental, we still have a window to tighten this validation if we want to.

Thanks to @gauravkghildiyal for catching this in #2320!

@robscott robscott added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-blocker MUST be completed to complete the milestone labels Aug 18, 2023
@robscott robscott added this to the v1.0.0 milestone Aug 18, 2023
@youngnick
Copy link
Contributor

I'm really starting to like the word "distinct" for this sort of thing.

Something like:

parentRefs must be distinct. Distinct in this context means either that:

  • they select a different object. If this is the case, then parentRef entries are distinct. In terms of the fields, this means that the multi-part key defined by group, kind, namespace, and name must be a unique one across parentRef entries for this to be considered distinct.
  • If they do not select different objects, then for each optional field that's used, each reference that selects the same object must use the same set of optional fields. If one sets sectionName, all must set sectionName. If one sets port, all must set port. If one sets a combination of optional fields, all must set the same combination.

@shaneutt shaneutt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Aug 22, 2023
@shaneutt shaneutt assigned youngnick and unassigned robscott Aug 22, 2023
@shaneutt shaneutt moved this from Next to In Progress in Gateway API: The Road to GA Aug 22, 2023
@frankbu
Copy link
Contributor

frankbu commented Aug 23, 2023

Maybe a stupid question, but why is it necessary that the references are distinct? Seems simple enough to say that the route applies to the union of the parents, overlapping or not, just like adding entries in a set. Maybe it's a little sloppy, but it's not like it won't work as expected just because the same parent is referenced twice.

@youngnick
Copy link
Contributor

It's more that allowing overlaps doesn't add anything except the possibility of confusion. We slice status by parentRef already, if there can be overlaps, it will create weird edge cases and differences in handling.

As much as I'd like not to have to be this specific, allowing overlapping references is, in our experience, a ticket to trouble-town.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-blocker MUST be completed to complete the milestone
Projects
No open projects
4 participants