From b0a6ef8e59e07a7f4047d64ac8a2de3f99986dad Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Wed, 29 Jul 2020 21:40:15 +0300 Subject: [PATCH 1/3] Fix findOpenPorts portAllocator function There could be the possible case when we set PortPolicy as an empty string on update of GameServer. --- pkg/gameservers/portallocator.go | 3 +++ pkg/gameservers/portallocator_test.go | 31 +++++++++++++++++++++++++++ 2 files changed, 34 insertions(+) diff --git a/pkg/gameservers/portallocator.go b/pkg/gameservers/portallocator.go index b072e99079..9f619ee4aa 100644 --- a/pkg/gameservers/portallocator.go +++ b/pkg/gameservers/portallocator.go @@ -120,6 +120,9 @@ func (pa *PortAllocator) Allocate(gs *agonesv1.GameServer) *agonesv1.GameServer // Also the return gives an escape from the double loop findOpenPorts := func(amount int) []pn { var ports []pn + if amount <= 0 { + return ports + } for _, n := range pa.portAllocations { for p, taken := range n { if !taken { diff --git a/pkg/gameservers/portallocator_test.go b/pkg/gameservers/portallocator_test.go index ab0a2ba861..ed9ea43ab9 100644 --- a/pkg/gameservers/portallocator_test.go +++ b/pkg/gameservers/portallocator_test.go @@ -40,6 +40,37 @@ var ( n3 = corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3", UID: "node3"}} ) +func TestEmptyPortPolicy(t *testing.T) { + t.Parallel() + fixture := dynamicGameServerFixture() + + t.Run("test portPolicy as an empty string", func(t *testing.T) { + m := agtesting.NewMocks() + pa := NewPortAllocator(10, 50, m.KubeInformerFactory, m.AgonesInformerFactory) + nodeWatch := watch.NewFake() + m.KubeClient.AddWatchReactor("nodes", k8stesting.DefaultWatchReactor(nodeWatch, nil)) + + stop, cancel := agtesting.StartInformers(m, pa.nodeSynced) + defer cancel() + + // Make sure the add's don't corrupt the sync + // (no longer an issue, but leave this here for posterity) + nodeWatch.Add(&n1) + nodeWatch.Add(&n2) + assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced)) + + err := pa.syncAll() + assert.Nil(t, err) + + // single port empty + fd := fixture.DeepCopy() + fd.Spec.Ports[0].PortPolicy = "" + pa.Allocate(fd) + assert.Nil(t, err) + assert.Equal(t, 0, countTotalAllocatedPorts(pa)) + }) +} + func TestPortAllocatorAllocate(t *testing.T) { t.Parallel() fixture := dynamicGameServerFixture() From 5294800c8c4b182509ab1ef78da9d3e69a1acf41 Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Wed, 5 Aug 2020 00:00:53 +0300 Subject: [PATCH 2/3] Applying comments from PR --- pkg/gameservers/portallocator_test.go | 57 ++++++++++++--------------- 1 file changed, 26 insertions(+), 31 deletions(-) diff --git a/pkg/gameservers/portallocator_test.go b/pkg/gameservers/portallocator_test.go index ed9ea43ab9..1a8c810ccf 100644 --- a/pkg/gameservers/portallocator_test.go +++ b/pkg/gameservers/portallocator_test.go @@ -40,37 +40,6 @@ var ( n3 = corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node3", UID: "node3"}} ) -func TestEmptyPortPolicy(t *testing.T) { - t.Parallel() - fixture := dynamicGameServerFixture() - - t.Run("test portPolicy as an empty string", func(t *testing.T) { - m := agtesting.NewMocks() - pa := NewPortAllocator(10, 50, m.KubeInformerFactory, m.AgonesInformerFactory) - nodeWatch := watch.NewFake() - m.KubeClient.AddWatchReactor("nodes", k8stesting.DefaultWatchReactor(nodeWatch, nil)) - - stop, cancel := agtesting.StartInformers(m, pa.nodeSynced) - defer cancel() - - // Make sure the add's don't corrupt the sync - // (no longer an issue, but leave this here for posterity) - nodeWatch.Add(&n1) - nodeWatch.Add(&n2) - assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced)) - - err := pa.syncAll() - assert.Nil(t, err) - - // single port empty - fd := fixture.DeepCopy() - fd.Spec.Ports[0].PortPolicy = "" - pa.Allocate(fd) - assert.Nil(t, err) - assert.Equal(t, 0, countTotalAllocatedPorts(pa)) - }) -} - func TestPortAllocatorAllocate(t *testing.T) { t.Parallel() fixture := dynamicGameServerFixture() @@ -267,6 +236,32 @@ func TestPortAllocatorAllocate(t *testing.T) { ports = append(ports, gs.Spec.Ports[0].HostPort) } }) + + t.Run("portPolicy as an empty string", func(t *testing.T) { + m := agtesting.NewMocks() + pa := NewPortAllocator(10, 50, m.KubeInformerFactory, m.AgonesInformerFactory) + nodeWatch := watch.NewFake() + m.KubeClient.AddWatchReactor("nodes", k8stesting.DefaultWatchReactor(nodeWatch, nil)) + + stop, cancel := agtesting.StartInformers(m, pa.nodeSynced) + defer cancel() + + // Make sure the add's don't corrupt the sync + // (no longer an issue, but leave this here for posterity) + nodeWatch.Add(&n1) + nodeWatch.Add(&n2) + assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced)) + + err := pa.syncAll() + require.Nil(t, err) + + // single port empty + fd := fixture.DeepCopy() + fd.Spec.Ports[0].PortPolicy = "" + gs := pa.Allocate(fd) + assert.NotNil(t, gs) + assert.Equal(t, 0, countTotalAllocatedPorts(pa)) + }) } func TestPortAllocatorMultithreadAllocate(t *testing.T) { From fc49377e7b5037abf07319e40e0382f8bcc584b1 Mon Sep 17 00:00:00 2001 From: Alexander Apalikov Date: Thu, 6 Aug 2020 11:00:22 +0300 Subject: [PATCH 3/3] Fix Nil - NoError --- pkg/gameservers/portallocator_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/gameservers/portallocator_test.go b/pkg/gameservers/portallocator_test.go index 1a8c810ccf..1ea3cec8ab 100644 --- a/pkg/gameservers/portallocator_test.go +++ b/pkg/gameservers/portallocator_test.go @@ -253,7 +253,7 @@ func TestPortAllocatorAllocate(t *testing.T) { assert.True(t, cache.WaitForCacheSync(stop, pa.nodeSynced)) err := pa.syncAll() - require.Nil(t, err) + require.NoError(t, err) // single port empty fd := fixture.DeepCopy()