Skip to content

Commit

Permalink
Remove the pod reference in v1beta1 AddressGroup
Browse files Browse the repository at this point in the history 
To be backwards compatible, AddressGroup v1beta1 must not include
PodReferences in its members, otherwise the agents that use v1beta1 will
think the members have changed and will add new IPs to the openflow
entries of relevant NetworkPolicies and delete old IPs from them, while
the old IPs and new IPs are actually the same. In current
implementation, deleting old IPs is executed after adding new IPs, so it
leads to some IPs missing in the openflow entries.
  • Loading branch information
@tnqn
tnqn committed Nov 20, 2020
1 parent c59ed4e commit 87e0d2d
Show file tree
Hide file tree
Showing 5 changed files with 83 additions and 39 deletions.
27 changes: 17 additions & 10 deletions pkg/apis/controlplane/v1beta1/conversion.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,20 @@ func Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(in *GroupMemberP
return nil
}

func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, s conversion.Scope) error {
// Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod converts controlplane GroupMember to v1beta1 GroupMember
// based on whether it's required to include Pod reference in the result. We must not include Pod reference when the
// conversion is called for an AddressGroup as agents don't expect it in v1beta1 version.
// This function doesn't match the pattern of conversion function which requires the last parameter to be
// conversion.Scope so won't be registered to schema.
func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, includePodRef bool) error {
if in.Pod == nil || len(in.IPs) > 1 {
return fmt.Errorf("cannot convert ExternalEntity or dual stack Pod into GroupMemberPod")
}
out.Pod = &PodReference{
Name: in.Pod.Name,
Namespace: in.Pod.Namespace,
if includePodRef {
out.Pod = &PodReference{
Name: in.Pod.Name,
Namespace: in.Pod.Namespace,
}
}
if len(in.IPs) > 0 {
out.IP = IPAddress(in.IPs[0])
Expand Down Expand Up @@ -172,7 +179,7 @@ func Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup(in *controlplane.
m := in.GroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
return err
}
pods = append(pods, pod)
Expand Down Expand Up @@ -227,7 +234,7 @@ func Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(in *con
m := in.AddedGroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
return err
}
addedPods = append(addedPods, pod)
Expand All @@ -241,7 +248,7 @@ func Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(in *con
m := in.RemovedGroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, false); err != nil {
return err
}
removedPods = append(removedPods, pod)
Expand Down Expand Up @@ -285,7 +292,7 @@ func Convert_controlplane_AppliedToGroup_To_v1beta1_AppliedToGroup(in *controlpl
m := in.GroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
return err
}
pods = append(pods, pod)
Expand Down Expand Up @@ -341,7 +348,7 @@ func Convert_controlplane_AppliedToGroupPatch_To_v1beta1_AppliedToGroupPatch(in
m := in.AddedGroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
return err
}
addedPods = append(addedPods, pod)
Expand All @@ -355,7 +362,7 @@ func Convert_controlplane_AppliedToGroupPatch_To_v1beta1_AppliedToGroupPatch(in
m := in.RemovedGroupMembers[i]
if m.Pod != nil {
var pod GroupMemberPod
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, nil); err != nil {
if err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&m, &pod, true); err != nil {
return err
}
removedPods = append(removedPods, pod)
Expand Down
10 changes: 8 additions & 2 deletions pkg/apis/controlplane/v1beta1/conversion_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,11 +146,17 @@ func TestConvertBetweenV1beta1GroupMemberPodAndControlplaneGroupMember(t *testin

var convertedCPGroupMember controlplane.GroupMember
var convertedV1B1GroupMemberPod GroupMemberPod
err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpGroupMember, &convertedV1B1GroupMemberPod, nil)
err := Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpGroupMember, &convertedV1B1GroupMemberPod, true)
require.Errorf(t, err, "should not be able to convert group member with multiple IPs to GroupMemberPod")
require.NoError(t,
Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPod, nil))
Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPod, true))
assert.Equal(t, v1b1GroupMemberPod, convertedV1B1GroupMemberPod, "controlplane.GroupMember -> v1beta1.GroupMemberPod")
var convertedV1B1GroupMemberPodWithoutPodRef GroupMemberPod
require.NoError(t,
Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(&cpPodGroupMember, &convertedV1B1GroupMemberPodWithoutPodRef, false))
expectedV1b1GroupMemberPodWithoutPodRef := *v1b1GroupMemberPod.DeepCopy()
expectedV1b1GroupMemberPodWithoutPodRef.Pod = nil
assert.Equal(t, expectedV1b1GroupMemberPodWithoutPodRef, convertedV1B1GroupMemberPodWithoutPodRef, "controlplane.GroupMember -> v1beta1.GroupMemberPod")
require.NoError(t,
Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(&v1b1GroupMemberPod, &convertedCPGroupMember, nil))
assert.Equal(t, cpPodGroupMember, convertedCPGroupMember, "v1beta1.GroupMemberPod -> controlplane.GroupMember")
Expand Down
5 changes: 0 additions & 5 deletions pkg/apis/controlplane/v1beta1/zz_generated.conversion.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

65 changes: 45 additions & 20 deletions test/e2e/networkpolicy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,27 +157,43 @@ func TestDifferentNamedPorts(t *testing.T) {
t.Fatalf("Error when setting up test: %v", err)
}
defer teardownTest(t, data)
data.testDifferentNamedPorts(t)
checkFn, cleanupFn := data.setupDifferentNamedPorts(t)
defer cleanupFn()
checkFn()
}

func (data *TestData) testDifferentNamedPorts(t *testing.T) {
func (data *TestData) setupDifferentNamedPorts(t *testing.T) (checkFn func(), cleanupFn func()) {
var success bool
var cleanupFuncs []func()
cleanupFn = func() {
for i := len(cleanupFuncs) - 1; i >= 0; i-- {
cleanupFuncs[i]()
}
}
// Call cleanupFn only if the function fails. In case of success, we will call cleanupFn in callers.
defer func() {
if !success {
cleanupFn()
}
}()

server0Port := 80
_, server0IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
server0Name, server0IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
return data.createServerPod(name, "http", server0Port, false)
}, "test-server-", "")
defer cleanupFunc()
cleanupFuncs = append(cleanupFuncs, cleanupFunc)

server1Port := 8080
_, server1IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
server1Name, server1IPs, cleanupFunc := createAndWaitForPod(t, data, func(name string, nodeName string) error {
return data.createServerPod(name, "http", server1Port, false)
}, "test-server-", "")
defer cleanupFunc()
cleanupFuncs = append(cleanupFuncs, cleanupFunc)

client0Name, _, cleanupFunc := createAndWaitForPod(t, data, data.createBusyboxPodOnNode, "test-client-", "")
defer cleanupFunc()
cleanupFuncs = append(cleanupFuncs, cleanupFunc)

client1Name, _, cleanupFunc := createAndWaitForPod(t, data, data.createBusyboxPodOnNode, "test-client-", "")
defer cleanupFunc()
cleanupFuncs = append(cleanupFuncs, cleanupFunc)

preCheckFunc := func(server0IP, server1IP string) {
// Both clients can connect to both servers.
Expand All @@ -201,8 +217,14 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {

// Create NetworkPolicy rule.
spec := &networkingv1.NetworkPolicySpec{
// Apply to all Pods.
PodSelector: metav1.LabelSelector{},
// Apply to two server Pods.
PodSelector: metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{
{
Key: "antrea-e2e",
Operator: metav1.LabelSelectorOpIn,
Values: []string{server0Name, server1Name},
},
}},
// Allow client0 to access named port: "http".
Ingress: []networkingv1.NetworkPolicyIngressRule{{
Ports: []networkingv1.NetworkPolicyPort{{
Expand All @@ -217,15 +239,15 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {
},
}},
}
np, err := data.createNetworkPolicy("test-networkpolicy-allow-client0-to-http", spec)
np, err := data.createNetworkPolicy(randName("test-networkpolicy-allow-client0-to-http"), spec)
if err != nil {
t.Fatalf("Error when creating network policy: %v", err)
}
defer func() {
cleanupFuncs = append(cleanupFuncs, func() {
if err = data.deleteNetworkpolicy(np); err != nil {
t.Fatalf("Error when deleting network policy: %v", err)
}
}()
})

npCheck := func(server0IP, server1IP string) {
// client0 can connect to both servers.
Expand All @@ -244,15 +266,18 @@ func (data *TestData) testDifferentNamedPorts(t *testing.T) {
}
}

// NetworkPolicy check.
if clusterInfo.podV4NetworkCIDR != "" {
npCheck(server0IPs.ipv4.String(), server1IPs.ipv4.String())
}
checkFn = func() {
// NetworkPolicy check.
if clusterInfo.podV4NetworkCIDR != "" {
npCheck(server0IPs.ipv4.String(), server1IPs.ipv4.String())
}

if clusterInfo.podV6NetworkCIDR != "" {
npCheck(server0IPs.ipv6.String(), server1IPs.ipv6.String())
if clusterInfo.podV6NetworkCIDR != "" {
npCheck(server0IPs.ipv6.String(), server1IPs.ipv6.String())
}
}

success = true
return
}

func TestDefaultDenyEgressPolicy(t *testing.T) {
Expand Down
15 changes: 13 additions & 2 deletions test/e2e/upgrade_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,12 @@ func TestUpgrade(t *testing.T) {

data.testPodConnectivitySameNode(t)
data.testPodConnectivityDifferentNodes(t)
data.testDifferentNamedPorts(t)
// We test NetworkPolicy with 2 scenarios:
// 1. The NetworkPolicy is created before upgrading controller.
// 2. The NetworkPolicy is created after upgrading controller.
checkFn, cleanupFn := data.setupDifferentNamedPorts(t)
defer cleanupFn()
checkFn()

if t.Failed() {
t.FailNow()
Expand All @@ -97,7 +102,13 @@ func TestUpgrade(t *testing.T) {

data.testPodConnectivitySameNode(t)
data.testPodConnectivityDifferentNodes(t)
data.testDifferentNamedPorts(t)
// Verify that the NetworkPolicy created before upgrading still works.
checkFn()
// Verify that the NetworkPolicy created after upgrading works.
// random resource names are used in the test so it's OK to call setupDifferentNamedPorts the second time.
checkFn, cleanupFn = data.setupDifferentNamedPorts(t)
defer cleanupFn()
checkFn()

t.Logf("Deleting namespace '%s'", namespace)
if err := data.deleteNamespace(namespace, defaultTimeout); err != nil {
Expand Down

0 comments on commit 87e0d2d

Please sign in to comment.