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.

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

feat: Support high performance ingress with no label selector #224

Merged
merged 2 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
77 changes: 49 additions & 28 deletions cloud/linode/cilium_loadbalancers.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"slices"
Expand All @@ -29,6 +28,8 @@
ciliumLBClass = "io.cilium/bgp-control-plane"
ipHolderLabelPrefix = "linode-ccm-ip-holder"
ciliumBGPPeeringPolicyName = "linode-ccm-bgp-peering"

commonControlPlaneLabel = "node-role.kubernetes.io/control-plane"
bcm820 marked this conversation as resolved.
Show resolved Hide resolved
)

// This mapping is unfortunately necessary since there is no way to get the
Expand Down Expand Up @@ -63,7 +64,6 @@
"ap-southeast": 16, // Sydney (Australia)
"ap-northeast": 11, // Tokyo (Japan)
}
errNoBGPSelector = errors.New("no BGP node selector set to configure IP sharing")
)

// getExistingSharedIPsInCluster determines the list of addresses to share on nodes by checking the
Expand Down Expand Up @@ -123,6 +123,9 @@
if err != nil {
return err
}
if node.Labels == nil {
node.Labels = make(map[string]string)
}
node.Labels[annotations.AnnLinodeNodeIPSharingUpdated] = "true"
retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
_, err := l.kubeClient.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{})
Expand All @@ -148,15 +151,17 @@
klog.Info("skipping IP while providerID is unset")
return nil
}
if Options.BGPNodeSelector == "" {
return errNoBGPSelector
}
// If performing Service load-balancing via IP sharing + BGP, check for a special annotation
// added by the CCM gets set when load-balancer IPs have been successfully shared on the node
kv := strings.Split(Options.BGPNodeSelector, "=")
// Check if node should be participating in IP sharing via the given selector
if val, ok := node.Labels[kv[0]]; !ok || len(kv) != 2 || val != kv[1] {
// not a selected Node
if Options.BGPNodeSelector != "" {
kv := strings.Split(Options.BGPNodeSelector, "=")

Check warning on line 157 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L156-L157

Added lines #L156 - L157 were not covered by tests
// Check if node should be participating in IP sharing via the given selector
if val, ok := node.Labels[kv[0]]; !ok || len(kv) != 2 || val != kv[1] {

Check warning on line 159 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L159

Added line #L159 was not covered by tests
// not a selected Node
return nil

Check warning on line 161 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L161

Added line #L161 was not covered by tests
}
} else if _, ok := node.Labels[commonControlPlaneLabel]; ok {

Check warning on line 163 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L163

Added line #L163 was not covered by tests
// If there is no node selector specified, default to sharing across worker nodes only
return nil
}
// check if node has been updated with IPs to share
Expand Down Expand Up @@ -200,10 +205,6 @@
// createSharedIP requests an additional IP that can be shared on Nodes to support
// loadbalancing via Cilium LB IPAM + BGP Control Plane.
func (l *loadbalancers) createSharedIP(ctx context.Context, nodes []*v1.Node) (string, error) {
if Options.BGPNodeSelector == "" {
return "", errNoBGPSelector
}

ipHolder, err := l.ensureIPHolder(ctx)
if err != nil {
return "", err
Expand Down Expand Up @@ -237,11 +238,21 @@
}

// share the IPs with nodes participating in Cilium BGP peering
kv := strings.Split(Options.BGPNodeSelector, "=")
for _, node := range nodes {
if val, ok := node.Labels[kv[0]]; ok && len(kv) == 2 && val == kv[1] {
if err = l.shareIPs(ctx, addrs, node); err != nil {
return "", err
if Options.BGPNodeSelector == "" {
for _, node := range nodes {
if _, ok := node.Labels[commonControlPlaneLabel]; !ok {
if err = l.shareIPs(ctx, addrs, node); err != nil {
return "", err

Check warning on line 245 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L245

Added line #L245 was not covered by tests
}
}
}
} else {
kv := strings.Split(Options.BGPNodeSelector, "=")
for _, node := range nodes {
if val, ok := node.Labels[kv[0]]; ok && len(kv) == 2 && val == kv[1] {
if err = l.shareIPs(ctx, addrs, node); err != nil {
return "", err

Check warning on line 254 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L254

Added line #L254 was not covered by tests
}
}
}
}
Expand All @@ -252,9 +263,6 @@
// deleteSharedIP cleans up the shared IP for a LoadBalancer Service if it was assigned
// by Cilium LB IPAM, removing it from the ip-holder
func (l *loadbalancers) deleteSharedIP(ctx context.Context, service *v1.Service) error {
if Options.BGPNodeSelector == "" {
return errNoBGPSelector
}
err := l.retrieveKubeClient()
if err != nil {
return err
Expand Down Expand Up @@ -421,9 +429,6 @@

// NOTE: Cilium CRDs must be installed for this to work
func (l *loadbalancers) ensureCiliumBGPPeeringPolicy(ctx context.Context) error {
if Options.BGPNodeSelector == "" {
return errNoBGPSelector
}
regionID, ok := regionIDMap[l.zone]
if !ok {
return fmt.Errorf("unsupported region for BGP: %s", l.zone)
Expand All @@ -443,16 +448,32 @@
}

// otherwise create it
kv := strings.Split(Options.BGPNodeSelector, "=")
if len(kv) != 2 {
return fmt.Errorf("invalid node selector %s", Options.BGPNodeSelector)
var nodeSelector slimv1.LabelSelector
// If no BGPNodeSelector is specified, select all worker nodes.
if Options.BGPNodeSelector == "" {
nodeSelector = slimv1.LabelSelector{
MatchExpressions: []slimv1.LabelSelectorRequirement{
{
Key: commonControlPlaneLabel,
Operator: slimv1.LabelSelectorOpDoesNotExist,
},
},
}
} else {
kv := strings.Split(Options.BGPNodeSelector, "=")
if len(kv) != 2 {
return fmt.Errorf("invalid node selector %s", Options.BGPNodeSelector)

Check warning on line 465 in cloud/linode/cilium_loadbalancers.go

View check run for this annotation

Codecov / codecov/patch

cloud/linode/cilium_loadbalancers.go#L465

Added line #L465 was not covered by tests
}

nodeSelector = slimv1.LabelSelector{MatchLabels: map[string]string{kv[0]: kv[1]}}
}

ciliumBGPPeeringPolicy := &v2alpha1.CiliumBGPPeeringPolicy{
ObjectMeta: metav1.ObjectMeta{
Name: ciliumBGPPeeringPolicyName,
},
Spec: v2alpha1.CiliumBGPPeeringPolicySpec{
NodeSelector: &slimv1.LabelSelector{MatchLabels: map[string]string{kv[0]: kv[1]}},
NodeSelector: &nodeSelector,
AshleyDumaine marked this conversation as resolved.
Show resolved Hide resolved
VirtualRouters: []v2alpha1.CiliumBGPVirtualRouter{{
LocalASN: 65001,
ExportPodCIDR: ptr.To(true),
Expand Down
46 changes: 41 additions & 5 deletions cloud/linode/cilium_loadbalancers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package linode
import (
"context"
"encoding/json"
"errors"
"fmt"
"net"
"testing"
Expand Down Expand Up @@ -47,6 +46,17 @@ var (
ProviderID: fmt.Sprintf("%s%d", providerIDPrefix, 33333),
},
},
{
ObjectMeta: metav1.ObjectMeta{
Name: "node-control",
Labels: map[string]string{
commonControlPlaneLabel: "",
},
},
Spec: v1.NodeSpec{
ProviderID: fmt.Sprintf("%s%d", providerIDPrefix, 44444),
},
},
}
publicIPv4 = net.ParseIP("45.76.101.25")
ipHolderInstance = linodego.Instance{
Expand Down Expand Up @@ -139,19 +149,45 @@ func addNodes(t *testing.T, kubeClient kubernetes.Interface, nodes []*v1.Node) {
}

func testNoBGPNodeLabel(t *testing.T, mc *mocks.MockClient) {
Options.BGPNodeSelector = ""
svc := createTestService()

kubeClient, _ := k8sClient.NewFakeClientset()
ciliumClient := &fakev2alpha1.FakeCiliumV2alpha1{Fake: &kubeClient.CiliumFakeClientset.Fake}
addService(t, kubeClient, svc)
addNodes(t, kubeClient, nodes)
lb := &loadbalancers{mc, zone, kubeClient, ciliumClient, ciliumLBType}

filter := map[string]string{"label": fmt.Sprintf("%s-%s", ipHolderLabelPrefix, zone)}
rawFilter, _ := json.Marshal(filter)
mc.EXPECT().ListInstances(gomock.Any(), linodego.NewListOptions(1, string(rawFilter))).Times(1).Return([]linodego.Instance{}, nil)
dummySharedIP := "45.76.101.26"
mc.EXPECT().CreateInstance(gomock.Any(), gomock.Any()).Times(1).Return(&ipHolderInstance, nil)
mc.EXPECT().GetInstanceIPAddresses(gomock.Any(), ipHolderInstance.ID).Times(1).Return(&linodego.InstanceIPAddressResponse{
IPv4: &linodego.InstanceIPv4Response{
Shared: []*linodego.InstanceIP{{Address: dummySharedIP}},
},
}, nil)
mc.EXPECT().AddInstanceIPAddress(gomock.Any(), ipHolderInstance.ID, true).Times(1).Return(&linodego.InstanceIP{Address: dummySharedIP}, nil)
mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
IPs: []string{dummySharedIP},
LinodeID: 11111,
}).Times(1)
mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
IPs: []string{dummySharedIP},
LinodeID: 22222,
}).Times(1)
mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
IPs: []string{dummySharedIP},
LinodeID: 33333,
}).Times(1)

lbStatus, err := lb.EnsureLoadBalancer(context.TODO(), "linodelb", svc, nodes)
if !errors.Is(err, errNoBGPSelector) {
t.Fatalf("expected %v, got %v... %s", errNoBGPSelector, err, Options.BGPNodeSelector)
if err != nil {
t.Fatalf("expected a nil error, got %v", err)
}
if lbStatus != nil {
t.Fatalf("expected a nil lbStatus, got %v", lbStatus)
if lbStatus == nil {
t.Fatal("expected non-nil lbStatus")
}
}

Expand Down
Loading