From e941970547662b3f6ea6d3c6b3dba4206d81b87b Mon Sep 17 00:00:00 2001 From: Gerrit Date: Mon, 28 Oct 2024 13:57:54 +0100 Subject: [PATCH] Review comments. --- metal/cloud.go | 5 +++-- pkg/controllers/loadbalancer/config/addresspool.go | 9 +-------- pkg/controllers/loadbalancer/config/cilium_config.go | 4 ++++ .../loadbalancer/config/cilium_config_test.go | 2 +- pkg/controllers/loadbalancer/config/config.go | 7 ++++--- pkg/controllers/loadbalancer/config/metallb_config.go | 2 ++ .../loadbalancer/config/metallb_config_test.go | 2 +- pkg/controllers/loadbalancer/loadbalancer.go | 4 ++-- 8 files changed, 18 insertions(+), 17 deletions(-) diff --git a/metal/cloud.go b/metal/cloud.go index cfbbfba..5ad71fc 100644 --- a/metal/cloud.go +++ b/metal/cloud.go @@ -13,6 +13,7 @@ import ( "github.com/metal-stack/metal-ccm/pkg/controllers/housekeeping" "github.com/metal-stack/metal-ccm/pkg/controllers/instances" "github.com/metal-stack/metal-ccm/pkg/controllers/loadbalancer" + "github.com/metal-stack/metal-ccm/pkg/controllers/loadbalancer/config" "github.com/metal-stack/metal-ccm/pkg/controllers/zones" "github.com/metal-stack/metal-ccm/pkg/resources/constants" "github.com/metal-stack/metal-ccm/pkg/resources/metal" @@ -47,7 +48,7 @@ func NewCloud(_ io.Reader) (cloudprovider.Interface, error) { partitionID := os.Getenv(constants.MetalPartitionIDEnvVar) clusterID := os.Getenv(constants.MetalClusterIDEnvVar) defaultExternalNetworkID := os.Getenv(constants.MetalDefaultExternalNetworkEnvVar) - loadbalancerType := os.Getenv(constants.Loadbalancer) + loadbalancerType := config.LoadBalancerType(os.Getenv(constants.Loadbalancer)) var ( additionalNetworksString = os.Getenv(constants.MetalAdditionalNetworks) @@ -80,7 +81,7 @@ func NewCloud(_ io.Reader) (cloudprovider.Interface, error) { return nil, fmt.Errorf("environment variable %q or %q is required", constants.MetalAuthTokenEnvVar, constants.MetalAuthHMACEnvVar) } - if !slices.Contains([]string{"cilium", "metallb", ""}, loadbalancerType) { + if !slices.Contains([]config.LoadBalancerType{config.LoadBalancerTypeCilium, config.LoadBalancerTypeMetalLB, ""}, loadbalancerType) { klog.Fatalf("only cilium or metallb load balancer types are supported") } diff --git a/pkg/controllers/loadbalancer/config/addresspool.go b/pkg/controllers/loadbalancer/config/addresspool.go index 5470cb5..821712e 100644 --- a/pkg/controllers/loadbalancer/config/addresspool.go +++ b/pkg/controllers/loadbalancer/config/addresspool.go @@ -42,14 +42,7 @@ func (pool *addressPool) appendIP(ip *models.V1IPResponse) error { return err } - var cidr string - if parsed.Is4() { - cidr = parsed.String() + "/32" - } else if parsed.Is6() { - cidr = parsed.String() + "/128" - } else { - return fmt.Errorf("unknown addressfamily of ip: %s", parsed.String()) - } + cidr := fmt.Sprintf("%s/%d", parsed.String(), parsed.BitLen()) if pool.containsCIDR(cidr) { return nil diff --git a/pkg/controllers/loadbalancer/config/cilium_config.go b/pkg/controllers/loadbalancer/config/cilium_config.go index ac6cd7e..931dac5 100644 --- a/pkg/controllers/loadbalancer/config/cilium_config.go +++ b/pkg/controllers/loadbalancer/config/cilium_config.go @@ -20,6 +20,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) +const ( + LoadBalancerTypeCilium LoadBalancerType = "cilium" +) + type ciliumConfig struct { base *baseConfig client client.Client diff --git a/pkg/controllers/loadbalancer/config/cilium_config_test.go b/pkg/controllers/loadbalancer/config/cilium_config_test.go index df8389c..d89a34d 100644 --- a/pkg/controllers/loadbalancer/config/cilium_config_test.go +++ b/pkg/controllers/loadbalancer/config/cilium_config_test.go @@ -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, nil) + cfg, err := New(LoadBalancerTypeCilium, 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 583741b..f79d421 100644 --- a/pkg/controllers/loadbalancer/config/config.go +++ b/pkg/controllers/loadbalancer/config/config.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +type LoadBalancerType string type LoadBalancerConfig interface { WriteCRs(ctx context.Context) error } @@ -24,16 +25,16 @@ 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, c client.Client, k8sClientSet clientset.Interface) (LoadBalancerConfig, error) { +func New(loadBalancerType LoadBalancerType, 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 } switch loadBalancerType { - case "metallb": + case LoadBalancerTypeMetalLB: return newMetalLBConfig(bc, c), nil - case "cilium": + case LoadBalancerTypeCilium: return newCiliumConfig(bc, c, k8sClientSet), nil default: return newMetalLBConfig(bc, c), nil diff --git a/pkg/controllers/loadbalancer/config/metallb_config.go b/pkg/controllers/loadbalancer/config/metallb_config.go index 1e9a9ed..7dfc8de 100644 --- a/pkg/controllers/loadbalancer/config/metallb_config.go +++ b/pkg/controllers/loadbalancer/config/metallb_config.go @@ -16,6 +16,8 @@ import ( ) const ( + LoadBalancerTypeMetalLB LoadBalancerType = "metallb" + metallbNamespace = "metallb-system" ) diff --git a/pkg/controllers/loadbalancer/config/metallb_config_test.go b/pkg/controllers/loadbalancer/config/metallb_config_test.go index 4fad08a..99a2b6b 100644 --- a/pkg/controllers/loadbalancer/config/metallb_config_test.go +++ b/pkg/controllers/loadbalancer/config/metallb_config_test.go @@ -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, nil) + cfg, err := New(LoadBalancerTypeMetalLB, 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 9c17746..647114e 100644 --- a/pkg/controllers/loadbalancer/loadbalancer.go +++ b/pkg/controllers/loadbalancer/loadbalancer.go @@ -42,11 +42,11 @@ type LoadBalancerController struct { configWriteMutex *sync.Mutex ipAllocateMutex *sync.Mutex ipUpdateMutex *sync.Mutex - loadBalancerType string + loadBalancerType config.LoadBalancerType } // New returns a new load balancer controller that satisfies the kubernetes cloud provider load balancer interface -func New(partitionID, projectID, clusterID, defaultExternalNetworkID string, additionalNetworks []string, loadBalancerType string) *LoadBalancerController { +func New(partitionID, projectID, clusterID, defaultExternalNetworkID string, additionalNetworks []string, loadBalancerType config.LoadBalancerType) *LoadBalancerController { return &LoadBalancerController{ partitionID: partitionID, projectID: projectID,