Allow services to use same port with different protocols #907
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
Hey @caglar10ur Seems that tests are failing. Do you mind updating your PR with a description of what's happening here / being changed as well? |
|
@cdrage sure thing. While at it I'll also add a fixture for this work. |
k8s-ci-robot
added
size/XL
|
@cdrage done and now tests are passing too |
| } | ||
| name = fmt.Sprintf("%s-%s", name, port.Protocol) | ||
| } | ||
| name = strings.ToLower(name) |
There was a problem hiding this comment.
I think that the output should be uppercase rather than lowercase. Only reason being is that UDP and TCP is by-default uppercase within Kubernetes. Would make more sense for the names to be: 80-UDP or 80-UDP by default.
| if _, ok := seenPorts[int(port.HostPort)]; ok { | ||
| // https://github.com/kubernetes/kubernetes/issues/2995 | ||
| if service.ServiceType == string(api.ServiceTypeLoadBalancer) { | ||
| panic("Services of type LoadBalancer cannot use both TCP and UDP for same port " + name) |
There was a problem hiding this comment.
This function should really be returning an err, but as of right now, that's in a future refactor.
Instead of a panic, can you please use log.Fatalf ? Would be helpful to output which service we're using too.
log.Fatalf("Service %s of type LoadBalancer cannot use TCP and UDP for the same port", name)
|
@cdrage pushed a new commit to address your comments. Thanks! |
|
@caglar10ur Do you mind if you merge the two commits? Push with 1 commit? 👍 |
kompose fails if compose file declares different protocols for the same port. eg;
...
ports:
- 666:666/udp
- 666:666/tcp
...
This PR adds the port to the output and also makes sure that names are unique for each port/protocol pair.
This is supported with LoadBalancer (kubernetes/kubernetes#2995) so trying to use this config with LB panics.
|
@cdrage squashed and pushed |
|
@caglar10ur , semaphore CI is breaking :( |
|
@surajnarwade not sure what is going on there as I didn't see anything alarming in the logs and assumed intermittent CI failure. Do you see an error that I'm not seeing or missing? |
|
Sorry for that @caglar10ur, @surajnarwade is wrong. Our OpenShift tests have been kind of flakey lately. Ignore that test. I'll re-run it anyways! |
|
This LGTM, @surajnarwade mind doing a quick review and we can merge this in? 👍 |
kompose fails if compose file declares different protocols for the same port. eg;
This PR adds the port to the output and also makes sure that names are unique for each port/protocol pair. This is supported with LoadBalancer (kubernetes/kubernetes#2995
) so trying to use this config with LB panics