kubernetes
/
kops
mirror of https://github.com/kubernetes/kops.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Validate IGs more strictly after defaults have applied 

This commit will ensure IGs are validated in clientset prior to write similar to clusters. Also introduces strict flag similar to cluster, which only validate values where we have defaults after defaults have been applied.
This commit is contained in:
Ole Markus With 2021-11-01 15:45:16 +01:00
parent f30dabc712
commit aa493a3273
10 changed files with 88 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cmd/kops/create_instancegroup.go
View File

@ -259,7 +259,7 @@ func RunCreateInstanceGroup(ctx context.Context, f *util.Factory, out io.Writer,
return fmt.Errorf("unexpected object type: %T", obj)
}
err = validation.CrossValidateInstanceGroup(group, cluster, cloud).ToAggregate()
err = validation.CrossValidateInstanceGroup(group, cluster, cloud, true).ToAggregate()
if err != nil {
return err
}

2
cmd/kops/edit_instancegroup.go
View File

@ -304,7 +304,7 @@ func updateInstanceGroup(ctx context.Context, clientset simple.Clientset, channe
return fmt.Sprintf("error populating cluster spec: %s", err), nil
}
err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster, cloud).ToAggregate()
err = validation.CrossValidateInstanceGroup(fullGroup, fullCluster, cloud, true).ToAggregate()
if err != nil {
return fmt.Sprintf("validation failed: %s", err), nil
}

2
pkg/apis/kops/validation/aws_test.go
View File

@ -386,7 +386,7 @@ func TestInstanceMetadataOptions(t *testing.T) {
}
for _, test := range tests {
errs := ValidateInstanceGroup(test.ig, cloud)
errs := ValidateInstanceGroup(test.ig, cloud, true)
testErrors(t, test.ig.ObjectMeta.Name, errs, test.expected)
}
}

20
pkg/apis/kops/validation/instancegroup.go
View File

@ -25,13 +25,14 @@ import (
"github.com/aws/aws-sdk-go/aws/arn"
"github.com/aws/aws-sdk-go/service/ec2"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
)
// ValidateInstanceGroup is responsible for validating the configuration of a instancegroup
func ValidateInstanceGroup(g *kops.InstanceGroup, cloud fi.Cloud) field.ErrorList {
func ValidateInstanceGroup(g *kops.InstanceGroup, cloud fi.Cloud, strict bool) field.ErrorList {
allErrs := field.ErrorList{}
if g.ObjectMeta.Name == "" {
@ -60,12 +61,25 @@ func ValidateInstanceGroup(g *kops.InstanceGroup, cloud fi.Cloud) field.ErrorLis
allErrs = append(allErrs, IsValidValue(field.NewPath("spec", "tenancy"), &g.Spec.Tenancy, ec2.Tenancy_Values())...)
}
if strict {
if g.Spec.MaxSize == nil {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "maxSize"), "maxSize must be set"))
}
if g.Spec.MinSize == nil {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "minSize"), "minSize must be set"))
}
}
if g.Spec.MaxSize != nil && g.Spec.MinSize != nil {
if *g.Spec.MaxSize < *g.Spec.MinSize {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "maxSize"), "maxSize must be greater than or equal to minSize."))
}
}
if strict && g.Spec.Image == "" {
allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "image"), "image must be specified."))
}
if fi.Int32Value(g.Spec.RootVolumeIOPS) < 0 {
allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "rootVolumeIops"), g.Spec.RootVolumeIOPS, "RootVolumeIOPS must be greater than 0"))
}
@ -180,8 +194,8 @@ func validateVolumeMountSpec(path *field.Path, spec kops.VolumeMountSpec) field.
// CrossValidateInstanceGroup performs validation of the instance group, including that it is consistent with the Cluster
// It calls ValidateInstanceGroup, so all that validation is included.
func CrossValidateInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster, cloud fi.Cloud) field.ErrorList {
allErrs := ValidateInstanceGroup(g, cloud)
func CrossValidateInstanceGroup(g *kops.InstanceGroup, cluster *kops.Cluster, cloud fi.Cloud, strict bool) field.ErrorList {
allErrs := ValidateInstanceGroup(g, cloud, strict)
if g.Spec.Role == kops.InstanceGroupRoleMaster {
allErrs = append(allErrs, ValidateMasterInstanceGroup(g, cluster)...)

96
pkg/apis/kops/validation/instancegroup_test.go
View File

@ -218,16 +218,9 @@ func TestValidBootDevice(t *testing.T) {
}
for _, g := range grid {
ig := &kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
Role: "Node",
RootVolumeType: fi.String(g.volumeType),
},
}
errs := CrossValidateInstanceGroup(ig, cluster, nil)
ig := createMinimalInstanceGroup()
ig.Spec.RootVolumeType = fi.String(g.volumeType)
errs := CrossValidateInstanceGroup(ig, cluster, nil, true)
testErrors(t, g.volumeType, errs, g.expected)
}
}
@ -253,17 +246,11 @@ func TestValidNodeLabels(t *testing.T) {
}
for _, g := range grid {
ig := &kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
Role: "Node",
NodeLabels: make(map[string]string),
},
}
ig := createMinimalInstanceGroup()
ig.Spec.NodeLabels = make(map[string]string)
ig.Spec.NodeLabels[g.label] = "placeholder"
errs := ValidateInstanceGroup(ig, nil)
errs := ValidateInstanceGroup(ig, nil, true)
testErrors(t, g.label, errs, g.expected)
}
}
@ -289,17 +276,10 @@ func TestValidateIGCloudLabels(t *testing.T) {
}
for _, g := range grid {
ig := &kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
Role: "Node",
CloudLabels: make(map[string]string),
},
}
ig := createMinimalInstanceGroup()
ig.Spec.CloudLabels[g.label] = "placeholder"
errs := ValidateInstanceGroup(ig, nil)
errs := ValidateInstanceGroup(ig, nil, true)
testErrors(t, g.label, errs, g.expected)
}
}
@ -319,17 +299,10 @@ func TestIGCloudLabelIsIGName(t *testing.T) {
}
for _, g := range grid {
ig := &kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
Role: "Node",
CloudLabels: make(map[string]string),
},
}
ig := createMinimalInstanceGroup()
ig.Spec.CloudLabels[aws.CloudTagInstanceGroupName] = g.label
errs := ValidateInstanceGroup(ig, nil)
errs := ValidateInstanceGroup(ig, nil, true)
testErrors(t, g.label, errs, g.expected)
}
}
@ -363,18 +336,11 @@ func TestIGUpdatePolicy(t *testing.T) {
expected: []string{unsupportedValueError},
},
} {
ig := kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
Role: "Node",
CloudLabels: make(map[string]string),
},
}
ig := createMinimalInstanceGroup()
t.Run(test.label, func(t *testing.T) {
ig.Spec.UpdatePolicy = test.policy
errs := ValidateInstanceGroup(&ig, nil)
errs := ValidateInstanceGroup(ig, nil, true)
testErrors(t, test.label, errs, test.expected)
})
}
@ -394,6 +360,9 @@ func TestValidInstanceGroup(t *testing.T) {
Spec: kops.InstanceGroupSpec{
Role: kops.InstanceGroupRoleMaster,
Subnets: []string{"eu-central-1a"},
MaxSize: fi.Int32(1),
MinSize: fi.Int32(1),
Image: "my-image",
},
},
ExpectedErrors: 0,
@ -407,6 +376,9 @@ func TestValidInstanceGroup(t *testing.T) {
Spec: kops.InstanceGroupSpec{
Role: kops.InstanceGroupRoleAPIServer,
Subnets: []string{"eu-central-1a"},
MaxSize: fi.Int32(1),
MinSize: fi.Int32(1),
Image: "my-image",
},
},
ExpectedErrors: 0,
@ -420,6 +392,9 @@ func TestValidInstanceGroup(t *testing.T) {
Spec: kops.InstanceGroupSpec{
Role: kops.InstanceGroupRoleNode,
Subnets: []string{"eu-central-1a"},
MaxSize: fi.Int32(1),
MinSize: fi.Int32(1),
Image: "my-image",
},
},
ExpectedErrors: 0,
@ -433,6 +408,9 @@ func TestValidInstanceGroup(t *testing.T) {
Spec: kops.InstanceGroupSpec{
Role: kops.InstanceGroupRoleBastion,
Subnets: []string{"eu-central-1a"},
MaxSize: fi.Int32(1),
MinSize: fi.Int32(1),
Image: "my-image",
},
},
ExpectedErrors: 0,
@ -440,7 +418,23 @@ func TestValidInstanceGroup(t *testing.T) {
},
}
for _, g := range grid {
errList := ValidateInstanceGroup(g.IG, nil)
errList := ValidateInstanceGroup(g.IG, nil, true)
testErrors(t, g.Description, errList, []string{})
}
}
func createMinimalInstanceGroup() *kops.InstanceGroup {
ig := &kops.InstanceGroup{
ObjectMeta: v1.ObjectMeta{
Name: "some-ig",
},
Spec: kops.InstanceGroupSpec{
CloudLabels: make(map[string]string),
Role: "Node",
MaxSize: fi.Int32(1),
MinSize: fi.Int32(1),
Image: "my-image",
},
}
return ig
}

2
pkg/apis/kops/validation/legacy.go
View File

@ -440,7 +440,7 @@ func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool, cl
}
for _, g := range groups {
errs := CrossValidateInstanceGroup(g, c, cloud)
errs := CrossValidateInstanceGroup(g, c, cloud, strict)
// Additional cloud-specific validation rules
if kops.CloudProviderID(c.Spec.CloudProvider) != kops.CloudProviderAWS && len(g.Spec.Volumes) > 0 {

4
pkg/client/simple/vfsclientset/instancegroup.go
View File

@ -54,7 +54,7 @@ func newInstanceGroupVFS(c *VFSClientset, cluster *kopsapi.Cluster) *InstanceGro
}
r.init(kind, c.basePath.Join(clusterName, "instancegroup"), StoreVersion)
r.validate = func(o runtime.Object) error {
return validation.ValidateInstanceGroup(o.(*kopsapi.InstanceGroup), nil).ToAggregate()
return validation.ValidateInstanceGroup(o.(*kopsapi.InstanceGroup), nil, true).ToAggregate()
}
return r
}
@ -100,6 +100,7 @@ func (c *InstanceGroupVFS) List(ctx context.Context, options metav1.ListOptions)
}
func (c *InstanceGroupVFS) Create(ctx context.Context, g *kopsapi.InstanceGroup, opts metav1.CreateOptions) (*kopsapi.InstanceGroup, error) {
validation.ValidateInstanceGroup(g, nil, true)
err := c.create(ctx, c.cluster, g)
if err != nil {
return nil, err
@ -117,6 +118,7 @@ func (c *InstanceGroupVFS) Update(ctx context.Context, g *kopsapi.InstanceGroup,
g.SetGeneration(old.GetGeneration() + 1)
}
validation.ValidateInstanceGroup(g, nil, true)
err = c.update(ctx, c.cluster, g)
if err != nil {
return nil, err

2
pkg/commands/helpers_readwrite.go
View File

@ -84,7 +84,7 @@ func UpdateInstanceGroup(ctx context.Context, clientset simple.Clientset, cluste
return err
}
err = validation.CrossValidateInstanceGroup(instanceGroupToUpdate, fullCluster, cloud).ToAggregate()
err = validation.CrossValidateInstanceGroup(instanceGroupToUpdate, fullCluster, cloud, true).ToAggregate()
if err != nil {
return err
}

2
upup/pkg/fi/cloudup/populate_instancegroup_spec.go
View File

@ -64,7 +64,7 @@ var awsDedicatedInstanceExceptions = map[string]bool{
// The InstanceGroup is simpler than the cluster spec, so we just populate in place (like the rest of k8s)
func PopulateInstanceGroupSpec(cluster *kops.Cluster, input *kops.InstanceGroup, cloud fi.Cloud, channel *kops.Channel) (*kops.InstanceGroup, error) {
var err error
err = validation.ValidateInstanceGroup(input, nil).ToAggregate()
err = validation.ValidateInstanceGroup(input, nil, false).ToAggregate()
if err != nil {
return nil, err
}

17
upup/pkg/fi/cloudup/populate_instancegroup_spec_test.go
View File

@ -22,6 +22,7 @@ import (
"testing"
kopsapi "k8s.io/kops/pkg/apis/kops"
"k8s.io/kops/upup/pkg/fi"
"k8s.io/kops/util/pkg/architectures"
)
@ -29,6 +30,9 @@ func buildMinimalNodeInstanceGroup(subnets ...string) *kopsapi.InstanceGroup {
g := &kopsapi.InstanceGroup{}
g.ObjectMeta.Name = "nodes"
g.Spec.Role = kopsapi.InstanceGroupRoleNode
g.Spec.MinSize = fi.Int32(1)
g.Spec.MaxSize = fi.Int32(1)
g.Spec.Image = "my-image"
g.Spec.Subnets = subnets
return g
@ -38,6 +42,9 @@ func buildMinimalMasterInstanceGroup(subnet string) *kopsapi.InstanceGroup {
g := &kopsapi.InstanceGroup{}
g.ObjectMeta.Name = "master-" + subnet
g.Spec.Role = kopsapi.InstanceGroupRoleMaster
g.Spec.MinSize = fi.Int32(1)
g.Spec.MaxSize = fi.Int32(1)
g.Spec.Image = "my-image"
g.Spec.Subnets = []string{subnet}
return g
@ -63,6 +70,16 @@ func TestPopulateInstanceGroup_Role_Required(t *testing.T) {
expectErrorFromPopulateInstanceGroup(t, cluster, g, channel, "spec.role")
}
func TestPopulateInstanceGroup_Image_Required(t *testing.T) {
_, cluster := buildMinimalCluster()
g := buildMinimalNodeInstanceGroup()
g.Spec.Image = ""
channel := &kopsapi.Channel{}
expectErrorFromPopulateInstanceGroup(t, cluster, g, channel, "unable to determine default image for InstanceGroup nodes")
}
func expectErrorFromPopulateInstanceGroup(t *testing.T, cluster *kopsapi.Cluster, g *kopsapi.InstanceGroup, channel *kopsapi.Channel, message string) {
cloud, err := BuildCloud(cluster)
if err != nil {