From c3a9ae7fe711ffdaf29735d4a1096410ea01f416 Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 28 Oct 2024 09:37:51 +0100 Subject: [PATCH] Slight improvements. --- .../loadbalancer/config/cilium_config.go | 59 ++++++++++++------- .../loadbalancer/config/cilium_config_test.go | 10 ++-- pkg/controllers/loadbalancer/config/config.go | 10 ++-- .../loadbalancer/config/metallb_config.go | 48 +++++++++------ .../config/metallb_config_test.go | 10 ++-- pkg/controllers/loadbalancer/loadbalancer.go | 4 +- 6 files changed, 83 insertions(+), 58 deletions(-) diff --git a/pkg/controllers/loadbalancer/config/cilium_config.go b/pkg/controllers/loadbalancer/config/cilium_config.go index 12fff17..ac6cd7e 100644 --- a/pkg/controllers/loadbalancer/config/cilium_config.go +++ b/pkg/controllers/loadbalancer/config/cilium_config.go @@ -21,26 +21,27 @@ import ( ) type ciliumConfig struct { - cfg *baseConfig + base *baseConfig + client client.Client k8sClient clientset.Interface } -func newCiliumConfig(cfg *baseConfig, k8sClient clientset.Interface) *ciliumConfig { - return &ciliumConfig{cfg: cfg, k8sClient: k8sClient} +func newCiliumConfig(base *baseConfig, c client.Client, k8sClient clientset.Interface) *ciliumConfig { + return &ciliumConfig{base: base, client: c, k8sClient: k8sClient} } -func (cfg *ciliumConfig) WriteCRs(ctx context.Context, c client.Client) error { - err := cfg.writeCiliumBGPPeeringPolicies(ctx, c) +func (c *ciliumConfig) WriteCRs(ctx context.Context) error { + err := c.writeCiliumBGPPeeringPolicies(ctx) if err != nil { return fmt.Errorf("failed to write ciliumbgppeeringpolicy resources %w", err) } - err = cfg.writeCiliumLoadBalancerIPPools(ctx, c) + err = c.writeCiliumLoadBalancerIPPools(ctx) if err != nil { return fmt.Errorf("failed to write ciliumloadbalancerippool resources %w", err) } - err = cfg.writeNodeAnnotations(ctx) + err = c.writeNodeAnnotations(ctx) if err != nil { return fmt.Errorf("failed to write node annotations %w", err) } @@ -48,30 +49,33 @@ func (cfg *ciliumConfig) WriteCRs(ctx context.Context, c client.Client) error { return nil } -func (cfg *ciliumConfig) writeCiliumBGPPeeringPolicies(ctx context.Context, c client.Client) error { +func (c *ciliumConfig) writeCiliumBGPPeeringPolicies(ctx context.Context) error { existingPolicies := ciliumv2alpha1.CiliumBGPPeeringPolicyList{} - err := c.List(ctx, &existingPolicies) + err := c.client.List(ctx, &existingPolicies) if err != nil { return err } + for _, existingPolicy := range existingPolicies.Items { existingPolicy := existingPolicy found := false - for _, peer := range cfg.cfg.Peers { + + for _, peer := range c.base.Peers { if fmt.Sprintf("%d", peer.ASN) == existingPolicy.Name { found = true break } } + if !found { - err := c.Delete(ctx, &existingPolicy) + err := c.client.Delete(ctx, &existingPolicy) if err != nil { return err } } } - for _, peer := range cfg.cfg.Peers { + for _, peer := range c.base.Peers { bgpPeeringPolicy := &ciliumv2alpha1.CiliumBGPPeeringPolicy{ TypeMeta: metav1.TypeMeta{ APIVersion: ciliumv2alpha1.CustomResourceDefinitionGroup + "/" + ciliumv2alpha1.CustomResourceDefinitionVersion, @@ -81,7 +85,8 @@ func (cfg *ciliumConfig) writeCiliumBGPPeeringPolicies(ctx context.Context, c cl Name: fmt.Sprintf("%d", peer.ASN), }, } - res, err := controllerutil.CreateOrUpdate(ctx, c, bgpPeeringPolicy, func() error { + + res, err := controllerutil.CreateOrUpdate(ctx, c.client, bgpPeeringPolicy, func() error { bgpPeeringPolicy.Spec = ciliumv2alpha1.CiliumBGPPeeringPolicySpec{ NodeSelector: convertNodeSelector(&peer.NodeSelector), VirtualRouters: []ciliumv2alpha1.CiliumBGPVirtualRouter{ @@ -113,6 +118,7 @@ func (cfg *ciliumConfig) writeCiliumBGPPeeringPolicies(ctx context.Context, c cl if err != nil { return err } + if res != controllerutil.OperationResultNone { klog.Infof("bgppeer: %v", res) } @@ -121,30 +127,33 @@ func (cfg *ciliumConfig) writeCiliumBGPPeeringPolicies(ctx context.Context, c cl return nil } -func (cfg *ciliumConfig) writeCiliumLoadBalancerIPPools(ctx context.Context, c client.Client) error { +func (c *ciliumConfig) writeCiliumLoadBalancerIPPools(ctx context.Context) error { existingPools := ciliumv2alpha1.CiliumLoadBalancerIPPoolList{} - err := c.List(ctx, &existingPools) + err := c.client.List(ctx, &existingPools) if err != nil { return err } + for _, existingPool := range existingPools.Items { existingPool := existingPool found := false - for _, pool := range cfg.cfg.AddressPools { + + for _, pool := range c.base.AddressPools { if pool.Name == existingPool.Name { found = true break } } + if !found { - err := c.Delete(ctx, &existingPool) + err := c.client.Delete(ctx, &existingPool) if err != nil { return err } } } - for _, pool := range cfg.cfg.AddressPools { + for _, pool := range c.base.AddressPools { ipPool := &ciliumv2alpha1.CiliumLoadBalancerIPPool{ TypeMeta: metav1.TypeMeta{ APIVersion: ciliumv2alpha1.CustomResourceDefinitionGroup + "/" + ciliumv2alpha1.CustomResourceDefinitionVersion, @@ -154,7 +163,8 @@ func (cfg *ciliumConfig) writeCiliumLoadBalancerIPPools(ctx context.Context, c c Name: pool.Name, }, } - res, err := controllerutil.CreateOrUpdate(ctx, c, ipPool, func() error { + + res, err := controllerutil.CreateOrUpdate(ctx, c.client, ipPool, func() error { cidrs := make([]ciliumv2alpha1.CiliumLoadBalancerIPPoolIPBlock, 0) for _, cidr := range pool.CIDRs { ipPoolBlock := ciliumv2alpha1.CiliumLoadBalancerIPPoolIPBlock{ @@ -170,6 +180,7 @@ func (cfg *ciliumConfig) writeCiliumLoadBalancerIPPools(ctx context.Context, c c if err != nil { return err } + if res != controllerutil.OperationResultNone { klog.Infof("ipaddresspool: %v", res) } @@ -178,25 +189,29 @@ func (cfg *ciliumConfig) writeCiliumLoadBalancerIPPools(ctx context.Context, c c return nil } -func (cfg *ciliumConfig) writeNodeAnnotations(ctx context.Context) error { - nodes, err := kubernetes.GetNodes(ctx, cfg.k8sClient) +func (c *ciliumConfig) writeNodeAnnotations(ctx context.Context) error { + nodes, err := kubernetes.GetNodes(ctx, c.k8sClient) if err != nil { return fmt.Errorf("failed to write node annotations: %w", err) } + backoff := wait.Backoff{ Steps: 20, Duration: 50 * time.Millisecond, Jitter: 1.0, } + for _, n := range nodes { asn, err := getASNFromNodeLabels(n) if err != nil { return fmt.Errorf("failed to write node annotations for node %s: %w", n.Name, err) } + annotations := map[string]string{ fmt.Sprintf("cilium.io/bgp-virtual-router.%d", asn): "router-id=127.0.0.1", } - err = kubernetes.UpdateNodeAnnotationsWithBackoff(ctx, cfg.k8sClient, n.Name, annotations, backoff) + + err = kubernetes.UpdateNodeAnnotationsWithBackoff(ctx, c.k8sClient, n.Name, annotations, backoff) if err != nil { return fmt.Errorf("failed to write node annotations for node %s: %w", n.Name, err) } diff --git a/pkg/controllers/loadbalancer/config/cilium_config_test.go b/pkg/controllers/loadbalancer/config/cilium_config_test.go index 0361267..df8389c 100644 --- a/pkg/controllers/loadbalancer/config/cilium_config_test.go +++ b/pkg/controllers/loadbalancer/config/cilium_config_test.go @@ -40,7 +40,7 @@ func TestCiliumConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &ciliumConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -83,7 +83,7 @@ func TestCiliumConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &ciliumConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -137,7 +137,7 @@ func TestCiliumConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &ciliumConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -239,7 +239,7 @@ func TestCiliumConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &ciliumConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -298,7 +298,7 @@ func TestCiliumConfig(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - cfg, err := New("cilium", tt.ips, tt.nws, tt.nodes, nil) + cfg, err := New("cilium", tt.ips, tt.nws, tt.nodes, nil, nil) if diff := cmp.Diff(err, tt.wantErr); diff != "" { t.Errorf("error = %v", diff) return diff --git a/pkg/controllers/loadbalancer/config/config.go b/pkg/controllers/loadbalancer/config/config.go index 728c2cd..583741b 100644 --- a/pkg/controllers/loadbalancer/config/config.go +++ b/pkg/controllers/loadbalancer/config/config.go @@ -16,7 +16,7 @@ import ( ) type LoadBalancerConfig interface { - WriteCRs(ctx context.Context, c client.Client) error + WriteCRs(ctx context.Context) error } type baseConfig struct { @@ -24,7 +24,7 @@ type baseConfig struct { AddressPools addressPools `json:"address-pools,omitempty" yaml:"address-pools,omitempty"` } -func New(loadBalancerType string, ips []*models.V1IPResponse, nws sets.Set[string], nodes []v1.Node, k8sClientSet clientset.Interface) (LoadBalancerConfig, error) { +func New(loadBalancerType string, ips []*models.V1IPResponse, nws sets.Set[string], nodes []v1.Node, c client.Client, k8sClientSet clientset.Interface) (LoadBalancerConfig, error) { bc, err := newBaseConfig(ips, nws, nodes) if err != nil { return nil, err @@ -32,11 +32,11 @@ func New(loadBalancerType string, ips []*models.V1IPResponse, nws sets.Set[strin switch loadBalancerType { case "metallb": - return newMetalLBConfig(bc), nil + return newMetalLBConfig(bc, c), nil case "cilium": - return newCiliumConfig(bc, k8sClientSet), nil + return newCiliumConfig(bc, c, k8sClientSet), nil default: - return newMetalLBConfig(bc), nil + return newMetalLBConfig(bc, c), nil } } diff --git a/pkg/controllers/loadbalancer/config/metallb_config.go b/pkg/controllers/loadbalancer/config/metallb_config.go index 844fcab..1e9a9ed 100644 --- a/pkg/controllers/loadbalancer/config/metallb_config.go +++ b/pkg/controllers/loadbalancer/config/metallb_config.go @@ -20,37 +20,40 @@ const ( ) type metalLBConfig struct { - cfg *baseConfig + base *baseConfig + client client.Client } -func newMetalLBConfig(cfg *baseConfig) *metalLBConfig { - return &metalLBConfig{cfg: cfg} +func newMetalLBConfig(base *baseConfig, c client.Client) *metalLBConfig { + return &metalLBConfig{base: base, client: c} } -func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { +func (m *metalLBConfig) WriteCRs(ctx context.Context) error { bgpPeerList := metallbv1beta2.BGPPeerList{} - err := c.List(ctx, &bgpPeerList, client.InNamespace(metallbNamespace)) + err := m.client.List(ctx, &bgpPeerList, client.InNamespace(metallbNamespace)) if err != nil { return err } for _, existingPeer := range bgpPeerList.Items { existingPeer := existingPeer found := false - for _, peer := range cfg.cfg.Peers { + + for _, peer := range m.base.Peers { if fmt.Sprintf("peer-%d", peer.ASN) == existingPeer.Name { found = true break } } + if !found { - err := c.Delete(ctx, &existingPeer) + err := m.client.Delete(ctx, &existingPeer) if err != nil { return err } } } - for _, peer := range cfg.cfg.Peers { + for _, peer := range m.base.Peers { bgpPeer := &metallbv1beta2.BGPPeer{ TypeMeta: metav1.TypeMeta{ APIVersion: "metallb.io/v1beta2", @@ -61,7 +64,8 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { Namespace: metallbNamespace, }, } - res, err := controllerutil.CreateOrUpdate(ctx, c, bgpPeer, func() error { + + res, err := controllerutil.CreateOrUpdate(ctx, m.client, bgpPeer, func() error { bgpPeer.Spec = metallbv1beta2.BGPPeerSpec{ MyASN: peer.MyASN, ASN: peer.ASN, @@ -75,33 +79,34 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { if err != nil { return err } + if res != controllerutil.OperationResultNone { klog.Infof("bgppeer: %v", res) } } addressPoolList := metallbv1beta1.IPAddressPoolList{} - err = c.List(ctx, &addressPoolList, client.InNamespace(metallbNamespace)) + err = m.client.List(ctx, &addressPoolList, client.InNamespace(metallbNamespace)) if err != nil { return err } for _, existingPool := range addressPoolList.Items { found := false - for _, pool := range cfg.cfg.AddressPools { + for _, pool := range m.base.AddressPools { if pool.Name == existingPool.Name { found = true break } } if !found { - err := c.Delete(ctx, &existingPool) + err := m.client.Delete(ctx, &existingPool) if err != nil { return err } } } - for _, pool := range cfg.cfg.AddressPools { + for _, pool := range m.base.AddressPools { ipAddressPool := &metallbv1beta1.IPAddressPool{ TypeMeta: metav1.TypeMeta{ APIVersion: "metallb.io/v1beta1", @@ -112,7 +117,8 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { Namespace: metallbNamespace, }, } - res, err := controllerutil.CreateOrUpdate(ctx, c, ipAddressPool, func() error { + + res, err := controllerutil.CreateOrUpdate(ctx, m.client, ipAddressPool, func() error { ipAddressPool.Spec = metallbv1beta1.IPAddressPoolSpec{ Addresses: pool.CIDRs, AutoAssign: pool.AutoAssign, @@ -123,28 +129,30 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { if err != nil { return err } + if res != controllerutil.OperationResultNone { klog.Infof("ipaddresspool: %v", res) } } - for _, pool := range cfg.cfg.AddressPools { + for _, pool := range m.base.AddressPools { bgpAdvertisementList := metallbv1beta1.BGPAdvertisementList{} - err = c.List(ctx, &bgpAdvertisementList, client.InNamespace(metallbNamespace)) + err = m.client.List(ctx, &bgpAdvertisementList, client.InNamespace(metallbNamespace)) if err != nil { return err } + for _, existingAdvertisement := range bgpAdvertisementList.Items { existingAdvertisement := existingAdvertisement found := false - for _, pool := range cfg.cfg.AddressPools { + for _, pool := range m.base.AddressPools { if pool.Name == existingAdvertisement.Name { found = true break } } if !found { - err := c.Delete(ctx, &existingAdvertisement) + err := m.client.Delete(ctx, &existingAdvertisement) if err != nil { return err } @@ -161,7 +169,8 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { Namespace: metallbNamespace, }, } - res, err := controllerutil.CreateOrUpdate(ctx, c, bgpAdvertisement, func() error { + + res, err := controllerutil.CreateOrUpdate(ctx, m.client, bgpAdvertisement, func() error { bgpAdvertisement.Spec = metallbv1beta1.BGPAdvertisementSpec{ IPAddressPools: []string{pool.Name}, } @@ -170,6 +179,7 @@ func (cfg *metalLBConfig) WriteCRs(ctx context.Context, c client.Client) error { if err != nil { return err } + if res != controllerutil.OperationResultNone { klog.Infof("bgpadvertisement: %v", res) } diff --git a/pkg/controllers/loadbalancer/config/metallb_config_test.go b/pkg/controllers/loadbalancer/config/metallb_config_test.go index 53da552..4fad08a 100644 --- a/pkg/controllers/loadbalancer/config/metallb_config_test.go +++ b/pkg/controllers/loadbalancer/config/metallb_config_test.go @@ -49,7 +49,7 @@ func TestMetalLBConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &metalLBConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -90,7 +90,7 @@ func TestMetalLBConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &metalLBConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -144,7 +144,7 @@ func TestMetalLBConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &metalLBConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -246,7 +246,7 @@ func TestMetalLBConfig(t *testing.T) { nodes: []v1.Node{}, wantErr: nil, want: &metalLBConfig{ - cfg: &baseConfig{ + base: &baseConfig{ AddressPools: addressPools{ { Name: "internet-ephemeral", @@ -306,7 +306,7 @@ func TestMetalLBConfig(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - cfg, err := New("metallb", tt.ips, tt.nws, tt.nodes, nil) + cfg, err := New("metallb", tt.ips, tt.nws, tt.nodes, nil, nil) if diff := cmp.Diff(err, tt.wantErr); diff != "" { t.Errorf("error = %v", diff) return diff --git a/pkg/controllers/loadbalancer/loadbalancer.go b/pkg/controllers/loadbalancer/loadbalancer.go index e3f4f73..9c17746 100644 --- a/pkg/controllers/loadbalancer/loadbalancer.go +++ b/pkg/controllers/loadbalancer/loadbalancer.go @@ -341,12 +341,12 @@ func (l *LoadBalancerController) updateLoadBalancerConfig(ctx context.Context, n return fmt.Errorf("could not find ips of this project's cluster: %w", err) } - cfg, err := config.New(l.loadBalancerType, ips, l.additionalNetworks, nodes, l.K8sClientSet) + cfg, err := config.New(l.loadBalancerType, ips, l.additionalNetworks, nodes, l.K8sClient, l.K8sClientSet) if err != nil { return err } - err = cfg.WriteCRs(ctx, l.K8sClient) + err = cfg.WriteCRs(ctx) if err != nil { return err }