Merge pull request #7561 from zetaab/feature/cleansecgroup

Clean security groups if api/ssh ips are removed from config

pkg/model/openstackmodel/firewall.go

@@ -270,26 +270,28 @@ func (b *FirewallModelBuilder) addHTTPSRules(c *fi.ModelBuilderContext, sgMap ma
 	return nil
 }
 
-// addKubeletRules - Add rules to 10250 to the KubernetesAPIAccess list
+// addKubeletRules - Add rules to 10250 port
 func (b *FirewallModelBuilder) addKubeletRules(c *fi.ModelBuilderContext, sgMap map[string]*openstacktasks.SecurityGroup) error {
 
 	//TODO: This is the default port for kubelet and may be overwridden
 	masterName := b.SecurityGroupName(kops.InstanceGroupRoleMaster)
 	nodeName := b.SecurityGroupName(kops.InstanceGroupRoleNode)
 	masterSG := sgMap[masterName]
 	nodeSG := sgMap[nodeName]
+
+	kubeletRule := &openstacktasks.SecurityGroupRule{
+		Lifecycle:    b.Lifecycle,
+		Direction:    s(string(rules.DirIngress)),
+		Protocol:     s(IPProtocolTCP),
+		EtherType:    s(IPV4),
+		PortRangeMin: i(10250),
+		PortRangeMax: i(10250),
+	}
+
+	// allow node-node, node-master and master-master and master-node
 	for _, sgName := range []*openstacktasks.SecurityGroup{masterSG, nodeSG} {
-		for _, apiAccess := range b.Cluster.Spec.KubernetesAPIAccess {
-			addDirectionalGroupRule(c, sgName, nil, &openstacktasks.SecurityGroupRule{
-				Lifecycle:      b.Lifecycle,
-				Direction:      s(string(rules.DirIngress)),
-				Protocol:       s(IPProtocolTCP),
-				EtherType:      s(IPV4),
-				PortRangeMin:   i(10250),
-				PortRangeMax:   i(10250),
-				RemoteIPPrefix: s(apiAccess),
-			})
-		}
+		addDirectionalGroupRule(c, masterSG, sgName, kubeletRule)
+		addDirectionalGroupRule(c, nodeSG, sgName, kubeletRule)
 	}
 	return nil
 }
@@ -465,8 +467,9 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error {
 
 	if b.UseLoadBalancerForAPI() {
 		sg := &openstacktasks.SecurityGroup{
-			Name:      s(b.Cluster.Spec.MasterPublicName),
-			Lifecycle: b.Lifecycle,
+			Name:             s(b.Cluster.Spec.MasterPublicName),
+			Lifecycle:        b.Lifecycle,
+			RemoveExtraRules: []string{"port=443"},
 		}
 		c.AddTask(sg)
 		sgMap[b.Cluster.Spec.MasterPublicName] = sg
@@ -479,6 +482,13 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error {
 			Name:      s(groupName),
 			Lifecycle: b.Lifecycle,
 		}
+		if role == kops.InstanceGroupRoleBastion {
+			sg.RemoveExtraRules = []string{"port=22"}
+		} else if role == kops.InstanceGroupRoleNode {
+			sg.RemoveExtraRules = []string{"port=22", "port=10250"}
+		} else if role == kops.InstanceGroupRoleMaster {
+			sg.RemoveExtraRules = []string{"port=22", "port=443", "port=10250"}
+		}
 		c.AddTask(sg)
 		sgMap[groupName] = sg
 	}

upup/pkg/fi/cloudup/openstack/cloud.go

@@ -135,6 +135,9 @@ type OpenstackCloud interface {
 	//DeleteSecurityGroup will delete securitygroup
 	DeleteSecurityGroup(sgID string) error
 
+	//DeleteSecurityGroupRule will delete securitygrouprule
+	DeleteSecurityGroupRule(ruleID string) error
+
 	//ListSecurityGroupRules will return the Neutron security group rules which match the options
 	ListSecurityGroupRules(opt sgr.ListOpts) ([]sgr.SecGroupRule, error)
 

upup/pkg/fi/cloudup/openstack/security_group.go

@@ -119,7 +119,24 @@ func (c *openstackCloud) DeleteSecurityGroup(sgID string) error {
 	done, err := vfs.RetryWithBackoff(writeBackoff, func() (bool, error) {
 		err := sg.Delete(c.neutronClient, sgID).ExtractErr()
 		if err != nil && !isNotFound(err) {
-			return false, fmt.Errorf("error deleting network: %v", err)
+			return false, fmt.Errorf("error deleting security group: %v", err)
+		}
+		return true, nil
+	})
+	if err != nil {
+		return err
+	} else if done {
+		return nil
+	} else {
+		return wait.ErrWaitTimeout
+	}
+}
+
+func (c *openstackCloud) DeleteSecurityGroupRule(ruleID string) error {
+	done, err := vfs.RetryWithBackoff(writeBackoff, func() (bool, error) {
+		err := sgr.Delete(c.neutronClient, ruleID).ExtractErr()
+		if err != nil && !isNotFound(err) {
+			return false, fmt.Errorf("error deleting security group rule: %v", err)
 		}
 		return true, nil
 	})

upup/pkg/fi/cloudup/openstacktasks/lb.go

@@ -119,13 +119,19 @@ func NewLBTaskFromCloud(cloud openstack.OpenstackCloud, lifecycle *fi.Lifecycle,
 		return nil, err
 	}
 
+	sg, err := getSecurityGroupByName(&SecurityGroup{Name: fi.String(lb.Name)}, osCloud)
+	if err != nil {
+		return nil, err
+	}
+
 	actual := &LB{
-		ID:        fi.String(lb.ID),
-		Name:      fi.String(lb.Name),
-		Lifecycle: lifecycle,
-		PortID:    fi.String(lb.VipPortID),
-		Subnet:    fi.String(sub.Name),
-		VipSubnet: fi.String(lb.VipSubnetID),
+		ID:            fi.String(lb.ID),
+		Name:          fi.String(lb.Name),
+		Lifecycle:     lifecycle,
+		PortID:        fi.String(lb.VipPortID),
+		Subnet:        fi.String(sub.Name),
+		VipSubnet:     fi.String(lb.VipSubnetID),
+		SecurityGroup: sg,
 	}
 
 	if find != nil {

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

@@ -18,19 +18,23 @@ package openstacktasks
 
 import (
 	"fmt"
+	"strconv"
+	"strings"
 
 	sg "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/groups"
+	sgr "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/security/rules"
 	"k8s.io/klog"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/openstack"
 )
 
 //go:generate fitask -type=SecurityGroup
 type SecurityGroup struct {
-	ID          *string
-	Name        *string
-	Description *string
-	Lifecycle   *fi.Lifecycle
+	ID               *string
+	Name             *string
+	Description      *string
+	RemoveExtraRules []string
+	Lifecycle        *fi.Lifecycle
 }
 
 var _ fi.CompareWithID = &SecurityGroup{}
@@ -41,10 +45,10 @@ func (s *SecurityGroup) CompareWithID() *string {
 
 func (s *SecurityGroup) Find(context *fi.Context) (*SecurityGroup, error) {
 	cloud := context.Cloud.(openstack.OpenstackCloud)
-	return s.getSecurityGroupByName(cloud)
+	return getSecurityGroupByName(s, cloud)
 }
 
-func (s *SecurityGroup) getSecurityGroupByName(cloud openstack.OpenstackCloud) (*SecurityGroup, error) {
+func getSecurityGroupByName(s *SecurityGroup, cloud openstack.OpenstackCloud) (*SecurityGroup, error) {
 	opt := sg.ListOpts{
 		Name: fi.StringValue(s.Name),
 	}
@@ -65,6 +69,7 @@ func (s *SecurityGroup) getSecurityGroupByName(cloud openstack.OpenstackCloud) (
 		Description: fi.String(g.Description),
 		Lifecycle:   s.Lifecycle,
 	}
+	actual.RemoveExtraRules = s.RemoveExtraRules
 	s.ID = actual.ID
 	return actual, nil
 }
@@ -110,3 +115,180 @@ func (_ *SecurityGroup) RenderOpenstack(t *openstack.OpenstackAPITarget, a, e, c
 	klog.V(2).Infof("Openstack task SecurityGroup::RenderOpenstack did nothing")
 	return nil
 }
+
+func (s *SecurityGroup) FindDeletions(c *fi.Context) ([]fi.Deletion, error) {
+	var removals []fi.Deletion
+
+	if len(s.RemoveExtraRules) == 0 {
+		return nil, nil
+	}
+
+	var rules []RemovalRule
+	for _, r := range s.RemoveExtraRules {
+		rule, err := ParseRemovalRule(r)
+		if err != nil {
+			return nil, fmt.Errorf("cannot parse rule %q: %v", r, err)
+		}
+		rules = append(rules, rule)
+	}
+
+	cloud := c.Cloud.(openstack.OpenstackCloud)
+	sg, err := getSecurityGroupByName(s, cloud)
+	if err != nil {
+		return nil, err
+	}
+	if sg == nil {
+		return nil, nil
+	}
+
+	sgRules, err := cloud.ListSecurityGroupRules(sgr.ListOpts{
+		SecGroupID: fi.StringValue(sg.ID),
+	})
+	if err != nil {
+		return nil, err
+	}
+
+	for _, permission := range sgRules {
+		match := false
+		for _, rule := range rules {
+			if rule.Matches(permission) {
+				klog.V(2).Infof("permission matches rule %s: %v", rule, permission)
+				match = true
+				break
+			}
+		}
+		if !match {
+			klog.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)
+			if !ok {
+				continue
+			}
+
+			if matches(er, permission) {
+				found = true
+			}
+		}
+		if !found {
+			removals = append(removals, &deleteSecurityGroupRule{
+				rule:          permission,
+				securityGroup: s,
+			})
+		}
+	}
+	return removals, nil
+}
+
+func matches(t *SecurityGroupRule, perm sgr.SecGroupRule) bool {
+	if fi.IntValue(t.PortRangeMin) != perm.PortRangeMin {
+		return false
+	}
+
+	if fi.IntValue(t.PortRangeMax) != perm.PortRangeMax {
+		return false
+	}
+
+	if perm.Protocol != "tcp" {
+		return false
+	}
+
+	if perm.RemoteIPPrefix != fi.StringValue(t.RemoteIPPrefix) {
+		return false
+	}
+
+	return true
+}
+
+type deleteSecurityGroupRule struct {
+	rule          sgr.SecGroupRule
+	securityGroup *SecurityGroup
+}
+
+var _ fi.Deletion = &deleteSecurityGroupRule{}
+
+func (d *deleteSecurityGroupRule) Delete(t fi.Target) error {
+	klog.V(2).Infof("deleting security group permission: %v", fi.DebugAsJsonString(d.rule))
+
+	os, ok := t.(*openstack.OpenstackAPITarget)
+	if !ok {
+		return fmt.Errorf("unexpected target type for deletion: %T", t)
+	}
+	err := os.Cloud.DeleteSecurityGroupRule(d.rule.ID)
+	if err != nil {
+		return fmt.Errorf("error revoking SecurityGroupRule: %v", err)
+	}
+	return nil
+}
+
+func (d *deleteSecurityGroupRule) TaskName() string {
+	return "SecurityGroupRule"
+}
+
+func (d *deleteSecurityGroupRule) Item() string {
+	s := ""
+	if d.rule.PortRangeMin != 0 {
+		s += fmt.Sprintf(" port=%d", d.rule.PortRangeMin)
+		if d.rule.PortRangeMin != d.rule.PortRangeMax {
+			s += fmt.Sprintf("-%d", d.rule.PortRangeMax)
+		}
+	}
+	s += " protocol=tcp"
+	s += fmt.Sprintf(" ip=%s", d.rule.RemoteIPPrefix)
+	s += fmt.Sprintf(" securitygroup=%s", fi.StringValue(d.securityGroup.Name))
+	return s
+}
+
+// RemovalRule is a rule that filters the permissions we should remove
+type RemovalRule interface {
+	Matches(sgr.SecGroupRule) 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(rule sgr.SecGroupRule) bool {
+	// Check if port matches
+	if rule.PortRangeMin != r.Port {
+		return false
+	}
+	if rule.PortRangeMax != r.Port {
+		return false
+	}
+	return true
+}