diff --git a/pkg/operator/api/v1alpha1/gameserver_webhook.go b/pkg/operator/api/v1alpha1/gameserver_webhook.go index 0a18934e..c1111a64 100644 --- a/pkg/operator/api/v1alpha1/gameserver_webhook.go +++ b/pkg/operator/api/v1alpha1/gameserver_webhook.go @@ -17,9 +17,6 @@ limitations under the License. package v1alpha1 import ( - "fmt" - - corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -97,40 +94,10 @@ func (r *GameServer) validateOwnerReferences() *field.Error { } } return field.Invalid(field.NewPath("OwnerReferences"), r.Name, - "a GameServer must have a GameServerBuild as an owner") + errNoOwner) } -// validatePortsToExpose makes the following validations for ports in portsToExpose: -// 1. if a port number is in portsToExpose, there must be at least one -// matching port in the pod containers spec, this validation is skipped -// if the GameServer has HostNetwork enabled -// 2. if a port number is in portsToExpose, the matching ports in the -// pod containers spec must have a name +// validatePortsToExpose validates the portsToExpose slice func (r *GameServer) validatePortsToExpose() field.ErrorList { - var portsGroupedByNumber = make(map[int32][]corev1.ContainerPort) - for i := 0; i < len(r.Spec.Template.Spec.Containers); i++ { - container := r.Spec.Template.Spec.Containers[i] - for j := 0; j < len(container.Ports); j++ { - port := container.Ports[j] - if port.ContainerPort != 0 { - portsGroupedByNumber[port.ContainerPort] = append(portsGroupedByNumber[port.ContainerPort], port) - } - } - } - var errs field.ErrorList - for i := 0; i < len(r.Spec.PortsToExpose); i++ { - ports := portsGroupedByNumber[r.Spec.PortsToExpose[i]] - if len(ports) < 1 && !r.Spec.Template.Spec.HostNetwork { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), r.Name, - fmt.Sprintf("there must be at least one port that matches each value in portsToExpose, error in port %d", r.Spec.PortsToExpose[i]))) - } - for j := 0; j < len(ports); j++ { - port := ports[j] - if port.Name == "" { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), r.Name, - fmt.Sprintf("ports to expose must have a name, error in port %d", port.ContainerPort))) - } - } - } - return errs + return validatePortsToExposeInternal(r.Name, &r.Spec.Template.Spec, r.Spec.PortsToExpose, false /* validateHostPort */) } diff --git a/pkg/operator/api/v1alpha1/gameserver_webhook_test.go b/pkg/operator/api/v1alpha1/gameserver_webhook_test.go index d66a229c..2ba0dd9d 100644 --- a/pkg/operator/api/v1alpha1/gameserver_webhook_test.go +++ b/pkg/operator/api/v1alpha1/gameserver_webhook_test.go @@ -17,7 +17,7 @@ var _ = Describe("GameServer webhook tests", func() { gs.ObjectMeta.OwnerReferences = make([]metav1.OwnerReference, 0) err := k8sClient.Create(ctx, &gs) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("a GameServer must have a GameServerBuild as an owner")) + Expect(err.Error()).Should(ContainSubstring(errNoOwner)) }) It("validates that the port to expose exists", func() { @@ -26,7 +26,7 @@ var _ = Describe("GameServer webhook tests", func() { gs.Spec.PortsToExpose = append(gs.Spec.PortsToExpose, 70) err := k8sClient.Create(ctx, &gs) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("there must be at least one port that matches each value in portsToExpose")) + Expect(err.Error()).Should(ContainSubstring(errPortsMatchingPortsToExpose)) }) It("validates that the port to expose does not need to exist if the HostNetwork is enabled", func() { @@ -43,7 +43,7 @@ var _ = Describe("GameServer webhook tests", func() { gs.Spec.Template.Spec.Containers[0].Ports[0].Name = "" err := k8sClient.Create(ctx, &gs) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("ports to expose must have a name")) + Expect(err.Error()).Should(ContainSubstring(errNoPortName)) }) }) }) diff --git a/pkg/operator/api/v1alpha1/gameserverbuild_webhook.go b/pkg/operator/api/v1alpha1/gameserverbuild_webhook.go index 1aa294d7..ffd82774 100644 --- a/pkg/operator/api/v1alpha1/gameserverbuild_webhook.go +++ b/pkg/operator/api/v1alpha1/gameserverbuild_webhook.go @@ -37,6 +37,16 @@ var ( c client.Client ) +const ( + errNoHostPort = "ports to expose must not have a hostPort value" + errNoPortName = "ports to expose must have a name" + errBuildIdUnique = "cannot have more than one GameServerBuild with the same BuildID" + errBuildIdImmutable = "changing buildID on an existing GameServerBuild is not allowed" + errPortsMatchingPortsToExpose = "there must be at least one port that matches each value in portsToExpose" + errNoOwner = "a GameServer must have a GameServerBuild as an owner" + errStandingByLessThanMax = "standingby must be less or equal than max" +) + func (r *GameServerBuild) SetupWebhookWithManager(mgr ctrl.Manager) error { // this should be a live API reader but this won't in this case since we're querying the GameServerBuild via spec.buildID // and arbitrary field CRD selectors are not working at this time @@ -114,7 +124,7 @@ func (r *GameServerBuild) validateCreateBuildID() *field.Error { gsb := gsbList.Items[i] if r.Name != gsb.Name { return field.Invalid(field.NewPath("spec").Child("buildID"), - r.Name, "cannot have more than one GameServerBuild with the same BuildID") + r.Name, errBuildIdUnique) } } return nil @@ -124,22 +134,41 @@ func (r *GameServerBuild) validateCreateBuildID() *field.Error { func (r *GameServerBuild) validateUpdateBuildID(old runtime.Object) *field.Error { if r.Spec.BuildID != old.(*GameServerBuild).Spec.BuildID { return field.Invalid(field.NewPath("spec").Child("buildID"), - r.Name, "changing buildID on an existing GameServerBuild is not allowed") + r.Name, errBuildIdImmutable) + } + return nil +} + +// validatePortsToExpose validates the portsToExpose slice +func (r *GameServerBuild) validatePortsToExpose() field.ErrorList { + return validatePortsToExposeInternal(r.Name, &r.Spec.Template.Spec, r.Spec.PortsToExpose, true /* validateHostPort */) +} + +// validateStandingBy checks that the standingBy value is less or equal than max +func (r *GameServerBuild) validateStandingBy() *field.Error { + if r.Spec.StandingBy > r.Spec.Max { + return field.Invalid(field.NewPath("spec").Child("standingby"), + r.Name, errStandingByLessThanMax) } return nil } -// validatePortsToExpose makes the following validations for ports in portsToExpose: -// 1. if a port number is in portsToExpose, there must be at least one +// validatePortsToExposeInternal validates portsToExpose slice +// it performs the following validations +// - if a port number is in portsToExpose, there must be at least one // matching port in the pod containers spec -// 2. if a port number is in portsToExpose, the matching ports in the -// pod containers spec must have a name -// 3. if a port number is in portsToExpose, the matching ports in the +// This part of validation is skipped if the GameServer has HostNetwork enabled +// This can happen when the user creates a multi-container GameServer with hostNetwork enabled +// and has selected a hostPort for an existing container +// - if a port number is in portsToExpose, the matching ports in the +// pod containers spec must have a name. This is because the name will be used by the GSDK to reference the port +// - if a port number is in portsToExpose, the matching ports in the // pod containers spec must not have a hostPort -func (r *GameServerBuild) validatePortsToExpose() field.ErrorList { +// We set validateHostPort to true only for GameServerBuild validation. When the GameServer is created, we assign a HostPort so no need for validation +func validatePortsToExposeInternal(name string, spec *corev1.PodSpec, portsToExpose []int32, validateHostPort bool) field.ErrorList { var portsGroupedByNumber = make(map[int32][]corev1.ContainerPort) - for i := 0; i < len(r.Spec.Template.Spec.Containers); i++ { - container := r.Spec.Template.Spec.Containers[i] + for i := 0; i < len(spec.Containers); i++ { + container := spec.Containers[i] for j := 0; j < len(container.Ports); j++ { port := container.Ports[j] if port.ContainerPort != 0 { @@ -148,32 +177,23 @@ func (r *GameServerBuild) validatePortsToExpose() field.ErrorList { } } var errs field.ErrorList - for i := 0; i < len(r.Spec.PortsToExpose); i++ { - ports := portsGroupedByNumber[r.Spec.PortsToExpose[i]] - if len(ports) < 1 { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), r.Name, - fmt.Sprintf("there must be at least one port that matches each value in portsToExpose, error in port %d", r.Spec.PortsToExpose[i]))) + for i := 0; i < len(portsToExpose); i++ { + ports := portsGroupedByNumber[portsToExpose[i]] + if !spec.HostNetwork && len(ports) < 1 { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), name, + fmt.Sprintf("%s: error in port %d", errPortsMatchingPortsToExpose, portsToExpose[i]))) } for j := 0; j < len(ports); j++ { port := ports[j] if port.Name == "" { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), r.Name, - fmt.Sprintf("ports to expose must have a name, error in port %d", port.ContainerPort))) + errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), name, + fmt.Sprintf("%s: error in port %d", errNoPortName, port.ContainerPort))) } - if port.HostPort != 0 { - errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), r.Name, - fmt.Sprintf("ports to expose must not have a hostPort value, error in port %d", port.ContainerPort))) + if validateHostPort && port.HostPort != 0 { + errs = append(errs, field.Invalid(field.NewPath("spec").Child("portsToExpose"), name, + fmt.Sprintf("%s: error in port %d", errNoHostPort, port.ContainerPort))) } } } return errs } - -// validateStandingBy checks that the standingBy value is less or equal than max -func (r *GameServerBuild) validateStandingBy() *field.Error { - if r.Spec.StandingBy > r.Spec.Max { - return field.Invalid(field.NewPath("spec").Child("standingby"), - r.Name, "standingby must be less or equal than max") - } - return nil -} diff --git a/pkg/operator/api/v1alpha1/gameserverbuild_webhook_test.go b/pkg/operator/api/v1alpha1/gameserverbuild_webhook_test.go index c5cd6979..64bd0368 100644 --- a/pkg/operator/api/v1alpha1/gameserverbuild_webhook_test.go +++ b/pkg/operator/api/v1alpha1/gameserverbuild_webhook_test.go @@ -24,7 +24,7 @@ var _ = Describe("GameServerBuild webhook tests", func() { gsb = createTestGameServerBuild(buildName2, buildID, 2, 4, false) err := k8sClient.Create(ctx, &gsb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("cannot have more than one GameServerBuild with the same BuildID")) + Expect(err.Error()).Should(ContainSubstring(errBuildIdUnique)) }) It("validates that updating the buildID is not allowed", func() { @@ -35,7 +35,7 @@ var _ = Describe("GameServerBuild webhook tests", func() { gsb.Spec.BuildID = buildID2 err := k8sClient.Update(ctx, &gsb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("changing buildID on an existing GameServerBuild is not allowed")) + Expect(err.Error()).Should(ContainSubstring(errBuildIdImmutable)) }) It("validates that the port to expose exists", func() { @@ -44,7 +44,7 @@ var _ = Describe("GameServerBuild webhook tests", func() { gsb.Spec.PortsToExpose = append(gsb.Spec.PortsToExpose, 70) err := k8sClient.Create(ctx, &gsb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("there must be at least one port that matches each value in portsToExpose")) + Expect(err.Error()).Should(ContainSubstring(errPortsMatchingPortsToExpose)) }) It("validates that the port to expose has a name", func() { @@ -53,7 +53,7 @@ var _ = Describe("GameServerBuild webhook tests", func() { gsb.Spec.Template.Spec.Containers[0].Ports[0].Name = "" err := k8sClient.Create(ctx, &gsb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("ports to expose must have a name")) + Expect(err.Error()).Should(ContainSubstring(errNoPortName)) }) It("validates that the port to expose doesn't have a hostPort", func() { @@ -62,8 +62,17 @@ var _ = Describe("GameServerBuild webhook tests", func() { gsb.Spec.Template.Spec.Containers[0].Ports[0].HostPort = 1000 err := k8sClient.Create(ctx, &gsb) Expect(err).To(HaveOccurred()) - Expect(err.Error()).Should(ContainSubstring("ports to expose must not have a hostPort value")) + Expect(err.Error()).Should(ContainSubstring(errNoHostPort)) }) + + It("validates that standingBy must be less than equal to max", func() { + buildName, buildID := getNewNameAndID() + gsb := createTestGameServerBuild(buildName, buildID, 5, 4, false) + err := k8sClient.Create(ctx, &gsb) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).Should(ContainSubstring(errStandingByLessThanMax)) + }) + }) })