diff --git a/pkg/apis/awsproviderconfig/v1alpha1/awsmachineproviderconfig_types.go b/pkg/apis/awsproviderconfig/v1alpha1/awsmachineproviderconfig_types.go index e974f7dfeb..50619c1144 100644 --- a/pkg/apis/awsproviderconfig/v1alpha1/awsmachineproviderconfig_types.go +++ b/pkg/apis/awsproviderconfig/v1alpha1/awsmachineproviderconfig_types.go @@ -133,9 +133,9 @@ type AWSMachineProviderConfig struct { // Placement specifies where to create the instance in AWS Placement Placement - // LoadBalancerIDs is the IDs of the load balancers to which the new instance + // LoadBalancerNames is the names of the load balancers to which the new instance // should be added once it is created. - LoadBalancerIDs []AWSResourceReference + LoadBalancerNames []string } // AWSResourceReference is a reference to a specific AWS resource by ID, ARN, or filters. diff --git a/pkg/apis/awsproviderconfig/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/awsproviderconfig/v1alpha1/zz_generated.deepcopy.go index d949a9c9f0..142489564b 100644 --- a/pkg/apis/awsproviderconfig/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/awsproviderconfig/v1alpha1/zz_generated.deepcopy.go @@ -87,12 +87,10 @@ func (in *AWSMachineProviderConfig) DeepCopyInto(out *AWSMachineProviderConfig) } in.Subnet.DeepCopyInto(&out.Subnet) out.Placement = in.Placement - if in.LoadBalancerIDs != nil { - in, out := &in.LoadBalancerIDs, &out.LoadBalancerIDs - *out = make([]AWSResourceReference, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } + if in.LoadBalancerNames != nil { + in, out := &in.LoadBalancerNames, &out.LoadBalancerNames + *out = make([]string, len(*in)) + copy(*out, *in) } return } diff --git a/pkg/cloud/aws/actuators/machine/actuator.go b/pkg/cloud/aws/actuators/machine/actuator.go index a1abcc91d8..00e98b1e91 100644 --- a/pkg/cloud/aws/actuators/machine/actuator.go +++ b/pkg/cloud/aws/actuators/machine/actuator.go @@ -35,6 +35,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/elb" "k8s.io/apimachinery/pkg/runtime" awsclient "sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/aws/client" @@ -121,12 +122,6 @@ func (a *Actuator) Create(cluster *clusterv1.Cluster, machine *clusterv1.Machine } return err } - - // TODO(csrwng): - // Part of the status that gets updated when the machine gets created is the PublicIP. - // However, after a call to runInstance, most of the time a PublicIP is not yet allocated. - // If we don't yet have complete status (ie. the instance state is Pending, instead of Running), - // maybe we should return an error so the machine controller keeps retrying until we have complete status we can set. return a.updateStatus(machine, instance, mLog) } @@ -433,14 +428,11 @@ func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1. return nil, fmt.Errorf("unexpected reservation creating instance") } - // TOOD(csrwng): - // One issue we have right now with how this works, is that usually at the end of the RunInstances call, - // the instance state is not yet 'Running'. Rather it is 'Pending'. Therefore the status - // is not yet complete (like PublicIP). One possible fix would be to wait and poll here - // until the instance is in the Running state. The other approach is to return an error - // so that the machine is requeued and in the exists function return false if the status doesn't match. - // That would require making the create re-entrant so we can just update the status. - return runResult.Instances[0], nil + instance := runResult.Instances[0] + + err = a.UpdateLoadBalancers(client, machineProviderConfig, instance, mLog) + + return instance, err } // Delete deletes a machine and updates its finalizer @@ -547,6 +539,11 @@ func (a *Actuator) Update(cluster *clusterv1.Cluster, machine *clusterv1.Machine } + err = a.UpdateLoadBalancers(client, machineProviderConfig, newestInstance, mLog) + if err != nil { + return err + } + // We do not support making changes to pre-existing instances, just update status. return a.updateStatus(machine, newestInstance, mLog) } @@ -613,6 +610,30 @@ func (a *Actuator) getMachineInstances(cluster *clusterv1.Cluster, machine *clus return GetRunningInstances(machine, client) } +// UpdateLoadBalancers adds a given machine instance to the load balancers specified in its provider config +func (a *Actuator) UpdateLoadBalancers(client awsclient.Client, providerConfig *providerconfigv1.AWSMachineProviderConfig, instance *ec2.Instance, mLog log.FieldLogger) error { + if len(providerConfig.LoadBalancerNames) == 0 { + return nil + } + elbInstance := &elb.Instance{InstanceId: instance.InstanceId} + var errs []error + for _, elbName := range providerConfig.LoadBalancerNames { + req := &elb.RegisterInstancesWithLoadBalancerInput{ + Instances: []*elb.Instance{elbInstance}, + LoadBalancerName: aws.String(elbName), + } + _, err := client.RegisterInstancesWithLoadBalancer(req) + if err != nil { + errs = append(errs, fmt.Errorf("%s: %v", elbName, err)) + } + } + + if len(errs) > 0 { + return fmt.Errorf("failed to register instances with elbs: %v", errs) + } + return nil +} + // updateStatus calculates the new machine status, checks if anything has changed, and updates if so. func (a *Actuator) updateStatus(machine *clusterv1.Machine, instance *ec2.Instance, mLog log.FieldLogger) error { diff --git a/pkg/cloud/aws/actuators/machine/actuator_test.go b/pkg/cloud/aws/actuators/machine/actuator_test.go index a140f32ed7..25f3fac2be 100644 --- a/pkg/cloud/aws/actuators/machine/actuator_test.go +++ b/pkg/cloud/aws/actuators/machine/actuator_test.go @@ -113,6 +113,11 @@ func testMachineAPIResources(clusterID string) (*clusterv1.Machine, *clusterv1.C {ID: aws.String("sg-08b1ffd32874d59a2")}, // aws-actuator_infra_k8s }, PublicIP: aws.Bool(true), + LoadBalancerNames: []string{ + "cluster-con", + "cluster-ext", + "cluster-int", + }, } codec, err := providerconfigv1.NewCodec() @@ -207,6 +212,7 @@ func TestCreateAndDeleteMachine(t *testing.T) { mockRunInstances(mockAWSClient, tc.createErrorExpected) mockDescribeInstances(mockAWSClient, tc.createErrorExpected) mockTerminateInstances(mockAWSClient) + mockRegisterInstancesWithLoadBalancer(mockAWSClient, tc.createErrorExpected) // Create the machine createErr := actuator.Create(cluster, machine) @@ -369,3 +375,14 @@ func TestRemoveDuplicatedTags(t *testing.T) { } } } + +func mockRegisterInstancesWithLoadBalancer(mockAWSClient *mockaws.MockClient, createError bool) { + if createError { + return + } + // RegisterInstancesWithLoadBalancer should be called for every load balancer name in the machine + // spec for create and for update (3 * 2 = 6) + for i := 0; i < 6; i++ { + mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()) + } +}