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

ProtocolType may not work as expected because of the case #3184

Open
spacewander opened this issue Jul 5, 2024 · 4 comments
Open

ProtocolType may not work as expected because of the case #3184

spacewander opened this issue Jul 5, 2024 · 4 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@spacewander
Copy link
Contributor

What happened:

Provide a configuration like:

apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: default
spec:
  gatewayClassName: istio
  listeners:
  - name: tcp
    port: 10001
    protocol: tcp
    allowedRoutes:
      kinds:
      - kind: TCPRoute

The result is that this resource is accepted (because "tcp" is valid protocol name), but doesn't treat it as TCP protocol (because it is not equal to TCPProtocolType constant).

What you expected to happen:

Either this resource is rejected (because the TCP protocol name is "TCP", no "tcp"), or handle it as TCP protocol.

How to reproduce it (as minimally and precisely as possible):

Anything else we need to know?:

IMHO, we can either require the ProtocolType to be treated as case-insensitive, or require all Core protocols to be uppercase.

@spacewander spacewander added the kind/bug Categorizes issue or PR as related to a bug. label Jul 5, 2024
@gauravkghildiyal
Copy link
Member

we can either require the ProtocolType to be treated as case-insensitive,

Sounds reasonable. This could be documented, though would eventually be implementation dependent

or require all Core protocols to be uppercase.

At this point, given Gateway resource is v1, a validation enforcing such a change would be treated as a breaking change and hence would not be feasible.

@spacewander
Copy link
Contributor Author

we can either require the ProtocolType to be treated as case-insensitive

I would vote for this too.

I have just checked Kong Ingress controller, istio and our company's internal implementation, only a few places need to be adjusted (some ==, != and switch with the ProtocolType constant).

@shaneutt @robscott
I am pleased to invite you to share your expert opinion on this issue.

@youngnick
Copy link
Contributor

@gauravkghildiyal raises a good point here, we should have made this case-specific at the beginning, but now the only way to fix this is to mandate that the values must be treated case-insensitively.

Thanks for raising this @spacewander.

@shaneutt
Copy link
Member

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Status: Triage
Development

No branches or pull requests

5 participants