Skip to content

Commit

Permalink
addressed comments and updated unit tests for gke test
Browse files Browse the repository at this point in the history
  • Loading branch information
@vicentefb
vicentefb committed May 25, 2024
1 parent 346c89a commit 2d25b41
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 18 deletions.
21 changes: 19 additions & 2 deletions pkg/cloudproduct/gke/gke.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,10 +134,18 @@ func (*gkeAutopilot) NewPortAllocator(portRanges map[string]portallocator.PortRa

func (*gkeAutopilot) WaitOnFreePorts() bool { return true }

func checkPassthroughPortPolicy(PortPolicy agonesv1.PortPolicy) bool {
// if feature is not enabled and port is Passthrough return true because that should be an invalid port
// if feature is not enabled and port is not Passthrough you can return false because there's no error but check for None port
// if feature is enabled and port is passthrough return false because there is no error
// if feature is enabled and port is not passthrough return false because there is no error but check for None port
return (!runtime.FeatureEnabled(runtime.FeatureAutopilotPassthroughPort) && PortPolicy == agonesv1.Passthrough) || PortPolicy == agonesv1.Static
}

func (g *gkeAutopilot) ValidateGameServerSpec(gss *agonesv1.GameServerSpec, fldPath *field.Path) field.ErrorList {
allErrs := g.ValidateScheduling(gss.Scheduling, fldPath.Child("scheduling"))
for i, p := range gss.Ports {
if p.PortPolicy != agonesv1.Dynamic && (p.PortPolicy != agonesv1.None || !runtime.FeatureEnabled(runtime.FeaturePortPolicyNone)) && !runtime.FeatureEnabled(runtime.FeatureAutopilotPassthroughPort) {
if p.PortPolicy != agonesv1.Dynamic && (p.PortPolicy != agonesv1.None || !runtime.FeatureEnabled(runtime.FeaturePortPolicyNone)) && checkPassthroughPortPolicy(p.PortPolicy) {
allErrs = append(allErrs, field.Invalid(fldPath.Child("ports").Index(i).Child("portPolicy"), string(p.PortPolicy), errPortPolicyMustBeDynamicOrNone))
}
if p.Range != agonesv1.DefaultPortRange && (p.PortPolicy != agonesv1.None || !runtime.FeatureEnabled(runtime.FeaturePortPolicyNone)) {
Expand Down Expand Up @@ -256,14 +264,23 @@ type autopilotPortAllocator struct {
func (*autopilotPortAllocator) Run(_ context.Context) error { return nil }
func (*autopilotPortAllocator) DeAllocate(gs *agonesv1.GameServer) {}

func checkPassthroughPortPolicyForAutopilot(PortPolicy agonesv1.PortPolicy) bool {
// Autopilot can have Dynamic or Passthrough
// if feature is not enabled and port is Passthrough -> true
// if feature is not enabled and port is not Passthrough -> true
// if feature is enabled and port is Passthrough -> false
// if feature is enabled and port is not Passthrough -> true
return !(runtime.FeatureEnabled(runtime.FeatureAutopilotPassthroughPort) && PortPolicy == agonesv1.Passthrough)
}

func (apa *autopilotPortAllocator) Allocate(gs *agonesv1.GameServer) *agonesv1.GameServer {
if len(gs.Spec.Ports) == 0 {
return gs // Nothing to do.
}

var ports []agonesv1.GameServerPort
for i, p := range gs.Spec.Ports {
if p.PortPolicy != agonesv1.Dynamic && !runtime.FeatureEnabled(runtime.FeatureAutopilotPassthroughPort) {
if p.PortPolicy != agonesv1.Dynamic && checkPassthroughPortPolicyForAutopilot(p.PortPolicy) {
logger.WithField("gs", gs.Name).WithField("portPolicy", p.PortPolicy).Error(
"GameServer has invalid PortPolicy for Autopilot - this should have been rejected by webhooks. Refusing to assign ports.")
return gs
Expand Down
149 changes: 137 additions & 12 deletions pkg/cloudproduct/gke/gke_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,16 @@ func TestSyncPodPortsToGameServer(t *testing.T) {

func TestValidateGameServer(t *testing.T) {
for name, tc := range map[string]struct {
edPods bool
ports []agonesv1.GameServerPort
scheduling apis.SchedulingStrategy
safeToEvict agonesv1.EvictionSafe
want field.ErrorList
edPods bool
ports []agonesv1.GameServerPort
scheduling apis.SchedulingStrategy
safeToEvict agonesv1.EvictionSafe
want field.ErrorList
passthroughFlag string
}{
"no ports => validated": {scheduling: apis.Packed},
"no ports => validated": {passthroughFlag: "false", scheduling: apis.Packed},
"good ports => validated": {
passthroughFlag: "true",
ports: []agonesv1.GameServerPort{
{
Name: "some-tcpudp",
Expand Down Expand Up @@ -115,11 +117,26 @@ func TestValidateGameServer(t *testing.T) {
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-udp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
},
safeToEvict: agonesv1.EvictionSafeAlways,
scheduling: apis.Packed,
},
"bad port range => fails validation": {
passthroughFlag: "true",
ports: []agonesv1.GameServerPort{
{
Name: "best-tcpudp",
Expand All @@ -142,15 +159,32 @@ func TestValidateGameServer(t *testing.T) {
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-udp-bad-range",
PortPolicy: agonesv1.Passthrough,
Range: "passthrough",
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp-bad-range",
PortPolicy: agonesv1.Passthrough,
Range: "games",
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
},
safeToEvict: agonesv1.EvictionSafeAlways,
scheduling: apis.Packed,
want: field.ErrorList{
field.Invalid(field.NewPath("spec", "ports").Index(1).Child("range"), "game", "range must not be used on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(2).Child("range"), "game", "range must not be used on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(3).Child("range"), "passthrough", "range must not be used on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(4).Child("range"), "games", "range must not be used on GKE Autopilot"),
},
},
"bad policy (no feature gates) => fails validation": {
passthroughFlag: "false",
ports: []agonesv1.GameServerPort{
{
Name: "best-tcpudp",
Expand All @@ -173,18 +207,35 @@ func TestValidateGameServer(t *testing.T) {
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
{
Name: "passthrough-udp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
},
safeToEvict: agonesv1.EvictionSafeOnUpgrade,
scheduling: apis.Distributed,
want: field.ErrorList{
field.Invalid(field.NewPath("spec", "scheduling"), "Distributed", "scheduling strategy must be Packed on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(1).Child("portPolicy"), "Static", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(2).Child("portPolicy"), "Static", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(3).Child("portPolicy"), "Passthrough", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(4).Child("portPolicy"), "Passthrough", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "eviction", "safe"), "OnUpgrade", "eviction.safe OnUpgrade not supported on GKE Autopilot"),
},
},
"bad policy (GKEAutopilotExtendedDurationPods enabled) => fails validation but OnUpgrade works": {
edPods: true,
edPods: true,
passthroughFlag: "false",
ports: []agonesv1.GameServerPort{
{
Name: "best-tcpudp",
Expand All @@ -207,21 +258,29 @@ func TestValidateGameServer(t *testing.T) {
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-udp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
},
safeToEvict: agonesv1.EvictionSafeOnUpgrade,
scheduling: apis.Distributed,
want: field.ErrorList{
field.Invalid(field.NewPath("spec", "scheduling"), "Distributed", "scheduling strategy must be Packed on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(1).Child("portPolicy"), "Static", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(2).Child("portPolicy"), "Static", "portPolicy must be Dynamic or None on GKE Autopilot"),
field.Invalid(field.NewPath("spec", "ports").Index(3).Child("portPolicy"), "Passthrough", "portPolicy must be Dynamic or None on GKE Autopilot"),
},
},
} {
t.Run(name, func(t *testing.T) {
// PortPolicy None is behind a feature flag
runtime.FeatureTestMutex.Lock()
defer runtime.FeatureTestMutex.Unlock()
require.NoError(t, runtime.ParseFeatures(string(runtime.FeaturePortPolicyNone)+"=true"))
require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s=true&%s="+tc.passthroughFlag, runtime.FeaturePortPolicyNone, runtime.FeatureAutopilotPassthroughPort)))

causes := (&gkeAutopilot{useExtendedDurationPods: tc.edPods}).ValidateGameServerSpec(&agonesv1.GameServerSpec{
Ports: tc.ports,
Expand Down Expand Up @@ -451,12 +510,14 @@ func TestSetEvictionNoExtended(t *testing.T) {

func TestAutopilotPortAllocator(t *testing.T) {
for name, tc := range map[string]struct {
ports []agonesv1.GameServerPort
wantPorts []agonesv1.GameServerPort
wantAnnotation bool
ports []agonesv1.GameServerPort
wantPorts []agonesv1.GameServerPort
passthroughFlag string
wantAnnotation bool
}{
"no ports => no change": {},
"no ports => no change": {passthroughFlag: "false"},
"ports => assigned and annotated": {
passthroughFlag: "true",
ports: []agonesv1.GameServerPort{
{
Name: "some-tcpudp",
Expand All @@ -482,6 +543,19 @@ func TestAutopilotPortAllocator(t *testing.T) {
ContainerPort: 5678,
Protocol: agonesv1.ProtocolTCPUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
{
Name: "passthrough-tcpudp",
PortPolicy: agonesv1.Passthrough,
ContainerPort: 5678,
Protocol: agonesv1.ProtocolTCPUDP,
},
},
wantPorts: []agonesv1.GameServerPort{
{
Expand Down Expand Up @@ -526,17 +600,52 @@ func TestAutopilotPortAllocator(t *testing.T) {
HostPort: 4,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
Range: agonesv1.DefaultPortRange,
ContainerPort: 1234,
HostPort: 5,
Protocol: corev1.ProtocolTCP,
},
{
Name: "passthrough-tcpudp-tcp",
PortPolicy: agonesv1.Passthrough,
ContainerPort: 5678,
HostPort: 6,
Protocol: corev1.ProtocolTCP,
},
{
Name: "passthrough-tcpudp-udp",
PortPolicy: agonesv1.Passthrough,
ContainerPort: 5678,
HostPort: 6,
Protocol: corev1.ProtocolUDP,
},
},
wantAnnotation: true,
},
"bad policy => no change (should be rejected by webhooks previously)": {
passthroughFlag: "false",
ports: []agonesv1.GameServerPort{
{
Name: "awesome-udp",
PortPolicy: agonesv1.Static,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "awesome-none-udp",
PortPolicy: agonesv1.None,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
},
wantPorts: []agonesv1.GameServerPort{
{
Expand All @@ -545,10 +654,26 @@ func TestAutopilotPortAllocator(t *testing.T) {
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "awesome-none-udp",
PortPolicy: agonesv1.None,
ContainerPort: 1234,
Protocol: corev1.ProtocolUDP,
},
{
Name: "passthrough-tcp",
PortPolicy: agonesv1.Passthrough,
ContainerPort: 1234,
Protocol: corev1.ProtocolTCP,
},
},
},
} {
t.Run(name, func(t *testing.T) {
// PortPolicy None is behind a feature flag
runtime.FeatureTestMutex.Lock()
defer runtime.FeatureTestMutex.Unlock()
require.NoError(t, runtime.ParseFeatures(fmt.Sprintf("%s="+tc.passthroughFlag, runtime.FeatureAutopilotPassthroughPort)))
gs := (&autopilotPortAllocator{minPort: 8000, maxPort: 9000}).Allocate(&agonesv1.GameServer{Spec: agonesv1.GameServerSpec{Ports: tc.ports}})
wantGS := &agonesv1.GameServer{Spec: agonesv1.GameServerSpec{Ports: tc.wantPorts}}
if tc.wantAnnotation {
Expand Down
5 changes: 3 additions & 2 deletions pkg/gameservers/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ func (ext *Extensions) creationValidationHandler(review admissionv1.AdmissionRev
// creationMutationHandlerPod that mutates a GameServer pod when it is created
// Should only be called on gameserver pod create operations.
func (ext *Extensions) creationMutationHandlerPod(review admissionv1.AdmissionReview) (admissionv1.AdmissionReview, error) {
logger := runtime.NewLoggerWithSource("gke")
obj := review.Request.Object
pod := &corev1.Pod{}
err := json.Unmarshal(obj.Raw, pod)
Expand All @@ -333,7 +332,9 @@ func (ext *Extensions) creationMutationHandlerPod(review admissionv1.AdmissionRe
return review, nil
}

logger.WithField("review", review).Debug("creationMutationHandlerPod")
ext.baseLogger.WithField("pod.Name", pod.Name).Debug("creationMutationHandlerPod")

// TODO: We need to deal with case of multiple and mixed type ports before enabling the feature gate.
pod.Spec.Containers[1].Ports[0].ContainerPort = pod.Spec.Containers[1].Ports[0].HostPort

newPod, err := json.Marshal(pod)
Expand Down
9 changes: 8 additions & 1 deletion pkg/gameservers/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2277,6 +2277,13 @@ func newSingleContainerSpec() agonesv1.GameServerSpec {

func newPassthroughPortSingleContainerSpec() corev1.PodSpec {
return corev1.PodSpec{
Containers: []corev1.Container{{Name: "agones-gameserver-sidecar", Image: "container/image", Env: []corev1.EnvVar{{Name: passthroughPortEnvVar, Value: "TRUE"}}}, {Name: "simple-game-server", Image: "container2/image", Ports: []corev1.ContainerPort{{HostPort: 7777, ContainerPort: 555}}, Env: []corev1.EnvVar{{Name: passthroughPortEnvVar, Value: "TRUE"}}}},
Containers: []corev1.Container{
{Name: "agones-gameserver-sidecar",
Image: "container/image",
Env: []corev1.EnvVar{{Name: passthroughPortEnvVar, Value: "TRUE"}}},
{Name: "simple-game-server",
Image: "container2/image",
Ports: []corev1.ContainerPort{{HostPort: 7777, ContainerPort: 555}},
Env: []corev1.EnvVar{{Name: passthroughPortEnvVar, Value: "TRUE"}}}},
}
}
2 changes: 1 addition & 1 deletion pkg/util/webhooks/webhooks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ func (wh *WebHook) handle(path string, w http.ResponseWriter, r *http.Request) e
review.Response = &admissionv1.AdmissionResponse{Allowed: true}
}
review.Response.UID = review.Request.UID
wh.logger.WithField("name", review.Request.Name).WithField("path", path).WithField("kind", review.Request.Kind.Kind).WithField("group", review.Request.Kind.Group).Info("handling webhook request")
wh.logger.WithField("name", review.Request.Name).WithField("path", path).WithField("kind", review.Request.Kind.Kind).WithField("group", review.Request.Kind.Group).Debug("handling webhook request")

for _, oh := range wh.handlers[path] {
if oh.operation == review.Request.Operation &&
Expand Down

0 comments on commit 2d25b41

Please sign in to comment.