From 096f2e048ed00ac2ab13dbae4c38fe9d2467cfb4 Mon Sep 17 00:00:00 2001 From: Joe Kratzat Date: Mon, 2 May 2022 08:50:51 -0400 Subject: [PATCH] feat: update OCICluster webhook validations (#77) --- api/v1beta1/ocicluster_webhook.go | 23 +- api/v1beta1/ocicluster_webhook_test.go | 327 +++++++++++++++++++++++-- api/v1beta1/types.go | 3 + api/v1beta1/validator.go | 182 +++++++++++++- cloud/ociutil/ociutil.go | 9 + 5 files changed, 522 insertions(+), 22 deletions(-) diff --git a/api/v1beta1/ocicluster_webhook.go b/api/v1beta1/ocicluster_webhook.go index 5fc466d6..dfa3bf1c 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,11 @@ 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()...) + if c.Spec.CompartmentId != oldCluster.Spec.CompartmentId { + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "compartmentId"), c.Spec.CompartmentId, "field is immutable")) + } + + allErrs = append(allErrs, c.validate(oldCluster)...) if len(allErrs) == 0 { return nil @@ -102,9 +106,16 @@ 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, validateNetworkSpec(c.Spec.NetworkSpec, oldNetworkSpec, field.NewPath("spec").Child("networkSpec"))...) + if len(c.Spec.CompartmentId) <= 0 { allErrs = append( allErrs, @@ -125,6 +136,12 @@ func (c *OCICluster) validate() field.ErrorList { field.Invalid(field.NewPath("spec", "ociResourceIdentifier"), c.Spec.OCIResourceIdentifier, "field is required")) } + if !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 db23c23f..a138715c 100644 --- a/api/v1beta1/ocicluster_webhook_test.go +++ b/api/v1beta1/ocicluster_webhook_test.go @@ -20,20 +20,74 @@ package v1beta1 import ( + "strings" "testing" "github.com/onsi/gomega" . "github.com/onsi/gomega" + "github.com/oracle/oci-go-sdk/v63/common" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func TestOCICluster_ValidateCreate(t *testing.T) { + goodSubnets := []*Subnet{ + &Subnet{ + Role: ControlPlaneRole, + 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", + }, + } + badSubnetRole := []*Subnet{ + &Subnet{ + Role: "not-control-plane", + }, + } 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 +96,8 @@ func TestOCICluster_ValidateCreate(t *testing.T) { CompartmentId: "badocid", }, }, - expectErr: true, + errorMgsShouldContain: "compartmentId", + expectErr: true, }, { name: "shouldn't allow blank CompartmentId", @@ -50,7 +105,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 +131,186 @@ 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 subnet role outside of pre-defined roles", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: badSubnetRole, + }, + }, + }, + }, + errorMgsShouldContain: "subnet role invalid", + 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: "shouldn't allow bad NSG egress cidr", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroups: []*NSG{{ + EgressRules: []EgressSecurityRuleForNSG{{ + EgressSecurityRule: EgressSecurityRule{ + Destination: common.String("bad/15"), + DestinationType: EgressSecurityRuleDestinationTypeCidrBlock, + }, + }}, + }}, + }, + }, + }, + }, + errorMgsShouldContain: "invalid egressRules CIDR format", + expectErr: true, + }, + { + name: "shouldn't allow bad NSG ingress cidr", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroups: []*NSG{{ + IngressRules: []IngressSecurityRuleForNSG{{ + IngressSecurityRule: IngressSecurityRule{ + Source: common.String("bad/15"), + SourceType: IngressSecurityRuleSourceTypeCidrBlock, + }, + }}, + }}, + }, + }, + }, + }, + errorMgsShouldContain: "invalid ingressRule CIDR format", + expectErr: true, + }, + { + name: "shouldn't allow bad NSG role", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + NetworkSecurityGroups: []*NSG{{ + Role: "bad-role", + }}, + }, + }, + }, + }, + errorMgsShouldContain: "networkSecurityGroup role invalid", + expectErr: true, + }, + { + name: "should allow blank region", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-name", + }, + Spec: OCIClusterSpec{ + Region: "", CompartmentId: "ocid", OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: goodSubnets, + }, + }, + }, + }, + expectErr: false, + }, + { + 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 +321,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,14 +332,23 @@ func TestOCICluster_ValidateCreate(t *testing.T) { } func TestOCICluster_ValidateUpdate(t *testing.T) { + goodSubnets := []*Subnet{ + &Subnet{ + Role: ControlPlaneRole, + 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 + errorMgsShouldContain string + expectErr bool }{ { - name: "shouldn't region change", + name: "shouldn't allow region change", c: &OCICluster{ ObjectMeta: metav1.ObjectMeta{}, Spec: OCIClusterSpec{ @@ -108,7 +361,25 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { Region: "old-region", }, }, - expectErr: true, + errorMgsShouldContain: "region", + expectErr: true, + }, + { + name: "shouldn't allow compartmentId change", + c: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + CompartmentId: "ocid.old", + }, + }, + old: &OCICluster{ + ObjectMeta: metav1.ObjectMeta{}, + Spec: OCIClusterSpec{ + CompartmentId: "ocid.new", + }, + }, + errorMgsShouldContain: "compartmentId", + expectErr: true, }, { name: "shouldn't change OCIResourceIdentifier", @@ -127,23 +398,41 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { OCIResourceIdentifier: "uuid-2", }, }, - expectErr: true, + errorMgsShouldContain: "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", + CompartmentId: "ocid", OCIResourceIdentifier: "uuid", + NetworkSpec: NetworkSpec{ + Vcn: VCN{ + CIDR: "10.0.0.0/16", + Subnets: goodSubnets, + }, + }, }, }, expectErr: false, @@ -155,7 +444,9 @@ func TestOCICluster_ValidateUpdate(t *testing.T) { g := gomega.NewWithT(t) if test.expectErr { - 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.errorMgsShouldContain)).To(gomega.BeTrue()) } else { g.Expect(test.c.ValidateUpdate(test.old)).To(gomega.Succeed()) } diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go index 6f75181f..73f5a7f3 100644 --- a/api/v1beta1/types.go +++ b/api/v1beta1/types.go @@ -25,6 +25,9 @@ const ( Public = "public" ) +// SubnetRoles a slice of all the subnet roles +var SubnetRoles = [...]Role{ControlPlaneRole, ControlPlaneEndpointRole, WorkerRole, ServiceLoadBalancerRole} + // NetworkDetails defines the configuration options for the network type NetworkDetails struct { SubnetId *string `json:"subnetId,omitempty"` diff --git a/api/v1beta1/validator.go b/api/v1beta1/validator.go index f830cd13..50126bfb 100644 --- a/api/v1beta1/validator.go +++ b/api/v1beta1/validator.go @@ -19,6 +19,18 @@ package v1beta1 +import ( + "fmt" + "net" + "regexp" + + "github.com/oracle/cluster-api-provider-oci/cloud/ociutil" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// invalidNameRegex is a broad regex used to validate allows names in OCI +var invalidNameRegex = 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 +42,175 @@ 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 { + + // region can be blank since the regional information + // can be derived from other sources + if stringRegion == "" { + return true + } + + if invalidNameRegex.MatchString(stringRegion) { + return false + } + return true +} + +// 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 networkSpec.Vcn.NetworkSecurityGroups != nil { + allErrs = append(allErrs, validateNSGs(networkSpec.Vcn.NetworkSecurityGroups, networkSpec.Vcn, fldPath.Child("networkSecurityGroups"))...) + } + + 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 + + subnetCidrIP, _, err := net.ParseCIDR(subnetCidr) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, subnetCidr, "invalid CIDR format")) + } + + // Check subnet is in vcnCidr if vcnCidr is set + if len(vcnCidr) > 0 { + var vcnNetwork *net.IPNet + if _, parseNetwork, err := net.ParseCIDR(vcnCidr); err == nil { + vcnNetwork = parseNetwork + } + + 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 +} + +// validateNSGs validates a list of Subnets. +func validateNSGs(networkSecurityGroups []*NSG, vcn VCN, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + for i, nsg := range networkSecurityGroups { + if err := validateRole(nsg.Role, fldPath.Index(i).Child("role"), "networkSecurityGroup role invalid"); err != nil { + allErrs = append(allErrs, err) + } + allErrs = append(allErrs, validateEgressSecurityRuleForNSG(nsg.EgressRules, fldPath.Index(i).Child("egressRules"))...) + allErrs = append(allErrs, validateIngressSecurityRuleForNSG(nsg.IngressRules, fldPath.Index(i).Child("ingressRules"))...) + } + + return allErrs +} + +// validateEgressSecurityRuleForNSG validates the Egress Security Rule of a Subnet. +func validateEgressSecurityRuleForNSG(egressRules []EgressSecurityRuleForNSG, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + for _, r := range egressRules { + rule := r.EgressSecurityRule + + if rule.DestinationType == EgressSecurityRuleDestinationTypeCidrBlock && rule.Destination != nil { + if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Destination)); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Destination, "invalid egressRules CIDR format")) + } + } + } + + return allErrs +} + +// validateEgressSecurityRuleForNSG validates the Egress Security Rule of a Subnet. +func validateIngressSecurityRuleForNSG(egressRules []IngressSecurityRuleForNSG, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + + for _, r := range egressRules { + rule := r.IngressSecurityRule + + if rule.SourceType == IngressSecurityRuleSourceTypeCidrBlock && rule.Source != nil { + if _, _, err := net.ParseCIDR(ociutil.DerefString(rule.Source)); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "invalid ingressRule CIDR format")) + } + } + } + + 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 + + if err := validateRole(subnet.Role, fldPath.Index(i).Child("role"), "subnet role invalid"); err != nil { + allErrs = append(allErrs, err) + } + + 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 invalidNameRegex.Match([]byte(name)) || name == "" { + return field.Invalid(fldPath, name, + fmt.Sprintf("subnet name invalid")) + } + return nil +} + +// validateRole validates that the subnet role is one of the allowed types +func validateRole(subnetRole Role, fldPath *field.Path, errorMsg string) *field.Error { + for _, role := range SubnetRoles { + if subnetRole == role { + return nil + } + } + return field.Invalid(fldPath, subnetRole, + fmt.Sprintf(errorMsg)) +} diff --git a/cloud/ociutil/ociutil.go b/cloud/ociutil/ociutil.go index e0637ab7..4198bf22 100644 --- a/cloud/ociutil/ociutil.go +++ b/cloud/ociutil/ociutil.go @@ -116,3 +116,12 @@ func BuildClusterTags(ClusterResourceUID string) map[string]string { tags[ClusterResourceIdentifier] = ClusterResourceUID return tags } + +// DerefString returns the string value if the pointer isn't nil, otherwise returns empty string +func DerefString(s *string) string { + if s != nil { + return *s + } + + return "" +}