Skip to content

Commit

Permalink
Merge pull request #3526 from dlmather/support-multiple-additional-se…
Browse files Browse the repository at this point in the history 
…curity-groups

Allow multiple security group filter matches
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Oct 28, 2022
2 parents 7c2c702 + d05f2c1 commit a879df7
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 30 deletions.
6 changes: 3 additions & 3 deletions controllers/awsmachine_controller_unit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -915,7 +915,7 @@ func TestAWSMachineReconciler(t *testing.T) {
g.Expect(ms.AWSMachine.Finalizers).To(ContainElement(infrav1.MachineFinalizer))
expectConditions(g, ms.AWSMachine, []conditionAssertion{{infrav1.SecurityGroupsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityError, infrav1.SecurityGroupsFailedReason}})
})
t.Run("Should return silently if ensureSecurityGroups fails to fetch additional security groups", func(t *testing.T) {
t.Run("Should fail if ensureSecurityGroups fails to fetch additional security groups", func(t *testing.T) {
g := NewWithT(t)
awsMachine := getAWSMachine()
setup(t, g, awsMachine)
Expand All @@ -941,9 +941,9 @@ func TestAWSMachineReconciler(t *testing.T) {
ec2Svc.EXPECT().GetAdditionalSecurityGroupsIDs(gomock.Any()).Return([]string{"sg-1"}, errors.New("failed to get filtered SGs"))

_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs, cs, cs)
g.Expect(err).To(BeNil())
g.Expect(err).ToNot(BeNil())
g.Expect(ms.AWSMachine.Finalizers).To(ContainElement(infrav1.MachineFinalizer))
expectConditions(g, ms.AWSMachine, []conditionAssertion{{infrav1.SecurityGroupsReadyCondition, corev1.ConditionTrue, "", ""}})
expectConditions(g, ms.AWSMachine, []conditionAssertion{{infrav1.SecurityGroupsReadyCondition, corev1.ConditionFalse, clusterv1.ConditionSeverityError, infrav1.SecurityGroupsFailedReason}})
})
t.Run("Should fail to update security group", func(t *testing.T) {
g := NewWithT(t)
Expand Down
2 changes: 1 addition & 1 deletion controllers/awsmachine_security_groups.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (r *AWSMachineReconciler) ensureSecurityGroups(ec2svc service.EC2Interface,

additionalSecurityGroupsIDs, err := ec2svc.GetAdditionalSecurityGroupsIDs(additional)
if err != nil {
return false, nil //nolint:nilerr
return false, err
}

changed, ids := r.securityGroupsChanged(annotation, core, additionalSecurityGroupsIDs, existing)
Expand Down
77 changes: 62 additions & 15 deletions pkg/cloud/services/ec2/instances_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3079,10 +3079,10 @@ func TestGetFilteredSecurityGroupID(t *testing.T) {
name string
securityGroup infrav1.AWSResourceReference
expect func(m *mocks.MockEC2APIMockRecorder)
check func(id string, err error)
check func(ids []string, err error)
}{
{
name: "successfully return security group id",
name: "successfully return single security group id",
securityGroup: infrav1.AWSResourceReference{
Filters: []infrav1.Filter{
{
Expand All @@ -3107,27 +3107,71 @@ func TestGetFilteredSecurityGroupID(t *testing.T) {
},
}, nil)
},
check: func(id string, err error) {
check: func(ids []string, err error) {
if err != nil {
t.Fatalf("did not expect error: %v", err)
}

if id != securityGroupID {
t.Fatalf("expected security group id %v but got: %v", securityGroupID, id)
if ids[0] != securityGroupID {
t.Fatalf("expected security group id %v but got: %v", securityGroupID, ids[0])
}
},
},
{
name: "allow returning multiple security groups",
securityGroup: infrav1.AWSResourceReference{
Filters: []infrav1.Filter{
{
Name: securityGroupFilterName, Values: securityGroupFilterValues,
},
},
},
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DescribeSecurityGroups(gomock.Eq(&ec2.DescribeSecurityGroupsInput{
Filters: []*ec2.Filter{
{
Name: aws.String(securityGroupFilterName),
Values: aws.StringSlice(securityGroupFilterValues),
},
},
})).Return(
&ec2.DescribeSecurityGroupsOutput{
SecurityGroups: []*ec2.SecurityGroup{
{
GroupId: aws.String(securityGroupID),
},
{
GroupId: aws.String(securityGroupID),
},
{
GroupId: aws.String(securityGroupID),
},
},
}, nil)
},
check: func(ids []string, err error) {
if err != nil {
t.Fatalf("did not expect error: %v", err)
}

for _, id := range ids {
if id != securityGroupID {
t.Fatalf("expected security group id %v but got: %v", securityGroupID, id)
}
}
},
},
{
name: "return early when filters are missing",
securityGroup: infrav1.AWSResourceReference{},
expect: func(m *mocks.MockEC2APIMockRecorder) {},
check: func(id string, err error) {
check: func(ids []string, err error) {
if err != nil {
t.Fatalf("did not expect error: %v", err)
}

if id != "" {
t.Fatalf("didn't expect secutity group id %v", id)
if len(ids) > 0 {
t.Fatalf("didn't expect security group ids %v", ids)
}
},
},
Expand All @@ -3150,14 +3194,14 @@ func TestGetFilteredSecurityGroupID(t *testing.T) {
},
})).Return(nil, errors.New("some error"))
},
check: func(id string, err error) {
check: func(_ []string, err error) {
if err == nil {
t.Fatalf("expected error but got none.")
}
},
},
{
name: "error when no security groups found",
name: "no error when no security groups found",
securityGroup: infrav1.AWSResourceReference{
Filters: []infrav1.Filter{
{
Expand All @@ -3178,9 +3222,12 @@ func TestGetFilteredSecurityGroupID(t *testing.T) {
SecurityGroups: []*ec2.SecurityGroup{},
}, nil)
},
check: func(id string, err error) {
if err == nil {
t.Fatalf("expected error but got none.")
check: func(ids []string, err error) {
if err != nil {
t.Fatalf("did not expect error: %v", err)
}
if len(ids) > 0 {
t.Fatalf("didn't expect security group ids %v", ids)
}
},
},
Expand All @@ -3195,8 +3242,8 @@ func TestGetFilteredSecurityGroupID(t *testing.T) {
EC2Client: ec2Mock,
}

id, err := s.getFilteredSecurityGroupID(tc.securityGroup)
tc.check(id, err)
ids, err := s.getFilteredSecurityGroupIDs(tc.securityGroup)
tc.check(ids, err)
})
}
}
21 changes: 10 additions & 11 deletions pkg/cloud/services/ec2/launchtemplate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package ec2
import (
"encoding/base64"
"encoding/json"
"fmt"
"sort"
"strconv"
"strings"
Expand Down Expand Up @@ -772,12 +771,12 @@ func (s *Service) GetAdditionalSecurityGroupsIDs(securityGroups []infrav1.AWSRes
if sg.ID != nil {
additionalSecurityGroupsIDs = append(additionalSecurityGroupsIDs, *sg.ID)
} else if sg.Filters != nil {
id, err := s.getFilteredSecurityGroupID(sg)
ids, err := s.getFilteredSecurityGroupIDs(sg)
if err != nil {
return nil, err
}

additionalSecurityGroupsIDs = append(additionalSecurityGroupsIDs, id)
additionalSecurityGroupsIDs = append(additionalSecurityGroupsIDs, ids...)
}
}

Expand Down Expand Up @@ -822,10 +821,10 @@ func (s *Service) buildLaunchTemplateTagSpecificationRequest(scope scope.LaunchT
return tagSpecifications
}

// getFilteredSecurityGroupID get security group ID using filters.
func (s *Service) getFilteredSecurityGroupID(securityGroup infrav1.AWSResourceReference) (string, error) {
// getFilteredSecurityGroupIDs get security group IDs using filters.
func (s *Service) getFilteredSecurityGroupIDs(securityGroup infrav1.AWSResourceReference) ([]string, error) {
if securityGroup.Filters == nil {
return "", nil
return nil, nil
}

filters := []*ec2.Filter{}
Expand All @@ -835,14 +834,14 @@ func (s *Service) getFilteredSecurityGroupID(securityGroup infrav1.AWSResourceRe

sgs, err := s.EC2Client.DescribeSecurityGroups(&ec2.DescribeSecurityGroupsInput{Filters: filters})
if err != nil {
return "", err
return nil, err
}

if len(sgs.SecurityGroups) == 0 {
return "", fmt.Errorf("failed to find security group matching filters: %q, reason: %w", filters, err)
ids := make([]string, 0, len(sgs.SecurityGroups))
for _, sg := range sgs.SecurityGroups {
ids = append(ids, *sg.GroupId)
}

return *sgs.SecurityGroups[0].GroupId, nil
return ids, nil
}

func getLaunchTemplateInstanceMarketOptionsRequest(spotMarketOptions *infrav1.SpotMarketOptions) *ec2.LaunchTemplateInstanceMarketOptionsRequest {
Expand Down

0 comments on commit a879df7

Please sign in to comment.