[NET-7534] v2: Make port names in consul-k8s compatible with NET-5586#3528
[NET-7534] v2: Make port names in consul-k8s compatible with NET-5586#3528ndhanushkodi merged 4 commits intomainfrom
Conversation
ndhanushkodi
changed the title
ndhanushkodi
force-pushed
the
nd/net-7534-fix-consulk8s-ports
branch
from
9f7fb0b to
eef95ee
Compare
| // If still no match for the target port, fall back to string-ifying the target port name, which | ||
| // is what the PodController will do when converting unnamed ContainerPorts to Workload ports. | ||
| return targetPort.String() | ||
| return "cslport-" + targetPort.String() |
There was a problem hiding this comment.
This feels like it belongs in some constants package.
There was a problem hiding this comment.
Ah yep will refactor
ndhanushkodi
added
pr/no-changelog
| isNum = true | ||
| } | ||
| if name == "" || isNum { | ||
| name = "cslport-" + strconv.Itoa(int(port.ContainerPort)) |
There was a problem hiding this comment.
should we use the constant UnnamedWorkloadPortNamePrefix here?
There was a problem hiding this comment.
yup updated thanks
|
|
||
| // WorkloadPortName returns the container port's name if it has one, and if not, constructs a name from the port number | ||
| // and adds a constant prefix. The port name must be 1-15 characters and must have at least 1 alpha character. | ||
| func WorkloadPortName(port *corev1.ContainerPort) string { |
There was a problem hiding this comment.
can we add some unit tests for this function?
There was a problem hiding this comment.
ah yep i forgot to add unit tests on this side after refactoring
| // created by the pod controller when creating workloads. | ||
| for _, c := range pod.Spec.Containers { | ||
| for _, p := range c.Ports { | ||
| if strings.HasPrefix(p.Name, constants.UnnamedWorkloadPortNamePrefix) { |
There was a problem hiding this comment.
not necessarily for this PR but are we tracking anywhere restrictions like this that we should document for end users?
There was a problem hiding this comment.
For this particular case I'd not expect users to run into this and would expect if they do that it's easily discoverable through the pod not being accepted for injection. It definitely feels like a thing that'd be glazed over in any docs. Any other restrictions (so far) are enforced through kube validations
jm96441n
left a comment
jm96441n
left a comment
There was a problem hiding this comment.
LGTM! Thanks for jumping on this!
| func WorkloadPortName(port *corev1.ContainerPort) string { | ||
| name := port.Name | ||
| var isNum bool | ||
| if _, err := strconv.Atoi(name); err == nil { |
There was a problem hiding this comment.
codegolf, nonblocking, unnecessary optimization:
You don't have to try to Atoi the empty string, since you know it won't work.
There was a problem hiding this comment.
Ah right, makes sense, I may skip for now because just trying to get the tests green to merge before code freeze 🤞
|
For posterity, ACK this change and thank you @ndhanushkodi for handling it! |
5 participants
Changes proposed in this PR
If a K8s service targetport is allowed to be the stringified version of a number, it will be ambiguous in consul what to interpret the string "portID" as.
This change makes it such that the string port can never be a number, and will always also have alpha characters by prefixing "cslport-" to the workload port if the workload port name is unspecified.
How I've tested this PR
How I expect reviewers to test this PR
Checklist