Allow ports to be added to any container in a GS pod (#1396)

* Allow ports to be added to any container in a gs
* Adding E2E tests + feature flag
* Update docs
* Add feature info to feature stage page

Co-authored-by: Mark Mandel <markmandel@google.com>

build/Makefile

@@ -287,7 +287,7 @@ install: PING_SERVICE_TYPE := "LoadBalancer"
 install: ALLOCATOR_SERVICE_TYPE := "LoadBalancer"
 install: CRD_CLEANUP := true
 install: LOG_LEVEL := "debug"
-install: FEATURE_GATES := "PlayerTracking=true"
+install: FEATURE_GATES := "PlayerTracking=true&ContainerPortAllocation=true"
 install: $(ensure-build-image) install-custom-pull-secret
 	$(DOCKER_RUN) \
 		helm upgrade --install --wait --namespace=agones-system\

examples/gameserver.yaml

@@ -42,6 +42,8 @@ spec:
     # - "Passthrough" dynamically sets the `containerPort` to the same value as the dynamically selected hostPort.
     #      This will mean that users will need to lookup what port has been opened through the server side SDK.
     portPolicy: Static
+    # (Alpha) the name of the container to open the port on. Defaults to the game server container if omitted or empty.
+    container: simple-udp
     # the port that is being opened on the game server process
     containerPort: 7654
     # the port exposed on the host, only required when `portPolicy` is "Static". Overwritten when portPolicy is "Dynamic".

pkg/apis/agones/v1/common.go

@@ -32,6 +32,7 @@ const (
 	ErrPortPolicyStatic         = "PortPolicy must be Static"
 	ErrContainerPortRequired    = "ContainerPort must be defined for Dynamic and Static PortPolicies"
 	ErrContainerPortPassthrough = "ContainerPort cannot be specified with Passthrough PortPolicy"
+	ErrContainerNameInvalid     = "Container must be empty or the name of a container in the pod template"
 )
 
 // crd is an interface to get Name and Kind of CRD

pkg/apis/agones/v1/gameserver.go

@@ -192,7 +192,11 @@ type GameServerPort struct {
 	// When `Static` portPolicy is specified, `HostPort` is required, to specify the port that game clients will
 	// connect to
 	PortPolicy PortPolicy `json:"portPolicy,omitempty"`
-	// ContainerPort is the port that is being opened on the game server process
+	// Container is the name of the container on which to open the port. Defaults to the game server container.
+	// This field is alpha-level and is only honored by servers that enable the "ContainerPortAllocation" feature.
+	// +optional
+	Container *string `json:"container,omitempty"`
+	// ContainerPort is the port that is being opened on the specified container's process
 	ContainerPort int32 `json:"containerPort,omitempty"`
 	// HostPort the port exposed on the host for clients to connect to
 	HostPort int32 `json:"hostPort,omitempty"`
@@ -328,6 +332,10 @@ func (gss *GameServerSpec) applyPortDefaults() {
 		if p.Protocol == "" {
 			gss.Ports[i].Protocol = "UDP"
 		}
+
+		if runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) && (p.Container == nil || *p.Container == "") {
+			gss.Ports[i].Container = &gss.Container
+		}
 	}
 }
 
@@ -390,6 +398,16 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo
 			})
 		}
 
+		// make sure the container value points to a valid container
+		_, _, err := gss.FindContainer(gss.Container)
+		if err != nil {
+			causes = append(causes, metav1.StatusCause{
+				Type:    metav1.CauseTypeFieldValueInvalid,
+				Field:   "container",
+				Message: err.Error(),
+			})
+		}
+
 		// no host port when using dynamic PortPolicy
 		for _, p := range gss.Ports {
 			if p.PortPolicy == Dynamic || p.PortPolicy == Static {
@@ -417,16 +435,17 @@ func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bo
 					Message: ErrHostPortDynamic,
 				})
 			}
-		}
 
-		// make sure the container value points to a valid container
-		_, _, err := gss.FindGameServerContainer()
-		if err != nil {
-			causes = append(causes, metav1.StatusCause{
-				Type:    metav1.CauseTypeFieldValueInvalid,
-				Field:   "container",
-				Message: err.Error(),
-			})
+			if p.Container != nil && gss.Container != "" {
+				_, _, err := gss.FindContainer(*p.Container)
+				if err != nil {
+					causes = append(causes, metav1.StatusCause{
+						Type:    metav1.CauseTypeFieldValueInvalid,
+						Field:   fmt.Sprintf("%s.container", p.Name),
+						Message: ErrContainerNameInvalid,
+					})
+				}
+			}
 		}
 	}
 	objMetaCauses := validateObjectMeta(&gss.Template.ObjectMeta)
@@ -490,37 +509,44 @@ func (gs *GameServer) IsBeforeReady() bool {
 	return false
 }
 
-// FindGameServerContainer returns the container that is specified in
-// gameServer.Spec.Container. Returns the index and the value.
-// Returns an error if not found
-func (gss *GameServerSpec) FindGameServerContainer() (int, corev1.Container, error) {
+// FindContainer returns the container specified by the name parameter. Returns the index and the value.
+// Returns an error if not found.
+func (gss *GameServerSpec) FindContainer(name string) (int, corev1.Container, error) {
 	for i, c := range gss.Template.Spec.Containers {
-		if c.Name == gss.Container {
+		if c.Name == name {
 			return i, c, nil
 		}
 	}
 
-	return -1, corev1.Container{}, errors.Errorf("Could not find a container named %s", gss.Container)
+	return -1, corev1.Container{}, errors.Errorf("Could not find a container named %s", name)
 }
 
 // FindGameServerContainer returns the container that is specified in
 // gameServer.Spec.Container. Returns the index and the value.
 // Returns an error if not found
 func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) {
-	return gs.Spec.FindGameServerContainer()
+	return gs.Spec.FindContainer(gs.Spec.Container)
 }
 
-// ApplyToPodGameServerContainer applies func(v1.Container) to the pod's gameserver container
-func (gs *GameServer) ApplyToPodGameServerContainer(pod *corev1.Pod, f func(corev1.Container) corev1.Container) *corev1.Pod {
+// ApplyToPodContainer applies func(v1.Container) to the specified container in the pod.
+// Returns an error if the container is not found.
+func (gs *GameServer) ApplyToPodContainer(pod *corev1.Pod, containerName string, f func(corev1.Container) corev1.Container) error {
+	var container corev1.Container
+	containerIndex := -1
 	for i, c := range pod.Spec.Containers {
-		if c.Name == gs.Spec.Container {
-			c = f(c)
-			pod.Spec.Containers[i] = c
-			break
+		if c.Name == containerName {
+			container = c
+			containerIndex = i
 		}
 	}
+	if containerIndex == -1 {
+		return errors.Errorf("failed to find container named %q in pod spec", containerName)
+	}
+
+	container = f(container)
+	pod.Spec.Containers[containerIndex] = container
 
-	return pod
+	return nil
 }
 
 // Pod creates a new Pod from the PodTemplateSpec
@@ -532,23 +558,39 @@ func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) {
 	}
 
 	gs.podObjectMeta(pod)
+	if runtime.FeatureEnabled(runtime.FeatureContainerPortAllocation) {
+		for _, p := range gs.Spec.Ports {
+			cp := corev1.ContainerPort{
+				ContainerPort: p.ContainerPort,
+				HostPort:      p.HostPort,
+				Protocol:      p.Protocol,
+			}
+			err := gs.ApplyToPodContainer(pod, *p.Container, func(c corev1.Container) corev1.Container {
+				c.Ports = append(c.Ports, cp)
 
-	i, gsContainer, err := gs.FindGameServerContainer()
-	// this shouldn't happen, but if it does.
-	if err != nil {
-		return pod, err
-	}
+				return c
+			})
+			if err != nil {
+				return nil, err
+			}
+		}
+	} else {
+		i, gsContainer, err := gs.FindGameServerContainer()
+		// this shouldn't happen, but if it does.
+		if err != nil {
+			return pod, err
+		}
 
-	for _, p := range gs.Spec.Ports {
-		cp := corev1.ContainerPort{
-			ContainerPort: p.ContainerPort,
-			HostPort:      p.HostPort,
-			Protocol:      p.Protocol,
+		for _, p := range gs.Spec.Ports {
+			cp := corev1.ContainerPort{
+				ContainerPort: p.ContainerPort,
+				HostPort:      p.HostPort,
+				Protocol:      p.Protocol,
+			}
+			gsContainer.Ports = append(gsContainer.Ports, cp)
 		}
-		gsContainer.Ports = append(gsContainer.Ports, cp)
+		pod.Spec.Containers[i] = gsContainer
 	}
-	pod.Spec.Containers[i] = gsContainer
-
 	pod.Spec.Containers = append(pod.Spec.Containers, sidecars...)
 
 	gs.podScheduling(pod)
@@ -620,13 +662,13 @@ func (gs *GameServer) podScheduling(pod *corev1.Pod) {
 }
 
 // DisableServiceAccount disables the service account for the gameserver container
-func (gs *GameServer) DisableServiceAccount(pod *corev1.Pod) {
+func (gs *GameServer) DisableServiceAccount(pod *corev1.Pod) error {
 	// gameservers don't get access to the k8s api.
 	emptyVol := corev1.Volume{Name: "empty", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}}
 	pod.Spec.Volumes = append(pod.Spec.Volumes, emptyVol)
 	mount := corev1.VolumeMount{MountPath: "/var/run/secrets/kubernetes.io/serviceaccount", Name: emptyVol.Name, ReadOnly: true}
 
-	gs.ApplyToPodGameServerContainer(pod, func(c corev1.Container) corev1.Container {
+	return gs.ApplyToPodContainer(pod, gs.Spec.Container, func(c corev1.Container) corev1.Container {
 		c.VolumeMounts = append(c.VolumeMounts, mount)
 
 		return c

pkg/apis/agones/v1/gameserver_test.go

@@ -368,13 +368,20 @@ func TestGameServerValidate(t *testing.T) {
 	assert.True(t, ok)
 	assert.Empty(t, causes)
 
+	containerWithPort := "anotherTest"
 	gs = GameServer{
 		Spec: GameServerSpec{
 			Container: "",
 			Ports: []GameServerPort{{
 				Name:       "main",
 				HostPort:   5001,
 				PortPolicy: Dynamic,
+			}, {
+				Name:          "sidecar",
+				HostPort:      5002,
+				PortPolicy:    Static,
+				ContainerPort: 5002,
+				Container:     &containerWithPort,
 			}},
 			Template: corev1.PodTemplateSpec{
 				Spec: corev1.PodSpec{Containers: []corev1.Container{
@@ -558,6 +565,45 @@ func TestGameServerPod(t *testing.T) {
 	assert.True(t, metav1.IsControlledBy(pod, fixture))
 }
 
+func TestGameServerPodWithMultiplePortAllocations(t *testing.T) {
+	runtime.FeatureTestMutex.Lock()
+	defer runtime.FeatureTestMutex.Unlock()
+
+	err := runtime.ParseFeatures(string(runtime.FeatureContainerPortAllocation) + "=true")
+	assert.NoError(t, err)
+
+	fixture := defaultGameServer()
+	containerName := "authContainer"
+	fixture.Spec.Template.Spec.Containers = append(fixture.Spec.Template.Spec.Containers, corev1.Container{
+		Name: containerName,
+	})
+	fixture.Spec.Ports = append(fixture.Spec.Ports, GameServerPort{
+		Name:          "containerPort",
+		PortPolicy:    Dynamic,
+		Container:     &containerName,
+		ContainerPort: 5000,
+	})
+	fixture.Spec.Container = fixture.Spec.Template.Spec.Containers[0].Name
+	fixture.ApplyDefaults()
+
+	pod, err := fixture.Pod()
+	assert.NoError(t, err, "Pod should not return an error")
+	assert.Equal(t, fixture.ObjectMeta.Name, pod.ObjectMeta.Name)
+	assert.Equal(t, fixture.ObjectMeta.Namespace, pod.ObjectMeta.Namespace)
+	assert.Equal(t, "gameserver", pod.ObjectMeta.Labels[agones.GroupName+"/role"])
+	assert.Equal(t, fixture.ObjectMeta.Name, pod.ObjectMeta.Labels[GameServerPodLabel])
+	assert.Equal(t, fixture.Spec.Container, pod.ObjectMeta.Annotations[GameServerContainerAnnotation])
+	assert.Equal(t, fixture.Spec.Ports[0].HostPort, pod.Spec.Containers[0].Ports[0].HostPort)
+	assert.Equal(t, fixture.Spec.Ports[0].ContainerPort, pod.Spec.Containers[0].Ports[0].ContainerPort)
+	assert.Equal(t, *fixture.Spec.Ports[0].Container, pod.Spec.Containers[0].Name)
+	assert.Equal(t, corev1.Protocol("UDP"), pod.Spec.Containers[0].Ports[0].Protocol)
+	assert.True(t, metav1.IsControlledBy(pod, fixture))
+	assert.Equal(t, fixture.Spec.Ports[1].HostPort, pod.Spec.Containers[1].Ports[0].HostPort)
+	assert.Equal(t, fixture.Spec.Ports[1].ContainerPort, pod.Spec.Containers[1].Ports[0].ContainerPort)
+	assert.Equal(t, *fixture.Spec.Ports[1].Container, pod.Spec.Containers[1].Name)
+	assert.Equal(t, corev1.Protocol("UDP"), pod.Spec.Containers[1].Ports[0].Protocol)
+}
+
 func TestGameServerPodObjectMeta(t *testing.T) {
 	fixture := &GameServer{ObjectMeta: metav1.ObjectMeta{Name: "lucy"},
 		Spec: GameServerSpec{Container: "goat"}}
@@ -634,7 +680,8 @@ func TestGameServerDisableServiceAccount(t *testing.T) {
 	assert.Len(t, pod.Spec.Containers, 1)
 	assert.Empty(t, pod.Spec.Containers[0].VolumeMounts)
 
-	gs.DisableServiceAccount(pod)
+	err = gs.DisableServiceAccount(pod)
+	assert.NoError(t, err)
 	assert.Len(t, pod.Spec.Containers, 1)
 	assert.Len(t, pod.Spec.Containers[0].VolumeMounts, 1)
 	assert.Equal(t, "/var/run/secrets/kubernetes.io/serviceaccount", pod.Spec.Containers[0].VolumeMounts[0].MountPath)
@@ -744,7 +791,7 @@ func TestGameServerIsBeforeReady(t *testing.T) {
 
 }
 
-func TestGameServerApplyToPodGameServerContainer(t *testing.T) {
+func TestGameServerApplyToPodContainer(t *testing.T) {
 	t.Parallel()
 
 	name := "mycontainer"
@@ -762,18 +809,19 @@ func TestGameServerApplyToPodGameServerContainer(t *testing.T) {
 		},
 	}
 
-	p1 := &corev1.Pod{Spec: *gs.Spec.Template.Spec.DeepCopy()}
+	pod := &corev1.Pod{Spec: *gs.Spec.Template.Spec.DeepCopy()}
 
-	p2 := gs.ApplyToPodGameServerContainer(p1, func(c corev1.Container) corev1.Container {
+	err := gs.ApplyToPodContainer(pod, gs.Spec.Container, func(c corev1.Container) corev1.Container {
 		//  easy thing to change and test for
 		c.TTY = true
 
 		return c
 	})
 
-	assert.Len(t, p2.Spec.Containers, 2)
-	assert.True(t, p2.Spec.Containers[0].TTY)
-	assert.False(t, p2.Spec.Containers[1].TTY)
+	assert.NoError(t, err)
+	assert.Len(t, pod.Spec.Containers, 2)
+	assert.True(t, pod.Spec.Containers[0].TTY)
+	assert.False(t, pod.Spec.Containers[1].TTY)
 }
 
 func defaultGameServer() *GameServer {

pkg/gameservers/controller.go

@@ -559,10 +559,16 @@ func (c *Controller) createGameServerPod(gs *agonesv1.GameServer) (*agonesv1.Gam
 	// doing, and don't disable the gameserver container.
 	if pod.Spec.ServiceAccountName == "" {
 		pod.Spec.ServiceAccountName = c.sdkServiceAccount
-		gs.DisableServiceAccount(pod)
+		err = gs.DisableServiceAccount(pod)
+		if err != nil {
+			return gs, err
+		}
 	}
 
-	c.addGameServerHealthCheck(gs, pod)
+	err = c.addGameServerHealthCheck(gs, pod)
+	if err != nil {
+		return gs, err
+	}
 	c.addSDKServerEnvVars(gs, pod)
 
 	c.loggerForGameServer(gs).WithField("pod", pod).Debug("Creating Pod for GameServer")
@@ -658,12 +664,12 @@ func (c *Controller) sidecar(gs *agonesv1.GameServer) corev1.Container {
 }
 
 // addGameServerHealthCheck adds the http health check to the GameServer container
-func (c *Controller) addGameServerHealthCheck(gs *agonesv1.GameServer, pod *corev1.Pod) {
+func (c *Controller) addGameServerHealthCheck(gs *agonesv1.GameServer, pod *corev1.Pod) error {
 	if gs.Spec.Health.Disabled {
-		return
+		return nil
 	}
 
-	gs.ApplyToPodGameServerContainer(pod, func(c corev1.Container) corev1.Container {
+	return gs.ApplyToPodContainer(pod, gs.Spec.Container, func(c corev1.Container) corev1.Container {
 		if c.LivenessProbe == nil {
 			c.LivenessProbe = &corev1.Probe{
 				Handler: corev1.Handler{