Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the pod reference in v1beta1 AddressGroup #1586

Merged
merged 1 commit into from
Nov 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 48 additions & 4 deletions .github/workflows/kind_upgrade.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,18 @@ jobs:
sudo mv kind /usr/local/bin
- name: Run test
run: |
./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1
mkdir log
ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1
- name: Tar log files
if: ${{ failure() }}
run: tar -czf log.tar.gz log
- name: Upload test log
uses: actions/upload-artifact@v2
if: ${{ failure() }}
with:
name: upgrade-from-antrea-version-n-1.tar.gz
path: log.tar.gz
retention-days: 30

from-N-2:
name: Upgrade from Antrea version N-2
Expand Down Expand Up @@ -102,7 +113,18 @@ jobs:
sudo mv kind /usr/local/bin
- name: Run test
run: |
./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2
mkdir log
ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2
- name: Tar log files
if: ${{ failure() }}
run: tar -czf log.tar.gz log
- name: Upload test log
uses: actions/upload-artifact@v2
if: ${{ failure() }}
with:
name: upgrade-from-antrea-version-n-2.tar.gz
path: log.tar.gz
retention-days: 30

compatible-N-1:
name: API compatible with client version N-1
Expand Down Expand Up @@ -131,7 +153,18 @@ jobs:
sudo mv kind /usr/local/bin
- name: Run test
run: |
./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1 --controller-only
mkdir log
ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 1 --controller-only
- name: Tar log files
if: ${{ failure() }}
run: tar -czf log.tar.gz log
- name: Upload test log
uses: actions/upload-artifact@v2
if: ${{ failure() }}
with:
name: api-compatible-with-client-version-n-1.tar.gz
path: log.tar.gz
retention-days: 30

compatible-N-2:
name: API compatible with client version N-2
Expand Down Expand Up @@ -160,7 +193,18 @@ jobs:
sudo mv kind /usr/local/bin
- name: Run test
run: |
./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2 --controller-only
mkdir log
ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-upgrade-antrea.sh --from-version-n-minus 2 --controller-only
- name: Tar log files
if: ${{ failure() }}
run: tar -czf log.tar.gz log
- name: Upload test log
uses: actions/upload-artifact@v2
if: ${{ failure() }}
with:
name: api-compatible-with-client-version-n-2.tar.gz
path: log.tar.gz
retention-days: 30

# Runs after all other jobs in the workflow and deletes Antrea Docker images uploaded as temporary
# artifacts. It uses a third-party, MIT-licensed action (geekyeggo/delete-artifact). While Github
Expand Down
2 changes: 1 addition & 1 deletion ci/kind/test-upgrade-antrea.sh
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ popd
rm -rf $TMP_DIR

rc=0
go test -v -run=TestUpgrade github.com/vmware-tanzu/antrea/test/e2e -provider=kind -upgrade.toYML=antrea-new.yml --upgrade.controllerOnly=$CONTROLLER_ONLY || rc=$?
go test -v -run=TestUpgrade github.com/vmware-tanzu/antrea/test/e2e -provider=kind -upgrade.toYML=antrea-new.yml --upgrade.controllerOnly=$CONTROLLER_ONLY --logs-export-dir=$ANTREA_LOG_DIR || rc=$?

$THIS_DIR/kind-setup.sh destroy kind

Expand Down
35 changes: 22 additions & 13 deletions pkg/apis/controlplane/v1beta1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,11 @@ func Convert_controlplane_GroupMember_To_v1beta1_GroupMember(in *controlplane.Gr
}

func Convert_v1beta1_GroupMemberPod_To_controlplane_GroupMember(in *GroupMemberPod, out *controlplane.GroupMember, s conversion.Scope) error {
out.Pod = &controlplane.PodReference{
Name: in.Pod.Name,
Namespace: in.Pod.Namespace,
if in.Pod != nil {
out.Pod = &controlplane.PodReference{
Name: in.Pod.Name,
Namespace: in.Pod.Namespace,
}
}
out.IPs = []controlplane.IPAddress{controlplane.IPAddress(in.IP)}
ports := make([]controlplane.NamedPort, len(in.Ports))
Expand All @@ -124,13 +126,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.
Comment on lines +132 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, is there any advantage to having conversion functions for non resources registered to the schema? I understand why Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup needs to be registered, but it seems conversion functions for non resources will only be called indirectly by other conversion functions. Is it for auto-generated conversion functions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question, I wondered this as well, haven't found answer so blindly followed what K8s code does.
I can take a look what it would affect later and see if we could remove them. Created #1616 to track it.

func Convert_controlplane_GroupMember_To_v1beta1_GroupMemberPod(in *controlplane.GroupMember, out *GroupMemberPod, includePodRef bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could have been a private function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely can be private, I kept its name as it was as code in this file doesn't follow normal go style anyway.
Do you prefer it to be private?

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 +181,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 +236,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 +250,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 +294,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 +350,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 +364,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
19 changes: 15 additions & 4 deletions pkg/apis/controlplane/v1beta1/conversion_test.go
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 All @@ -169,9 +175,11 @@ func TestConvertBetweenV1beta1AndControlplaneAddressGroup(t *testing.T) {
}
var convertedCPAddressGroup controlplane.AddressGroup
var convertedV1B1AddressGroup AddressGroup
expectedV1B1AddressGroup := v1b1AddressGroup.DeepCopy()
expectedV1B1AddressGroup.Pods[0].Pod = nil
require.NoError(t,
Convert_controlplane_AddressGroup_To_v1beta1_AddressGroup(&cpAddressGroup, &convertedV1B1AddressGroup, nil))
assert.Equal(t, v1b1AddressGroup, convertedV1B1AddressGroup)
assert.Equal(t, *expectedV1B1AddressGroup, convertedV1B1AddressGroup)
require.NoError(t,
Convert_v1beta1_AddressGroup_To_controlplane_AddressGroup(&v1b1AddressGroup, &convertedCPAddressGroup, nil))
assert.Equal(t, cpAddressGroup, convertedCPAddressGroup)
Expand All @@ -193,9 +201,12 @@ func TestConvertBetweenV1beta1AndControlplaneAddressGroupPatch(t *testing.T) {
}
var convertedCPPatch controlplane.AddressGroupPatch
var convertedV1B1Patch AddressGroupPatch
expectedV1B1AddressGroupPatch := v1b1AddressGroupPatch.DeepCopy()
expectedV1B1AddressGroupPatch.AddedPods[0].Pod = nil
expectedV1B1AddressGroupPatch.RemovedPods[0].Pod = nil
require.NoError(t,
Convert_controlplane_AddressGroupPatch_To_v1beta1_AddressGroupPatch(&cpAddressGroupPatch, &convertedV1B1Patch, nil))
assert.Equal(t, v1b1AddressGroupPatch, convertedV1B1Patch)
assert.Equal(t, *expectedV1B1AddressGroupPatch, convertedV1B1Patch)
require.NoError(t,
Convert_v1beta1_AddressGroupPatch_To_controlplane_AddressGroupPatch(&v1b1AddressGroupPatch, &convertedCPPatch, nil))
assert.Equal(t, cpAddressGroupPatch, convertedCPPatch)
Expand Down
5 changes: 0 additions & 5 deletions pkg/apis/controlplane/v1beta1/zz_generated.conversion.go

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

66 changes: 46 additions & 20 deletions test/e2e/networkpolicy_test.go
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,16 @@ 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)
}
}()
})
time.Sleep(networkPolicyDelay)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's changing the podSelector to "MatchExpressions" caused the networkpolicy takes a little more time to realize. The test now has a chance to fail without the sleep, I can confirm it's not because of the conversion function change as it also failed when I just changed the test code in this PR: https://github.com/vmware-tanzu/antrea/runs/1431829031?check_suite_focus=true


npCheck := func(server0IP, server1IP string) {
// client0 can connect to both servers.
Expand All @@ -244,15 +267,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
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