fix: Ensure we set ports / addresses while waiting for pod IPs

pkg/gameservers/controller.go

@@ -576,9 +576,8 @@ func (c *Controller) syncGameServerCreatingState(ctx context.Context, gs *agones
 	loggerForGameServer(gs, c.baseLogger).Debug("Syncing Create State")
 
 	// Maybe something went wrong, and the pod was created, but the state was never moved to Starting, so let's check
-	_, err := c.gameServerPod(gs)
+	pod, err := c.gameServerPod(gs)
 	if k8serrors.IsNotFound(err) {
-
 		for i := range gs.Spec.Ports {
 			if gs.Spec.Ports[i].PortPolicy == agonesv1.Static && gs.Spec.Ports[i].Protocol == agonesv1.ProtocolTCPUDP {
 				name := gs.Spec.Ports[i].Name
@@ -607,6 +606,9 @@ func (c *Controller) syncGameServerCreatingState(ctx context.Context, gs *agones
 	}
 
 	gsCopy := gs.DeepCopy()
+
+	gsCopy, _ = applyGameServerPodIP(gsCopy, pod)
+
 	gsCopy.Status.State = agonesv1.GameServerStateStarting
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gsCopy, metav1.UpdateOptions{})
 	if err != nil {
@@ -906,11 +908,6 @@ func (c *Controller) syncGameServerStartingState(ctx context.Context, gs *agones
 		return gs, workerqueue.NewTraceError(errors.Errorf("node not yet populated for Pod %s", pod.ObjectMeta.Name))
 	}
 
-	// Ensure the pod IPs are populated
-	if len(pod.Status.PodIPs) == 0 {
-		return gs, workerqueue.NewTraceError(errors.Errorf("pod IPs not yet populated for Pod %s", pod.ObjectMeta.Name))
-	}
-
 	node, err := c.nodeLister.Get(pod.Spec.NodeName)
 	if err != nil {
 		return gs, errors.Wrapf(err, "error retrieving node %s for Pod %s", pod.Spec.NodeName, pod.ObjectMeta.Name)
@@ -921,6 +918,8 @@ func (c *Controller) syncGameServerStartingState(ctx context.Context, gs *agones
 		return gs, err
 	}
 
+	gsCopy, _ = applyGameServerPodIP(gsCopy, pod)
+
 	gsCopy.Status.State = agonesv1.GameServerStateScheduled
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gsCopy, metav1.UpdateOptions{})
 	if err != nil {
@@ -954,12 +953,28 @@ func (c *Controller) syncGameServerRequestReadyState(ctx context.Context, gs *ag
 		return nil, err
 	}
 
+	var gsUpdated bool
+
+	gsCopy, podIPUpdated := applyGameServerPodIP(gsCopy, pod)
+	if podIPUpdated {
+		// defer the update of the GameServer until the end of this function
+		// in case there is no gs state update and the podIP is available and populated
+		defer func(gs *agonesv1.GameServer, alreadyUpdated *bool) {
+			// Only update if the GameServer is not already updated
+			if alreadyUpdated == nil || *alreadyUpdated {
+				return
+			}
+			_, _ = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gs, metav1.UpdateOptions{})
+		}(gsCopy, &gsUpdated)
+	}
+
 	// if the address hasn't been populated, and the Ready request comes
 	// before the controller has had a chance to do it, then
 	// do it here instead
 	addressPopulated := false
-	if gs.Status.NodeName == "" {
+	if gsCopy.Status.NodeName == "" {
 		addressPopulated = true
+
 		if pod.Spec.NodeName == "" {
 			return gs, workerqueue.NewTraceError(errors.Errorf("node not yet populated for Pod %s", pod.ObjectMeta.Name))
 		}
@@ -978,6 +993,9 @@ func (c *Controller) syncGameServerRequestReadyState(ctx context.Context, gs *ag
 		return gs, err
 	}
 
+	// if the address is already populated, then we don't need to re-update for podIPs
+	gsUpdated = true
+
 	gsCopy.Status.State = agonesv1.GameServerStateReady
 	gs, err = c.gameServerGetter.GameServers(gs.ObjectMeta.Namespace).Update(ctx, gsCopy, metav1.UpdateOptions{})
 	if err != nil {

pkg/gameservers/controller_test.go

@@ -1189,33 +1189,6 @@ func TestControllerSyncGameServerStartingState(t *testing.T) {
 		assert.NotEmpty(t, gs.Status.Ports)
 	})
 
-	t.Run("Error on podIPs not populated", func(t *testing.T) {
-		c, m := newFakeController()
-		gsFixture := newFixture()
-		gsFixture.ApplyDefaults()
-		pod, err := gsFixture.Pod(agtesting.FakeAPIHooks{})
-		require.NoError(t, err)
-		pod.Spec.NodeName = nodeFixtureName
-
-		m.KubeClient.AddReactor("list", "nodes", func(_ k8stesting.Action) (bool, runtime.Object, error) {
-			return true, &corev1.NodeList{Items: []corev1.Node{node}}, nil
-		})
-		m.KubeClient.AddReactor("list", "pods", func(_ k8stesting.Action) (bool, runtime.Object, error) {
-			return true, &corev1.PodList{Items: []corev1.Pod{*pod}}, nil
-		})
-		m.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) {
-			ua := action.(k8stesting.UpdateAction)
-			gs := ua.GetObject().(*agonesv1.GameServer)
-			assert.Equal(t, agonesv1.GameServerStateScheduled, gs.Status.State)
-			return true, gs, errors.New("update-err")
-		})
-		ctx, cancel := agtesting.StartInformers(m, c.gameServerSynced, c.podSynced, c.nodeSynced)
-		defer cancel()
-
-		_, err = c.syncGameServerStartingState(ctx, gsFixture)
-		assert.Error(t, err)
-	})
-
 	t.Run("Error on update", func(t *testing.T) {
 		c, m := newFakeController()
 		gsFixture := newFixture()
@@ -1455,6 +1428,9 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 			Spec: newSingleContainerSpec(), Status: agonesv1.GameServerStatus{State: agonesv1.GameServerStateRequestReady}}
 		gsFixture.ApplyDefaults()
 		gsFixture.Status.NodeName = nodeName
+		gsFixture.Status.Addresses = []corev1.NodeAddress{
+			{Type: agonesv1.NodePodIP, Address: "0.0.0.0"},
+		}
 		pod, err := gsFixture.Pod(agtesting.FakeAPIHooks{})
 		require.NoError(t, err)
 		pod.Status.ContainerStatuses = []corev1.ContainerStatus{
@@ -1515,6 +1491,9 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 			Spec: newSingleContainerSpec(), Status: agonesv1.GameServerStatus{State: agonesv1.GameServerStateRequestReady}}
 		gsFixture.ApplyDefaults()
 		gsFixture.Status.NodeName = nodeName
+		gsFixture.Status.Addresses = []corev1.NodeAddress{
+			{Type: agonesv1.NodePodIP, Address: "0.0.0.0"},
+		}
 		pod, err := gsFixture.Pod(agtesting.FakeAPIHooks{})
 		require.NoError(t, err)
 		pod.Status.ContainerStatuses = []corev1.ContainerStatus{
@@ -1559,6 +1538,9 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 			Spec: newSingleContainerSpec(), Status: agonesv1.GameServerStatus{State: agonesv1.GameServerStateRequestReady}}
 		gsFixture.ApplyDefaults()
 		gsFixture.Status.NodeName = nodeName
+		gsFixture.Status.Addresses = []corev1.NodeAddress{
+			{Type: agonesv1.NodePodIP, Address: "0.0.0.0"},
+		}
 		pod, err := gsFixture.Pod(agtesting.FakeAPIHooks{})
 		require.NoError(t, err)
 		pod.Status.ContainerStatuses = []corev1.ContainerStatus{
@@ -1602,6 +1584,9 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 			Spec: newSingleContainerSpec(), Status: agonesv1.GameServerStatus{State: agonesv1.GameServerStateRequestReady}}
 		gsFixture.ApplyDefaults()
 		gsFixture.Status.NodeName = nodeName
+		gsFixture.Status.Addresses = []corev1.NodeAddress{
+			{Type: agonesv1.NodePodIP, Address: "0.0.0.0"},
+		}
 		pod, err := gsFixture.Pod(agtesting.FakeAPIHooks{})
 		require.NoError(t, err)
 		pod.ObjectMeta.Annotations = map[string]string{agonesv1.GameServerReadyContainerIDAnnotation: containerID}
@@ -1659,6 +1644,7 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 				ContainerID: containerID,
 			},
 		}
+		pod.Status.PodIPs = []corev1.PodIP{{IP: ipv6Fixture}}
 		gsUpdated := false
 		podUpdated := false
 
@@ -1699,7 +1685,10 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) {
 
 		assert.Equal(t, gs.Status.NodeName, nodeFixture.ObjectMeta.Name)
 		assert.Equal(t, gs.Status.Address, ipFixture)
-		assert.Equal(t, []corev1.NodeAddress{{Address: ipFixture, Type: "ExternalIP"}}, gs.Status.Addresses)
+		assert.Equal(t, []corev1.NodeAddress{
+			{Address: ipFixture, Type: "ExternalIP"},
+			{Address: ipv6Fixture, Type: "PodIP"},
+		}, gs.Status.Addresses)
 
 		agtesting.AssertEventContains(t, m.FakeRecorder.Events, "Address and port populated")
 		agtesting.AssertEventContains(t, m.FakeRecorder.Events, "SDK.Ready() complete")

pkg/gameservers/gameservers.go

@@ -93,12 +93,7 @@ func applyGameServerAddressAndPort(gs *agonesv1.GameServer, node *corev1.Node, p
 	gs.Status.Addresses = addrs
 	gs.Status.NodeName = pod.Spec.NodeName
 
-	for _, ip := range pod.Status.PodIPs {
-		gs.Status.Addresses = append(gs.Status.Addresses, corev1.NodeAddress{
-			Type:    agonesv1.NodePodIP,
-			Address: ip.IP,
-		})
-	}
+	gs, _ = applyGameServerPodIP(gs, pod)
 
 	if err := syncPodPortsToGameServer(gs, pod); err != nil {
 		return gs, errors.Wrapf(err, "cloud product error syncing ports on GameServer %s", gs.ObjectMeta.Name)
@@ -114,6 +109,34 @@ func applyGameServerAddressAndPort(gs *agonesv1.GameServer, node *corev1.Node, p
 	return gs, nil
 }
 
+// applyGameServerPodIP checks the pod IPs and adds them to the GameServer status if they are not already there.
+func applyGameServerPodIP(gs *agonesv1.GameServer, pod *corev1.Pod) (*agonesv1.GameServer, bool) {
+	var updated bool
+
+	if pod == nil {
+		return gs, false
+	}
+
+	gsPodIPs := make(map[string]interface{})
+	for _, addr := range gs.Status.Addresses {
+		if addr.Type == agonesv1.NodePodIP {
+			gsPodIPs[addr.Address] = nil
+		}
+	}
+
+	for _, ip := range pod.Status.PodIPs {
+		if _, ok := gsPodIPs[ip.IP]; !ok {
+			gs.Status.Addresses = append(gs.Status.Addresses, corev1.NodeAddress{
+				Type:    agonesv1.NodePodIP,
+				Address: ip.IP,
+			})
+			updated = true
+		}
+	}
+
+	return gs, updated
+}
+
 // isBeforePodCreated checks to see if the GameServer is in a state in which the pod could not have been
 // created yet. This includes "Starting" in which a pod MAY exist, but may not yet be available, depending on when the
 // informer cache updates