diff --git a/pkg/model/openstackmodel/firewall.go b/pkg/model/openstackmodel/firewall.go index a304eea0ba2c1..9d2607a9aee86 100644 --- a/pkg/model/openstackmodel/firewall.go +++ b/pkg/model/openstackmodel/firewall.go @@ -465,8 +465,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 +480,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 } diff --git a/upup/pkg/fi/cloudup/openstack/cloud.go b/upup/pkg/fi/cloudup/openstack/cloud.go index 626531e95ce53..064fa459cc4b0 100644 --- a/upup/pkg/fi/cloudup/openstack/cloud.go +++ b/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) diff --git a/upup/pkg/fi/cloudup/openstack/security_group.go b/upup/pkg/fi/cloudup/openstack/security_group.go index 0fcfc6577eb41..94d9f375368c5 100644 --- a/upup/pkg/fi/cloudup/openstack/security_group.go +++ b/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 }) diff --git a/upup/pkg/fi/cloudup/openstacktasks/lb.go b/upup/pkg/fi/cloudup/openstacktasks/lb.go index a30cc62af2629..e6ebae120a290 100644 --- a/upup/pkg/fi/cloudup/openstacktasks/lb.go +++ b/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 { diff --git a/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go b/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go index ad56ed10fa96c..b6704ff18594a 100644 --- a/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go +++ b/upup/pkg/fi/cloudup/openstacktasks/securitygroup.go @@ -18,8 +18,11 @@ 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" @@ -27,10 +30,11 @@ import ( //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 +}