From 184feed8fd13628cfac3e55823e45c3a1f5e46ed Mon Sep 17 00:00:00 2001 From: Joe Kratzat Date: Thu, 28 Apr 2022 13:35:16 -0400 Subject: [PATCH] feat: update OCICluster webhook validations --- api/v1beta1/ocicluster_webhook.go | 20 ++- api/v1beta1/ocicluster_webhook_test.go | 203 ++++++++++++++++++++++--- api/v1beta1/validator.go | 133 +++++++++++++++- 3 files changed, 335 insertions(+), 21 deletions(-) diff --git a/api/v1beta1/ocicluster_webhook.go b/api/v1beta1/ocicluster_webhook.go index 5fc466d6d..7eb47f7a2 100644 --- a/api/v1beta1/ocicluster_webhook.go +++ b/api/v1beta1/ocicluster_webhook.go @@ -58,7 +58,7 @@ func (c *OCICluster) ValidateCreate() error { var allErrs field.ErrorList - allErrs = append(allErrs, c.validate()...) + allErrs = append(allErrs, c.validate(nil)...) if len(allErrs) == 0 { return nil @@ -93,7 +93,7 @@ func (c *OCICluster) ValidateUpdate(old runtime.Object) error { allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "ociResourceIdentifier"), c.Spec.OCIResourceIdentifier, "field is immutable")) } - allErrs = append(allErrs, c.validate()...) + allErrs = append(allErrs, c.validate(oldCluster)...) if len(allErrs) == 0 { return nil @@ -102,9 +102,17 @@ func (c *OCICluster) ValidateUpdate(old runtime.Object) error { return apierrors.NewInvalid(c.GroupVersionKind().GroupKind(), c.Name, allErrs) } -func (c *OCICluster) validate() field.ErrorList { +func (c *OCICluster) validate(old *OCICluster) field.ErrorList { var allErrs field.ErrorList + var oldNetworkSpec NetworkSpec + if old != nil { + oldNetworkSpec = old.Spec.NetworkSpec + } + + allErrs = append(allErrs, c.validateClusterName()...) + allErrs = append(allErrs, validateNetworkSpec(c.Spec.NetworkSpec, oldNetworkSpec, field.NewPath("spec").Child("networkSpec"))...) + if len(c.Spec.CompartmentId) <= 0 { allErrs = append( allErrs, @@ -125,6 +133,12 @@ func (c *OCICluster) validate() field.ErrorList { field.Invalid(field.NewPath("spec", "ociResourceIdentifier"), c.Spec.OCIResourceIdentifier, "field is required")) } + if len(c.Spec.Region) > 0 && !validRegion(c.Spec.Region) { + allErrs = append( + allErrs, + field.Invalid(field.NewPath("spec", "region"), c.Spec.Region, "field is invalid. See https://docs.oracle.com/en-us/iaas/Content/General/Concepts/regions.htm")) + } + if len(allErrs) == 0 { return nil } diff --git a/api/v1beta1/ocicluster_webhook_test.go b/api/v1beta1/ocicluster_webhook_test.go index db23c23f1..1a4e2697b 100644 --- a/api/v1beta1/ocicluster_webhook_test.go +++ b/api/v1beta1/ocicluster_webhook_test.go @@ -20,6 +20,7 @@ package v1beta1 import ( + "strings" "testing" "github.com/onsi/gomega" @@ -28,12 +29,58 @@ import ( ) func TestOCICluster_ValidateCreate(t *testing.T) { + goodSubnets := []*Subnet{ + &Subnet{ + Name: "test-subnet", + CIDR: "10.0.0.0/16", + }, + } + badSubnetCidr := []*Subnet{ + &Subnet{ + Name: "test-subnet", + CIDR: "10.1.0.0/16", + }, + } + badSubnetCidrFormat := []*Subnet{ + &Subnet{ + Name: "test-subnet", + CIDR: "no-a-cidr", + }, + } + dupSubnetNames := []*Subnet{ + &Subnet{ + Name: "dup-name", + CIDR: "10.0.0.0/16", + }, + &Subnet{ + Name: "dup-name", + CIDR: "10.0.0.0/16", + }, + } + badSubnetName := []*Subnet{ + &Subnet{ + Name: "", + CIDR: "10.0.0.0/16", + }, + } tests := []struct { - name string - c *OCICluster - expectErr bool + name string + c *OCICluster + errorMgsShouldContain string + expectErr bool }{ + { + name: "shouldn't allow spaces in region", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + Region: "us city 1", + }, + }, + errorMgsShouldContain: "region", + expectErr: true, + }, { name: "shouldn't allow bad CompartmentId", c: &OCICluster{ @@ -42,7 +89,8 @@ func TestOCICluster_ValidateCreate(t *testing.T) { CompartmentId: "badocid", }, }, - expectErr: true, + errorMgsShouldContain: "compartmentId", + expectErr: true, }, { name: "shouldn't allow blank CompartmentId", @@ -50,7 +98,23 @@ func TestOCICluster_ValidateCreate(t *testing.T) { ObjectMeta: metav1.ObjectMeta{}, Spec: OCIClusterSpec{}, }, - expectErr: true, + errorMgsShouldContain: "compartmentId", + expectErr: true, + }, + { + name: "shouldn't allow bad vcn cider", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "not-a-cidr", + }, + }, + }, + }, + errorMgsShouldContain: "invalid CIDR format", + expectErr: true, }, { name: "shouldn't allow blank OCIResourceIdentifier", @@ -60,15 +124,89 @@ func TestOCICluster_ValidateCreate(t *testing.T) { CompartmentId: "ocid", }, }, - expectErr: true, + errorMgsShouldContain: "ociResourceIdentifier", + expectErr: true, }, { - name: "should succeed", + name: "shouldn't allow subnet cidr outside of vcn cidr", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: badSubnetCidr, + }, + }, + }, + }, + errorMgsShouldContain: "cidr", + expectErr: true, + }, + { + name: "shouldn't allow subnet bad cidr format", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: badSubnetCidrFormat, + }, + }, + }, + }, + errorMgsShouldContain: "invalid CIDR format", + expectErr: true, + }, + { + name: "shouldn't allow subnet cidr outside of vcn cidr", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: dupSubnetNames, + }, + }, + }, + }, + errorMgsShouldContain: "networkSpec.subnets: Duplicate value", + expectErr: true, + }, + { + name: "shouldn't allow invalid subnet name", c: &OCICluster{ ObjectMeta: metav1.ObjectMeta{}, Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: badSubnetName, + }, + }, + }, + }, + errorMgsShouldContain: "subnet name invalid", + expectErr: true, + }, + { + name: "should succeed", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-name", + }, + Spec: OCIClusterSpec{ + Region: "us-lexington-1", CompartmentId: "ocid", OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: goodSubnets, + }, + }, }, }, expectErr: false, @@ -79,7 +217,9 @@ func TestOCICluster_ValidateCreate(t *testing.T) { g := gomega.NewWithT(t) if test.expectErr { - g.Expect(test.c.ValidateCreate()).NotTo(gomega.Succeed()) + err := test.c.ValidateCreate() + g.Expect(err).NotTo(gomega.Succeed()) + g.Expect(strings.Contains(err.Error(), test.errorMgsShouldContain)).To(gomega.BeTrue()) } else { g.Expect(test.c.ValidateCreate()).To(gomega.Succeed()) } @@ -88,11 +228,19 @@ func TestOCICluster_ValidateCreate(t *testing.T) { } func TestOCICluster_ValidateUpdate(t *testing.T) { + goodSubnets := []*Subnet{ + &Subnet{ + Name: "test-subnet", + CIDR: "10.0.0.0/16", + }, + } + tests := []struct { - name string - c *OCICluster - old *OCICluster - expectErr bool + name string + c *OCICluster + old *OCICluster + errorField string + expectErr bool }{ { name: "shouldn't region change", @@ -108,7 +256,8 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { Region: "old-region", }, }, - expectErr: true, + errorField: "region", + expectErr: true, }, { name: "shouldn't change OCIResourceIdentifier", @@ -127,23 +276,40 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { OCIResourceIdentifier: "uuid-2", }, }, - expectErr: true, + errorField: "ociResourceIdentifier", + expectErr: true, }, { name: "should succeed", c: &OCICluster{ - ObjectMeta: metav1.ObjectMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, Spec: OCIClusterSpec{ CompartmentId: "ocid", Region: "old-region", OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: goodSubnets, + }, + }, }, }, old: &OCICluster{ - ObjectMeta: metav1.ObjectMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-test", + }, Spec: OCIClusterSpec{ Region: "old-region", OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: goodSubnets, + }, + }, }, }, expectErr: false, @@ -155,7 +321,10 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { g := gomega.NewWithT(t) if test.expectErr { - g.Expect(test.c.ValidateUpdate(test.old)).NotTo(gomega.Succeed()) + //g.Expect(test.c.ValidateUpdate(test.old)).NotTo(gomega.Succeed()) + err := test.c.ValidateUpdate(test.old) + g.Expect(err).NotTo(gomega.Succeed()) + g.Expect(strings.Contains(err.Error(), test.errorField)).To(gomega.BeTrue()) } else { g.Expect(test.c.ValidateUpdate(test.old)).To(gomega.Succeed()) } diff --git a/api/v1beta1/validator.go b/api/v1beta1/validator.go index f830cd131..c3387aecf 100644 --- a/api/v1beta1/validator.go +++ b/api/v1beta1/validator.go @@ -19,6 +19,23 @@ package v1beta1 +import ( + "fmt" + "net" + "regexp" + + "k8s.io/apimachinery/pkg/util/validation/field" +) + +const ( + // max length of 150 to allow for cluster name to be used as a prefix for VMs and other resources that + // have limitations of max display name Max Length: 255 + // example compute: https://docs.oracle.com/en-us/iaas/api/#/en/iaas/20160918/LaunchInstanceDetails + clusterNameMaxLength = 150 +) + +var blankRegex = regexp.MustCompile("\\s") + // validOcid is a simple pre-flight // we will let the serverside handle the more complex and compete validation func validOcid(ocid string) bool { @@ -30,7 +47,121 @@ func validOcid(ocid string) bool { } // validShape is a simple pre-flight -// we will let the serverside handle the more complex and compete validation +// we will let the serverside handle the more complex and compete validation. func validShape(shape string) bool { return len(shape) > 0 } + +// validRegion test if the string can be a region. +func validRegion(stringRegion string) bool { + if blankRegex.MatchString(stringRegion) || stringRegion == "" { + return false + } + return true +} + +// validateClusterName validates ClusterName. +func (c *OCICluster) validateClusterName() field.ErrorList { + var allErrs field.ErrorList + if len(c.Name) > clusterNameMaxLength { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), c.Name, + fmt.Sprintf("Cluster Name longer than allowed length of %d characters", clusterNameMaxLength))) + } + + if blankRegex.MatchString(c.Name) { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), c.Name, + fmt.Sprintf("cluster name can not contain spaces"))) + } + + if c.Name == "" { + allErrs = append(allErrs, field.Invalid(field.NewPath("metadata").Child("name"), c.Name, + fmt.Sprintf("cluster name can not be empty"))) + } + + if len(allErrs) == 0 { + return nil + } + return allErrs +} + +// validateNetworkSpec validates the NetworkSpec +func validateNetworkSpec(networkSpec NetworkSpec, old NetworkSpec, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + if len(networkSpec.Vcn.CIDR) > 0 { + allErrs = append(allErrs, validateVCNCIDR(networkSpec.Vcn.CIDR, fldPath.Child("cidr"))...) + } + + if networkSpec.Vcn.Subnets != nil { + allErrs = append(allErrs, validateSubnets(networkSpec.Vcn.Subnets, networkSpec.Vcn, fldPath.Child("subnets"))...) + } + + if len(allErrs) == 0 { + return nil + } + return allErrs +} + +// validateVCNCIDR validates the CIDR of a VNC. +func validateVCNCIDR(vncCIDR string, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + if _, _, err := net.ParseCIDR(vncCIDR); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, vncCIDR, "invalid CIDR format")) + } + return allErrs +} + +// validateSubnetCIDR validates the CIDR blocks of a Subnet. +func validateSubnetCIDR(subnetCidr string, vcnCidr string, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + var vcnNetwork *net.IPNet + + if _, parseNetwork, err := net.ParseCIDR(vcnCidr); err == nil { + vcnNetwork = parseNetwork + } + + subnetCidrIP, _, err := net.ParseCIDR(subnetCidr) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, subnetCidr, "invalid CIDR format")) + } + + var found bool + if vcnNetwork != nil && vcnNetwork.Contains(subnetCidrIP) { + found = true + } + + if !found { + allErrs = append(allErrs, field.Invalid(fldPath, subnetCidr, fmt.Sprintf("subnet CIDR not in VCN address space: %s", vcnCidr))) + } + + return allErrs +} + +// validateSubnets validates a list of Subnets. +func validateSubnets(subnets []*Subnet, vcn VCN, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + subnetNames := make(map[string]bool, len(subnets)) + + for i, subnet := range subnets { + if err := validateSubnetName(subnet.Name, fldPath.Index(i).Child("name")); err != nil { + allErrs = append(allErrs, err) + } + if _, ok := subnetNames[subnet.Name]; ok { + allErrs = append(allErrs, field.Duplicate(fldPath, subnet.Name)) + } + subnetNames[subnet.Name] = true + + allErrs = append(allErrs, validateSubnetCIDR(subnet.CIDR, vcn.CIDR, fldPath.Index(i).Child("cidr"))...) + } + + return allErrs +} + +// validateSubnetName validates the Name of a Subnet. +func validateSubnetName(name string, fldPath *field.Path) *field.Error { + if blankRegex.Match([]byte(name)) || name == "" { + return field.Invalid(fldPath, name, + fmt.Sprintf("subnet name invalid")) + } + return nil +}