Skip to content

Commit d1c461e

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #3196 from shivi28/sg_testcases
Added test cases for pkg/cloud/services/securitygroup
2 parents f621c48 + 701ab1a commit d1c461e

File tree

2 files changed

+137
-29
lines changed

2 files changed

+137
-29
lines changed

‎pkg/cloud/services/securitygroup/securitygroups.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,7 @@ func (s *Service) revokeAllSecurityGroupIngressRules(id string) error {
423423

424424
securityGroups, err := s.EC2Client.DescribeSecurityGroups(describeInput)
425425
if err != nil {
426-
return errors.Wrapf(err, "failed to query security group %q", id)
426+
return err
427427
}
428428

429429
for _, sg := range securityGroups.SecurityGroups {
@@ -434,7 +434,7 @@ func (s *Service) revokeAllSecurityGroupIngressRules(id string) error {
434434
}
435435
if _, err := s.EC2Client.RevokeSecurityGroupIngress(revokeInput); err != nil {
436436
record.Warnf(s.scope.InfraCluster(), "FailedRevokeSecurityGroupIngressRules", "Failed to revoke all security group ingress rules for SecurityGroup %q: %v", *sg.GroupId, err)
437-
return errors.Wrapf(err, "failed to revoke security group %q ingress rules", id)
437+
return err
438438
}
439439
record.Eventf(s.scope.InfraCluster(), "SuccessfulRevokeSecurityGroupIngressRules", "Revoked all security group ingress rules for SecurityGroup %q", *sg.GroupId)
440440
}

‎pkg/cloud/services/securitygroup/securitygroups_test.go

Lines changed: 135 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,11 @@ limitations under the License.
1717
package securitygroup
1818

1919
import (
20-
"context"
2120
"strings"
2221
"testing"
2322

2423
"github.com/aws/aws-sdk-go/aws"
24+
"github.com/aws/aws-sdk-go/aws/awserr"
2525
"github.com/aws/aws-sdk-go/service/ec2"
2626
"github.com/golang/mock/gomock"
2727
. "github.com/onsi/gomega"
@@ -32,6 +32,7 @@ import (
3232
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3333

3434
infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1beta1"
35+
"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/awserrors"
3536
"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
3637
"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services"
3738
"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/ec2/mock_ec2iface"
@@ -340,7 +341,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
340341
scheme := runtime.NewScheme()
341342
_ = infrav1.AddToScheme(scheme)
342343
client := fake.NewClientBuilder().WithScheme(scheme).Build()
343-
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
344+
cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
344345
Client: client,
345346
Cluster: &clusterv1.Cluster{
346347
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
@@ -358,7 +359,7 @@ func TestReconcileSecurityGroups(t *testing.T) {
358359

359360
tc.expect(ec2Mock.EXPECT())
360361

361-
s := NewService(scope, testSecurityGroupRoles)
362+
s := NewService(cs, testSecurityGroupRoles)
362363
s.EC2Client = ec2Mock
363364

364365
if err := s.ReconcileSecurityGroups(); err != nil && tc.err != nil {
@@ -376,7 +377,7 @@ func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
376377
scheme := runtime.NewScheme()
377378
_ = infrav1.AddToScheme(scheme)
378379
client := fake.NewClientBuilder().WithScheme(scheme).Build()
379-
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
380+
cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
380381
Client: client,
381382
Cluster: &clusterv1.Cluster{
382383
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
@@ -387,7 +388,7 @@ func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
387388
t.Fatalf("Failed to create test context: %v", err)
388389
}
389390

390-
s := NewService(scope, testSecurityGroupRoles)
391+
s := NewService(cs, testSecurityGroupRoles)
391392
rules, err := s.getSecurityGroupIngressRules(infrav1.SecurityGroupControlPlane)
392393
if err != nil {
393394
t.Fatalf("Failed to lookup controlplane security group ingress rules: %v", err)
@@ -405,10 +406,10 @@ func TestDeleteSecurityGroups(t *testing.T) {
405406
defer mockCtrl.Finish()
406407

407408
testCases := []struct {
408-
name string
409-
input *infrav1.NetworkSpec
410-
expect func(m *mock_ec2iface.MockEC2APIMockRecorder)
411-
err error
409+
name string
410+
input *infrav1.NetworkSpec
411+
expect func(m *mock_ec2iface.MockEC2APIMockRecorder)
412+
wantErr bool
412413
}{
413414
{
414415
name: "do not delete overridden security groups",
@@ -439,17 +440,115 @@ func TestDeleteSecurityGroups(t *testing.T) {
439440
},
440441
},
441442
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
442-
m.DescribeSecurityGroupsPages(gomock.Any(), gomock.Any()).Return(nil)
443+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).Return(nil)
444+
},
445+
},
446+
{
447+
name: "Should skip SG deletion if VPC ID not present",
448+
input: &infrav1.NetworkSpec{
449+
VPC: infrav1.VPCSpec{},
450+
},
451+
},
452+
{
453+
name: "Should return error if unable to find cluster-owned security groups in vpc",
454+
input: &infrav1.NetworkSpec{
455+
VPC: infrav1.VPCSpec{ID: "vpc-id"},
456+
},
457+
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
458+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).Return(awserrors.NewFailedDependency("dependency-failure"))
459+
},
460+
wantErr: true,
461+
},
462+
{
463+
name: "Should return error if unable to describe any SG present in VPC and owned by cluster",
464+
input: &infrav1.NetworkSpec{
465+
VPC: infrav1.VPCSpec{ID: "vpc-id"},
466+
},
467+
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
468+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
469+
Do(processSecurityGroupsPage).Return(nil)
470+
m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(nil, awserr.New("dependency-failure", "dependency-failure", errors.Errorf("dependency-failure")))
471+
},
472+
wantErr: true,
473+
},
474+
{
475+
name: "Should not revoke Ingress rules for a SG if IP permissions are not set and able to delete the SG",
476+
input: &infrav1.NetworkSpec{
477+
VPC: infrav1.VPCSpec{ID: "vpc-id"},
478+
},
479+
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
480+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
481+
Do(processSecurityGroupsPage).Return(nil)
482+
m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
483+
SecurityGroups: []*ec2.SecurityGroup{
484+
{
485+
GroupId: aws.String("group-id"),
486+
GroupName: aws.String("group-name"),
487+
},
488+
},
489+
}, nil)
490+
m.DeleteSecurityGroup(gomock.AssignableToTypeOf(&ec2.DeleteSecurityGroupInput{})).Return(nil, nil)
491+
},
492+
},
493+
{
494+
name: "Should return error if failed to revoke Ingress rules for a SG",
495+
input: &infrav1.NetworkSpec{
496+
VPC: infrav1.VPCSpec{ID: "vpc-id"},
497+
},
498+
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
499+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
500+
Do(processSecurityGroupsPage).Return(nil)
501+
m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
502+
SecurityGroups: []*ec2.SecurityGroup{
503+
{
504+
GroupId: aws.String("group-id"),
505+
GroupName: aws.String("group-name"),
506+
IpPermissions: []*ec2.IpPermission{
507+
{
508+
ToPort: aws.Int64(4),
509+
},
510+
},
511+
},
512+
},
513+
}, nil)
514+
m.RevokeSecurityGroupIngress(gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{})).Return(nil, awserr.New("failure", "failure", errors.Errorf("failure")))
515+
},
516+
wantErr: true,
517+
},
518+
{
519+
name: "Should delete SG successfully",
520+
input: &infrav1.NetworkSpec{
521+
VPC: infrav1.VPCSpec{ID: "vpc-id"},
522+
},
523+
expect: func(m *mock_ec2iface.MockEC2APIMockRecorder) {
524+
m.DescribeSecurityGroupsPages(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{}), gomock.Any()).
525+
Do(processSecurityGroupsPage).Return(nil)
526+
m.DescribeSecurityGroups(gomock.AssignableToTypeOf(&ec2.DescribeSecurityGroupsInput{})).Return(&ec2.DescribeSecurityGroupsOutput{
527+
SecurityGroups: []*ec2.SecurityGroup{
528+
{
529+
GroupId: aws.String("group-id"),
530+
GroupName: aws.String("group-name"),
531+
IpPermissions: []*ec2.IpPermission{
532+
{
533+
ToPort: aws.Int64(4),
534+
},
535+
},
536+
},
537+
},
538+
}, nil)
539+
m.RevokeSecurityGroupIngress(gomock.AssignableToTypeOf(&ec2.RevokeSecurityGroupIngressInput{})).Return(nil, nil)
540+
m.DeleteSecurityGroup(gomock.AssignableToTypeOf(&ec2.DeleteSecurityGroupInput{})).Return(nil, nil)
443541
},
444542
},
445543
}
446-
447544
for _, tc := range testCases {
448545
t.Run(tc.name, func(t *testing.T) {
546+
g := NewWithT(t)
449547
ec2Mock := mock_ec2iface.NewMockEC2API(mockCtrl)
450548

451549
scheme := runtime.NewScheme()
452-
_ = infrav1.AddToScheme(scheme)
550+
g.Expect(infrav1.AddToScheme(scheme)).NotTo(HaveOccurred())
551+
453552
awsCluster := &infrav1.AWSCluster{
454553
TypeMeta: metav1.TypeMeta{
455554
APIVersion: infrav1.GroupVersion.String(),
@@ -460,34 +559,31 @@ func TestDeleteSecurityGroups(t *testing.T) {
460559
NetworkSpec: *tc.input,
461560
},
462561
}
463-
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(awsCluster).Build()
464562

465-
ctx := context.TODO()
466-
client.Create(ctx, awsCluster)
563+
client := fake.NewClientBuilder().WithScheme(scheme).WithObjects(awsCluster).Build()
467564

468-
scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
565+
cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
469566
Client: client,
470567
Cluster: &clusterv1.Cluster{
471568
ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
472569
},
473570
AWSCluster: awsCluster,
474571
})
475-
if err != nil {
476-
t.Fatalf("Failed to create test context: %v", err)
477-
}
572+
g.Expect(err).NotTo(HaveOccurred())
478573

479-
tc.expect(ec2Mock.EXPECT())
574+
if tc.expect != nil {
575+
tc.expect(ec2Mock.EXPECT())
576+
}
480577

481-
s := NewService(scope, testSecurityGroupRoles)
578+
s := NewService(cs, testSecurityGroupRoles)
482579
s.EC2Client = ec2Mock
483580

484-
if err := s.DeleteSecurityGroups(); err != nil && tc.err != nil {
485-
if !strings.Contains(err.Error(), tc.err.Error()) {
486-
t.Fatalf("was expecting error to look like '%v', but got '%v'", tc.err, err)
487-
}
488-
} else if err != nil {
489-
t.Fatalf("got an unexpected error: %v", err)
581+
err = s.DeleteSecurityGroups()
582+
if tc.wantErr {
583+
g.Expect(err).To(HaveOccurred())
584+
return
490585
}
586+
g.Expect(err).NotTo(HaveOccurred())
491587
})
492588
}
493589
}
@@ -575,3 +671,15 @@ func TestIngressRulesFromSDKType(t *testing.T) {
575671
})
576672
}
577673
}
674+
675+
var processSecurityGroupsPage = func(_, y interface{}) {
676+
funcType := y.(func(out *ec2.DescribeSecurityGroupsOutput, last bool) bool)
677+
funcType(&ec2.DescribeSecurityGroupsOutput{
678+
SecurityGroups: []*ec2.SecurityGroup{
679+
{
680+
GroupId: aws.String("group-id"),
681+
GroupName: aws.String("group-name"),
682+
},
683+
},
684+
}, true)
685+
}

0 commit comments

Comments
 (0)