Skip to content

Commit

Permalink
refactoring webhooks
Browse files Browse the repository at this point in the history
  • Loading branch information
@dgkanatsios
dgkanatsios committed Aug 10, 2022
1 parent 85a2d3c commit 1a3f5c9
Show file tree
Hide file tree
Showing 4 changed files with 69 additions and 73 deletions.
39 changes: 3 additions & 36 deletions pkg/operator/api/v1alpha1/gameserver_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 */)
}
6 changes: 3 additions & 3 deletions pkg/operator/api/v1alpha1/gameserver_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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))
})
})
})
Expand Down
78 changes: 49 additions & 29 deletions pkg/operator/api/v1alpha1/gameserverbuild_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand All @@ -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
}
19 changes: 14 additions & 5 deletions pkg/operator/api/v1alpha1/gameserverbuild_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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() {
Expand All @@ -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))
})

})
})

Expand Down

0 comments on commit 1a3f5c9

Please sign in to comment.