Make ensureSecurityRule() safely idempotent (kubernetes#2249)

`ensureSecurityRule()` is used to add a rule to an existing security
group. At first it was listing all the rules in the SG and then if the
requested rule wasn't present it was adding it. This has some risk - the
list of SG rules can be modified in-between by some other thread.

This commit changes the logic of that function to just try to create the
rule. Neutron API will return 409 Conflict if the rule already exists
and if it does OCCM will just ignore the error and move on.

Author: dulek

pkg/openstack/loadbalancer.go

@@ -2121,23 +2121,6 @@ func (lbaas *LbaasV2) ensureSecurityRule(
 	remoteIPPrefix, secGroupID string,
 	portRangeMin, portRangeMax int,
 ) error {
-	sgListopts := rules.ListOpts{
-		Direction:      string(direction),
-		Protocol:       string(protocol),
-		PortRangeMax:   portRangeMin,
-		PortRangeMin:   portRangeMax,
-		RemoteIPPrefix: remoteIPPrefix,
-		SecGroupID:     secGroupID,
-	}
-	sgRules, err := getSecurityGroupRules(lbaas.network, sgListopts)
-	if err != nil && !cpoerrors.IsNotFound(err) {
-		return fmt.Errorf(
-			"failed to find security group rules in %s: %v", secGroupID, err)
-	}
-	if len(sgRules) != 0 {
-		return nil
-	}
-
 	sgRuleCreateOpts := rules.CreateOpts{
 		Direction:      direction,
 		Protocol:       protocol,
@@ -2149,11 +2132,12 @@ func (lbaas *LbaasV2) ensureSecurityRule(
 	}
 
 	mc := metrics.NewMetricContext("security_group_rule", "create")
-	_, err = rules.Create(lbaas.network, sgRuleCreateOpts).Extract()
-	if mc.ObserveRequest(err) != nil {
-		return fmt.Errorf(
-			"failed to create rule for security group %s: %v",
-			secGroupID, err)
+	_, err := rules.Create(lbaas.network, sgRuleCreateOpts).Extract()
+	if err != nil && cpoerrors.IsConflictError(err) {
+		// Conflict means the SG rule already exists, so ignoring that error.
+		return mc.ObserveRequest(nil)
+	} else if mc.ObserveRequest(err) != nil {
+		return fmt.Errorf("failed to create rule for security group %s: %v", secGroupID, err)
 	}
 	return nil
 }

pkg/util/errors/errors.go

@@ -77,3 +77,17 @@ func IsInvalidError(err error) bool {
 
 	return false
 }
+
+func IsConflictError(err error) bool {
+	if _, ok := err.(gophercloud.ErrDefault409); ok {
+		return true
+	}
+
+	if errCode, ok := err.(gophercloud.ErrUnexpectedResponseCode); ok {
+		if errCode.Actual == http.StatusConflict {
+			return true
+		}
+	}
+
+	return false
+}