Skip to content

Commit

Permalink
Merge pull request #3004 from dlipovetsky/dont-store-default-lb-name
Browse files Browse the repository at this point in the history 
fix: Generate, but do not write to the spec, the default API server ELB name
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Dec 8, 2021
2 parents f1dbc04 + ab61b2d commit 9ac7adb
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 36 deletions.
83 changes: 47 additions & 36 deletions pkg/cloud/services/elb/loadbalancer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -147,17 +140,17 @@ 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, "")
if err := s.scope.PatchObject(); err != nil {
return err
}

apiELB, err := s.describeClassicELB(*elbName)
apiELB, err := s.describeClassicELB(elbName)
if IsNotFound(err) {
return nil
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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() {
Expand All @@ -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 {
Expand Down
63 changes: 63 additions & 0 deletions pkg/cloud/services/elb/loadbalancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 9ac7adb

Please sign in to comment.