From 7215ce30b1f0c2ea8986572c3b2faf66b1f47b82 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Thu, 26 Oct 2017 20:23:16 +0800 Subject: [PATCH 1/2] Add service.UID into security group name Related to: #53714 --- .../openstack/openstack_loadbalancer.go | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go b/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go index 98b5b44ed77..90c95557324 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go +++ b/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go @@ -292,8 +292,14 @@ func popMember(members []v2pools.Member, addr string, port int) []v2pools.Member return members } -func getSecurityGroupName(clusterName string, service *v1.Service) string { - return fmt.Sprintf("lb-sg-%s-%s-%s", clusterName, service.Namespace, service.Name) +func getSecurityGroupName(service *v1.Service) string { + securityGroupName := fmt.Sprintf("lb-sg-%s-%s-%s", service.UID, service.Namespace, service.Name) + //OpenStack requires that the name of a security group is shorter than 255 bytes. + if len(securityGroupName) > 255 { + securityGroupName = securityGroupName[:255] + } + + return securityGroupName } func getSecurityGroupRules(client *gophercloud.ServiceClient, opts rules.ListOpts) ([]rules.SecGroupRule, error) { @@ -899,7 +905,7 @@ func (lbaas *LbaasV2) ensureSecurityGroup(clusterName string, apiService *v1.Ser } // ensure security group for LB - lbSecGroupName := getSecurityGroupName(clusterName, apiService) + lbSecGroupName := getSecurityGroupName(apiService) lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) if err != nil { // check whether security group does not exist @@ -914,8 +920,8 @@ func (lbaas *LbaasV2) ensureSecurityGroup(clusterName string, apiService *v1.Ser if len(lbSecGroupID) == 0 { // create security group lbSecGroupCreateOpts := groups.CreateOpts{ - Name: getSecurityGroupName(clusterName, apiService), - Description: fmt.Sprintf("Securty Group for loadbalancer service %s/%s", apiService.Namespace, apiService.Name), + Name: getSecurityGroupName(apiService), + Description: fmt.Sprintf("Securty Group for %s/%s Service LoadBalancer in cluster %s", apiService.Namespace, apiService.Name, clusterName), } lbSecGroup, err := groups.Create(lbaas.network, lbSecGroupCreateOpts).Extract() @@ -1197,7 +1203,7 @@ func (lbaas *LbaasV2) updateSecurityGroup(clusterName string, apiService *v1.Ser removals := original.Difference(current) // Generate Name - lbSecGroupName := getSecurityGroupName(clusterName, apiService) + lbSecGroupName := getSecurityGroupName(apiService) lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) if err != nil { return fmt.Errorf("error occurred finding security group: %s: %v", lbSecGroupName, err) @@ -1369,7 +1375,7 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1. // Delete the Security Group if lbaas.opts.ManageSecurityGroups { // Generate Name - lbSecGroupName := getSecurityGroupName(clusterName, service) + lbSecGroupName := getSecurityGroupName(service) lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) if err != nil { // check whether security group does not exist From 669520f9bbc197496ab632cc1c34fb25343e28c2 Mon Sep 17 00:00:00 2001 From: FengyunPan Date: Tue, 21 Nov 2017 09:38:43 +0800 Subject: [PATCH 2/2] Add EnsureOldSecurityGroupDeleted to delete old security group Consider the migration from the old security group name to the new security group name, we need delete the old security group. At V1.10, we can assume everyone is using the new security group names and remove this code. --- .../openstack/openstack_loadbalancer.go | 165 ++++++++++++++---- 1 file changed, 127 insertions(+), 38 deletions(-) diff --git a/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go b/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go index 90c95557324..471fdc31a61 100644 --- a/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go +++ b/pkg/cloudprovider/providers/openstack/openstack_loadbalancer.go @@ -874,6 +874,14 @@ func (lbaas *LbaasV2) EnsureLoadBalancer(clusterName string, apiService *v1.Serv _ = lbaas.EnsureLoadBalancerDeleted(clusterName, apiService) return status, err } + + // delete the old Security Group for the service + // Related to #53764 + // TODO(FengyunPan): Remove it at V1.10 + err = lbaas.EnsureOldSecurityGroupDeleted(clusterName, apiService) + if err != nil { + return status, fmt.Errorf("Failed to delete the Security Group for loadbalancer service %s/%s: %v", apiService.Namespace, apiService.Name, err) + } } return status, nil @@ -921,7 +929,7 @@ func (lbaas *LbaasV2) ensureSecurityGroup(clusterName string, apiService *v1.Ser // create security group lbSecGroupCreateOpts := groups.CreateOpts{ Name: getSecurityGroupName(apiService), - Description: fmt.Sprintf("Securty Group for %s/%s Service LoadBalancer in cluster %s", apiService.Namespace, apiService.Name, clusterName), + Description: fmt.Sprintf("Security Group for %s/%s Service LoadBalancer in cluster %s", apiService.Namespace, apiService.Name, clusterName), } lbSecGroup, err := groups.Create(lbaas.network, lbSecGroupCreateOpts).Extract() @@ -1180,7 +1188,7 @@ func (lbaas *LbaasV2) UpdateLoadBalancer(clusterName string, service *v1.Service if lbaas.opts.ManageSecurityGroups { err := lbaas.updateSecurityGroup(clusterName, service, nodes, loadbalancer) if err != nil { - return fmt.Errorf("failed to update Securty Group for loadbalancer service %s/%s: %v", service.Namespace, service.Name, err) + return fmt.Errorf("failed to update Security Group for loadbalancer service %s/%s: %v", service.Namespace, service.Name, err) } } @@ -1374,50 +1382,131 @@ func (lbaas *LbaasV2) EnsureLoadBalancerDeleted(clusterName string, service *v1. // Delete the Security Group if lbaas.opts.ManageSecurityGroups { - // Generate Name - lbSecGroupName := getSecurityGroupName(service) - lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) + err := lbaas.EnsureSecurityGroupDeleted(clusterName, service) if err != nil { - // check whether security group does not exist - _, ok := err.(*gophercloud.ErrResourceNotFound) - if ok { - // It is OK when the security group has been deleted by others. - return nil - } else { - return fmt.Errorf("error occurred finding security group: %s: %v", lbSecGroupName, err) - } + return fmt.Errorf("Failed to delete Security Group for loadbalancer service %s/%s: %v", service.Namespace, service.Name, err) } - lbSecGroup := groups.Delete(lbaas.network, lbSecGroupID) - if lbSecGroup.Err != nil && !isNotFound(lbSecGroup.Err) { - return lbSecGroup.Err + // delete the old Security Group for the service + // Related to #53764 + // TODO(FengyunPan): Remove it at V1.10 + err = lbaas.EnsureOldSecurityGroupDeleted(clusterName, service) + if err != nil { + return fmt.Errorf("Failed to delete the Security Group for loadbalancer service %s/%s: %v", service.Namespace, service.Name, err) } + } - if len(lbaas.opts.NodeSecurityGroupIDs) == 0 { - // Just happen when nodes have not Security Group, or should not happen - // UpdateLoadBalancer and EnsureLoadBalancer can set lbaas.opts.NodeSecurityGroupIDs when it is empty - // And service controller call UpdateLoadBalancer to set lbaas.opts.NodeSecurityGroupIDs when controller manager service is restarted. - glog.Warningf("Can not find node-security-group from all the nodes of this cluser when delete loadbalancer service %s/%s", - service.Namespace, service.Name) + return nil +} + +// EnsureSecurityGroupDeleted deleting security group for specific loadbalancer service. +func (lbaas *LbaasV2) EnsureSecurityGroupDeleted(clusterName string, service *v1.Service) error { + // Generate Name + lbSecGroupName := getSecurityGroupName(service) + lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) + if err != nil { + // check whether security group does not exist + _, ok := err.(*gophercloud.ErrResourceNotFound) + if ok { + // It is OK when the security group has been deleted by others. + return nil } else { - // Delete the rules in the Node Security Group - for _, nodeSecurityGroupID := range lbaas.opts.NodeSecurityGroupIDs { - opts := rules.ListOpts{ - SecGroupID: nodeSecurityGroupID, - RemoteGroupID: lbSecGroupID, - } - secGroupRules, err := getSecurityGroupRules(lbaas.network, opts) + return fmt.Errorf("Error occurred finding security group: %s: %v", lbSecGroupName, err) + } + } - if err != nil && !isNotFound(err) { - msg := fmt.Sprintf("Error finding rules for remote group id %s in security group id %s: %v", lbSecGroupID, nodeSecurityGroupID, err) - return fmt.Errorf(msg) - } + lbSecGroup := groups.Delete(lbaas.network, lbSecGroupID) + if lbSecGroup.Err != nil && !isNotFound(lbSecGroup.Err) { + return lbSecGroup.Err + } - for _, rule := range secGroupRules { - res := rules.Delete(lbaas.network, rule.ID) - if res.Err != nil && !isNotFound(res.Err) { - return fmt.Errorf("error occurred deleting security group rule: %s: %v", rule.ID, res.Err) - } + if len(lbaas.opts.NodeSecurityGroupIDs) == 0 { + // Just happen when nodes have not Security Group, or should not happen + // UpdateLoadBalancer and EnsureLoadBalancer can set lbaas.opts.NodeSecurityGroupIDs when it is empty + // And service controller call UpdateLoadBalancer to set lbaas.opts.NodeSecurityGroupIDs when controller manager service is restarted. + glog.Warningf("Can not find node-security-group from all the nodes of this cluster when delete loadbalancer service %s/%s", + service.Namespace, service.Name) + } else { + // Delete the rules in the Node Security Group + for _, nodeSecurityGroupID := range lbaas.opts.NodeSecurityGroupIDs { + opts := rules.ListOpts{ + SecGroupID: nodeSecurityGroupID, + RemoteGroupID: lbSecGroupID, + } + secGroupRules, err := getSecurityGroupRules(lbaas.network, opts) + + if err != nil && !isNotFound(err) { + msg := fmt.Sprintf("Error finding rules for remote group id %s in security group id %s: %v", lbSecGroupID, nodeSecurityGroupID, err) + return fmt.Errorf(msg) + } + + for _, rule := range secGroupRules { + res := rules.Delete(lbaas.network, rule.ID) + if res.Err != nil && !isNotFound(res.Err) { + return fmt.Errorf("Error occurred deleting security group rule: %s: %v", rule.ID, res.Err) + } + } + } + } + + return nil +} + +// getOldSecurityGroupName is used to get the old security group name +// Related to #53764 +// TODO(FengyunPan): Remove it at V1.10 +func getOldSecurityGroupName(clusterName string, service *v1.Service) string { + return fmt.Sprintf("lb-sg-%s-%v", clusterName, service.Name) +} + +// EnsureOldSecurityGroupDeleted deleting old security group for specific loadbalancer service. +// Related to #53764 +// TODO(FengyunPan): Remove it at V1.10 +func (lbaas *LbaasV2) EnsureOldSecurityGroupDeleted(clusterName string, service *v1.Service) error { + glog.V(4).Infof("EnsureOldSecurityGroupDeleted(%v, %v)", clusterName, service) + // Generate Name + lbSecGroupName := getOldSecurityGroupName(clusterName, service) + lbSecGroupID, err := groups.IDFromName(lbaas.network, lbSecGroupName) + if err != nil { + // check whether security group does not exist + _, ok := err.(*gophercloud.ErrResourceNotFound) + if ok { + // It is OK when the security group has been deleted by others. + return nil + } else { + return fmt.Errorf("Error occurred finding security group: %s: %v", lbSecGroupName, err) + } + } + + lbSecGroup := groups.Delete(lbaas.network, lbSecGroupID) + if lbSecGroup.Err != nil && !isNotFound(lbSecGroup.Err) { + return lbSecGroup.Err + } + + if len(lbaas.opts.NodeSecurityGroupIDs) == 0 { + // Just happen when nodes have not Security Group, or should not happen + // UpdateLoadBalancer and EnsureLoadBalancer can set lbaas.opts.NodeSecurityGroupIDs when it is empty + // And service controller call UpdateLoadBalancer to set lbaas.opts.NodeSecurityGroupIDs when controller manager service is restarted. + glog.Warningf("Can not find node-security-group from all the nodes of this cluster when delete loadbalancer service %s/%s", + service.Namespace, service.Name) + } else { + // Delete the rules in the Node Security Group + for _, nodeSecurityGroupID := range lbaas.opts.NodeSecurityGroupIDs { + opts := rules.ListOpts{ + SecGroupID: nodeSecurityGroupID, + RemoteGroupID: lbSecGroupID, + } + secGroupRules, err := getSecurityGroupRules(lbaas.network, opts) + + if err != nil && !isNotFound(err) { + msg := fmt.Sprintf("Error finding rules for remote group id %s in security group id %s: %v", lbSecGroupID, nodeSecurityGroupID, err) + return fmt.Errorf(msg) + } + + for _, rule := range secGroupRules { + res := rules.Delete(lbaas.network, rule.ID) + if res.Err != nil && !isNotFound(res.Err) { + return fmt.Errorf("Error occurred deleting security group rule: %s: %v", rule.ID, res.Err) } } }