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

Clarification on behavior for duplicate parent refs for routes. #1925

Closed
jm96441n opened this issue Apr 7, 2023 · 11 comments · Fixed by #2433
Closed

Clarification on behavior for duplicate parent refs for routes. #1925

jm96441n opened this issue Apr 7, 2023 · 11 comments · Fixed by #2433
Assignees
Labels
area/conformance kind/documentation Categorizes issue or PR as related to documentation. 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 triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@jm96441n
Copy link

jm96441n commented Apr 7, 2023

What would you like to be added:
An additional conformance test specifying the behavior when a gateway has only a single listener and an http route specifies the gateway twice: once in specifying the gateway with the listener and once without specifying the specific listener. According to the docs this should result in an invalid route configuration.

This feels like a potentially uncovered edge case in the docs as it is neither two identical parents (as could be detected by the webhook) nor two distinct sections, and might be a valid configuration if the Gateway had a second listener which could match the parentRef not specifying a listener name.

Why this is needed:
To clarify the above behavior for implementers of the gateway spec.

@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 Apr 7, 2023
@shaneutt shaneutt added this to the v0.7.1 milestone Apr 7, 2023
@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Apr 7, 2023
@jm96441n
Copy link
Author

I wouldn't mind writing the tests themselves, more looking for clarification on what the expected behavior is here.

@shaneutt
Copy link
Member

I wouldn't mind writing the tests themselves, more looking for clarification on what the expected behavior is here.

Awesome, appreciate the interest! For extra clarity, can you provide an example YAML configuration of the currently ambiguous situation?

@shaneutt shaneutt added triage/needs-information Indicates an issue needs more information in order to work on it. and removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Apr 10, 2023
@shaneutt shaneutt self-assigned this Apr 10, 2023
@jm96441n
Copy link
Author

hey @shaneutt sorry this took a little bit to get back to you on, below is a yaml configuration that shows the ambiguous situation:

apiVersion: gateway.networking.k8s.io/v1beta1
kind: Gateway
metadata:
  name: apigw
spec:
  gatewayClassName: api-gateway
  listeners:
  - name: http-listener
    protocol: HTTP
    port: 30002
    hostname: '*.example.com'
    allowedRoutes:
      namespaces:
        from: All
---
apiVersion: gateway.networking.k8s.io/v1beta1
kind: HTTPRoute
metadata:
  name: http-route
spec:
  hostnames:
    - ref.example.com
  parentRefs:
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: apigw
    - group: gateway.networking.k8s.io
      kind: Gateway
      name: apigw
      sectionName: http-listener
  rules:
    - backendRefs:
      - kind: Service
        name: some-service
        port: 80
        weight: 1
      matches:
      - path:
          type: PathPrefix
          value: /

so this HTTPRoute references the same gateway twice, but specifies the listener in one parent ref and does not specify it in the other ref. This could be a potentially valid situation if the gateway had more than one listener.

@shaneutt
Copy link
Member

Thank you for the illustration. Yes this seems like something we would want to specify so let's do this:

/triage accepted

We'll need whoever takes this on to take some time to build consensus among the community about how to resolve it. It would probably be good to sort it out prior to GA:

/kind conformance
/milestone v1.0.0
/help

@k8s-ci-robot
Copy link
Contributor

@shaneutt:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

Thank you for the illustration. Yes this seems like something we would want to specify so let's do this:

/triage accepted

We'll need whoever takes this on to take some time to build consensus among the community about how to resolve it. It would probably be good to sort it out prior to GA:

/kind conformance
/milestone v1.0.0
/help

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Apr 28, 2023
@shaneutt shaneutt removed the triage/needs-information Indicates an issue needs more information in order to work on it. label Apr 28, 2023
@shaneutt shaneutt modified the milestones: v0.7.1, v0.8.0 Apr 28, 2023
@shaneutt shaneutt added the kind/documentation Categorizes issue or PR as related to documentation. label May 18, 2023
@shaneutt shaneutt modified the milestones: v0.8.0, v1.0.0 May 18, 2023
@shaneutt shaneutt added the release-blocker MUST be completed to complete the milestone label May 18, 2023
@shaneutt shaneutt removed their assignment May 18, 2023
@shaneutt
Copy link
Member

@robscott is going to create a follow-up issue for this to create a conformance test to resolve this.

@shaneutt shaneutt assigned shaneutt and unassigned shaneutt Jul 21, 2023
@kubernetes-sigs kubernetes-sigs deleted a comment from k8s-ci-robot Aug 2, 2023
@robscott
Copy link
Member

I haven not forgotten about this one, waiting for clarification on #2326 before writing up what we'd expect a conformance test to look like for this.

@shaneutt
Copy link
Member

I talked with @sunjayBhatia about this one and he can take this one on.

/assign @sunjayBhatia

We expect resolving #1626 completes most of this.

Thanks @sunjayBhatia !

@shaneutt shaneutt removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Aug 24, 2023
@sunjayBhatia
Copy link
Member

Looks like #2350 covers this issue? I refrained from making any changes regarding this in #2396 since that PR already existed

@youngnick
Copy link
Contributor

Ah sorry, yes it does, I should have taken this issue.

@sunjayBhatia
Copy link
Member

No worries! I can assign it over to you since you've got that in progress already (if I have powers to do so)

Generally looks like this has a lot of overlap with #2326 so probably best resolved together?

/assign @youngnick

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/conformance kind/documentation Categorizes issue or PR as related to documentation. 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 triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
No open projects
6 participants