Skip to content

Commit

Permalink
Merge pull request #105 from danwinship/fix-networkpolicy-vnid-0-4.2
Browse files Browse the repository at this point in the history 
Bug 1793952: [4.2] Fix reinitialization of deny-all NetworkPolicy state on restart
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot committed Feb 10, 2020
2 parents 098a641 + 2da9083 commit e9fab36
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 8 deletions.
14 changes: 10 additions & 4 deletions pkg/network/node/ovscontroller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -663,9 +663,11 @@ func (oc *ovsController) UpdateVXLANMulticastFlows(remoteIPs []string) error {

// FindPolicyVNIDs returns the set of VNIDs for which there are currently "policy" rules
// in OVS. (This is used to reinitialize the osdnPolicy after a restart.)
// We also include inUseVNIDs because if a namespace has only a deny all rule
// policyVNIDs won't include that namespace.
func (oc *ovsController) FindPolicyVNIDs() sets.Int {
_, policyVNIDs := oc.findInUseAndPolicyVNIDs()
return policyVNIDs
inUseVNIDs, policyVNIDs := oc.findInUseAndPolicyVNIDs()
return inUseVNIDs.Union(policyVNIDs)
}

// FindUnusedVNIDs returns a list of VNIDs for which there are table 80 "policy" rules,
Expand All @@ -676,12 +678,16 @@ func (oc *ovsController) FindPolicyVNIDs() sets.Int {
// race condition.
func (oc *ovsController) FindUnusedVNIDs() []int {
inUseVNIDs, policyVNIDs := oc.findInUseAndPolicyVNIDs()
// VNID 0 is always in use, even if there aren't any flows for it in table 60/70
inUseVNIDs.Insert(0)
return policyVNIDs.Difference(inUseVNIDs).UnsortedList()
}

// findInUseAndPolicyVNIDs returns two sets: the VNIDs that are currently in use by pods
// or services on this node, and the VNIDs that are currently in use by NetworkPolicies
// on this node.
func (oc *ovsController) findInUseAndPolicyVNIDs() (sets.Int, sets.Int) {
// VNID 0 is always in use, even if there aren't any explicit flows for it
inUseVNIDs := sets.NewInt(0)
inUseVNIDs := sets.NewInt()
policyVNIDs := sets.NewInt()

flows, err := oc.ovs.DumpFlows("")
Expand Down
106 changes: 102 additions & 4 deletions pkg/network/node/ovscontroller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"

"github.com/containernetworking/plugins/pkg/utils/hwaddr"
)
Expand Down Expand Up @@ -825,6 +826,32 @@ func TestFindUnusedVNIDs(t *testing.T) {
policy []int
unused []int
}{
{
/* VNID 0 is never unused, even if there are no table 60/70 rules for it */
flows: []string{
"table=60,priority=100,ip,nw_dst=172.30.0.1,nw_frag=later actions=load:0->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,ip,nw_dst=172.30.156.103,nw_frag=later actions=load:0xcb81e9->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,ip,nw_dst=172.30.76.192,nw_frag=later actions=load:0x55fac->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,tcp,nw_dst=172.30.0.1,tp_dst=443 actions=load:0->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,udp,nw_dst=172.30.0.1,tp_dst=53 actions=load:0->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,tcp,nw_dst=172.30.0.1,tp_dst=53 actions=load:0->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,tcp,nw_dst=172.30.156.103,tp_dst=5454 actions=load:0xcb81e9->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,tcp,nw_dst=172.30.76.192,tp_dst=5454 actions=load:0x55fac->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=100,tcp,nw_dst=172.30.76.192,tp_dst=5455 actions=load:0x55fac->NXM_NX_REG1[],load:0x2->NXM_NX_REG2[],goto_table:80",
"table=60,priority=0 actions=drop",
"table=70,priority=100,ip,nw_dst=10.129.0.2 actions=load:0x55fac->NXM_NX_REG1[],load:0x3->NXM_NX_REG2[],goto_table:80",
"table=70,priority=100,ip,nw_dst=10.129.0.3 actions=load:0xcb81e9->NXM_NX_REG1[],load:0x4->NXM_NX_REG2[],goto_table:80",
"table=70,priority=0 actions=drop",
"table=80,priority=300,ip,nw_src=10.129.0.1 actions=output:NXM_NX_REG2[]",
"table=80,priority=200,reg0=0 actions=output:NXM_NX_REG2[]",
"table=80,priority=200,reg1=0 actions=output:NXM_NX_REG2[]",
"table=80,priority=100,reg0=0x55fac,reg1=0x55fac actions=output:NXM_NX_REG2[]",
"table=80,priority=100,reg0=0xcb81e9,reg1=0xcb81e9 actions=output:NXM_NX_REG2[]",
"table=80,priority=0 actions=drop",
},
policy: []int{0x0, 0x55fac, 0xcb81e9},
unused: []int{},
},
{
/* Both VNIDs have 1 pod and 1 service, so they stay */
flows: []string{
Expand Down Expand Up @@ -956,10 +983,6 @@ func TestFindUnusedVNIDs(t *testing.T) {
t.Fatalf("(%d) unexpected error from AddFlow: %v", i, err)
}

policy := oc.FindPolicyVNIDs()
if !reflect.DeepEqual(policy.List(), tc.policy) {
t.Fatalf("(%d) wrong result for policy, expected %v, got %v", i, tc.policy, policy.List())
}
unused := oc.FindUnusedVNIDs()
sort.Ints(unused)
if !reflect.DeepEqual(unused, tc.unused) {
Expand All @@ -968,6 +991,81 @@ func TestFindUnusedVNIDs(t *testing.T) {
}
}

func TestFindPolicyVNIDs(t *testing.T) {
testcases := []struct {
flows []string
policyVNIDs sets.Int
}{
{
// No rules -> no VNIDs
flows: []string{
"table=80,priority=0 actions=drop",
},
policyVNIDs: sets.NewInt(),
},
{
// Namespaces without any rule on table 80 must be present.
flows: []string{
"table=60, priority=200 actions=output:tun0",
"table=60, priority=0 actions=drop",
"table=70, priority=100,ip,nw_dst=10.129.0.52 actions=load:0x2bd973->NXM_NX_REG1[],load:0x15->NXM_NX_REG2[],goto_table:80",
"table=70, priority=100,ip,nw_dst=10.129.0.53 actions=load:0x0->NXM_NX_REG1[],load:0x16->NXM_NX_REG2[],goto_table:80",
"table=70, priority=0 actions=drop",
"table=80, priority=300,ip,nw_src=10.129.0.1 actions=output:NXM_NX_REG2[]",
"table=80, priority=200,ct_state=+rpl,ip actions=output:NXM_NX_REG2[]",
"table=80, priority=0 actions=drop",
},
policyVNIDs: sets.NewInt(0x0, 0x2bd973),
},
{
// Namespaces present in table 80 must always be present, even if they don't have pods
flows: []string{
"table=80, priority=300,ip,nw_src=10.129.0.1 actions=output:NXM_NX_REG2[]",
"table=80, priority=200,ct_state=+rpl,ip actions=output:NXM_NX_REG2[]",
"table=80, priority=50,reg1=0x58bb64 actions=output:NXM_NX_REG2[]",
"table=80, priority=50,reg1=0x0 actions=output:NXM_NX_REG2[]",
"table=80, priority=0 actions=drop",
},
policyVNIDs: sets.NewInt(0x0, 0x58bb64),
},
{
// All tests combined
flows: []string{
"table=60, priority=200 actions=output:tun0",
"table=60, priority=0 actions=drop",
"table=70, priority=100,ip,nw_dst=10.129.0.52 actions=load:0x2bd973->NXM_NX_REG1[],load:0x15->NXM_NX_REG2[],goto_table:80",
"table=70, priority=100,ip,nw_dst=10.129.0.54 actions=load:0x58bb64->NXM_NX_REG1[],load:0x17->NXM_NX_REG2[],goto_table:80",
"table=70, priority=0 actions=drop",
"table=80, priority=300,ip,nw_src=10.129.0.1 actions=output:NXM_NX_REG2[]",
"table=80, priority=200,ct_state=+rpl,ip actions=output:NXM_NX_REG2[]",
"table=80, priority=50,reg1=0x58bb64 actions=output:NXM_NX_REG2[]",
"table=80, priority=50,reg1=0x243c14 actions=output:NXM_NX_REG2[]",
"table=80, priority=0 actions=drop",
},
policyVNIDs: sets.NewInt(0x2bd973, 0x58bb64, 0x243c14),
},
}

for i, tc := range testcases {
_, oc, _ := setupOVSController(t)

otx := oc.NewTransaction()
for _, flow := range tc.flows {
otx.AddFlow(flow)
}
if err := otx.Commit(); err != nil {
t.Fatalf("(%d) unexpected error from AddFlow: %v", i, err)
}

policyVNIDs := oc.FindPolicyVNIDs()

if !policyVNIDs.Equal(tc.policyVNIDs) {
t.Fatalf("(%d) wrong result for unused, expected %v, got %v", i, tc.policyVNIDs, policyVNIDs)
}
}

}

// Ensure that CNI's IP-addressed-based MAC addresses use the IP in the way we expect
func TestSetHWAddrByIP(t *testing.T) {
ip := net.ParseIP("1.2.3.4")
Expand Down

0 comments on commit e9fab36

Please sign in to comment.