Support explicit target protocol annotation for load balancers #751
Conversation
asaha2
changed the title
asaha2
force-pushed
the
asaha/lb-httpx-to-http-redirect
branch
from
62587f2 to
969cafd
Compare
asaha2
changed the title
| ## v0.1.55 (beta) - July 29, 2024 | ||
|
|
||
| * When using the LoadBalancer `service.beta.kubernetes.io/do-loadbalancer-type=REGIONAL_NETWORK` (under closed beta), firewall rules | ||
| are added to open up the underlying health check port and all the defined (port, protocols) defined on the service. This is to | ||
| permit traffic to arrive directly on the underlying worker nodes. | ||
| are added to open up the underlying health check port and all the defined (port, protocols) defined on the service. This is to |
There was a problem hiding this comment.
Is this just an extra blank now? Was it accidental?
There was a problem hiding this comment.
Nit whitespace adjustment really, I just noticed that the last version update didn't have a tab prepended to new lines stanza as other updates. I can revert this.
There was a problem hiding this comment.
No this is great. Thanks for cleaning up.
| if targetProtocol, err = getTargetProtocol(service); err != nil { | ||
| return nil, err | ||
| } | ||
| if targetProtocol == protocolTCP || targetProtocol == protocolUDP { |
There was a problem hiding this comment.
Do we need this explicit check still given that getTargetProtocol() now returns an error if a protocol other than from the allowed list (which does not include TCP or UDP) is specified?
Similar question for other occurrences of this check.
| return nil, errors.New("target protocol annotation is not supported for TCP or UDP") | ||
| } | ||
| if targetProtocol == "" { | ||
| targetProtocol = protocolHTTP |
There was a problem hiding this comment.
It looks like we don't need this anymore either given that getTargetProtocol() already returns protocolHTTP if the annotation is missing.
| @@ -936,7 +962,11 @@ func buildTLSForwardingRule(forwardingRule *godo.ForwardingRule, service *v1.Ser | |||
| // to match the EntryProtocol. | |||
| } else { | |||
| forwardingRule.CertificateID = certificateID | |||
| forwardingRule.TargetProtocol = protocolHTTP | |||
| if targetProtocol != "" { | |||
There was a problem hiding this comment.
Similarly, I don't think we need the if statement since targetProtocol is guaranteed to be populated by getTargetProtocol().
| @@ -80,6 +81,9 @@ const ( | |||
|
|
|||
| var ( | |||
| errLBNotFound = errors.New("loadbalancer not found") | |||
|
|
|||
| supportedEntryProtocols = []string{protocolTCP, protocolHTTP, protocolHTTPS, protocolHTTP2, protocolHTTP3} | |||
| supportedTargetProtocols = []string{protocolHTTP, protocolHTTPS, protocolHTTP2, protocolHTTP3} | |||
There was a problem hiding this comment.
To confirm, are TCP->TCP and UDP->UDP not supported?
|
I think the approach from this PR, where the target protocol is set for all ports, could be problematic. Here's why:
My recommendation would be to implement an annotation like |
service.beta.kubernetes.io/do-loadbalancer-target-protocolannotation that provides explicit specifier for forwarding rules target protocols, if set correctly overrides the implicit default ofhttptarget protocolFixes #367 #378