Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactoring webhooks #374

Merged
merged 1 commit into from
Aug 10, 2022
Merged

refactoring webhooks #374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 3 additions & 36 deletions pkg/operator/api/v1alpha1/gameserver_webhook.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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