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.

Sign up for GitHub

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

Support explicit target protocol annotation for load balancers #751

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support explicit target protocol annotation for load balancers #751

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
969cafd
Support explicit target protocol annotation for load balancers
asaha2 Aug 6, 2024
7936440
PR feedback
asaha2 Aug 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
## unreleased

* When using the LoadBalancer `service.beta.kubernetes.io/do-loadbalancer-target-protocol` annotation with any of `http`,
`https`, `http2` or `http3` specification, the forwarding target protocol is overridden from the implicit default `http`
protocol.

## 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just an extra blank now? Was it accidental?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is great. Thanks for cleaning up.

permit traffic to arrive directly on the underlying worker nodes.

## v0.1.54 (beta) - June 12, 2024

Expand Down
5 changes: 5 additions & 0 deletions cloud-controller-manager/do/lb_annotations.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ const (
// is overwritten to https. Options are tcp, http and https. Defaults to tcp.
annDOProtocol = annDOLoadBalancerBase + "protocol"

// annDOTargetProtocol is the annotation used to specify the target protocol
// for DO load balancers. If unspecified, load balancers are configured with
// annDOProtocol specification. Options are http, https, http2, and http3.
annDOTargetProtocol = annDOLoadBalancerBase + "target-protocol"

// annDOHealthCheckPath is the annotation used to specify the health check path
// for DO load balancers. Defaults to '/'.
annDOHealthCheckPath = annDOLoadBalancerBase + "healthcheck-path"
Expand Down
75 changes: 64 additions & 11 deletions cloud-controller-manager/do/loadbalancers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"net/http"
"reflect"
"slices"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm, are TCP->TCP and UDP->UDP not supported?

)

func buildK8sTag(val string) string {
Expand Down Expand Up @@ -733,13 +737,24 @@ func buildHTTP3ForwardingRule(ctx context.Context, service *v1.Service, godoClie
return nil, errors.New("certificate ID is required for HTTP3")
}

var targetProtocol string
if targetProtocol, err = getTargetProtocol(service); err != nil {
return nil, err
}
if targetProtocol == protocolTCP || targetProtocol == protocolUDP {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we don't need this anymore either given that getTargetProtocol() already returns protocolHTTP if the annotation is missing.

}

for _, port := range service.Spec.Ports {
if port.Port == int32(http3Port) {
return &godo.ForwardingRule{
EntryProtocol: protocolHTTP3,
EntryPort: http3Port,
CertificateID: certificateID,
TargetProtocol: protocolHTTP,
TargetProtocol: targetProtocol,
TargetPort: int(port.NodePort),
}, nil
}
Expand All @@ -753,7 +768,7 @@ func buildHTTP3ForwardingRule(ctx context.Context, service *v1.Service, godoClie
func buildForwardingRules(ctx context.Context, service *v1.Service, godoClient *godo.Client) ([]godo.ForwardingRule, error) {
var forwardingRules []godo.ForwardingRule

defaultProtocol, err := getProtocol(service)
defaultProtocol, err := getEntryProtocol(service)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -928,6 +943,17 @@ func buildTLSForwardingRule(forwardingRule *godo.ForwardingRule, service *v1.Ser
return errors.New("either certificate id should be set or tls pass through enabled, not both")
}

var (
targetProtocol string
err error
)
if targetProtocol, err = getTargetProtocol(service); err != nil {
return err
}
if targetProtocol == protocolTCP || targetProtocol == protocolUDP {
return errors.New("target protocol annotation is not supported for TCP or UDP")
}

if tlsPassThrough {
forwardingRule.TlsPassthrough = tlsPassThrough
// We don't explicitly set the TargetProtocol here since in buildForwardingRule
Expand All @@ -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 != "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, I don't think we need the if statement since targetProtocol is guaranteed to be populated by getTargetProtocol().

forwardingRule.TargetProtocol = targetProtocol
} else {
forwardingRule.TargetProtocol = protocolHTTP
}
}

return nil
Expand Down Expand Up @@ -968,17 +998,40 @@ func buildStickySessions(service *v1.Service) (*godo.StickySessions, error) {
}, nil
}

// getProtocol returns the desired protocol of service.
func getProtocol(service *v1.Service) (string, error) {
protocol, ok := service.Annotations[annDOProtocol]
func getEntryProtocol(service *v1.Service) (string, error) {
protocol, err := getProtocol(service, annDOProtocol, supportedEntryProtocols)
if err != nil {
return "", err
}

if protocol == "" {
protocol = protocolTCP
}

return protocol, nil
}

func getTargetProtocol(service *v1.Service) (string, error) {
protocol, err := getProtocol(service, annDOTargetProtocol, supportedTargetProtocols)
if err != nil {
return "", err
}

if protocol == "" {
protocol = protocolHTTP
}

return protocol, nil
}

func getProtocol(service *v1.Service, annotation string, supportedProtocols []string) (string, error) {
protocol, ok := service.Annotations[annotation]
if !ok {
return protocolTCP, nil
return "", nil
}

switch protocol {
case protocolTCP, protocolHTTP, protocolHTTPS, protocolHTTP2, protocolHTTP3:
default:
return "", fmt.Errorf("invalid protocol %q specified in annotation %q", protocol, annDOProtocol)
if !slices.Contains(supportedProtocols, protocol) {
return "", fmt.Errorf("invalid protocol %q specified in annotation %q", protocol, annotation)
}

return protocol, nil
Expand Down
124 changes: 123 additions & 1 deletion cloud-controller-manager/do/loadbalancers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ func Test_getProtocol(t *testing.T) {

for _, test := range testcases {
t.Run(test.name, func(t *testing.T) {
protocol, err := getProtocol(test.service)
protocol, err := getEntryProtocol(test.service)
if protocol != test.protocol {
t.Error("unexpected protocol")
t.Logf("expected: %q", test.protocol)
Expand Down Expand Up @@ -2161,6 +2161,128 @@ func Test_buildForwardingRules(t *testing.T) {
},
nil,
},
{
"invalid target protocol annotation returns error",
&v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
UID: "abc123",
Annotations: map[string]string{
annDOHTTP2Ports: "443",
annDOTargetProtocol: "abcd",
annDOCertificateID: "test-certificate",
},
},
Spec: v1.ServiceSpec{
Ports: []v1.ServicePort{
{
Name: "test",
Protocol: "TCP",
Port: int32(443),
NodePort: int32(18080),
},
},
},
},
nil,
errors.New("failed to build TLS part(s) of forwarding rule: invalid protocol \"abcd\" specified in annotation \"service.beta.kubernetes.io/do-loadbalancer-target-protocol\""),
},
{
"unsupported target protocol annotation value returns error",
&v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
UID: "abc123",
Annotations: map[string]string{
annDOHTTP2Ports: "443",
annDOTargetProtocol: "tcp",
annDOCertificateID: "test-certificate",
},
},
Spec: v1.ServiceSpec{
Ports: []v1.ServicePort{
{
Name: "test",
Protocol: "TCP",
Port: int32(443),
NodePort: int32(18080),
},
},
},
},
nil,
errors.New("failed to build TLS part(s) of forwarding rule: invalid protocol \"tcp\" specified in annotation \"service.beta.kubernetes.io/do-loadbalancer-target-protocol\""),
},
{
"support same SSL terminated forwarding protocols (http2 -> http2)",
&v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
UID: "abc123",
Annotations: map[string]string{
annDOHTTP2Ports: "443",
annDOTargetProtocol: "http2",
annDOCertificateID: "test-certificate",
},
},
Spec: v1.ServiceSpec{
Ports: []v1.ServicePort{
{
Name: "test",
Protocol: "TCP",
Port: int32(443),
NodePort: int32(18080),
},
},
},
},
[]godo.ForwardingRule{
{
EntryProtocol: "http2",
EntryPort: 443,
TargetProtocol: "http2",
TargetPort: 18080,
CertificateID: "test-certificate",
TlsPassthrough: false,
},
},
nil,
},
{
"support different SSL terminated forwarding protocols (http2 -> https)",
&v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
UID: "abc123",
Annotations: map[string]string{
annDOHTTP2Ports: "443",
annDOTargetProtocol: "https",
annDOCertificateID: "test-certificate",
},
},
Spec: v1.ServiceSpec{
Ports: []v1.ServicePort{
{
Name: "test",
Protocol: "TCP",
Port: int32(443),
NodePort: int32(18080),
},
},
},
},
[]godo.ForwardingRule{
{
EntryProtocol: "http2",
EntryPort: 443,
TargetProtocol: "https",
TargetPort: 18080,
CertificateID: "test-certificate",
TlsPassthrough: false,
},
},
nil,
},
}

for _, test := range testcases {
Expand Down
Loading