Skip to content

Commit

Permalink
ec2.securitygroup: fix add, implement revoke/update ingress and egres…
Browse files Browse the repository at this point in the history 
…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]>
  • Loading branch information
@chlunde
chlunde committed Nov 22, 2021
1 parent 3680b2b commit b74845d
Show file tree
Hide file tree
Showing 7 changed files with 577 additions and 236 deletions.
6 changes: 6 additions & 0 deletions pkg/clients/ec2/fake/securitygroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ type MockSecurityGroupClient struct {
MockDescribe func(ctx context.Context, input *ec2.DescribeSecurityGroupsInput, opts []func(*ec2.Options)) (*ec2.DescribeSecurityGroupsOutput, error)
MockAuthorizeIngress func(ctx context.Context, input *ec2.AuthorizeSecurityGroupIngressInput, opts []func(*ec2.Options)) (*ec2.AuthorizeSecurityGroupIngressOutput, error)
MockAuthorizeEgress func(ctx context.Context, input *ec2.AuthorizeSecurityGroupEgressInput, opts []func(*ec2.Options)) (*ec2.AuthorizeSecurityGroupEgressOutput, error)
MockRevokeIngress func(ctx context.Context, input *ec2.RevokeSecurityGroupIngressInput, opts []func(*ec2.Options)) (*ec2.RevokeSecurityGroupIngressOutput, error)
MockRevokeEgress func(ctx context.Context, input *ec2.RevokeSecurityGroupEgressInput, opts []func(*ec2.Options)) (*ec2.RevokeSecurityGroupEgressOutput, error)
MockCreateTags func(ctx context.Context, input *ec2.CreateTagsInput, opts []func(*ec2.Options)) (*ec2.CreateTagsOutput, error)
MockDeleteTags func(ctx context.Context, input *ec2.DeleteTagsInput, opts []func(*ec2.Options)) (*ec2.DeleteTagsOutput, error)
Expand Down Expand Up @@ -69,6 +70,11 @@ func (m *MockSecurityGroupClient) RevokeSecurityGroupEgress(ctx context.Context,
return m.MockRevokeEgress(ctx, input, opts)
}

// RevokeSecurityGroupIngress mocks RevokeSecurityGroupIngress method
func (m *MockSecurityGroupClient) RevokeSecurityGroupIngress(ctx context.Context, input *ec2.RevokeSecurityGroupIngressInput, opts ...func(*ec2.Options)) (*ec2.RevokeSecurityGroupIngressOutput, error) {
return m.MockRevokeIngress(ctx, input, opts)
}

// CreateTags mocks CreateTagsInput method
func (m *MockSecurityGroupClient) CreateTags(ctx context.Context, input *ec2.CreateTagsInput, opts ...func(*ec2.Options)) (*ec2.CreateTagsOutput, error) {
return m.MockCreateTags(ctx, input, opts)
Expand Down
119 changes: 12 additions & 107 deletions pkg/clients/ec2/securitygroup.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,13 @@ package ec2

import (
"context"
"encoding/json"
"errors"
"sort"
"strings"

awsgo "github.com/aws/aws-sdk-go-v2/aws"
"github.com/aws/aws-sdk-go-v2/service/ec2"
ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
"github.com/aws/smithy-go"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"

xpv1 "github.com/crossplane/crossplane-runtime/apis/common/v1"

"github.com/crossplane/provider-aws/apis/ec2/v1beta1"
aws "github.com/crossplane/provider-aws/pkg/clients"
Expand All @@ -36,6 +30,7 @@ type SecurityGroupClient interface {
DescribeSecurityGroups(ctx context.Context, input *ec2.DescribeSecurityGroupsInput, opts ...func(*ec2.Options)) (*ec2.DescribeSecurityGroupsOutput, error)
AuthorizeSecurityGroupIngress(ctx context.Context, input *ec2.AuthorizeSecurityGroupIngressInput, opts ...func(*ec2.Options)) (*ec2.AuthorizeSecurityGroupIngressOutput, error)
AuthorizeSecurityGroupEgress(ctx context.Context, input *ec2.AuthorizeSecurityGroupEgressInput, opts ...func(*ec2.Options)) (*ec2.AuthorizeSecurityGroupEgressOutput, error)
RevokeSecurityGroupIngress(ctx context.Context, input *ec2.RevokeSecurityGroupIngressInput, opts ...func(*ec2.Options)) (*ec2.RevokeSecurityGroupIngressOutput, error)
RevokeSecurityGroupEgress(ctx context.Context, input *ec2.RevokeSecurityGroupEgressInput, opts ...func(*ec2.Options)) (*ec2.RevokeSecurityGroupEgressOutput, error)
CreateTags(ctx context.Context, input *ec2.CreateTagsInput, opts ...func(*ec2.Options)) (*ec2.CreateTagsOutput, error)
DeleteTags(ctx context.Context, input *ec2.DeleteTagsInput, opts ...func(*ec2.Options)) (*ec2.DeleteTagsOutput, error)
Expand Down Expand Up @@ -257,110 +252,20 @@ func LateInitializeIPPermissions(spec []v1beta1.IPPermission, o []ec2types.IpPer
return spec
}

// CreateSGPatch creates a *v1beta1.SecurityGroupParameters that has only the changed
// values between the target *v1beta1.SecurityGroupParameters and the current
// *ec2types.SecurityGroup
func CreateSGPatch(in ec2types.SecurityGroup, target v1beta1.SecurityGroupParameters) (*v1beta1.SecurityGroupParameters, error) { // nolint:gocyclo
targetCopy := *target.DeepCopy()
v1beta1.SortTags(targetCopy.Tags, in.Tags)
sort.Slice(target.Egress, func(i, j int) bool {
return awsgo.ToInt32(target.Egress[i].FromPort) < awsgo.ToInt32(target.Egress[j].FromPort)
})
sort.Slice(target.Ingress, func(i, j int) bool {
return awsgo.ToInt32(target.Ingress[i].FromPort) < awsgo.ToInt32(target.Ingress[j].FromPort)
})
currentParams := &v1beta1.SecurityGroupParameters{
Description: awsclients.StringValue(in.Description),
GroupName: awsclients.StringValue(in.GroupName),
VPCID: in.VpcId,
}
currentParams.Tags = v1beta1.BuildFromEC2Tags(in.Tags)
currentParams.Ingress = GenerateIPPermissions(in.IpPermissions)
currentParams.Egress = GenerateIPPermissions(in.IpPermissionsEgress)
// NOTE(muvaf): Sending -1 as FromPort or ToPort is valid but the returned
// object does not have that value. So, in case we have sent -1, we assume
// that the returned value is also -1 in case if it's nil.
// See the following about usage of -1
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ec2-security-group-egress.html
mOne := int32(-1)
for i, spec := range targetCopy.Egress {
if len(currentParams.Egress) <= i {
break
}
if awsgo.ToInt32(spec.FromPort) == mOne {
currentParams.Egress[i].FromPort = awsclients.LateInitializeInt32Ptr(currentParams.Egress[i].FromPort, &mOne)
}
if awsgo.ToInt32(spec.ToPort) == mOne {
currentParams.Egress[i].ToPort = awsclients.LateInitializeInt32Ptr(currentParams.Egress[i].ToPort, &mOne)
}
}
// Same happens with VPCID in egress user group id pairs. The value of that
// field is not returned from AWS.
for i, ingress := range currentParams.Ingress {
for j, pair := range ingress.UserIDGroupPairs {
if awsclients.StringValue(pair.VPCID) == "" && len(targetCopy.Ingress) > i && len(targetCopy.Ingress[i].UserIDGroupPairs) > j {
currentParams.Ingress[i].UserIDGroupPairs[j].VPCID = targetCopy.Ingress[i].UserIDGroupPairs[j].VPCID
}
}
}

for i, spec := range targetCopy.Egress {
for j := range spec.UserIDGroupPairs {
targetCopy.Egress[i].UserIDGroupPairs[j].ClearRefSelectors()
}
}

for i, spec := range targetCopy.Ingress {
for j := range spec.UserIDGroupPairs {
targetCopy.Ingress[i].UserIDGroupPairs[j].ClearRefSelectors()
}
// IsSGUpToDate checks if the observed security group is up to equal to the desired state
func IsSGUpToDate(sg v1beta1.SecurityGroupParameters, observed ec2types.SecurityGroup) bool {
if !CompareTags(sg.Tags, observed.Tags) {
return false
}

sort.Slice(targetCopy.Egress, func(i, j int) bool {
return awsgo.ToInt32(targetCopy.Egress[i].FromPort) < awsgo.ToInt32(targetCopy.Egress[j].FromPort)
})
sort.Slice(targetCopy.Ingress, func(i, j int) bool {
return awsgo.ToInt32(targetCopy.Ingress[i].FromPort) < awsgo.ToInt32(targetCopy.Ingress[j].FromPort)
})

jsonPatch, err := awsclients.CreateJSONPatch(*currentParams, targetCopy)
if err != nil {
return nil, err
}
patch := &v1beta1.SecurityGroupParameters{}
if err := json.Unmarshal(jsonPatch, patch); err != nil {
return nil, err
add, remove := DiffPermissions(GenerateEC2Permissions(sg.Ingress), observed.IpPermissions)
if len(add) > 0 || len(remove) > 0 {
return false
}

return patch, nil
}

// IsSGUpToDate checks whether there is a change in any of the modifiable fields.
func IsSGUpToDate(p v1beta1.SecurityGroupParameters, sg ec2types.SecurityGroup) (bool, error) {
patch, err := CreateSGPatch(sg, p)
if err != nil {
return false, err
}
return cmp.Equal(&v1beta1.SecurityGroupParameters{}, patch,
cmpopts.IgnoreTypes(&xpv1.Reference{}, &xpv1.Selector{}),
cmpopts.IgnoreFields(v1beta1.SecurityGroupParameters{}, "Region"),
InsensitiveCases()), nil
}

// TODO(muvaf): We needed this for IPProtocol field; even if you send "TCP", AWS
// returns "tcp". However, this cmp.Option is probably useful for other providers,
// too. Consider making it part of crossplane-runtime.

// InsensitiveCases ignores the case sensitivity for string and *string types.
func InsensitiveCases() cmp.Option {
return cmp.Options{
cmp.FilterValues(func(_, _ interface{}) bool {
return true
}, cmp.Comparer(strings.EqualFold)),
cmp.FilterValues(func(_, _ interface{}) bool {
return true
}, cmp.Comparer(func(x, y *string) bool {
return strings.EqualFold(awsclients.StringValue(x), awsclients.StringValue(y))
})),
add, remove = DiffPermissions(GenerateEC2Permissions(sg.Egress), observed.IpPermissionsEgress)
if len(add) > 0 || len(remove) > 0 {
return false
}
return true
}
Loading

0 comments on commit b74845d

Please sign in to comment.