Merge pull request #657 from justinsb/filter_security_group_removal

Remove security group rules that match our filter

upup/models/cloudup/_aws/master/master.yaml

@@ -17,7 +17,9 @@ iamInstanceProfileRole/masters.{{ ClusterName }}:
 securityGroup/masters.{{ ClusterName }}:
   vpc: vpc/{{ ClusterName }}
   description: 'Security group for masters'
-  removeExtraRules: true
+  removeExtraRules:
+  - port=22
+  - port=443
 
 # Allow full egress
 securityGroupRule/master-egress:

upup/models/cloudup/_aws/nodes.yaml

@@ -17,7 +17,8 @@ iamInstanceProfileRole/nodes.{{ ClusterName }}:
 securityGroup/nodes.{{ ClusterName }}:
   vpc: vpc/{{ ClusterName }}
   description: 'Security group for nodes'
-  removeExtraRules: true
+  removeExtraRules:
+  - port=22
 
 # Allow full egress
 securityGroupRule/node-egress:

upup/pkg/fi/cloudup/awstasks/securitygroup.go

@@ -25,6 +25,8 @@ import (
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
+	"strconv"
+	"strings"
 )
 
 //go:generate fitask -type=SecurityGroup
@@ -35,7 +37,7 @@ type SecurityGroup struct {
 	Description *string
 	VPC         *VPC
 
-	RemoveExtraRules *bool
+	RemoveExtraRules []string
 }
 
 var _ fi.CompareWithID = &SecurityGroup{}
@@ -273,19 +275,22 @@ func expandPermissions(sgID *string, permission *ec2.IpPermission, egress bool)
 	return rules
 }
 
-// security group deletion is temporarily reverted - #478
 func (e *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
-	return nil, nil
-}
-
-// security group deletion is temporarily reverted - #478
-func (e *SecurityGroup) revertedFindDeletions(c *fi.Context) ([]fi.Deletion, error) {
 	var removals []fi.Deletion
 
-	if fi.BoolValue(e.RemoveExtraRules) != true {
+	if len(e.RemoveExtraRules) == 0 {
 		return nil, nil
 	}
 
+	var rules []RemovalRule
+	for _, s := range e.RemoveExtraRules {
+		rule, err := ParseRemovalRule(s)
+		if err != nil {
+			return nil, fmt.Errorf("cannot parse rule %q: %v", s, err)
+		}
+		rules = append(rules, rule)
+	}
+
 	sg, err := e.findEc2(c)
 	if err != nil {
 		return nil, err
@@ -301,6 +306,21 @@ func (e *SecurityGroup) revertedFindDeletions(c *fi.Context) ([]fi.Deletion, err
 	}
 
 	for _, permission := range ingress {
+		// Because of #478, we can't remove all non-matching security groups
+		// Instead we consider only certain rules to be 'in-scope'
+		// (in the model, we typically consider only rules on port 22 and 443)
+		match := false
+		for _, rule := range rules {
+			if rule.Matches(permission) {
+				glog.V(2).Infof("permission matches rule %s: %v", rule, permission)
+				match = true
+				break
+			}
+		}
+		if !match {
+			glog.V(4).Infof("Ignoring security group permission %q (did not match removal rules)", permission)
+			continue
+		}
 		found := false
 		for _, t := range c.AllTasks() {
 			er, ok := t.(*SecurityGroupRule)
@@ -347,3 +367,54 @@ func (e *SecurityGroup) revertedFindDeletions(c *fi.Context) ([]fi.Deletion, err
 
 	return removals, nil
 }
+
+// RemovalRule is a rule that filters the permissions we should remove
+type RemovalRule interface {
+	Matches(permission *ec2.IpPermission) bool
+}
+
+// ParseRemovalRule parses our removal rule DSL into a RemovalRule
+func ParseRemovalRule(rule string) (RemovalRule, error) {
+	rule = strings.TrimSpace(rule)
+	tokens := strings.Split(rule, "=")
+
+	// Simple little language:
+	//   port=N matches rules that filter (only) by port=N
+	//
+	// Note this language is internal, so isn't required to be stable
+
+	if len(tokens) == 2 {
+		if tokens[0] == "port" {
+			port, err := strconv.Atoi(tokens[1])
+			if err != nil {
+				return nil, fmt.Errorf("cannot parse rule %q", rule)
+			}
+
+			return &PortRemovalRule{Port: port}, nil
+		} else {
+			return nil, fmt.Errorf("cannot parse rule %q", rule)
+		}
+	}
+	return nil, fmt.Errorf("cannot parse rule %q", rule)
+}
+
+type PortRemovalRule struct {
+	Port int
+}
+
+var _ RemovalRule = &PortRemovalRule{}
+
+func (r *PortRemovalRule) String() string {
+	return fi.DebugAsJsonString(r)
+}
+
+func (r *PortRemovalRule) Matches(permission *ec2.IpPermission) bool {
+	// Check if port matches
+	if permission.FromPort == nil || *permission.FromPort != int64(r.Port) {
+		return false
+	}
+	if permission.ToPort == nil || *permission.ToPort != int64(r.Port) {
+		return false
+	}
+	return true
+}

upup/pkg/fi/cloudup/awstasks/securitygroup_test.go

@@ -0,0 +1,73 @@
+package awstasks
+
+import (
+	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"testing"
+)
+
+func TestParseRemovalRule(t *testing.T) {
+	testNotParse(t, "port 22")
+	testNotParse(t, "port22")
+	testNotParse(t, "port=a")
+	testNotParse(t, "port=22-23")
+
+	testParsesAsPort(t, "port=22", 22)
+	testParsesAsPort(t, "port=443", 443)
+}
+
+func testNotParse(t *testing.T, rule string) {
+	r, err := ParseRemovalRule(rule)
+	if err == nil {
+		t.Fatalf("expected failure to parse removal rule %q, got %v", rule, r)
+	}
+}
+
+func testParsesAsPort(t *testing.T, rule string, port int) {
+	r, err := ParseRemovalRule(rule)
+	if err != nil {
+		t.Fatalf("unexpected failure to parse rule %q: %v", rule, err)
+	}
+	portRemovalRule, ok := r.(*PortRemovalRule)
+	if !ok {
+		t.Fatalf("unexpected rule type for rule %q: %T", r, err)
+	}
+	if portRemovalRule.Port != port {
+		t.Fatalf("unexpected port for %q, expecting %d, got %q", rule, port, r)
+	}
+}
+
+func TestPortRemovalRule(t *testing.T) {
+	r := &PortRemovalRule{Port: 22}
+	testMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22), ToPort: aws.Int64(22)})
+
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(23), ToPort: aws.Int64(23)})
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(20), ToPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22), ToPort: aws.Int64(23)})
+	testNotMatches(t, r, &ec2.IpPermission{ToPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(22)})
+	testNotMatches(t, r, &ec2.IpPermission{})
+}
+
+func TestPortRemovalRule_Zero(t *testing.T) {
+	r := &PortRemovalRule{Port: 0}
+	testMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(0)})
+
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0), ToPort: aws.Int64(20)})
+	testNotMatches(t, r, &ec2.IpPermission{ToPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.IpPermission{FromPort: aws.Int64(0)})
+	testNotMatches(t, r, &ec2.IpPermission{})
+}
+
+func testMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.IpPermission) {
+	if !rule.Matches(permission) {
+		t.Fatalf("rule %q failed to match permission %q", rule, permission)
+	}
+}
+
+func testNotMatches(t *testing.T, rule *PortRemovalRule, permission *ec2.IpPermission) {
+	if rule.Matches(permission) {
+		t.Fatalf("rule %q unexpectedly matched permission %q", rule, permission)
+	}
+}