ec2.securitygroup: fix add, implement revoke/update ingress and egres…

…s rules When adding a rule, we should not send the complete rule set, only the new rule. When updating a rule, we must first delete it. And with that done, we also support deleting rules. Fixes crossplane-contrib#503 Fixes crossplane-contrib#300 Signed-off-by: Carl Henrik Lunde <[email protected]>
sparebank1utvikling · Jul 6, 2021 · 0905ade · 0905ade
1 parent fea3703
commit 0905ade
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 16 deletions.
diff --git a/pkg/clients/ec2/fake/securitygroup.go b/pkg/clients/ec2/fake/securitygroup.go
@@ -32,6 +32,7 @@ type MockSecurityGroupClient struct {
 	MockDescribe        func(*ec2.DescribeSecurityGroupsInput) ec2.DescribeSecurityGroupsRequest
 	MockAuthorizeIgress func(*ec2.AuthorizeSecurityGroupIngressInput) ec2.AuthorizeSecurityGroupIngressRequest
 	MockAuthorizeEgress func(*ec2.AuthorizeSecurityGroupEgressInput) ec2.AuthorizeSecurityGroupEgressRequest
+	MockRevokeIngress   func(*ec2.RevokeSecurityGroupIngressInput) ec2.RevokeSecurityGroupIngressRequest
 	MockRevokeEgress    func(*ec2.RevokeSecurityGroupEgressInput) ec2.RevokeSecurityGroupEgressRequest
 	MockCreateTags      func(*ec2.CreateTagsInput) ec2.CreateTagsRequest
 	MockDeleteTags      func(*ec2.DeleteTagsInput) ec2.DeleteTagsRequest
@@ -67,6 +68,11 @@ func (m *MockSecurityGroupClient) RevokeSecurityGroupEgressRequest(input *ec2.Re
 	return m.MockRevokeEgress(input)
 }
 
+// RevokeSecurityGroupIngressRequest mocks RevokeSecurityGroupIngressRequest method
+func (m *MockSecurityGroupClient) RevokeSecurityGroupIngressRequest(input *ec2.RevokeSecurityGroupIngressInput) ec2.RevokeSecurityGroupIngressRequest {
+	return m.MockRevokeIngress(input)
+}
+
 // CreateTagsRequest mocks CreateTagsInput method
 func (m *MockSecurityGroupClient) CreateTagsRequest(input *ec2.CreateTagsInput) ec2.CreateTagsRequest {
 	return m.MockCreateTags(input)

diff --git a/pkg/clients/ec2/securitygroup.go b/pkg/clients/ec2/securitygroup.go
@@ -34,6 +34,7 @@ type SecurityGroupClient interface {
 	DescribeSecurityGroupsRequest(input *ec2.DescribeSecurityGroupsInput) ec2.DescribeSecurityGroupsRequest
 	AuthorizeSecurityGroupIngressRequest(input *ec2.AuthorizeSecurityGroupIngressInput) ec2.AuthorizeSecurityGroupIngressRequest
 	AuthorizeSecurityGroupEgressRequest(input *ec2.AuthorizeSecurityGroupEgressInput) ec2.AuthorizeSecurityGroupEgressRequest
+	RevokeSecurityGroupIngressRequest(input *ec2.RevokeSecurityGroupIngressInput) ec2.RevokeSecurityGroupIngressRequest
 	RevokeSecurityGroupEgressRequest(input *ec2.RevokeSecurityGroupEgressInput) ec2.RevokeSecurityGroupEgressRequest
 	CreateTagsRequest(input *ec2.CreateTagsInput) ec2.CreateTagsRequest
 	DeleteTagsRequest(input *ec2.DeleteTagsInput) ec2.DeleteTagsRequest

diff --git a/pkg/controller/ec2/securitygroup/controller.go b/pkg/controller/ec2/securitygroup/controller.go
@@ -213,10 +213,13 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex
 		return managed.ExternalUpdate{}, awsclient.Wrap(resource.Ignore(ec2.IsSecurityGroupNotFoundErr, err), errDescribe)
 	}
 
-	patch, err := ec2.CreateSGPatch(response.SecurityGroups[0], cr.Spec.ForProvider)
-	if err != nil {
-		return managed.ExternalUpdate{}, errors.New(errUpdate)
-	}
+	/*
+		   TODO: it turns out we never reconcile description? is it mutable?
+		patch, err := ec2.CreateSGPatch(response.SecurityGroups[0], cr.Spec.ForProvider)
+		if err != nil {
+			return managed.ExternalUpdate{}, errors.New(errUpdate)
+		}
+	*/
 
 	add, remove := awsclient.DiffEC2Tags(v1beta1.GenerateEC2Tags(cr.Spec.ForProvider.Tags), response.SecurityGroups[0].Tags)
 	if len(remove) > 0 {
@@ -237,27 +240,78 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex
 		}
 	}
 
-	if patch.Ingress != nil {
-		if _, err := e.sg.AuthorizeSecurityGroupIngressRequest(&awsec2.AuthorizeSecurityGroupIngressInput{
-			GroupId:       aws.String(meta.GetExternalName(cr)),
-			IpPermissions: ec2.GenerateEC2Permissions(cr.Spec.ForProvider.Ingress),
-		}).Send(ctx); err != nil && !ec2.IsRuleAlreadyExistsErr(err) {
-			return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeIngress)
+	{
+		add, remove := diffPermissions(ec2.GenerateEC2Permissions(cr.Spec.ForProvider.Ingress), response.SecurityGroups[0].IpPermissions)
+		// spew.Dump("add", add, "remove", remove)
+		if len(remove) > 0 {
+			if _, err := e.sg.RevokeSecurityGroupIngressRequest(&awsec2.RevokeSecurityGroupIngressInput{
+				GroupId:       aws.String(meta.GetExternalName(cr)),
+				IpPermissions: remove,
+			}).Send(ctx); err != nil {
+				return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeIngress) // todo err
+			}
+		}
+		if len(add) > 0 {
+			if _, err := e.sg.AuthorizeSecurityGroupIngressRequest(&awsec2.AuthorizeSecurityGroupIngressInput{
+				GroupId:       aws.String(meta.GetExternalName(cr)),
+				IpPermissions: add,
+			}).Send(ctx); err != nil {
+				return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeIngress)
+			}
 		}
 	}
 
-	if patch.Egress != nil {
-		if _, err = e.sg.AuthorizeSecurityGroupEgressRequest(&awsec2.AuthorizeSecurityGroupEgressInput{
-			GroupId:       aws.String(meta.GetExternalName(cr)),
-			IpPermissions: ec2.GenerateEC2Permissions(cr.Spec.ForProvider.Egress),
-		}).Send(ctx); err != nil && !ec2.IsRuleAlreadyExistsErr(err) {
-			return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeEgress)
+	{
+		add, remove := diffPermissions(ec2.GenerateEC2Permissions(cr.Spec.ForProvider.Egress), response.SecurityGroups[0].IpPermissionsEgress)
+		// spew.Dump("add", add, "remove", remove)
+
+		if len(remove) > 0 {
+			if _, err = e.sg.RevokeSecurityGroupEgressRequest(&awsec2.RevokeSecurityGroupEgressInput{
+				GroupId:       aws.String(meta.GetExternalName(cr)),
+				IpPermissions: remove,
+			}).Send(ctx); err != nil && !ec2.IsRuleAlreadyExistsErr(err) {
+				return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeEgress) // todo err
+			}
+		}
+
+		if len(add) > 0 {
+			if _, err = e.sg.AuthorizeSecurityGroupEgressRequest(&awsec2.AuthorizeSecurityGroupEgressInput{
+				GroupId:       aws.String(meta.GetExternalName(cr)),
+				IpPermissions: add,
+			}).Send(ctx); err != nil && !ec2.IsRuleAlreadyExistsErr(err) {
+				return managed.ExternalUpdate{}, awsclient.Wrap(err, errAuthorizeEgress)
+			}
 		}
 	}
 
 	return managed.ExternalUpdate{}, nil
 }
 
+// TODO: warning O(n^2) - check port numbers? use maps?
+// TODO: add unit tests
+func diffPermissions(want, have []awsec2.IpPermission) (add, remove []awsec2.IpPermission) {
+	seen := make(map[int]bool)
+	for _, w := range want {
+		haveIt := false
+		for i, h := range have {
+			if diff := cmp.Diff(h, w); diff == "" { // TODO: are there any fields we should ignore or have a special comparator for, like tcp vs TCP. Others?
+				haveIt = true
+				seen[i] = true
+				break
+			}
+		}
+		if !haveIt {
+			add = append(add, w)
+		}
+	}
+	for i, h := range have {
+		if !seen[i] {
+			remove = append(remove, h)
+		}
+	}
+	return
+}
+
 func (e *external) Delete(ctx context.Context, mgd resource.Managed) error {
 	cr, ok := mgd.(*v1beta1.SecurityGroup)
 	if !ok {