Conversation
dadjeibaah
left a comment
dadjeibaah
left a comment
There was a problem hiding this comment.
Like the new flag! thanks for adding this. Left a few comments.
cli/cmd/inject.go
Outdated
| func injectPodSpec(t *v1.PodSpec, identity k8s.TLSIdentity, controlPlaneDNSNameOverride string, options *injectOptions, report *injectReport) bool { | ||
| // Pods with `hostNetwork=true` share a network namespace with the host. The | ||
| // check for ports with `protocol: udp`, which will not be routed by Linkerd | ||
| for _, container := range t.Containers { |
There was a problem hiding this comment.
Could we get away with making this loop a function and then returning true as soon as we encounter a port with using UDP protocol. Something like:
func checkUDPPorts(t *v1.PodSpec) bool{
// check for ports with `protocol: udp`, which will not be routed by Linkerd
for _, container := range t.Containers {
for _, port := range container.Ports {
if port.Protocol == v1.ProtocolUDP {
return true
}
}
}
return false
}
func injectPodSpec(t *v1.PodSpec, identity k8s.TLSIdentity, controlPlaneDNSNameOverride string, options *injectOptions, report *injectReport) bool {
...
report.udp = checkUDPPorts(t)
cli/cmd/inject.go
Outdated
| // for inject reports | ||
| hostNetworkDesc = "hostNetwork: pods do not use host networking" | ||
| unsupportedDesc = "supported: at least one resource injected" | ||
| udpDesc = "udp: pods do not use UDP protocol" |
There was a problem hiding this comment.
I am wondering if this should be a statement like all pods using TCP protocol.
There was a problem hiding this comment.
yeah, i went back and forth on this one. i'm a bit hesitant to use a phrase like all pods, because whether a pod has zero, one, or all pods set as UDP, we're still going to inject. the intent is more just a warning to the user that at least one pod is using UDP.
There was a problem hiding this comment.
Hmm, how about "pod specs do not include UDP ports"? I'd like to avoid "UDP protocol", since protocol is redundant.
| {"inject_emojivoto_deployment.input.yml", "inject_emojivoto_deployment_tls.golden.yml", tlsOptions}, | ||
| {"inject_emojivoto_pod.input.yml", "inject_emojivoto_pod_tls.golden.yml", tlsOptions}, | ||
| { | ||
| inputFileName: "inject_emojivoto_deployment.input.yml", |
There was a problem hiding this comment.
Was this formatting intended? I do like that the test cases are now readable.
There was a problem hiding this comment.
yup! since i was adding a new reportFileName field, it felt a bit more readable to call explicitly call out the key names.
klingerf
left a comment
klingerf
left a comment
There was a problem hiding this comment.
⭐️ Great, thanks for all the test improvements as well. I just had a small copy edit, but otherwise looks good to me.
cli/cmd/inject.go
Outdated
| // for inject reports | ||
| hostNetworkDesc = "hostNetwork: pods do not use host networking" | ||
| unsupportedDesc = "supported: at least one resource injected" | ||
| udpDesc = "udp: pods do not use UDP protocol" |
There was a problem hiding this comment.
Hmm, how about "pod specs do not include UDP ports"? I'd like to avoid "UDP protocol", since protocol is redundant.
linkerd only routes TCP data, but `linkerd inject` does not warn when it injects into pods with ports set to `protocol: UDP`. Modify `linkerd inject` to warn when injected into a pod with `protocol: UDP`. The Linkerd sidecar will still be injected, but the stderr output will include a warning. Also add stderr checking on all inject unit tests. Part of #1516. Signed-off-by: Andrew Seigner <[email protected]>
siggy
force-pushed
the
siggy/inject-udp
branch
from
bbf5dfb to
fae1560
Compare
3 participants
linkerd only routes TCP data, but
linkerd injectdoes not warn when itinjects into pods with ports set to
protocol: UDP.Modify
linkerd injectto warn when injected into a pod withprotocol: UDP. The Linkerd sidecar will still be injected, but thestderr output will include a warning.
Also add stderr checking on all inject unit tests.
Part of #1516.
Signed-off-by: Andrew Seigner [email protected]