Support Rule Name for Azure Cloud Rule (#313)

Azure NSG rules will have rule name constructed as rule name within ANP + '-' + ANP name. This will provide visibility as which ANP/Rule maps to what cloud rule. Signed-off-by: Rahul Jain <[email protected]>
antrea-io · Sep 28, 2023 · ad85f67 · ad85f67
1 parent 2aee30e
commit ad85f67
Show file tree

Hide file tree

Showing 7 changed files with 76 additions and 21 deletions.
diff --git a/pkg/cloudprovider/cloudresource/cloudresource.go b/pkg/cloudprovider/cloudresource/cloudresource.go
@@ -129,6 +129,7 @@ type IngressRule struct {
 	AppliedToGroup     map[string]struct{}
 	Priority           *float64
 	Action             *antreacrdv1beta1.RuleAction
+	RuleName           string
 }
 
 func (i *IngressRule) isRule() {}
@@ -142,6 +143,7 @@ type EgressRule struct {
 	AppliedToGroup   map[string]struct{}
 	Priority         *float64
 	Action           *antreacrdv1beta1.RuleAction
+	RuleName         string
 }
 
 func (e *EgressRule) isRule() {}

diff --git a/pkg/cloudprovider/plugins/azure/azure_nsg_rules.go b/pkg/cloudprovider/plugins/azure/azure_nsg_rules.go
@@ -106,8 +106,6 @@ func updateSecurityRuleNameAndPriority(existingRules []*armnetwork.SecurityRule,
 			newRuleDesc, _ := utils.ExtractCloudDescription(newRules[i].Properties.Description)
 			if newRuleDesc.Priority != nil && currRulePriority != nil && *newRuleDesc.Priority <= *currRulePriority {
 				newRules[i].Properties.Priority = to.Int32Ptr(rulePriority)
-				ruleName := fmt.Sprintf("%v-%v", rulePriority, *newRules[i].Properties.Direction)
-				newRules[i].Name = &ruleName
 				rules = append(rules, newRules[i])
 				rulePriority++
 			} else {
@@ -116,8 +114,6 @@ func updateSecurityRuleNameAndPriority(existingRules []*armnetwork.SecurityRule,
 		}
 
 		rule.Properties.Priority = to.Int32Ptr(rulePriority)
-		ruleName := fmt.Sprintf("%v-%v", rulePriority, *rule.Properties.Direction)
-		rule.Name = &ruleName
 		rules = append(rules, rule)
 		rulePriority++
 	}
@@ -128,8 +124,6 @@ func updateSecurityRuleNameAndPriority(existingRules []*armnetwork.SecurityRule,
 		}
 		// update priority for new rules.
 		newRules[i].Properties.Priority = to.Int32Ptr(rulePriority)
-		ruleName := fmt.Sprintf("%v-%v", rulePriority, *newRules[i].Properties.Direction)
-		newRules[i].Name = &ruleName
 		rules = append(rules, newRules[i])
 		rulePriority++
 	}
@@ -172,7 +166,7 @@ func convertIngressToNsgSecurityRules(appliedToGroupID *cloudresource.CloudResou
 		if len(rule.FromSrcIP) != 0 || len(rule.FromSecurityGroups) == 0 {
 			srcAddrPrefix, srcAddrPrefixes := convertToAzureAddressPrefix(rule.FromSrcIP)
 			if srcAddrPrefix != nil || srcAddrPrefixes != nil {
-				securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionInbound,
+				securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionInbound,
 					to.StringPtr(emptyPort), srcAddrPrefix, srcAddrPrefixes, nil,
 					&srcPort, nil, nil, []*armnetwork.ApplicationSecurityGroup{&dstAsgObj}, &description, action)
 				securityRules = append(securityRules, &securityRule)
@@ -184,7 +178,7 @@ func convertIngressToNsgSecurityRules(appliedToGroupID *cloudresource.CloudResou
 			return []*armnetwork.SecurityRule{}, err
 		}
 		if len(srcApplicationSecurityGroups) != 0 {
-			securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionInbound,
+			securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionInbound,
 				to.StringPtr(emptyPort), nil, nil, srcApplicationSecurityGroups,
 				&srcPort, nil, nil, []*armnetwork.ApplicationSecurityGroup{&dstAsgObj}, &description, action)
 			securityRules = append(securityRules, &securityRule)
@@ -223,7 +217,7 @@ func convertIngressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudR
 		if len(rule.FromSrcIP) != 0 || len(rule.FromSecurityGroups) == 0 {
 			srcAddrPrefix, srcAddrPrefixes := convertToAzureAddressPrefix(rule.FromSrcIP)
 			if srcAddrPrefix != nil || srcAddrPrefixes != nil {
-				securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionInbound,
+				securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionInbound,
 					to.StringPtr(emptyPort), srcAddrPrefix, srcAddrPrefixes, nil,
 					&srcPort, to.StringPtr(emptyPort), nil, nil, &description, action)
 				securityRules = append(securityRules, &securityRule)
@@ -237,7 +231,7 @@ func convertIngressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudR
 					return []*armnetwork.SecurityRule{}, err
 				}
 				if len(srcApplicationSecurityGroups) != 0 {
-					securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionInbound,
+					securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionInbound,
 						to.StringPtr(emptyPort), nil, nil, srcApplicationSecurityGroups,
 						&srcPort, to.StringPtr(emptyPort), nil, nil, &description, action)
 					securityRules = append(securityRules, &securityRule)
@@ -247,7 +241,7 @@ func convertIngressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudR
 			}
 		}
 		if flag == 0 {
-			securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionInbound,
+			securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionInbound,
 				to.StringPtr(emptyPort), ruleIP, nil, nil,
 				&srcPort, to.StringPtr(emptyPort), nil, nil, &description,
 				armnetwork.SecurityRuleAccessAllow)
@@ -294,7 +288,7 @@ func convertEgressToNsgSecurityRules(appliedToGroupID *cloudresource.CloudResour
 		if len(rule.ToDstIP) != 0 || len(rule.ToSecurityGroups) == 0 {
 			dstAddrPrefix, dstAddrPrefixes := convertToAzureAddressPrefix(rule.ToDstIP)
 			if dstAddrPrefix != nil || dstAddrPrefixes != nil {
-				securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionOutbound,
+				securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionOutbound,
 					to.StringPtr(emptyPort), nil, nil, []*armnetwork.ApplicationSecurityGroup{&srcAsgObj},
 					&dstPort, dstAddrPrefix, dstAddrPrefixes, nil, &description, action)
 				securityRules = append(securityRules, &securityRule)
@@ -306,7 +300,7 @@ func convertEgressToNsgSecurityRules(appliedToGroupID *cloudresource.CloudResour
 			return []*armnetwork.SecurityRule{}, err
 		}
 		if len(dstApplicationSecurityGroups) != 0 {
-			securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionOutbound,
+			securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionOutbound,
 				to.StringPtr(emptyPort), nil, nil, []*armnetwork.ApplicationSecurityGroup{&srcAsgObj},
 				&dstPort, nil, nil, dstApplicationSecurityGroups, &description, action)
 			securityRules = append(securityRules, &securityRule)
@@ -345,7 +339,7 @@ func convertEgressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudRe
 		if len(rule.ToDstIP) != 0 || len(rule.ToSecurityGroups) == 0 {
 			dstAddrPrefix, dstAddrPrefixes := convertToAzureAddressPrefix(rule.ToDstIP)
 			if dstAddrPrefix != nil || dstAddrPrefixes != nil {
-				securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionOutbound,
+				securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionOutbound,
 					to.StringPtr(emptyPort), to.StringPtr(emptyPort), nil, nil,
 					&dstPort, dstAddrPrefix, dstAddrPrefixes, nil, &description, armnetwork.SecurityRuleAccessAllow)
 				securityRules = append(securityRules, &securityRule)
@@ -359,7 +353,7 @@ func convertEgressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudRe
 					return []*armnetwork.SecurityRule{}, err
 				}
 				if len(dstApplicationSecurityGroups) != 0 {
-					securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionOutbound,
+					securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionOutbound,
 						to.StringPtr(emptyPort), to.StringPtr(emptyPort), nil, nil,
 						&dstPort, nil, nil, dstApplicationSecurityGroups, &description, action)
 					securityRules = append(securityRules, &securityRule)
@@ -369,7 +363,7 @@ func convertEgressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudRe
 			}
 		}
 		if flag == 0 {
-			securityRule := buildSecurityRule(protoName, armnetwork.SecurityRuleDirectionOutbound,
+			securityRule := buildSecurityRule(rule.RuleName, protoName, armnetwork.SecurityRuleDirectionOutbound,
 				to.StringPtr(emptyPort), to.StringPtr(emptyPort), nil, nil,
 				&dstPort, ruleIP, nil, nil, &description, action)
 			securityRules = append(securityRules, &securityRule)
@@ -382,12 +376,13 @@ func convertEgressToPeerNsgSecurityRules(appliedToGroupID *cloudresource.CloudRe
 // nolint:whitespace
 // suppress whitespace linter to keep the function in a more readable format.
 // buildSecurityRule builds Azure security rule with given parameters.
-func buildSecurityRule(protoName armnetwork.SecurityRuleProtocol, direction armnetwork.SecurityRuleDirection,
+func buildSecurityRule(ruleName string, protoName armnetwork.SecurityRuleProtocol, direction armnetwork.SecurityRuleDirection,
 	srcPort *string, srcAddrPrefix *string, srcAddrPrefixes []*string, srcASGs []*armnetwork.ApplicationSecurityGroup,
 	dstPort *string, dstAddrPrefix *string, dstAddrPrefixes []*string, dstASGs []*armnetwork.ApplicationSecurityGroup,
 	description *string, access armnetwork.SecurityRuleAccess) armnetwork.SecurityRule {
 
 	securityRule := armnetwork.SecurityRule{
+		Name: &ruleName,
 		Properties: &armnetwork.SecurityRulePropertiesFormat{
 			Protocol:                             &protoName,
 			SourcePortRange:                      srcPort,
@@ -596,6 +591,7 @@ func convertFromAzureIngressSecurityRuleToCloudRule(rule armnetwork.SecurityRule
 				Protocol:  protoNum,
 				Priority:  priority,
 				Action:    action,
+				RuleName:  *rule.Name,
 			},
 			AppliedToGrp:     sgID,
 			NpNamespacedName: npNamespacedName,
@@ -611,6 +607,7 @@ func convertFromAzureIngressSecurityRuleToCloudRule(rule armnetwork.SecurityRule
 				Protocol:           protoNum,
 				Priority:           priority,
 				Action:             action,
+				RuleName:           *rule.Name,
 			},
 			AppliedToGrp:     sgID,
 			NpNamespacedName: npNamespacedName,
@@ -650,6 +647,7 @@ func convertFromAzureEgressSecurityRuleToCloudRule(rule armnetwork.SecurityRule,
 				Protocol: protoNum,
 				Priority: priority,
 				Action:   action,
+				RuleName: *rule.Name,
 			},
 			AppliedToGrp:     sgID,
 			NpNamespacedName: npNamespacedName,
@@ -665,6 +663,7 @@ func convertFromAzureEgressSecurityRuleToCloudRule(rule armnetwork.SecurityRule,
 				Protocol:         protoNum,
 				Priority:         priority,
 				Action:           action,
+				RuleName:         *rule.Name,
 			},
 			AppliedToGrp:     sgID,
 			NpNamespacedName: npNamespacedName,

diff --git a/pkg/cloudprovider/securitygroup/securitygroup.go b/pkg/cloudprovider/securitygroup/securitygroup.go
@@ -134,9 +134,10 @@ type CloudSecurityGroupInterface interface {
 
 	// CloudProviderSupportsRulePriority Returns true is Cloud Provider supports priority in the rule.
 	CloudProviderSupportsRulePriority(provider string) bool
-
 	// CloudProviderSupportsRuleAction Returns true is Cloud Provider supports action in the rule.
 	CloudProviderSupportsRuleAction(provider string) bool
+	// CloudProviderSupportsRuleName Returns true is Cloud Provider supports name in the rule.
+	CloudProviderSupportsRuleName(provider string) bool
 }
 
 type CloudSecurityGroupImpl struct{}
@@ -307,3 +308,7 @@ func (sg *CloudSecurityGroupImpl) CloudProviderSupportsRulePriority(provider str
 func (sg *CloudSecurityGroupImpl) CloudProviderSupportsRuleAction(provider string) bool {
 	return provider == string(runtimev1alpha1.AzureCloudProvider)
 }
+
+func (sg *CloudSecurityGroupImpl) CloudProviderSupportsRuleName(provider string) bool {
+	return provider == string(runtimev1alpha1.AzureCloudProvider)
+}
diff --git a/pkg/controllers/networkpolicy/controller_test.go b/pkg/controllers/networkpolicy/controller_test.go
@@ -348,6 +348,7 @@ var _ = Describe("NetworkPolicy", func() {
 		// mock
 		mockCloudSecurityAPI.EXPECT().CloudProviderSupportsRulePriority(mock.Any()).Return(false).AnyTimes()
 		mockCloudSecurityAPI.EXPECT().CloudProviderSupportsRuleAction(mock.Any()).Return(false).AnyTimes()
+		mockCloudSecurityAPI.EXPECT().CloudProviderSupportsRuleName(mock.Any()).Return(false).AnyTimes()
 	})
 
 	AfterEach(func() {

diff --git a/pkg/controllers/networkpolicy/networkpolicy.go b/pkg/controllers/networkpolicy/networkpolicy.go
@@ -20,6 +20,7 @@ import (
 	"net"
 	"reflect"
 	"sort"
+	"strconv"
 	"strings"
 
 	"github.com/mohae/deepcopy"
@@ -915,11 +916,14 @@ func (a *appliedToSecurityGroup) getCloudRulesFromNps(nps []interface{}) ([]*clo
 			}
 			if !securitygroup.CloudSecurityGroup.CloudProviderSupportsRuleAction(a.id.CloudProvider) {
 				if r.Action != nil && *r.Action == antreacrdv1beta1.RuleActionDrop {
-					return nil, fmt.Errorf("cloud provider %s doesnt support %s action", a.id.CloudProvider, antreacrdv1beta1.RuleActionDrop)
+					return nil, fmt.Errorf("cloud provider %s doesn't support %s action", a.id.CloudProvider, antreacrdv1beta1.RuleActionDrop)
 				}
 				// Reset Action.
 				ruleCopy.Action = nil
 			}
+			if !securitygroup.CloudSecurityGroup.CloudProviderSupportsRuleAction(a.id.CloudProvider) {
+				ruleCopy.RuleName = ""
+			}
 			rule := &cloudresource.CloudRule{
 				Rule:             ruleCopy,
 				NpNamespacedName: npNamespacedName,
@@ -946,6 +950,9 @@ func (a *appliedToSecurityGroup) getCloudRulesFromNps(nps []interface{}) ([]*clo
 				// Reset Action.
 				ruleCopy.Action = nil
 			}
+			if !securitygroup.CloudSecurityGroup.CloudProviderSupportsRuleAction(a.id.CloudProvider) {
+				ruleCopy.RuleName = ""
+			}
 			rule := &cloudresource.CloudRule{
 				Rule:             ruleCopy,
 				NpNamespacedName: npNamespacedName,
@@ -970,6 +977,7 @@ func (a *appliedToSecurityGroup) computeCloudRulesFromNp(r *NetworkPolicyReconci
 	// get current rules for given np to compute rule update delta.
 	currentRules, err := a.getCloudRulesFromNps([]interface{}{np})
 	if err != nil {
+		r.Log.Error(err, "failed to compute rules for networkPolicy", "name", np.Name, "namespace", np.Namespace)
 		return nil, nil, err
 	}
 	currentRuleMap := make(map[string]*cloudresource.CloudRule)
@@ -1320,10 +1328,12 @@ type networkPolicyRule struct {
 }
 
 // rules generate cloud plug-in ingressRule and/or egressRule from an networkPolicyRule.
-func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, policyPriority *float64, policyAppliedToGroups []string) (
+func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, anpName string, tier *int32, policyPriority *float64,
+	policyAppliedToGroups []string) (
 	ingressList []*cloudresource.IngressRule, egressList []*cloudresource.EgressRule, ready bool) {
 	ready = true
 	rule := r.rule
+	rCount := 1
 	if rule.Direction == antreanetworking.DirectionIn {
 		iRules := make([]*cloudresource.IngressRule, 0)
 		for _, ip := range rule.From.IPBlocks {
@@ -1337,6 +1347,9 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 			setAppliedToGroup(rule.AppliedToGroups, policyAppliedToGroups, ingress)
 			ingress.Action = rule.Action
 			ingress.Priority = cloudresource.GetRulePriority(tier, policyPriority, rule.Priority)
+			// TODO: Create 1 Rule for All IPBlocks and let cloud provider plugin split it into multiple rules.
+			ingress.RuleName = rule.Name + "-" + anpName + "-" + strconv.Itoa(rCount)
+			rCount++
 			iRules = append(iRules, ingress)
 		}
 		for _, ag := range rule.From.AddressGroups {
@@ -1360,6 +1373,9 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 					setAppliedToGroup(rule.AppliedToGroups, policyAppliedToGroups, ingress)
 					ingress.Priority = cloudresource.GetRulePriority(tier, policyPriority, rule.Priority)
 					ingress.Action = rule.Action
+					// TODO: Create 1 Rule for All AddressGroups and let cloud provider plugin split it into multiple rules.
+					ingress.RuleName = rule.Name + "-" + anpName + "-" + strconv.Itoa(rCount)
+					rCount++
 					iRules = append(iRules, ingress)
 				}
 			}
@@ -1371,6 +1387,7 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 			ingressList = append(ingressList, iRules...)
 			return
 		}
+		pCount := 1
 		for _, s := range rule.Services {
 			var protocol *int
 			var fromPort *int
@@ -1387,12 +1404,15 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 				i := deepcopy.Copy(ingress).(*cloudresource.IngressRule)
 				i.FromPort = fromPort
 				i.Protocol = protocol
+				i.RuleName += "." + strconv.Itoa(pCount)
+				pCount++
 				ingressList = append(ingressList, i)
 			}
 		}
 		return
 	}
 	eRules := make([]*cloudresource.EgressRule, 0)
+	rCount = 1
 	for _, ip := range rule.To.IPBlocks {
 		egress := &cloudresource.EgressRule{}
 		egress.AppliedToGroup = make(map[string]struct{}, 0)
@@ -1404,6 +1424,8 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 		setAppliedToGroup(rule.AppliedToGroups, policyAppliedToGroups, egress)
 		egress.Priority = cloudresource.GetRulePriority(tier, policyPriority, rule.Priority)
 		egress.Action = rule.Action
+		egress.RuleName = rule.Name + "-" + anpName + "-" + strconv.Itoa(rCount)
+		rCount++
 		eRules = append(eRules, egress)
 	}
 	for _, ag := range rule.To.AddressGroups {
@@ -1427,6 +1449,8 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 				setAppliedToGroup(rule.AppliedToGroups, policyAppliedToGroups, egress)
 				egress.Priority = cloudresource.GetRulePriority(tier, policyPriority, rule.Priority)
 				egress.Action = rule.Action
+				egress.RuleName = rule.Name + "-" + anpName + "-" + strconv.Itoa(rCount)
+				rCount++
 				eRules = append(eRules, egress)
 			}
 		}
@@ -1438,6 +1462,7 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 		egressList = append(egressList, eRules...)
 		return
 	}
+	pCount := 1
 	for _, s := range rule.Services {
 		var protocol *int
 		var fromPort *int
@@ -1454,6 +1479,8 @@ func (r *networkPolicyRule) rules(rr *NetworkPolicyReconciler, tier *int32, poli
 			e := deepcopy.Copy(egress).(*cloudresource.EgressRule)
 			e.ToPort = fromPort
 			e.Protocol = protocol
+			e.RuleName += "." + strconv.Itoa(pCount)
+			pCount++
 			egressList = append(egressList, e)
 		}
 	}
@@ -1705,7 +1732,7 @@ func (n *networkPolicy) computeRules(rr *NetworkPolicyReconciler) bool {
 	n.egressRules = nil
 	n.rulesReady = false
 	for _, r := range n.Rules {
-		ing, eg, ready := (&networkPolicyRule{rule: &r}).rules(rr, n.TierPriority, n.Priority, n.AppliedToGroups)
+		ing, eg, ready := (&networkPolicyRule{rule: &r}).rules(rr, n.Name, n.TierPriority, n.Priority, n.AppliedToGroups)
 		if !ready {
 			n.ingressRules = nil
 			n.egressRules = nil

diff --git a/pkg/testing/cloudsecurity/mock.go b/pkg/testing/cloudsecurity/mock.go