Skip to content

Commit

Permalink
Fix findSubnet function's logic when subnet ID is specified
Browse files Browse the repository at this point in the history 
When subnet ID is specified in AWSMachine's spec, it should find the
subnet whether a failureDomain is specified or not.
  • Loading branch information
Winnie Kwon committed Sep 7, 2021
1 parent 8e95619 commit 64dae2f
Show file tree
Hide file tree
Showing 2 changed files with 180 additions and 25 deletions.
48 changes: 23 additions & 25 deletions pkg/cloud/services/ec2/instances.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,34 +262,32 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) {

switch {
case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.ID != nil:
if failureDomain != nil {
subnet := s.scope.Subnets().FindByID(*scope.AWSMachine.Spec.Subnet.ID)
if subnet == nil {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet with id %q not found", aws.StringValue(scope.AWSMachine.Spec.Subnet.ID))
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet with id %q not found",
scope.Name(),
aws.StringValue(scope.AWSMachine.Spec.Subnet.ID),
),
)
}
subnet := s.scope.Subnets().FindByID(*scope.AWSMachine.Spec.Subnet.ID)
if subnet == nil {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet with id %q not found", aws.StringValue(scope.AWSMachine.Spec.Subnet.ID))
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet with id %q not found",
scope.Name(),
aws.StringValue(scope.AWSMachine.Spec.Subnet.ID),
),
)
}

if subnet.AvailabilityZone != *failureDomain {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet's availability zone %q does not match with the failure domain %q",
if failureDomain != nil && subnet.AvailabilityZone != *failureDomain {
record.Warnf(scope.AWSMachine, "FailedCreate",
"Failed to create instance: subnet's availability zone %q does not match with the failure domain %q",
subnet.AvailabilityZone,
*failureDomain)
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet's availability zone %q does not match with the failure domain %q",
scope.Name(),
subnet.AvailabilityZone,
*failureDomain)
return "", awserrors.NewFailedDependency(
fmt.Sprintf("failed to run machine %q, subnet's availability zone %q does not match with the failure domain %q",
scope.Name(),
subnet.AvailabilityZone,
*failureDomain,
),
)
}
*failureDomain,
),
)
}
return *scope.AWSMachine.Spec.Subnet.ID, nil
return subnet.ID, nil
case scope.AWSMachine.Spec.Subnet != nil && scope.AWSMachine.Spec.Subnet.Filters != nil:
criteria := []*ec2.Filter{
filter.EC2.SubnetStates(ec2.SubnetStatePending, ec2.SubnetStateAvailable),
Expand Down
157 changes: 157 additions & 0 deletions pkg/cloud/services/ec2/instances_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,163 @@ func TestCreateInstance(t *testing.T) {
}
},
},
{
name: "with subnet ID that belongs to Cluster",
machine: clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"set": "node"},
},
Spec: clusterv1.MachineSpec{
Bootstrap: clusterv1.Bootstrap{
DataSecretName: pointer.StringPtr("bootstrap-data"),
},
},
},
machineConfig: &infrav1.AWSMachineSpec{
AMI: infrav1.AMIReference{
ID: aws.String("abc"),
},
InstanceType: "m5.large",
Subnet: &infrav1.AWSResourceReference{
ID: aws.String("matching-subnet"),
},
},
awsCluster: &infrav1.AWSCluster{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: infrav1.AWSClusterSpec{
NetworkSpec: infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: "vpc-id",
},
Subnets: infrav1.Subnets{{
ID: "matching-subnet",
}},
},
},
Status: infrav1.AWSClusterStatus{
Network: infrav1.NetworkStatus{
SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{
infrav1.SecurityGroupControlPlane: {
ID: "1",
},
infrav1.SecurityGroupNode: {
ID: "2",
},
infrav1.SecurityGroupLB: {
ID: "3",
},
},
APIServerELB: infrav1.ClassicELB{
DNSName: "test-apiserver.us-east-1.aws",
},
},
},
},
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
m.
RunInstances(gomock.Any()).
Return(&ec2.Reservation{
Instances: []*ec2.Instance{
{
State: &ec2.InstanceState{
Name: aws.String(ec2.InstanceStateNamePending),
},
IamInstanceProfile: &ec2.IamInstanceProfile{
Arn: aws.String("arn:aws:iam::123456789012:instance-profile/foo"),
},
InstanceId: aws.String("two"),
InstanceType: aws.String("m5.large"),
SubnetId: aws.String("matching-subnet"),
ImageId: aws.String("ami-1"),
RootDeviceName: aws.String("device-1"),
BlockDeviceMappings: []*ec2.InstanceBlockDeviceMapping{
{
DeviceName: aws.String("device-1"),
Ebs: &ec2.EbsInstanceBlockDevice{
VolumeId: aws.String("volume-1"),
},
},
},
Placement: &ec2.Placement{
AvailabilityZone: &az,
},
},
},
}, nil)
m.WaitUntilInstanceRunningWithContext(gomock.Any(), gomock.Any(), gomock.Any()).
Return(nil)
},
check: func(instance *infrav1.Instance, err error) {
if err != nil {
t.Fatalf("did not expect error: %v", err)
}
},
},
{
name: "with subnet ID that does not belong to Cluster",
machine: clusterv1.Machine{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"set": "node"},
},
Spec: clusterv1.MachineSpec{
Bootstrap: clusterv1.Bootstrap{
DataSecretName: pointer.StringPtr("bootstrap-data"),
},
},
},
machineConfig: &infrav1.AWSMachineSpec{
AMI: infrav1.AMIReference{
ID: aws.String("abc"),
},
InstanceType: "m5.large",
Subnet: &infrav1.AWSResourceReference{
ID: aws.String("non-matching-subnet"),
},
},
awsCluster: &infrav1.AWSCluster{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: infrav1.AWSClusterSpec{
NetworkSpec: infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: "vpc-id",
},
Subnets: infrav1.Subnets{{
ID: "subnet-1",
}},
},
},
Status: infrav1.AWSClusterStatus{
Network: infrav1.NetworkStatus{
SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{
infrav1.SecurityGroupControlPlane: {
ID: "1",
},
infrav1.SecurityGroupNode: {
ID: "2",
},
infrav1.SecurityGroupLB: {
ID: "3",
},
},
APIServerELB: infrav1.ClassicELB{
DNSName: "test-apiserver.us-east-1.aws",
},
},
},
},
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
},
check: func(instance *infrav1.Instance, err error) {
expectedErrMsg := "failed to run machine \"aws-test1\", subnet with id \"non-matching-subnet\" not found"
if err == nil {
t.Fatalf("Expected error, but got nil")
}

if !strings.Contains(err.Error(), expectedErrMsg) {
t.Fatalf("Expected error: %s\nInstead got: %s", expectedErrMsg, err.Error())
}
},
},
{
name: "subnet id and failureDomain don't match",
machine: clusterv1.Machine{
Expand Down

0 comments on commit 64dae2f

Please sign in to comment.