Skip to content

Commit

Permalink
Change MCNP secFlows implementation (antrea-io#4572)
Browse files Browse the repository at this point in the history 
This PR changes how we implement MultiClusterNetworkPolicy security
flows.

Before, we created a security rule for each MultiClusterNetworkPolicy
rule to install security flows.

Now, we use the default drop flow to implement the security flows. For
a MultiClusterNetworkPolicy rule, the drop flow will use the
UnknownLabelIdentity and the original destination matching to drop
packets.

Signed-off-by: graysonwu <wgrayson@vmware.com>
  • Loading branch information
@GraysonWu
GraysonWu authored Feb 7, 2023
1 parent 2b29229 commit a63314f
Show file tree
Hide file tree
Showing 10 changed files with 131 additions and 100 deletions.
71 changes: 15 additions & 56 deletions pkg/agent/controller/networkpolicy/cache.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (

"antrea.io/antrea/pkg/agent/config"
"antrea.io/antrea/pkg/agent/metrics"
"antrea.io/antrea/pkg/agent/openflow"
agenttypes "antrea.io/antrea/pkg/agent/types"
v1beta "antrea.io/antrea/pkg/apis/controlplane/v1beta2"
crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1"
Expand Down Expand Up @@ -695,38 +694,6 @@ func toRule(r *v1beta.NetworkPolicyRule, policy *v1beta.NetworkPolicy, maxPriori
return rule
}

// toStretchedNetworkPolicySecurityRule converts v1beta.NetworkPolicyRule to *rule
// which is used to drop all traffic initiated from Pods with UnknownLabelIdentity.
// Pods may have UnknownLabelIdentity when their Labels are completely new Labels
// among the whole ClusterSet. Before the new allocated LabelIdentity is imported
// in the Cluster, those Pods will have UnknownLabelIdentity.
func toStretchedNetworkPolicySecurityRule(r *v1beta.NetworkPolicyRule, policy *v1beta.NetworkPolicy, maxPriority int32) *rule {
appliedToGroups := policy.AppliedToGroups
if len(r.AppliedToGroups) != 0 {
appliedToGroups = r.AppliedToGroups
}
snpSecDropAction := crdv1alpha1.RuleActionDrop
rule := &rule{
Direction: r.Direction,
From: v1beta.NetworkPolicyPeer{LabelIdentities: []uint32{openflow.UnknownLabelIdentity}},
To: r.To,
Services: r.Services,
Action: &snpSecDropAction,
Priority: r.Priority,
PolicyPriority: policy.Priority,
TierPriority: policy.TierPriority,
AppliedToGroups: appliedToGroups,
Name: r.Name + "-security",
PolicyUID: policy.UID,
SourceRef: policy.SourceRef,
EnableLogging: r.EnableLogging,
}
rule.ID = hashRule(rule)
rule.PolicyName = policy.Name
rule.MaxPriority = maxPriority
return rule
}

// getMaxPriority returns the highest rule priority for v1beta.NetworkPolicy that is created
// by Antrea-native policies. For K8s NetworkPolicies, it always returns -1.
func getMaxPriority(policy *v1beta.NetworkPolicy) int32 {
Expand Down Expand Up @@ -808,31 +775,23 @@ func (c *ruleCache) updateNetworkPolicyLocked(policy *v1beta.NetworkPolicy) bool
anyRuleUpdate := false
maxPriority := getMaxPriority(policy)
for i := range policy.Rules {
rules := []*rule{toRule(&policy.Rules[i], policy, maxPriority)}
if len(policy.Rules[i].From.LabelIdentities) > 0 {
// If the rule is a StretchedNetworkPolicy rule, we also need to add a security rule.
// The security rule is used to drop all traffic initiated from Pods with
// UnknownLabelIdentity to make sure those traffic won't sneak around the Policy.
rules = append(rules, toStretchedNetworkPolicySecurityRule(&policy.Rules[i], policy, maxPriority))
}
for _, r := range rules {
if _, exists := ruleByID[r.ID]; exists {
// If rule already exists, remove it from the map so the ones left are orphaned,
// which means those rules need to be handled by dirtyRuleHandler.
klog.V(2).InfoS("Rule was not changed", "id", r.ID)
delete(ruleByID, r.ID)
r := toRule(&policy.Rules[i], policy, maxPriority)
if _, exists := ruleByID[r.ID]; exists {
// If rule already exists, remove it from the map so the ones left are orphaned,
// which means those rules need to be handled by dirtyRuleHandler.
klog.V(2).InfoS("Rule was not changed", "id", r.ID)
delete(ruleByID, r.ID)
} else {
// If rule doesn't exist, add it to cache and mark it as dirty.
c.rules.Add(r)
// Count up antrea_agent_ingress_networkpolicy_rule_count or antrea_agent_egress_networkpolicy_rule_count
if r.Direction == v1beta.DirectionIn {
metrics.IngressNetworkPolicyRuleCount.Inc()
} else {
// If rule doesn't exist, add it to cache and mark it as dirty.
c.rules.Add(r)
// Count up antrea_agent_ingress_networkpolicy_rule_count or antrea_agent_egress_networkpolicy_rule_count
if r.Direction == v1beta.DirectionIn {
metrics.IngressNetworkPolicyRuleCount.Inc()
} else {
metrics.EgressNetworkPolicyRuleCount.Inc()
}
c.dirtyRuleHandler(r.ID)
anyRuleUpdate = true
metrics.EgressNetworkPolicyRuleCount.Inc()
}
c.dirtyRuleHandler(r.ID)
anyRuleUpdate = true
}
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/agent/controller/networkpolicy/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -681,7 +681,6 @@ func TestRuleCacheAddNetworkPolicy(t *testing.T) {
rule2 := toRule(networkPolicyRule2, networkPolicy2, k8sNPMaxPriority)
rule3 := toRule(networkPolicyRule3, networkPolicy3, 0)
rule4 := toRule(networkPolicyRule4, networkPolicy4, 0)
rule4Sec := toStretchedNetworkPolicySecurityRule(networkPolicyRule4, networkPolicy4, 0)
tests := []struct {
name string
args *v1beta2.NetworkPolicy
Expand Down Expand Up @@ -709,8 +708,8 @@ func TestRuleCacheAddNetworkPolicy(t *testing.T) {
{
"rule-with-label-identity",
networkPolicy4,
[]*rule{rule4, rule4Sec},
sets.NewString(rule4.ID, rule4Sec.ID),
[]*rule{rule4},
sets.NewString(rule4.ID),
},
}
for _, tt := range tests {
Expand Down
12 changes: 6 additions & 6 deletions pkg/agent/controller/networkpolicy/reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,7 +745,7 @@ func (r *reconciler) update(lastRealized *lastRealized, newRule *CompletedRule,
} else {
addedTo := ofPortsToOFAddresses(newOFPorts.Difference(lastRealized.podOFPorts[igmpServicesKey]))
deletedTo := ofPortsToOFAddresses(lastRealized.podOFPorts[igmpServicesKey].Difference(newOFPorts))
if err := r.updateOFRule(ofID, nil, addedTo, nil, deletedTo, ofPriority, newRule.EnableLogging); err != nil {
if err := r.updateOFRule(ofID, nil, addedTo, nil, deletedTo, ofPriority, newRule.EnableLogging, false); err != nil {
return err
}
}
Expand Down Expand Up @@ -816,7 +816,7 @@ func (r *reconciler) update(lastRealized *lastRealized, newRule *CompletedRule,
addedTo = ofPortsToOFAddresses(newOFPorts.Difference(originalOfPortsSet))
deletedTo = ofPortsToOFAddresses(originalOfPortsSet.Difference(newOFPorts))
}
if err := r.updateOFRule(ofID, addedFrom, addedTo, deletedFrom, deletedTo, ofPriority, newRule.EnableLogging); err != nil {
if err := r.updateOFRule(ofID, addedFrom, addedTo, deletedFrom, deletedTo, ofPriority, newRule.EnableLogging, len(newRule.From.LabelIdentities) > 0); err != nil {
return err
}
// Delete valid servicesKey from staleOFIDs.
Expand Down Expand Up @@ -908,7 +908,7 @@ func (r *reconciler) update(lastRealized *lastRealized, newRule *CompletedRule,
addedTo = svcGroupIDsToOFAddresses(newGroupIDSet.Difference(originalGroupIDSet))
deletedTo = svcGroupIDsToOFAddresses(originalGroupIDSet.Difference(newGroupIDSet))
}
if err := r.updateOFRule(ofID, addedFrom, addedTo, deletedFrom, deletedTo, ofPriority, newRule.EnableLogging); err != nil {
if err := r.updateOFRule(ofID, addedFrom, addedTo, deletedFrom, deletedTo, ofPriority, newRule.EnableLogging, false); err != nil {
return err
}
if r.fqdnController != nil {
Expand Down Expand Up @@ -945,17 +945,17 @@ func (r *reconciler) installOFRule(ofRule *types.PolicyRule) error {
return nil
}

func (r *reconciler) updateOFRule(ofID uint32, addedFrom []types.Address, addedTo []types.Address, deletedFrom []types.Address, deletedTo []types.Address, priority *uint16, enableLogging bool) error {
func (r *reconciler) updateOFRule(ofID uint32, addedFrom []types.Address, addedTo []types.Address, deletedFrom []types.Address, deletedTo []types.Address, priority *uint16, enableLogging, isMCNPRule bool) error {
klog.V(2).Infof("Updating ofRule %d (addedFrom: %d, addedTo: %d, deleteFrom: %d, deletedTo: %d)",
ofID, len(addedFrom), len(addedTo), len(deletedFrom), len(deletedTo))
// TODO: This might be unnecessarily complex and hard for error handling, consider revising the Openflow interfaces.
if len(addedFrom) > 0 {
if err := r.ofClient.AddPolicyRuleAddress(ofID, types.SrcAddress, addedFrom, priority, enableLogging); err != nil {
if err := r.ofClient.AddPolicyRuleAddress(ofID, types.SrcAddress, addedFrom, priority, enableLogging, isMCNPRule); err != nil {
return fmt.Errorf("error adding policy rule source addresses for ofRule %v: %v", ofID, err)
}
}
if len(addedTo) > 0 {
if err := r.ofClient.AddPolicyRuleAddress(ofID, types.DstAddress, addedTo, priority, enableLogging); err != nil {
if err := r.ofClient.AddPolicyRuleAddress(ofID, types.DstAddress, addedTo, priority, enableLogging, isMCNPRule); err != nil {
return fmt.Errorf("error adding policy rule destination addresses for ofRule %v: %v", ofID, err)
}
}
Expand Down
31 changes: 29 additions & 2 deletions pkg/agent/controller/networkpolicy/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1099,6 +1099,7 @@ func TestReconcilerUpdate(t *testing.T) {
expectedDeletedFrom []types.Address
expectedDeletedTo []types.Address
expectUninstall bool
isMCNPRule bool
wantErr bool
}{
{
Expand All @@ -1119,6 +1120,7 @@ func TestReconcilerUpdate(t *testing.T) {
ofPortsToOFAddresses(sets.NewInt32(1)),
false,
false,
false,
},
{
"updating-egress-rule",
Expand All @@ -1138,6 +1140,7 @@ func TestReconcilerUpdate(t *testing.T) {
ipsToOFAddresses(sets.NewString("1.1.1.1")),
false,
false,
false,
},
{
"updating-ingress-rule-with-missing-ofport",
Expand All @@ -1157,6 +1160,7 @@ func TestReconcilerUpdate(t *testing.T) {
ofPortsToOFAddresses(sets.NewInt32(1)),
false,
false,
false,
},
{
"updating-egress-rule-with-missing-ip",
Expand All @@ -1176,6 +1180,7 @@ func TestReconcilerUpdate(t *testing.T) {
ipsToOFAddresses(sets.NewString("1.1.1.1")),
false,
false,
false,
},
{
"updating-egress-rule-with-duplicate-ip",
Expand Down Expand Up @@ -1207,6 +1212,7 @@ func TestReconcilerUpdate(t *testing.T) {
ipsToOFAddresses(sets.NewString("1.1.1.2", "1.1.1.3")),
false,
false,
false,
},
{
"updating-egress-rule-deny-all",
Expand All @@ -1226,6 +1232,7 @@ func TestReconcilerUpdate(t *testing.T) {
[]types.Address{},
false,
false,
false,
},
{
"updating-cnp-ingress-rule",
Expand All @@ -1245,6 +1252,7 @@ func TestReconcilerUpdate(t *testing.T) {
ofPortsToOFAddresses(sets.NewInt32(1)),
false,
false,
false,
},
{
"updating-cnp-ingress-rule-uninstall",
Expand All @@ -1264,6 +1272,25 @@ func TestReconcilerUpdate(t *testing.T) {
[]types.Address{},
true,
false,
false,
},
{
"updating-mcnp-ingress",
&CompletedRule{
rule: &rule{ID: "ingress-rule", Direction: v1beta2.DirectionIn, PolicyPriority: &policyPriority, TierPriority: &tierPriority, From: v1beta2.NetworkPolicyPeer{LabelIdentities: []uint32{1}}, SourceRef: &cnp1, EnableLogging: false},
TargetMembers: appliedToGroup1,
},
&CompletedRule{
rule: &rule{ID: "ingress-rule", Direction: v1beta2.DirectionIn, PolicyPriority: &policyPriority, TierPriority: &tierPriority, From: v1beta2.NetworkPolicyPeer{LabelIdentities: []uint32{1}}, SourceRef: &cnp1, EnableLogging: false},
TargetMembers: appliedToGroup2,
},
[]types.Address{},
ofPortsToOFAddresses(sets.NewInt32(2)),
[]types.Address{},
ofPortsToOFAddresses(sets.NewInt32(1)),
false,
true,
false,
},
}
for _, tt := range tests {
Expand All @@ -1280,10 +1307,10 @@ func TestReconcilerUpdate(t *testing.T) {
mockOFClient.EXPECT().UninstallPolicyRuleFlows(gomock.Any())
}
if len(tt.expectedAddedFrom) > 0 {
mockOFClient.EXPECT().AddPolicyRuleAddress(gomock.Any(), types.SrcAddress, gomock.InAnyOrder(tt.expectedAddedFrom), priority, false)
mockOFClient.EXPECT().AddPolicyRuleAddress(gomock.Any(), types.SrcAddress, gomock.InAnyOrder(tt.expectedAddedFrom), priority, false, tt.isMCNPRule)
}
if len(tt.expectedAddedTo) > 0 {
mockOFClient.EXPECT().AddPolicyRuleAddress(gomock.Any(), types.DstAddress, gomock.InAnyOrder(tt.expectedAddedTo), priority, false)
mockOFClient.EXPECT().AddPolicyRuleAddress(gomock.Any(), types.DstAddress, gomock.InAnyOrder(tt.expectedAddedTo), priority, false, tt.isMCNPRule)
}
if len(tt.expectedDeletedFrom) > 0 {
mockOFClient.EXPECT().DeletePolicyRuleAddress(gomock.Any(), types.SrcAddress, gomock.InAnyOrder(tt.expectedDeletedFrom), priority)
Expand Down
2 changes: 1 addition & 1 deletion pkg/agent/openflow/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ type Client interface {

// AddPolicyRuleAddress adds one or multiple addresses to the specified NetworkPolicy rule. If addrType is true, the
// addresses are added to PolicyRule.From, else to PolicyRule.To.
AddPolicyRuleAddress(ruleID uint32, addrType types.AddressType, addresses []types.Address, priority *uint16, enableLogging bool) error
AddPolicyRuleAddress(ruleID uint32, addrType types.AddressType, addresses []types.Address, priority *uint16, enableLogging, isMCNPRule bool) error

// DeletePolicyRuleAddress removes addresses from the specified NetworkPolicy rule. If addrType is srcAddress, the addresses
// are removed from PolicyRule.From, else from PolicyRule.To.
Expand Down
Loading

0 comments on commit a63314f

Please sign in to comment.