From cc7b4d7a789ffb54dd67c3e0e988c61607c78014 Mon Sep 17 00:00:00 2001 From: Alexander Liu Date: Tue, 20 Jun 2023 19:49:22 -0700 Subject: [PATCH] remove user rules in Azure that are in Nephe priority range Signed-off-by: Alexander Liu --- .../cloudapi/azure/azure_nsg_rules.go | 123 +++++++++++------- .../cloudapi/azure/azure_security.go | 18 ++- pkg/controllers/networkpolicy/controller.go | 2 +- pkg/controllers/networkpolicy/sync.go | 4 +- 4 files changed, 92 insertions(+), 55 deletions(-) diff --git a/pkg/cloudprovider/cloudapi/azure/azure_nsg_rules.go b/pkg/cloudprovider/cloudapi/azure/azure_nsg_rules.go index 5243be73..b964ac7f 100644 --- a/pkg/cloudprovider/cloudapi/azure/azure_nsg_rules.go +++ b/pkg/cloudprovider/cloudapi/azure/azure_nsg_rules.go @@ -68,6 +68,22 @@ func isAzureRuleAttachedToAtSg(rule *armnetwork.SecurityRule, asg string) bool { return false } +// getEmptyCloudRule returns a *securitygroup.CloudRule object with valid fields based on sync content, except nil for the rule field. +// If no valid options are available, nil is returned. +func getEmptyCloudRule(syncContent *securitygroup.SynchronizationContent) *securitygroup.CloudRule { + for _, rule := range append(syncContent.IngressRules, syncContent.EgressRules...) { + if rule.NpNamespacedName != "" { + emptyRule := &securitygroup.CloudRule{ + NpNamespacedName: rule.NpNamespacedName, + AppliedToGrp: rule.AppliedToGrp, + } + emptyRule.Hash = emptyRule.GetHash() + return emptyRule + } + } + return nil +} + // getUnusedPriority finds and returns the first unused priority starting from startPriority. func getUnusedPriority(existingRulePriority map[int32]struct{}, startPriority int32) int32 { _, ok := existingRulePriority[startPriority] @@ -495,69 +511,78 @@ func convertToAzureAddressPrefix(ruleIPs []*net.IPNet) (*string, []*string) { } // convertToCloudRulesByAppliedToSGName converts Azure rules to securitygroup.CloudRule and split them by security group names. +// It also returns a boolean as the third value indicating whether there are user rules in Nephe priority range or not. func convertToCloudRulesByAppliedToSGName(azureSecurityRules []*armnetwork.SecurityRule, - vnetID string) (map[string][]securitygroup.CloudRule, map[string][]securitygroup.CloudRule) { + vnetID string) (map[string][]securitygroup.CloudRule, map[string][]securitygroup.CloudRule, bool) { nepheControllerATSgNameToIngressRules := make(map[string][]securitygroup.CloudRule) nepheControllerATSgNameToEgressRules := make(map[string][]securitygroup.CloudRule) + removeUserRules := false for _, azureSecurityRule := range azureSecurityRules { if azureSecurityRule.Properties == nil { continue } - desc, _ := securitygroup.ExtractCloudDescription(azureSecurityRule.Properties.Description) - // Nephe inbound rule implies destination is AT sg. Nephe outbound rule implies source is AT sg. - // we don't care about syncing rules that doesn't match above pattern, as they will not conflict with any Nephe rules. - if *azureSecurityRule.Properties.Direction == armnetwork.SecurityRuleDirectionInbound { - for _, asg := range azureSecurityRule.Properties.DestinationApplicationSecurityGroups { - _, _, asgName, err := extractFieldsFromAzureResourceID(*asg.ID) - if err != nil { - azurePluginLogger().Error(err, "failed to extract asg name from resource id", "id", *asg.ID) - } - sgName, _, isATSg := securitygroup.IsNepheControllerCreatedSG(asgName) - if !isATSg { - continue - } - sgID := securitygroup.CloudResourceID{ - Name: sgName, - Vpc: vnetID, - } - ingressRule, err := convertFromAzureIngressSecurityRuleToCloudRule(*azureSecurityRule, sgID.String(), vnetID, desc) - if err != nil { - azurePluginLogger().Error(err, "failed to convert to ingress cloud rule", "ruleName", azureSecurityRule.Name) - continue - } - rules := nepheControllerATSgNameToIngressRules[sgName] - rules = append(rules, ingressRule...) - nepheControllerATSgNameToIngressRules[sgName] = rules + atAsgs := azureSecurityRule.Properties.DestinationApplicationSecurityGroups + ruleMap := nepheControllerATSgNameToIngressRules + convertFunc := convertFromAzureIngressSecurityRuleToCloudRule + if *azureSecurityRule.Properties.Direction == armnetwork.SecurityRuleDirectionOutbound { + atAsgs = azureSecurityRule.Properties.SourceApplicationSecurityGroups + ruleMap = nepheControllerATSgNameToEgressRules + convertFunc = convertFromAzureEgressSecurityRuleToCloudRule + } + isInNephePriorityRange := *azureSecurityRule.Properties.Priority >= ruleStartPriority + + // Nephe rule has correct description. + desc, ok := securitygroup.ExtractCloudDescription(azureSecurityRule.Properties.Description) + if !ok { + removeUserRules = removeUserRules || isInNephePriorityRange + // skip converting rule that is in Nephe priority range, as they will be removed. + if isInNephePriorityRange { + continue } - } else { - for _, asg := range azureSecurityRule.Properties.SourceApplicationSecurityGroups { - _, _, asgName, err := extractFieldsFromAzureResourceID(*asg.ID) - if err != nil { - azurePluginLogger().Error(err, "failed to extract asg name from resource id", "id", *asg.ID) - } - sgName, _, isATSg := securitygroup.IsNepheControllerCreatedSG(asgName) - if !isATSg { - continue - } - sgID := securitygroup.CloudResourceID{ - Name: sgName, - Vpc: vnetID, - } - egressRule, err := convertFromAzureEgressSecurityRuleToCloudRule(*azureSecurityRule, sgID.String(), vnetID, desc) - if err != nil { - azurePluginLogger().Error(err, "failed to convert to egress cloud rule", "ruleName", azureSecurityRule.Name) - continue - } - rules := nepheControllerATSgNameToEgressRules[sgName] - rules = append(rules, egressRule...) - nepheControllerATSgNameToEgressRules[sgName] = rules + } + + // Nephe rule has AT sg. + // we don't care about syncing rules that doesn't have Nephe AT sg, as they will not conflict with any Nephe rules. + if len(atAsgs) == 0 { + removeUserRules = removeUserRules || isInNephePriorityRange + continue + } + for _, asg := range atAsgs { + // Nephe rule has the correct AT sg naming format. + _, _, asgName, err := extractFieldsFromAzureResourceID(*asg.ID) + if err != nil { + azurePluginLogger().Error(err, "failed to extract asg name from resource id", "id", *asg.ID) + removeUserRules = removeUserRules || isInNephePriorityRange + continue + } + sgName, _, isATSg := securitygroup.IsNepheControllerCreatedSG(asgName) + if !isATSg { + removeUserRules = removeUserRules || isInNephePriorityRange + continue + } + + sgID := securitygroup.CloudResourceID{ + Name: sgName, + Vpc: vnetID, } + + rule, err := convertFunc(*azureSecurityRule, sgID.String(), vnetID, desc) + if err != nil { + azurePluginLogger().Error(err, "failed to convert to cloud rule", + "direction", azureSecurityRule.Properties.Direction, "ruleName", azureSecurityRule.Name) + removeUserRules = removeUserRules || isInNephePriorityRange + continue + } + + rules := ruleMap[sgName] + rules = append(rules, rule...) + ruleMap[sgName] = rules } } - return nepheControllerATSgNameToIngressRules, nepheControllerATSgNameToEgressRules + return nepheControllerATSgNameToIngressRules, nepheControllerATSgNameToEgressRules, removeUserRules } // convertFromAzureIngressSecurityRuleToCloudRule converts Azure ingress rules from armnetwork.SecurityRule to securitygroup.CloudRule. diff --git a/pkg/cloudprovider/cloudapi/azure/azure_security.go b/pkg/cloudprovider/cloudapi/azure/azure_security.go index 7a0428e1..72b4db85 100644 --- a/pkg/cloudprovider/cloudapi/azure/azure_security.go +++ b/pkg/cloudprovider/cloudapi/azure/azure_security.go @@ -344,8 +344,12 @@ func (computeCfg *computeServiceConfig) buildEffectiveNSGSecurityRulesToApply(ap } // check if the rule is Nephe rules based on ruleStartPriority and description. - _, ok := securitygroup.ExtractCloudDescription(rule.Properties.Description) - if *rule.Properties.Priority >= ruleStartPriority && ok { + if *rule.Properties.Priority >= ruleStartPriority { + _, ok := securitygroup.ExtractCloudDescription(rule.Properties.Description) + // remove user rule that is in Nephe priority range. + if !ok { + continue + } // check if the rule is created by current processing appliedToGroup. if isAzureRuleAttachedToAtSg(rule, appliedToGroupNepheControllerName) { // skip the rule if found in remove list. @@ -697,7 +701,7 @@ func (computeCfg *computeServiceConfig) getATGroupView(nepheControllerATSGNameTo if networkSecurityGroup.Properties == nil { continue } - nepheControllerATSgNameToIngressRulesMap, nepheControllerATSgNameToEgressRulesMap := + nepheControllerATSgNameToIngressRulesMap, nepheControllerATSgNameToEgressRulesMap, removeUserRules := convertToCloudRulesByAppliedToSGName(networkSecurityGroup.Properties.SecurityRules, vnetIDLowercase) for atSgName := range appliedToSgNameSet { @@ -717,6 +721,14 @@ func (computeCfg *computeServiceConfig) getATGroupView(nepheControllerATSGNameTo IngressRules: nepheControllerATSgNameToIngressRulesMap[atSgName], EgressRules: nepheControllerATSgNameToEgressRulesMap[atSgName], } + // if there are user rules needs to be removed, trick the sync to trigger a rule update by adding an empty valid rule. + // in case of no AT or NP for valid rule, it implies Nephe is not actively managing the Vnet, therefore user rules are ignored. + if removeUserRules { + if rule := getEmptyCloudRule(&groupSyncObj); rule != nil { + groupSyncObj.IngressRules = append(groupSyncObj.IngressRules, *rule) + removeUserRules = false + } + } enforcedSecurityCloudView = append(enforcedSecurityCloudView, groupSyncObj) } } diff --git a/pkg/controllers/networkpolicy/controller.go b/pkg/controllers/networkpolicy/controller.go index 308dd21f..916de4c4 100644 --- a/pkg/controllers/networkpolicy/controller.go +++ b/pkg/controllers/networkpolicy/controller.go @@ -693,7 +693,7 @@ func (r *NetworkPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error { }) // cloudRuleIndexer stores the realized rules on the cloud. r.cloudRuleIndexer = cache.NewIndexer( - // Each cloudRule is uniquely identified by its UUID. + // Each cloudRule is uniquely identified by its hash. func(obj interface{}) (string, error) { rule := obj.(*securitygroup.CloudRule) return rule.Hash, nil diff --git a/pkg/controllers/networkpolicy/sync.go b/pkg/controllers/networkpolicy/sync.go index 54bfa329..8e049d40 100644 --- a/pkg/controllers/networkpolicy/sync.go +++ b/pkg/controllers/networkpolicy/sync.go @@ -138,7 +138,7 @@ func (a *appliedToSecurityGroup) sync(syncContent *securitygroup.Synchronization // roughly count and compare rules in syncContent against nps. // also updates cloudRuleIndexer in the process. for _, rule := range syncContent.IngressRules { - if rule.NpNamespacedName != "" { + if rule.NpNamespacedName != "" && rule.Rule != nil { iRule := rule.Rule.(*securitygroup.IngressRule) countIngressRuleItems(iRule, items, true) } @@ -147,7 +147,7 @@ func (a *appliedToSecurityGroup) sync(syncContent *securitygroup.Synchronization } } for _, rule := range syncContent.EgressRules { - if rule.NpNamespacedName != "" { + if rule.NpNamespacedName != "" && rule.Rule != nil { eRule := rule.Rule.(*securitygroup.EgressRule) countEgressRuleItems(eRule, items, true) }