diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index 59b3ecbf96..7533199074 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -32,6 +32,7 @@ import ( infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1" "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/awserrors" "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/converters" + "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope" "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/wait" "sigs.k8s.io/cluster-api-provider-aws/pkg/hash" "sigs.k8s.io/cluster-api-provider-aws/pkg/record" @@ -63,21 +64,13 @@ func (s *Service) ReconcileLoadbalancers() error { // Generate a default control plane load balancer name. The load balancer name cannot be // generated by the defaulting webhook, because it is derived from the cluster name, and that // name is undefined at defaulting time when generateName is used. - if s.scope.ControlPlaneLoadBalancerName() == nil { - generatedName, err := GenerateELBName(s.scope.Name()) - if err != nil { - return errors.Wrap(err, "failed to generate control plane load balancer name") - } - - s.scope.ControlPlaneLoadBalancer().Name = aws.String(generatedName) - if err := s.scope.PatchObject(); err != nil { - return err - } - s.scope.V(4).Info("Patched control plane load balancer name") + name, err := ELBName(s.scope) + if err != nil { + return errors.Wrap(err, "failed to get control plane load balancer name") } // Get default api server spec. - spec, err := s.getAPIServerClassicELBSpec(*s.scope.ControlPlaneLoadBalancerName()) + spec, err := s.getAPIServerClassicELBSpec(name) if err != nil { return err } @@ -147,9 +140,9 @@ func (s *Service) ReconcileLoadbalancers() error { func (s *Service) deleteAPIServerELB() error { s.scope.V(2).Info("Deleting control plane load balancer") - elbName := s.scope.ControlPlaneLoadBalancerName() - if elbName == nil { - return fmt.Errorf("control plane load balancer name is not defined") + elbName, err := ELBName(s.scope) + if err != nil { + return errors.Wrap(err, "failed to get control plane load balancer name") } conditions.MarkFalse(s.scope.InfraCluster(), infrav1.LoadBalancerReadyCondition, clusterv1.DeletingReason, clusterv1.ConditionSeverityInfo, "") @@ -157,7 +150,7 @@ func (s *Service) deleteAPIServerELB() error { return err } - apiELB, err := s.describeClassicELB(*elbName) + apiELB, err := s.describeClassicELB(elbName) if IsNotFound(err) { return nil } @@ -170,14 +163,14 @@ func (s *Service) deleteAPIServerELB() error { return nil } - s.scope.V(3).Info("deleting load balancer", "name", *elbName) - if err := s.deleteClassicELB(*elbName); err != nil { + s.scope.V(3).Info("deleting load balancer", "name", elbName) + if err := s.deleteClassicELB(elbName); err != nil { conditions.MarkFalse(s.scope.InfraCluster(), infrav1.LoadBalancerReadyCondition, "DeletingFailed", clusterv1.ConditionSeverityWarning, err.Error()) return err } if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (done bool, err error) { - _, err = s.describeClassicELB(*elbName) + _, err = s.describeClassicELB(elbName) done = IsNotFound(err) return done, nil }); err != nil { @@ -240,21 +233,21 @@ func (s *Service) DeleteLoadbalancers() error { // InstanceIsRegisteredWithAPIServerELB returns true if the instance is already registered with the APIServer ELB. func (s *Service) InstanceIsRegisteredWithAPIServerELB(i *infrav1.Instance) (bool, error) { - name := s.scope.ControlPlaneLoadBalancerName() - if name == nil { - return false, fmt.Errorf("control plane load balancer name is not defined") + name, err := ELBName(s.scope) + if err != nil { + return false, errors.Wrap(err, "failed to get control plane load balancer name") } input := &elb.DescribeLoadBalancersInput{ - LoadBalancerNames: []*string{name}, + LoadBalancerNames: []*string{aws.String(name)}, } output, err := s.ELBClient.DescribeLoadBalancers(input) if err != nil { - return false, errors.Wrapf(err, "error describing ELB %q", *name) + return false, errors.Wrapf(err, "error describing ELB %q", name) } if len(output.LoadBalancerDescriptions) != 1 { - return false, errors.Errorf("expected 1 ELB description for %q, got %d", *name, len(output.LoadBalancerDescriptions)) + return false, errors.Errorf("expected 1 ELB description for %q, got %d", name, len(output.LoadBalancerDescriptions)) } for _, registeredInstance := range output.LoadBalancerDescriptions[0].Instances { @@ -268,11 +261,11 @@ func (s *Service) InstanceIsRegisteredWithAPIServerELB(i *infrav1.Instance) (boo // RegisterInstanceWithAPIServerELB registers an instance with a classic ELB. func (s *Service) RegisterInstanceWithAPIServerELB(i *infrav1.Instance) error { - name := s.scope.ControlPlaneLoadBalancerName() - if name == nil { - return fmt.Errorf("control plane load balancer name is not defined") + name, err := ELBName(s.scope) + if err != nil { + return errors.Wrap(err, "failed to get control plane load balancer name") } - out, err := s.describeClassicELB(*name) + out, err := s.describeClassicELB(name) if err != nil { return err } @@ -302,12 +295,12 @@ func (s *Service) RegisterInstanceWithAPIServerELB(i *infrav1.Instance) error { } } if !found { - return errors.Errorf("failed to register instance with APIServer ELB %q: instance is in availability zone %q, no public subnets attached to the ELB in the same zone", *name, instanceAZ) + return errors.Errorf("failed to register instance with APIServer ELB %q: instance is in availability zone %q, no public subnets attached to the ELB in the same zone", name, instanceAZ) } input := &elb.RegisterInstancesWithLoadBalancerInput{ Instances: []*elb.Instance{{InstanceId: aws.String(i.ID)}}, - LoadBalancerName: name, + LoadBalancerName: aws.String(name), } _, err = s.ELBClient.RegisterInstancesWithLoadBalancer(input) @@ -339,17 +332,17 @@ func (s *Service) getControlPlaneLoadBalancerSubnets() (infrav1.Subnets, error) // DeregisterInstanceFromAPIServerELB de-registers an instance from a classic ELB. func (s *Service) DeregisterInstanceFromAPIServerELB(i *infrav1.Instance) error { - name := s.scope.ControlPlaneLoadBalancerName() - if name == nil { - return fmt.Errorf("control plane load balancer name is not defined") + name, err := ELBName(s.scope) + if err != nil { + return errors.Wrap(err, "failed to get control plane load balancer name") } input := &elb.DeregisterInstancesFromLoadBalancerInput{ Instances: []*elb.Instance{{InstanceId: aws.String(i.ID)}}, - LoadBalancerName: name, + LoadBalancerName: aws.String(name), } - _, err := s.ELBClient.DeregisterInstancesFromLoadBalancer(input) + _, err = s.ELBClient.DeregisterInstancesFromLoadBalancer(input) if err != nil { if aerr, ok := err.(awserr.Error); ok { switch aerr.Code() { @@ -364,9 +357,27 @@ func (s *Service) DeregisterInstanceFromAPIServerELB(i *infrav1.Instance) error return err } +// ELBName returns the user-defined API Server ELB name, or a generated default if the user has not defined the ELB +// name. +func ELBName(s scope.ELBScope) (string, error) { + if userDefinedName := s.ControlPlaneLoadBalancerName(); userDefinedName != nil { + return *userDefinedName, nil + } + name, err := GenerateELBName(s.Name()) + if err != nil { + return "", fmt.Errorf("failed to generate name: %w", err) + } + return name, nil +} + // GenerateELBName generates a formatted ELB name via either // concatenating the cluster name to the "-apiserver" suffix // or computing a hash for clusters with names above 32 characters. +// +// WARNING If this function's output is changed, a controller using the +// new function will fail to generate the load balancer of an existing +// cluster whose load balancer name was generated using the old +// function. func GenerateELBName(clusterName string) (string, error) { standardELBName := generateStandardELBName(clusterName) if len(standardELBName) <= 32 { diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go index 259cdf7a00..5b21b3c9a1 100644 --- a/pkg/cloud/services/elb/loadbalancer_test.go +++ b/pkg/cloud/services/elb/loadbalancer_test.go @@ -41,6 +41,69 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +func TestELBName(t *testing.T) { + tests := []struct { + name string + awsCluster infrav1.AWSCluster + expected string + }{ + { + name: "name is not defined by user, so generate the default", + awsCluster: infrav1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: metav1.NamespaceDefault, + }, + }, + expected: "example-apiserver", + }, + { + name: "name is defined by user, so use it", + awsCluster: infrav1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "example", + Namespace: metav1.NamespaceDefault, + }, + Spec: infrav1.AWSClusterSpec{ + ControlPlaneLoadBalancer: &infrav1.AWSLoadBalancerSpec{ + Name: pointer.String("myapiserver"), + }, + }, + }, + expected: "myapiserver", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + _ = infrav1.AddToScheme(scheme) + client := fake.NewClientBuilder().WithScheme(scheme).Build() + + scope, err := scope.NewClusterScope(scope.ClusterScopeParams{ + Client: client, + Cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: tt.awsCluster.Name, + Namespace: tt.awsCluster.Namespace, + }, + }, + AWSCluster: &tt.awsCluster, + }) + if err != nil { + t.Fatalf("failed to create scope: %s", err) + } + + elbName, err := ELBName(scope) + if err != nil { + t.Fatalf("unable to get ELB name: %v", err) + } + if elbName != tt.expected { + t.Fatalf("expected ELB name: %v, got name: %v", tt.expected, elbName) + } + }) + } +} + func TestGenerateELBName(t *testing.T) { tests := []struct { name string