Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

Add basic ElasticsearchCluster resource validation #199

Merged
merged 7 commits into from
Jan 17, 2018
Merged

Add basic ElasticsearchCluster resource validation #199

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d8716bd
Add basic ElasticsearchCluster resource validation
munnerz Jan 11, 2018
eaeb7bc
Run validation on ESC updates
munnerz Jan 12, 2018
99a05a0
Make PullPolicy field use upstream type
munnerz Jan 17, 2018
724a647
Validate image pull policy. Validate node pool has role.
munnerz Jan 17, 2018
75e081e
Add unit tests for validation functions
munnerz Jan 17, 2018
9554779
Switch persistence.size field to be resource.Quantity and add validation
munnerz Jan 17, 2018
520322d
Expand elasticsearch persistence validation test
munnerz Jan 17, 2018
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
5 changes: 3 additions & 2 deletions pkg/apis/navigator/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package navigator

import (
"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -119,14 +120,14 @@ const (

type ElasticsearchClusterPersistenceConfig struct {
Enabled bool
Size string
Size resource.Quantity
StorageClass string
}

type ImageSpec struct {
Repository string
Tag string
PullPolicy string
PullPolicy v1.PullPolicy
}

type ElasticsearchPilotImage struct {
Expand Down
5 changes: 3 additions & 2 deletions pkg/apis/navigator/v1alpha1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package v1alpha1

import (
"k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Expand Down Expand Up @@ -163,7 +164,7 @@ type ElasticsearchClusterPersistenceConfig struct {

// Size of the persistent volume to provision (required if persistence is
// enabled).
Size string `json:"size"`
Size resource.Quantity `json:"size"`

// StorageClass to use for the persistent volume claim. If not set, the
// default cluster storage class will be used.
Expand All @@ -180,7 +181,7 @@ type ImageSpec struct {

// PullPolicy is the policy for pulling docker images. If not set, the
// cluster default will be used.
PullPolicy string `json:"pullPolicy"`
PullPolicy v1.PullPolicy `json:"pullPolicy"`
}

type ElasticsearchPilotImage struct {
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/navigator/v1alpha1/zz_generated.conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ func Convert_navigator_ElasticsearchPilotStatus_To_v1alpha1_ElasticsearchPilotSt
func autoConvert_v1alpha1_ImageSpec_To_navigator_ImageSpec(in *ImageSpec, out *navigator.ImageSpec, s conversion.Scope) error {
out.Repository = in.Repository
out.Tag = in.Tag
out.PullPolicy = in.PullPolicy
out.PullPolicy = v1.PullPolicy(in.PullPolicy)
return nil
}

Expand All @@ -512,7 +512,7 @@ func Convert_v1alpha1_ImageSpec_To_navigator_ImageSpec(in *ImageSpec, out *navig
func autoConvert_navigator_ImageSpec_To_v1alpha1_ImageSpec(in *navigator.ImageSpec, out *ImageSpec, s conversion.Scope) error {
out.Repository = in.Repository
out.Tag = in.Tag
out.PullPolicy = in.PullPolicy
out.PullPolicy = v1.PullPolicy(in.PullPolicy)
return nil
}

Expand Down
3 changes: 2 additions & 1 deletion pkg/apis/navigator/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,7 +365,7 @@ func (in *ElasticsearchClusterNodePool) DeepCopyInto(out *ElasticsearchClusterNo
(*in).DeepCopyInto(*out)
}
}
out.Persistence = in.Persistence
in.Persistence.DeepCopyInto(&out.Persistence)
if in.Config != nil {
in, out := &in.Config, &out.Config
*out = make(map[string]string, len(*in))
Expand Down Expand Up @@ -405,6 +405,7 @@ func (in *ElasticsearchClusterNodePoolStatus) DeepCopy() *ElasticsearchClusterNo
// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil.
func (in *ElasticsearchClusterPersistenceConfig) DeepCopyInto(out *ElasticsearchClusterPersistenceConfig) {
*out = *in
out.Size = in.Size.DeepCopy()
return
}

Expand Down
106 changes: 106 additions & 0 deletions pkg/apis/navigator/validation/elasticsearch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package validation

import (
corev1 "k8s.io/api/core/v1"
apimachineryvalidation "k8s.io/apimachinery/pkg/api/validation"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"

"github.com/jetstack/navigator/pkg/apis/navigator"
)

var supportedPullPolicies = []string{
string(corev1.PullNever),
string(corev1.PullIfNotPresent),
string(corev1.PullAlways),
"",
}

func ValidateImageSpec(img *navigator.ImageSpec, fldPath *field.Path) field.ErrorList {
el := field.ErrorList{}
if img.Tag == "" {
el = append(el, field.Required(fldPath.Child("tag"), ""))
}
if img.Repository == "" {
el = append(el, field.Required(fldPath.Child("repository"), ""))
}
if img.PullPolicy != corev1.PullNever &&
img.PullPolicy != corev1.PullIfNotPresent &&
img.PullPolicy != corev1.PullAlways &&
img.PullPolicy != "" {
el = append(el, field.NotSupported(fldPath.Child("pullPolicy"), img.PullPolicy, supportedPullPolicies))
}
return el
}

var supportedESClusterRoles = []string{
string(navigator.ElasticsearchRoleData),
string(navigator.ElasticsearchRoleIngest),
string(navigator.ElasticsearchRoleMaster),
}

func ValidateElasticsearchClusterRole(r navigator.ElasticsearchClusterRole, fldPath *field.Path) field.ErrorList {
el := field.ErrorList{}
switch r {
case navigator.ElasticsearchRoleData:
case navigator.ElasticsearchRoleIngest:
case navigator.ElasticsearchRoleMaster:
default:
el = append(el, field.NotSupported(fldPath, r, supportedESClusterRoles))
}
return el
}

func ValidateElasticsearchClusterNodePool(np *navigator.ElasticsearchClusterNodePool, fldPath *field.Path) field.ErrorList {
el := ValidateDNS1123Subdomain(np.Name, fldPath.Child("name"))
el = append(el, ValidateElasticsearchPersistence(&np.Persistence, fldPath.Child("persistence"))...)
rolesPath := fldPath.Child("roles")
if len(np.Roles) == 0 {
el = append(el, field.Required(rolesPath, "at least one role must be specified"))
}
for i, r := range np.Roles {
idxPath := rolesPath.Index(i)
el = append(el, ValidateElasticsearchClusterRole(r, idxPath)...)
}
if np.Replicas < 0 {
el = append(el, field.Invalid(fldPath.Child("replicas"), np.Replicas, "must be greater than zero"))
}
// TODO: call k8s.io/kubernetes/pkg/apis/core/validation.ValidateResourceRequirements on np.Resources
// this will require vendoring kubernetes/kubernetes and switching to use the corev1 ResourceRequirements
// struct
return el
}

func ValidateElasticsearchPersistence(cfg *navigator.ElasticsearchClusterPersistenceConfig, fldPath *field.Path) field.ErrorList {
el := field.ErrorList{}
if cfg.Enabled && cfg.Size.IsZero() {
el = append(el, field.Required(fldPath.Child("size"), ""))
}
if cfg.Size.Sign() == -1 {
el = append(el, field.Invalid(fldPath.Child("size"), cfg.Size, "must be greater than zero"))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps instead check that the value is >0 and this could all be replaced with, if cgf.Size.Sign() != 1

The docs say:

// Sign returns 0 if the quantity is zero, -1 if the quantity is less than zero, or 1 if the
// quantity is greater than zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in my other comment, there are a few cases to cover:

  1. Persistence enabled, size is >0 (VALID)
  2. Persistence enabled, size = 0 (INVALID)
  3. Persistence disabled, size = 0 (VALID)
  4. Size<0 - ALWAYS invalid

I've expanded our test suite to explicitly cover all the cases above, and they are still passing 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see the case for 3 Persistence disabled, size = 0 (VALID)
Unless that's how we determine that persistence is not required.
In which case, would that be better expressed as Persistence *ElasticsearchClusterPersistenceConfig and Persistence = nil ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see you just explained below.

Copy link
Contributor Author

@munnerz munnerz Jan 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if I set:

persistence:
  enabled: false

I have implicitly also said the size=0. It would we weird/awkward to require users to write:

persistence:
  enabled: false
  # size is ignored
  size: 1Gi

return el
}

func ValidateElasticsearchClusterSpec(spec *navigator.ElasticsearchClusterSpec, fldPath *field.Path) field.ErrorList {
allErrs := ValidateImageSpec(&spec.Image.ImageSpec, fldPath.Child("image"))
allErrs = append(allErrs, ValidateImageSpec(&spec.Pilot.ImageSpec, fldPath.Child("pilot"))...)
npPath := fldPath.Child("nodePools")
allNames := sets.String{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, sets! That's useful.

for i, np := range spec.NodePools {
idxPath := npPath.Index(i)
if allNames.Has(np.Name) {
allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), np.Name))
} else {
allNames.Insert(np.Name)
}
allErrs = append(allErrs, ValidateElasticsearchClusterNodePool(&np, idxPath)...)
}
return allErrs
}

func ValidateElasticsearchCluster(esc *navigator.ElasticsearchCluster) field.ErrorList {
allErrs := ValidateObjectMeta(&esc.ObjectMeta, true, apimachineryvalidation.NameIsDNSSubdomain, field.NewPath("metadata"))
allErrs = append(allErrs, ValidateElasticsearchClusterSpec(&esc.Spec, field.NewPath("spec"))...)
return allErrs
}
Loading