From 8e2fe44391f60e8f7273635d79a9a701443ab243 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Tue, 28 Jan 2020 22:03:36 -0800 Subject: [PATCH] Return more errors at once during Cluster validation --- pkg/apis/kops/validation/cluster.go | 6 +- pkg/apis/kops/validation/legacy.go | 412 +++++++++---------- pkg/client/simple/vfsclientset/cluster.go | 4 +- upup/pkg/fi/cloudup/populate_cluster_spec.go | 8 +- upup/pkg/fi/cloudup/validation_test.go | 20 +- 5 files changed, 215 insertions(+), 235 deletions(-) diff --git a/pkg/apis/kops/validation/cluster.go b/pkg/apis/kops/validation/cluster.go index 5d2e1890b4..a1aed2afaa 100644 --- a/pkg/apis/kops/validation/cluster.go +++ b/pkg/apis/kops/validation/cluster.go @@ -23,11 +23,7 @@ import ( ) func ValidateClusterUpdate(obj *kops.Cluster, status *kops.ClusterStatus, old *kops.Cluster) field.ErrorList { - allErrs := field.ErrorList{} - - if err := ValidateCluster(obj, false); err != nil { - allErrs = append(allErrs, err) - } + allErrs := ValidateCluster(obj, false) // Validate etcd cluster changes { diff --git a/pkg/apis/kops/validation/legacy.go b/pkg/apis/kops/validation/legacy.go index 85969df6bf..4a928145c5 100644 --- a/pkg/apis/kops/validation/legacy.go +++ b/pkg/apis/kops/validation/legacy.go @@ -17,7 +17,6 @@ limitations under the License. package validation import ( - "errors" "fmt" "net" "strings" @@ -37,67 +36,66 @@ import ( // legacy contains validation functions that don't match the apimachinery style // ValidateCluster is responsible for checking the validity of the Cluster spec -func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { +func ValidateCluster(c *kops.Cluster, strict bool) field.ErrorList { fieldSpec := field.NewPath("spec") - var err error + allErrs := field.ErrorList{} // kubernetesRelease is the version with only major & minor fields - var kubernetesRelease semver.Version + kubernetesRelease := semver.Version{Major: 1, Minor: 15} // KubernetesVersion if c.Spec.KubernetesVersion == "" { - return field.Required(fieldSpec.Child("KubernetesVersion"), "") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubernetesVersion"), "")) + } else { + sv, err := util.ParseKubernetesVersion(c.Spec.KubernetesVersion) + if err != nil { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubernetesVersion"), c.Spec.KubernetesVersion, "unable to determine kubernetes version")) + } else { + kubernetesRelease = semver.Version{Major: sv.Major, Minor: sv.Minor} + } } - sv, err := util.ParseKubernetesVersion(c.Spec.KubernetesVersion) - if err != nil { - return field.Invalid(fieldSpec.Child("KubernetesVersion"), c.Spec.KubernetesVersion, "unable to determine kubernetes version") - } - kubernetesRelease = semver.Version{Major: sv.Major, Minor: sv.Minor} - if c.ObjectMeta.Name == "" { - return field.Required(field.NewPath("Name"), "Cluster Name is required (e.g. --name=mycluster.myzone.com)") - } - - { + allErrs = append(allErrs, field.Required(field.NewPath("Name"), "Cluster Name is required (e.g. --name=mycluster.myzone.com)")) + } else { // Must be a dns name errs := validation.IsDNS1123Subdomain(c.ObjectMeta.Name) if len(errs) != 0 { - return field.Invalid(field.NewPath("Name"), c.ObjectMeta.Name, fmt.Sprintf("Cluster Name must be a valid DNS name (e.g. --name=mycluster.myzone.com) errors: %s", strings.Join(errs, ", "))) - } - - if !strings.Contains(c.ObjectMeta.Name, ".") { + allErrs = append(allErrs, field.Invalid(field.NewPath("Name"), c.ObjectMeta.Name, fmt.Sprintf("Cluster Name must be a valid DNS name (e.g. --name=mycluster.myzone.com) errors: %s", strings.Join(errs, ", ")))) + } else if !strings.Contains(c.ObjectMeta.Name, ".") { // Tolerate if this is a cluster we are importing for upgrade if c.ObjectMeta.Annotations[kops.AnnotationNameManagement] != kops.AnnotationValueManagementImported { - return field.Invalid(field.NewPath("Name"), c.ObjectMeta.Name, "Cluster Name must be a fully-qualified DNS name (e.g. --name=mycluster.myzone.com)") + allErrs = append(allErrs, field.Invalid(field.NewPath("Name"), c.ObjectMeta.Name, "Cluster Name must be a fully-qualified DNS name (e.g. --name=mycluster.myzone.com)")) } } } if c.Spec.Assets != nil && c.Spec.Assets.ContainerProxy != nil && c.Spec.Assets.ContainerRegistry != nil { - return field.Forbidden(fieldSpec.Child("Assets", "ContainerProxy"), "ContainerProxy cannot be used in conjunction with ContainerRegistry as represent mutually exclusive concepts. Please consult the documentation for details.") - } - - if c.Spec.CloudProvider == "" { - return field.Required(fieldSpec.Child("CloudProvider"), "") + allErrs = append(allErrs, field.Forbidden(fieldSpec.Child("Assets", "ContainerProxy"), "ContainerProxy cannot be used in conjunction with ContainerRegistry as represent mutually exclusive concepts. Please consult the documentation for details.")) } requiresSubnets := true requiresNetworkCIDR := true requiresSubnetCIDR := true switch kops.CloudProviderID(c.Spec.CloudProvider) { + case "": + allErrs = append(allErrs, field.Required(fieldSpec.Child("CloudProvider"), "")) + requiresSubnets = false + requiresSubnetCIDR = false + requiresNetworkCIDR = false + case kops.CloudProviderBareMetal: requiresSubnets = false requiresSubnetCIDR = false requiresNetworkCIDR = false if c.Spec.NetworkCIDR != "" { - return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on bare metal")) } case kops.CloudProviderGCE: requiresNetworkCIDR = false if c.Spec.NetworkCIDR != "" { - return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on GCE") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on GCE")) } requiresSubnetCIDR = false @@ -106,7 +104,7 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { requiresSubnetCIDR = false requiresNetworkCIDR = false if c.Spec.NetworkCIDR != "" { - return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on DigitalOcean") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, "NetworkCIDR should not be set on DigitalOcean")) } case kops.CloudProviderALI: requiresSubnets = false @@ -119,50 +117,51 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { requiresSubnetCIDR = false default: - return field.Invalid(fieldSpec.Child("CloudProvider"), c.Spec.CloudProvider, "CloudProvider not recognized") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("CloudProvider"), c.Spec.CloudProvider, "CloudProvider not recognized")) } if requiresSubnets && len(c.Spec.Subnets) == 0 { // TODO: Auto choose zones from region? - return field.Required(fieldSpec.Child("Subnets"), "must configure at least one Subnet (use --zones)") + allErrs = append(allErrs, field.Required(fieldSpec.Child("Subnets"), "must configure at least one Subnet (use --zones)")) } if strict && c.Spec.Kubelet == nil { - return field.Required(fieldSpec.Child("Kubelet"), "Kubelet not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("Kubelet"), "Kubelet not configured")) } if strict && c.Spec.MasterKubelet == nil { - return field.Required(fieldSpec.Child("MasterKubelet"), "MasterKubelet not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("MasterKubelet"), "MasterKubelet not configured")) } if strict && c.Spec.KubeControllerManager == nil { - return field.Required(fieldSpec.Child("KubeControllerManager"), "KubeControllerManager not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeControllerManager"), "KubeControllerManager not configured")) } if strict && c.Spec.KubeDNS == nil { - return field.Required(fieldSpec.Child("KubeDNS"), "KubeDNS not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeDNS"), "KubeDNS not configured")) } if strict && c.Spec.KubeScheduler == nil { - return field.Required(fieldSpec.Child("KubeScheduler"), "KubeScheduler not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeScheduler"), "KubeScheduler not configured")) } if strict && c.Spec.KubeAPIServer == nil { - return field.Required(fieldSpec.Child("KubeAPIServer"), "KubeAPIServer not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeAPIServer"), "KubeAPIServer not configured")) } if strict && c.Spec.KubeProxy == nil { - return field.Required(fieldSpec.Child("KubeProxy"), "KubeProxy not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeProxy"), "KubeProxy not configured")) } if strict && c.Spec.Docker == nil { - return field.Required(fieldSpec.Child("Docker"), "Docker not configured") + allErrs = append(allErrs, field.Required(fieldSpec.Child("Docker"), "Docker not configured")) } // Check NetworkCIDR var networkCIDR *net.IPNet + var err error { if c.Spec.NetworkCIDR == "" { if requiresNetworkCIDR { - return field.Required(fieldSpec.Child("NetworkCIDR"), "Cluster did not have NetworkCIDR set") + allErrs = append(allErrs, field.Required(fieldSpec.Child("NetworkCIDR"), "Cluster did not have NetworkCIDR set")) } } else { _, networkCIDR, err = net.ParseCIDR(c.Spec.NetworkCIDR) if err != nil { - return field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, fmt.Sprintf("Cluster had an invalid NetworkCIDR")) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NetworkCIDR"), c.Spec.NetworkCIDR, fmt.Sprintf("Cluster had an invalid NetworkCIDR"))) } } } @@ -174,7 +173,7 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { for _, AdditionalNetworkCIDR := range c.Spec.AdditionalNetworkCIDRs { _, IPNetAdditionalNetworkCIDR, err := net.ParseCIDR(AdditionalNetworkCIDR) if err != nil { - return field.Invalid(fieldSpec.Child("AdditionalNetworkCIDRs"), AdditionalNetworkCIDR, fmt.Sprintf("Cluster had an invalid AdditionalNetworkCIDRs")) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("AdditionalNetworkCIDRs"), AdditionalNetworkCIDR, fmt.Sprintf("Cluster had an invalid AdditionalNetworkCIDRs"))) } additionalNetworkCIDRs = append(additionalNetworkCIDRs, IPNetAdditionalNetworkCIDR) } @@ -195,27 +194,26 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { nonMasqueradeCIDRString := c.Spec.NonMasqueradeCIDR if nonMasqueradeCIDRString == "" { if nonMasqueradeCIDRRequired { - return field.Required(fieldSpec.Child("NonMasqueradeCIDR"), "Cluster did not have NonMasqueradeCIDR set") + allErrs = append(allErrs, field.Required(fieldSpec.Child("NonMasqueradeCIDR"), "Cluster did not have NonMasqueradeCIDR set")) } } else { _, nonMasqueradeCIDR, err = net.ParseCIDR(nonMasqueradeCIDRString) if err != nil { - return field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "Cluster had an invalid NonMasqueradeCIDR") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "Cluster had an invalid NonMasqueradeCIDR")) } if networkCIDR != nil && subnet.Overlap(nonMasqueradeCIDR, networkCIDR) && c.Spec.Networking != nil && c.Spec.Networking.AmazonVPC == nil && c.Spec.Networking.LyftVPC == nil { - - return field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, fmt.Sprintf("NonMasqueradeCIDR %q cannot overlap with NetworkCIDR %q", nonMasqueradeCIDRString, c.Spec.NetworkCIDR)) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, fmt.Sprintf("NonMasqueradeCIDR %q cannot overlap with NetworkCIDR %q", nonMasqueradeCIDRString, c.Spec.NetworkCIDR))) } if c.Spec.Kubelet != nil && c.Spec.Kubelet.NonMasqueradeCIDR != nonMasqueradeCIDRString { if strict || c.Spec.Kubelet.NonMasqueradeCIDR != "" { - return field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "Kubelet NonMasqueradeCIDR did not match cluster NonMasqueradeCIDR") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "Kubelet NonMasqueradeCIDR did not match cluster NonMasqueradeCIDR")) } } if c.Spec.MasterKubelet != nil && c.Spec.MasterKubelet.NonMasqueradeCIDR != nonMasqueradeCIDRString { if strict || c.Spec.MasterKubelet.NonMasqueradeCIDR != "" { - return field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "MasterKubelet NonMasqueradeCIDR did not match cluster NonMasqueradeCIDR") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("NonMasqueradeCIDR"), nonMasqueradeCIDRString, "MasterKubelet NonMasqueradeCIDR did not match cluster NonMasqueradeCIDR")) } } } @@ -227,21 +225,21 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { serviceClusterIPRangeString := c.Spec.ServiceClusterIPRange if serviceClusterIPRangeString == "" { if strict { - return field.Required(fieldSpec.Child("ServiceClusterIPRange"), "Cluster did not have ServiceClusterIPRange set") + allErrs = append(allErrs, field.Required(fieldSpec.Child("ServiceClusterIPRange"), "Cluster did not have ServiceClusterIPRange set")) } } else { _, serviceClusterIPRange, err = net.ParseCIDR(serviceClusterIPRangeString) if err != nil { - return field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, "Cluster had an invalid ServiceClusterIPRange") - } + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, "Cluster had an invalid ServiceClusterIPRange")) + } else { + if nonMasqueradeCIDR != nil && serviceClusterMustBeSubnetOfNonMasqueradeCIDR && !subnet.BelongsTo(nonMasqueradeCIDR, serviceClusterIPRange) { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, fmt.Sprintf("ServiceClusterIPRange %q must be a subnet of NonMasqueradeCIDR %q", serviceClusterIPRangeString, c.Spec.NonMasqueradeCIDR))) + } - if nonMasqueradeCIDR != nil && serviceClusterMustBeSubnetOfNonMasqueradeCIDR && !subnet.BelongsTo(nonMasqueradeCIDR, serviceClusterIPRange) { - return field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, fmt.Sprintf("ServiceClusterIPRange %q must be a subnet of NonMasqueradeCIDR %q", serviceClusterIPRangeString, c.Spec.NonMasqueradeCIDR)) - } - - if c.Spec.KubeAPIServer != nil && c.Spec.KubeAPIServer.ServiceClusterIPRange != serviceClusterIPRangeString { - if strict || c.Spec.KubeAPIServer.ServiceClusterIPRange != "" { - return field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, "KubeAPIServer ServiceClusterIPRange did not match cluster ServiceClusterIPRange") + if c.Spec.KubeAPIServer != nil && c.Spec.KubeAPIServer.ServiceClusterIPRange != serviceClusterIPRangeString { + if strict || c.Spec.KubeAPIServer.ServiceClusterIPRange != "" { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("ServiceClusterIPRange"), serviceClusterIPRangeString, "KubeAPIServer ServiceClusterIPRange did not match cluster ServiceClusterIPRange")) + } } } } @@ -253,21 +251,21 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { switch action { case "", "ACCEPT", "DROP", "RETURN": default: - return field.Invalid(fieldSpec.Child("Networking", "Canal", "DefaultEndpointToHostAction"), action, fmt.Sprintf("Unsupported value: %s, supports 'ACCEPT', 'DROP' or 'RETURN'", action)) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Networking", "Canal", "DefaultEndpointToHostAction"), action, fmt.Sprintf("Unsupported value: %s, supports 'ACCEPT', 'DROP' or 'RETURN'", action))) } chainInsertMode := c.Spec.Networking.Canal.ChainInsertMode switch chainInsertMode { case "", "insert", "append": default: - return field.Invalid(fieldSpec.Child("Networking", "Canal", "ChainInsertMode"), chainInsertMode, fmt.Sprintf("Unsupported value: %s, supports 'insert' or 'append'", chainInsertMode)) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Networking", "Canal", "ChainInsertMode"), chainInsertMode, fmt.Sprintf("Unsupported value: %s, supports 'insert' or 'append'", chainInsertMode))) } logSeveritySys := c.Spec.Networking.Canal.LogSeveritySys switch logSeveritySys { case "", "INFO", "DEBUG", "WARNING", "ERROR", "CRITICAL", "NONE": default: - return field.Invalid(fieldSpec.Child("Networking", "Canal", "LogSeveritySys"), logSeveritySys, fmt.Sprintf("Unsupported value: %s, supports 'INFO', 'DEBUG', 'WARNING', 'ERROR', 'CRITICAL' or 'NONE'", logSeveritySys)) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Networking", "Canal", "LogSeveritySys"), logSeveritySys, fmt.Sprintf("Unsupported value: %s, supports 'INFO', 'DEBUG', 'WARNING', 'ERROR', 'CRITICAL' or 'NONE'", logSeveritySys))) } } @@ -278,11 +276,9 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { if clusterCIDRString != "" { _, clusterCIDR, err = net.ParseCIDR(clusterCIDRString) if err != nil { - return field.Invalid(fieldSpec.Child("KubeControllerManager", "ClusterCIDR"), clusterCIDRString, "Cluster had an invalid KubeControllerManager.ClusterCIDR") - } - - if nonMasqueradeCIDR != nil && !subnet.BelongsTo(nonMasqueradeCIDR, clusterCIDR) { - return field.Invalid(fieldSpec.Child("KubeControllerManager", "ClusterCIDR"), clusterCIDRString, fmt.Sprintf("KubeControllerManager.ClusterCIDR %q must be a subnet of NonMasqueradeCIDR %q", clusterCIDRString, c.Spec.NonMasqueradeCIDR)) + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubeControllerManager", "ClusterCIDR"), clusterCIDRString, "Cluster had an invalid KubeControllerManager.ClusterCIDR")) + } else if nonMasqueradeCIDR != nil && !subnet.BelongsTo(nonMasqueradeCIDR, clusterCIDR) { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubeControllerManager", "ClusterCIDR"), clusterCIDRString, fmt.Sprintf("KubeControllerManager.ClusterCIDR %q must be a subnet of NonMasqueradeCIDR %q", clusterCIDRString, c.Spec.NonMasqueradeCIDR))) } } } @@ -293,17 +289,18 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { address := c.Spec.KubeDNS.ServerIP ip := net.ParseIP(address) if ip == nil { - return field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "Cluster had an invalid kubeDNS.serverIP") - } - if serviceClusterIPRange != nil && !serviceClusterIPRange.Contains(ip) { - return field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, fmt.Sprintf("ServiceClusterIPRange %q must contain the DNS Server IP %q", c.Spec.ServiceClusterIPRange, address)) - } - if !featureflag.ExperimentalClusterDNS.Enabled() { - if c.Spec.Kubelet != nil && c.Spec.Kubelet.ClusterDNS != c.Spec.KubeDNS.ServerIP { - return field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "Kubelet ClusterDNS did not match cluster kubeDNS.serverIP") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "Cluster had an invalid kubeDNS.serverIP")) + } else { + if serviceClusterIPRange != nil && !serviceClusterIPRange.Contains(ip) { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, fmt.Sprintf("ServiceClusterIPRange %q must contain the DNS Server IP %q", c.Spec.ServiceClusterIPRange, address))) } - if c.Spec.MasterKubelet != nil && c.Spec.MasterKubelet.ClusterDNS != c.Spec.KubeDNS.ServerIP { - return field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "MasterKubelet ClusterDNS did not match cluster kubeDNS.serverIP") + if !featureflag.ExperimentalClusterDNS.Enabled() { + if c.Spec.Kubelet != nil && c.Spec.Kubelet.ClusterDNS != c.Spec.KubeDNS.ServerIP { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "Kubelet ClusterDNS did not match cluster kubeDNS.serverIP")) + } + if c.Spec.MasterKubelet != nil && c.Spec.MasterKubelet.ClusterDNS != c.Spec.KubeDNS.ServerIP { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "serverIP"), address, "MasterKubelet ClusterDNS did not match cluster kubeDNS.serverIP")) + } } } } @@ -311,20 +308,18 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { // @check the nameservers are valid for i, x := range c.Spec.KubeDNS.UpstreamNameservers { if ip := net.ParseIP(x); ip == nil { - return field.Invalid(fieldSpec.Child("kubeDNS", "upstreamNameservers").Index(i), x, "Invalid nameserver given, should be a valid ip address") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "upstreamNameservers").Index(i), x, "Invalid nameserver given, should be a valid ip address")) } } // @check the stubdomain if any - if c.Spec.KubeDNS.StubDomains != nil { - for domain, nameservers := range c.Spec.KubeDNS.StubDomains { - if len(nameservers) <= 0 { - return field.Invalid(fieldSpec.Child("kubeDNS", "stubDomains").Key(domain), domain, "No nameservers specified for the stub domain") - } - for i, x := range nameservers { - if ip := net.ParseIP(x); ip == nil { - return field.Invalid(fieldSpec.Child("kubeDNS", "stubDomains").Key(domain).Index(i), x, "Invalid nameserver given, should be a valid ip address") - } + for domain, nameservers := range c.Spec.KubeDNS.StubDomains { + if len(nameservers) <= 0 { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "stubDomains").Key(domain), domain, "No nameservers specified for the stub domain")) + } + for i, x := range nameservers { + if ip := net.ParseIP(x); ip == nil { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("kubeDNS", "stubDomains").Key(domain).Index(i), x, "Invalid nameserver given, should be a valid ip address")) } } } @@ -350,28 +345,31 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { case kops.CloudProviderALI: k8sCloudProvider = "alicloud" default: - return field.Invalid(fieldSpec.Child("CloudProvider"), c.Spec.CloudProvider, "unknown cloudprovider") + // We already added an error above + k8sCloudProvider = "ignore" } - if c.Spec.Kubelet != nil && (strict || c.Spec.Kubelet.CloudProvider != "") { - if c.Spec.Kubelet.CloudProvider != "external" && k8sCloudProvider != c.Spec.Kubelet.CloudProvider { - return field.Invalid(fieldSpec.Child("Kubelet", "CloudProvider"), c.Spec.Kubelet.CloudProvider, "Did not match cluster CloudProvider") + if k8sCloudProvider != "ignore" { + if c.Spec.Kubelet != nil && (strict || c.Spec.Kubelet.CloudProvider != "") { + if c.Spec.Kubelet.CloudProvider != "external" && k8sCloudProvider != c.Spec.Kubelet.CloudProvider { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Kubelet", "CloudProvider"), c.Spec.Kubelet.CloudProvider, "Did not match cluster CloudProvider")) + } } - } - if c.Spec.MasterKubelet != nil && (strict || c.Spec.MasterKubelet.CloudProvider != "") { - if c.Spec.MasterKubelet.CloudProvider != "external" && k8sCloudProvider != c.Spec.MasterKubelet.CloudProvider { - return field.Invalid(fieldSpec.Child("MasterKubelet", "CloudProvider"), c.Spec.MasterKubelet.CloudProvider, "Did not match cluster CloudProvider") + if c.Spec.MasterKubelet != nil && (strict || c.Spec.MasterKubelet.CloudProvider != "") { + if c.Spec.MasterKubelet.CloudProvider != "external" && k8sCloudProvider != c.Spec.MasterKubelet.CloudProvider { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("MasterKubelet", "CloudProvider"), c.Spec.MasterKubelet.CloudProvider, "Did not match cluster CloudProvider")) + } } - } - if c.Spec.KubeAPIServer != nil && (strict || c.Spec.KubeAPIServer.CloudProvider != "") { - if c.Spec.KubeAPIServer.CloudProvider != "external" && k8sCloudProvider != c.Spec.KubeAPIServer.CloudProvider { - return field.Invalid(fieldSpec.Child("KubeAPIServer", "CloudProvider"), c.Spec.KubeAPIServer.CloudProvider, "Did not match cluster CloudProvider") + if c.Spec.KubeAPIServer != nil && (strict || c.Spec.KubeAPIServer.CloudProvider != "") { + if c.Spec.KubeAPIServer.CloudProvider != "external" && k8sCloudProvider != c.Spec.KubeAPIServer.CloudProvider { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubeAPIServer", "CloudProvider"), c.Spec.KubeAPIServer.CloudProvider, "Did not match cluster CloudProvider")) + } } - } - if c.Spec.KubeControllerManager != nil && (strict || c.Spec.KubeControllerManager.CloudProvider != "") { - if c.Spec.KubeControllerManager.CloudProvider != "external" && k8sCloudProvider != c.Spec.KubeControllerManager.CloudProvider { - return field.Invalid(fieldSpec.Child("KubeControllerManager", "CloudProvider"), c.Spec.KubeControllerManager.CloudProvider, "Did not match cluster CloudProvider") + if c.Spec.KubeControllerManager != nil && (strict || c.Spec.KubeControllerManager.CloudProvider != "") { + if c.Spec.KubeControllerManager.CloudProvider != "external" && k8sCloudProvider != c.Spec.KubeControllerManager.CloudProvider { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubeControllerManager", "CloudProvider"), c.Spec.KubeControllerManager.CloudProvider, "Did not match cluster CloudProvider")) + } } } } @@ -382,16 +380,14 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { fieldSubnet := fieldSpec.Child("Subnets").Index(i) if s.CIDR == "" { if requiresSubnetCIDR && strict { - return field.Required(fieldSubnet.Child("CIDR"), "Subnet did not have a CIDR set") + allErrs = append(allErrs, field.Required(fieldSubnet.Child("CIDR"), "Subnet did not have a CIDR set")) } } else { _, subnetCIDR, err := net.ParseCIDR(s.CIDR) if err != nil { - return field.Invalid(fieldSubnet.Child("CIDR"), s.CIDR, "Subnet had an invalid CIDR") - } - - if networkCIDR != nil && !validateSubnetCIDR(networkCIDR, additionalNetworkCIDRs, subnetCIDR) { - return field.Invalid(fieldSubnet.Child("CIDR"), s.CIDR, fmt.Sprintf("Subnet %q had a CIDR %q that was not a subnet of the NetworkCIDR %q", s.Name, s.CIDR, c.Spec.NetworkCIDR)) + allErrs = append(allErrs, field.Invalid(fieldSubnet.Child("CIDR"), s.CIDR, "Subnet had an invalid CIDR")) + } else if networkCIDR != nil && !validateSubnetCIDR(networkCIDR, additionalNetworkCIDRs, subnetCIDR) { + allErrs = append(allErrs, field.Invalid(fieldSubnet.Child("CIDR"), s.CIDR, fmt.Sprintf("Subnet %q had a CIDR %q that was not a subnet of the NetworkCIDR %q", s.Name, s.CIDR, c.Spec.NetworkCIDR))) } } } @@ -401,34 +397,31 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { if c.Spec.NodeAuthorization != nil { // @check the feature gate is enabled for this if !featureflag.EnableNodeAuthorization.Enabled() { - return field.Invalid(field.NewPath("nodeAuthorization"), nil, "node authorization is experimental feature; set `export KOPS_FEATURE_FLAGS=EnableNodeAuthorization`") - } - if c.Spec.NodeAuthorization.NodeAuthorizer == nil { - return field.Invalid(field.NewPath("nodeAuthorization"), nil, "no node authorization policy has been set") - } - // NodeAuthorizer - if c.Spec.NodeAuthorization.NodeAuthorizer != nil { - path := field.NewPath("nodeAuthorization").Child("nodeAuthorizer") - if c.Spec.NodeAuthorization.NodeAuthorizer.Port < 0 || c.Spec.NodeAuthorization.NodeAuthorizer.Port >= 65535 { - return field.Invalid(path.Child("port"), c.Spec.NodeAuthorization.NodeAuthorizer.Port, "invalid port") - } - if c.Spec.NodeAuthorization.NodeAuthorizer.Timeout != nil && c.Spec.NodeAuthorization.NodeAuthorizer.Timeout.Duration <= 0 { - return field.Invalid(path.Child("timeout"), c.Spec.NodeAuthorization.NodeAuthorizer.Timeout, "must be greater than zero") - } - if c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL != nil && c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL.Duration < 0 { - return field.Invalid(path.Child("tokenTTL"), c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL, "must be greater than or equal to zero") - } + allErrs = append(allErrs, field.Invalid(field.NewPath("nodeAuthorization"), nil, "node authorization is experimental feature; set `export KOPS_FEATURE_FLAGS=EnableNodeAuthorization`")) + } else { + if c.Spec.NodeAuthorization.NodeAuthorizer == nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("nodeAuthorization"), nil, "no node authorization policy has been set")) + } else { + path := field.NewPath("nodeAuthorization").Child("nodeAuthorizer") + if c.Spec.NodeAuthorization.NodeAuthorizer.Port < 0 || c.Spec.NodeAuthorization.NodeAuthorizer.Port >= 65535 { + allErrs = append(allErrs, field.Invalid(path.Child("port"), c.Spec.NodeAuthorization.NodeAuthorizer.Port, "invalid port")) + } + if c.Spec.NodeAuthorization.NodeAuthorizer.Timeout != nil && c.Spec.NodeAuthorization.NodeAuthorizer.Timeout.Duration <= 0 { + allErrs = append(allErrs, field.Invalid(path.Child("timeout"), c.Spec.NodeAuthorization.NodeAuthorizer.Timeout, "must be greater than zero")) + } + if c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL != nil && c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL.Duration < 0 { + allErrs = append(allErrs, field.Invalid(path.Child("tokenTTL"), c.Spec.NodeAuthorization.NodeAuthorizer.TokenTTL, "must be greater than or equal to zero")) + } - // @question: we could probably just default these settings in the model when the node-authorizer is enabled?? - if c.Spec.KubeAPIServer == nil { - return field.Invalid(field.NewPath("kubeAPIServer"), c.Spec.KubeAPIServer, "bootstrap token authentication is not enabled in the kube-apiserver") - } - if c.Spec.KubeAPIServer.EnableBootstrapAuthToken == nil { - return field.Invalid(field.NewPath("kubeAPIServer").Child("enableBootstrapAuthToken"), nil, "kube-apiserver has not been configured to use bootstrap tokens") - } - if !fi.BoolValue(c.Spec.KubeAPIServer.EnableBootstrapAuthToken) { - return field.Invalid(field.NewPath("kubeAPIServer").Child("enableBootstrapAuthToken"), - c.Spec.KubeAPIServer.EnableBootstrapAuthToken, "bootstrap tokens in the kube-apiserver has been disabled") + // @question: we could probably just default these settings in the model when the node-authorizer is enabled?? + if c.Spec.KubeAPIServer == nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("kubeAPIServer"), c.Spec.KubeAPIServer, "bootstrap token authentication is not enabled in the kube-apiserver")) + } else if c.Spec.KubeAPIServer.EnableBootstrapAuthToken == nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("kubeAPIServer").Child("enableBootstrapAuthToken"), nil, "kube-apiserver has not been configured to use bootstrap tokens")) + } else if !fi.BoolValue(c.Spec.KubeAPIServer.EnableBootstrapAuthToken) { + allErrs = append(allErrs, field.Invalid(field.NewPath("kubeAPIServer").Child("enableBootstrapAuthToken"), + c.Spec.KubeAPIServer.EnableBootstrapAuthToken, "bootstrap tokens in the kube-apiserver has been disabled")) + } } } } @@ -439,7 +432,7 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { case kops.UpdatePolicyExternal: // Valid default: - return field.Invalid(fieldSpec.Child("UpdatePolicy"), *c.Spec.UpdatePolicy, "unrecognized value for UpdatePolicy") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("UpdatePolicy"), *c.Spec.UpdatePolicy, "unrecognized value for UpdatePolicy")) } } @@ -450,12 +443,12 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { for i, x := range c.Spec.KubeProxy.IPVSExcludeCIDRS { if _, _, err := net.ParseCIDR(x); err != nil { - return field.Invalid(kubeProxyPath.Child("ipvsExcludeCIDRS").Index(i), x, "Invalid network CIDR") + allErrs = append(allErrs, field.Invalid(kubeProxyPath.Child("ipvsExcludeCIDRS").Index(i), x, "Invalid network CIDR")) } } if master != "" && !isValidAPIServersURL(master) { - return field.Invalid(kubeProxyPath.Child("Master"), master, "Not a valid APIServer URL") + allErrs = append(allErrs, field.Invalid(kubeProxyPath.Child("Master"), master, "Not a valid APIServer URL")) } } @@ -464,9 +457,9 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { if kubernetesRelease.GTE(semver.MustParse("1.10.0")) { if len(c.Spec.KubeAPIServer.AdmissionControl) > 0 { if len(c.Spec.KubeAPIServer.DisableAdmissionPlugins) > 0 { - return field.Invalid(fieldSpec.Child("KubeAPIServer").Child("DisableAdmissionPlugins"), + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("KubeAPIServer").Child("DisableAdmissionPlugins"), strings.Join(c.Spec.KubeAPIServer.DisableAdmissionPlugins, ","), - "DisableAdmissionPlugins is mutually exclusive, you cannot use both AdmissionControl and DisableAdmissionPlugins together") + "DisableAdmissionPlugins is mutually exclusive, you cannot use both AdmissionControl and DisableAdmissionPlugins together")) } } } @@ -479,31 +472,31 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { { // Flag removed in 1.6 if c.Spec.Kubelet.APIServers != "" { - return field.Invalid( + allErrs = append(allErrs, field.Invalid( kubeletPath.Child("APIServers"), c.Spec.Kubelet.APIServers, - "api-servers flag was removed in 1.6") + "api-servers flag was removed in 1.6")) } } if kubernetesRelease.GTE(semver.MustParse("1.10.0")) { // Flag removed in 1.10 if c.Spec.Kubelet.RequireKubeconfig != nil { - return field.Invalid( + allErrs = append(allErrs, field.Invalid( kubeletPath.Child("requireKubeconfig"), *c.Spec.Kubelet.RequireKubeconfig, - "require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)") + "require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)")) } } if c.Spec.Kubelet.BootstrapKubeconfig != "" { if c.Spec.KubeAPIServer == nil { - return field.Required(fieldSpec.Child("KubeAPIServer"), "bootstrap token require the NodeRestriction admissions controller") + allErrs = append(allErrs, field.Required(fieldSpec.Child("KubeAPIServer"), "bootstrap token require the NodeRestriction admissions controller")) } } if c.Spec.Kubelet.APIServers != "" && !isValidAPIServersURL(c.Spec.Kubelet.APIServers) { - return field.Invalid(kubeletPath.Child("APIServers"), c.Spec.Kubelet.APIServers, "Not a valid APIServer URL") + allErrs = append(allErrs, field.Invalid(kubeletPath.Child("APIServers"), c.Spec.Kubelet.APIServers, "Not a valid APIServer URL")) } } @@ -514,25 +507,25 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { { // Flag removed in 1.6 if c.Spec.MasterKubelet.APIServers != "" { - return field.Invalid( + allErrs = append(allErrs, field.Invalid( masterKubeletPath.Child("APIServers"), c.Spec.MasterKubelet.APIServers, - "api-servers flag was removed in 1.6") + "api-servers flag was removed in 1.6")) } } if kubernetesRelease.GTE(semver.MustParse("1.10.0")) { // Flag removed in 1.10 if c.Spec.MasterKubelet.RequireKubeconfig != nil { - return field.Invalid( + allErrs = append(allErrs, field.Invalid( masterKubeletPath.Child("requireKubeconfig"), *c.Spec.MasterKubelet.RequireKubeconfig, - "require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)") + "require-kubeconfig flag was removed in 1.10. (Please be sure you are not using a cluster config from `kops get cluster --full`)")) } } if c.Spec.MasterKubelet.APIServers != "" && !isValidAPIServersURL(c.Spec.MasterKubelet.APIServers) { - return field.Invalid(masterKubeletPath.Child("APIServers"), c.Spec.MasterKubelet.APIServers, "Not a valid APIServer URL") + allErrs = append(allErrs, field.Invalid(masterKubeletPath.Child("APIServers"), c.Spec.MasterKubelet.APIServers, "Not a valid APIServer URL")) } } @@ -540,24 +533,25 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { if c.Spec.Topology != nil { if c.Spec.Topology.Masters != "" && c.Spec.Topology.Nodes != "" { if c.Spec.Topology.Masters != kops.TopologyPublic && c.Spec.Topology.Masters != kops.TopologyPrivate { - return field.Invalid(fieldSpec.Child("Topology", "Masters"), c.Spec.Topology.Masters, "Invalid Masters value for Topology") - } else if c.Spec.Topology.Nodes != kops.TopologyPublic && c.Spec.Topology.Nodes != kops.TopologyPrivate { - return field.Invalid(fieldSpec.Child("Topology", "Nodes"), c.Spec.Topology.Nodes, "Invalid Nodes value for Topology") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Topology", "Masters"), c.Spec.Topology.Masters, "Invalid Masters value for Topology")) + } + if c.Spec.Topology.Nodes != kops.TopologyPublic && c.Spec.Topology.Nodes != kops.TopologyPrivate { + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Topology", "Nodes"), c.Spec.Topology.Nodes, "Invalid Nodes value for Topology")) } } else { - return field.Required(fieldSpec.Child("Masters"), "Topology requires non-nil values for Masters and Nodes") + allErrs = append(allErrs, field.Required(fieldSpec.Child("Masters"), "Topology requires non-nil values for Masters and Nodes")) } if c.Spec.Topology.Bastion != nil { bastion := c.Spec.Topology.Bastion if c.Spec.Topology.Masters == kops.TopologyPublic || c.Spec.Topology.Nodes == kops.TopologyPublic { - return field.Invalid(fieldSpec.Child("Topology", "Masters"), c.Spec.Topology.Masters, "Bastion supports only Private Masters and Nodes") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Topology", "Masters"), c.Spec.Topology.Masters, "Bastion supports only Private Masters and Nodes")) } if bastion.IdleTimeoutSeconds != nil && *bastion.IdleTimeoutSeconds <= 0 { - return field.Invalid(fieldSpec.Child("Topology", "Bastion", "IdleTimeoutSeconds"), *bastion.IdleTimeoutSeconds, "Bastion IdleTimeoutSeconds should be greater than zero") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Topology", "Bastion", "IdleTimeoutSeconds"), *bastion.IdleTimeoutSeconds, "Bastion IdleTimeoutSeconds should be greater than zero")) } if bastion.IdleTimeoutSeconds != nil && *bastion.IdleTimeoutSeconds > 3600 { - return field.Invalid(fieldSpec.Child("Topology", "Bastion", "IdleTimeoutSeconds"), *bastion.IdleTimeoutSeconds, "Bastion IdleTimeoutSeconds cannot be greater than one hour") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Topology", "Bastion", "IdleTimeoutSeconds"), *bastion.IdleTimeoutSeconds, "Bastion IdleTimeoutSeconds cannot be greater than one hour")) } } @@ -570,10 +564,10 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { } fieldSubnet := fieldSpec.Child("Subnets").Index(i) if !strings.HasPrefix(s.Egress, "nat-") && !strings.HasPrefix(s.Egress, "i-") && s.Egress != kops.EgressExternal { - return field.Invalid(fieldSubnet.Child("Egress"), s.Egress, "egress must be of type NAT Gateway or NAT EC2 Instance or 'External'") + allErrs = append(allErrs, field.Invalid(fieldSubnet.Child("Egress"), s.Egress, "egress must be of type NAT Gateway or NAT EC2 Instance or 'External'")) } if s.Egress != kops.EgressExternal && s.Type != "Private" { - return field.Invalid(fieldSubnet.Child("Egress"), s.Egress, "egress can only be specified for Private subnets") + allErrs = append(allErrs, field.Invalid(fieldSubnet.Child("Egress"), s.Egress, "egress can only be specified for Private subnets")) } } } @@ -583,37 +577,30 @@ func ValidateCluster(c *kops.Cluster, strict bool) *field.Error { fieldEtcdClusters := fieldSpec.Child("EtcdClusters") if len(c.Spec.EtcdClusters) == 0 { - return field.Required(fieldEtcdClusters, "") - } - for i, x := range c.Spec.EtcdClusters { - if err := validateEtcdClusterSpecLegacy(x, fieldEtcdClusters.Index(i)); err != nil { - return err + allErrs = append(allErrs, field.Required(fieldEtcdClusters, "")) + } else { + for i, x := range c.Spec.EtcdClusters { + allErrs = append(allErrs, validateEtcdClusterSpecLegacy(x, fieldEtcdClusters.Index(i))...) } - } - if err := validateEtcdTLS(c.Spec.EtcdClusters, fieldEtcdClusters); err != nil { - return err - } - if err := validateEtcdStorage(c.Spec.EtcdClusters, fieldEtcdClusters); err != nil { - return err + allErrs = append(allErrs, validateEtcdTLS(c.Spec.EtcdClusters, fieldEtcdClusters)...) + allErrs = append(allErrs, validateEtcdStorage(c.Spec.EtcdClusters, fieldEtcdClusters)...) } } { if c.Spec.Networking != nil && c.Spec.Networking.Classic != nil { - return field.Invalid(fieldSpec.Child("Networking"), "classic", "classic networking is not supported with kubernetes versions 1.4 and later") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Networking"), "classic", "classic networking is not supported with kubernetes versions 1.4 and later")) } } if c.Spec.Networking != nil && (c.Spec.Networking.AmazonVPC != nil || c.Spec.Networking.LyftVPC != nil) && c.Spec.CloudProvider != "aws" { - return field.Invalid(fieldSpec.Child("Networking"), "amazon-vpc-routed-eni", "amazon-vpc-routed-eni networking is supported only in AWS") + allErrs = append(allErrs, field.Invalid(fieldSpec.Child("Networking"), "amazon-vpc-routed-eni", "amazon-vpc-routed-eni networking is supported only in AWS")) } - if errs := newValidateCluster(c); len(errs) != 0 { - return errs[0] - } + allErrs = append(allErrs, newValidateCluster(c)...) - return nil + return allErrs } // validateSubnetCIDR is responsible for validating subnets are part of the CIDRs assigned to the cluster. @@ -632,31 +619,28 @@ func validateSubnetCIDR(networkCIDR *net.IPNet, additionalNetworkCIDRs []*net.IP } // validateEtcdClusterSpecLegacy is responsible for validating the etcd cluster spec -func validateEtcdClusterSpecLegacy(spec *kops.EtcdClusterSpec, fieldPath *field.Path) *field.Error { +func validateEtcdClusterSpecLegacy(spec *kops.EtcdClusterSpec, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} if spec.Name == "" { - return field.Required(fieldPath.Child("Name"), "EtcdCluster did not have name") + allErrs = append(allErrs, field.Required(fieldPath.Child("Name"), "EtcdCluster did not have name")) } if len(spec.Members) == 0 { - return field.Required(fieldPath.Child("Members"), "No members defined in etcd cluster") - } - if (len(spec.Members) % 2) == 0 { + allErrs = append(allErrs, field.Required(fieldPath.Child("Members"), "No members defined in etcd cluster")) + } else if (len(spec.Members) % 2) == 0 { // Not technically a requirement, but doesn't really make sense to allow - return field.Invalid(fieldPath.Child("Members"), len(spec.Members), "Should be an odd number of master-zones for quorum. Use --zones and --master-zones to declare node zones and master zones separately") - } - if err := validateEtcdVersion(spec, fieldPath, nil); err != nil { - return err + allErrs = append(allErrs, field.Invalid(fieldPath.Child("Members"), len(spec.Members), "Should be an odd number of master-zones for quorum. Use --zones and --master-zones to declare node zones and master zones separately")) } + allErrs = append(allErrs, validateEtcdVersion(spec, fieldPath, nil)...) for _, m := range spec.Members { - if err := validateEtcdMemberSpec(m, fieldPath); err != nil { - return err - } + allErrs = append(allErrs, validateEtcdMemberSpec(m, fieldPath)...) } - return nil + return allErrs } // validateEtcdTLS checks the TLS settings for etcd are valid -func validateEtcdTLS(specs []*kops.EtcdClusterSpec, fieldPath *field.Path) *field.Error { +func validateEtcdTLS(specs []*kops.EtcdClusterSpec, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} var usingTLS int for _, x := range specs { if x.EnableEtcdTLS { @@ -665,27 +649,28 @@ func validateEtcdTLS(specs []*kops.EtcdClusterSpec, fieldPath *field.Path) *fiel } // check both clusters are using tls if one us enabled if usingTLS > 0 && usingTLS != len(specs) { - return field.Invalid(fieldPath.Index(0).Child("EnableEtcdTLS"), false, "Both etcd clusters must have TLS enabled or none at all") + allErrs = append(allErrs, field.Invalid(fieldPath.Index(0).Child("EnableEtcdTLS"), false, "Both etcd clusters must have TLS enabled or none at all")) } - return nil + return allErrs } // validateEtcdStorage is responsible for checks version are identical -func validateEtcdStorage(specs []*kops.EtcdClusterSpec, fieldPath *field.Path) *field.Error { +func validateEtcdStorage(specs []*kops.EtcdClusterSpec, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} version := specs[0].Version for i, x := range specs { if x.Version != "" && x.Version != version { - return field.Invalid(fieldPath.Index(i).Child("Version"), x.Version, fmt.Sprintf("cluster: %q, has a different storage versions: %q, both must be the same", x.Name, x.Version)) + allErrs = append(allErrs, field.Invalid(fieldPath.Index(i).Child("Version"), x.Version, fmt.Sprintf("cluster: %q, has a different storage versions: %q, both must be the same", x.Name, x.Version))) } } - return nil + return allErrs } // validateEtcdVersion is responsible for validating the storage version of etcd // @TODO semvar package doesn't appear to ignore a 'v' in v1.1.1 should could be a problem later down the line -func validateEtcdVersion(spec *kops.EtcdClusterSpec, fieldPath *field.Path, minimalVersion *semver.Version) *field.Error { +func validateEtcdVersion(spec *kops.EtcdClusterSpec, fieldPath *field.Path, minimalVersion *semver.Version) field.ErrorList { // @check if the storage is specified, that's is valid if minimalVersion == nil { @@ -700,37 +685,38 @@ func validateEtcdVersion(spec *kops.EtcdClusterSpec, fieldPath *field.Path, mini sem, err := semver.Parse(strings.TrimPrefix(version, "v")) if err != nil { - return field.Invalid(fieldPath.Child("Version"), version, "the storage version is invalid") + return field.ErrorList{field.Invalid(fieldPath.Child("Version"), version, "the storage version is invalid")} } // we only support v3 and v2 for now if sem.Major == 3 || sem.Major == 2 { if sem.LT(*minimalVersion) { - return field.Invalid(fieldPath.Child("Version"), version, fmt.Sprintf("minimal version required is %s", minimalVersion.String())) + return field.ErrorList{field.Invalid(fieldPath.Child("Version"), version, fmt.Sprintf("minimal version required is %s", minimalVersion.String()))} } return nil } - return field.Invalid(fieldPath.Child("Version"), version, "unsupported storage version, we only support major versions 2 and 3") + return field.ErrorList{field.Invalid(fieldPath.Child("Version"), version, "unsupported storage version, we only support major versions 2 and 3")} } // validateEtcdMemberSpec is responsible for validate the cluster member -func validateEtcdMemberSpec(spec *kops.EtcdMemberSpec, fieldPath *field.Path) *field.Error { +func validateEtcdMemberSpec(spec *kops.EtcdMemberSpec, fieldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} if spec.Name == "" { - return field.Required(fieldPath.Child("Name"), "EtcdMember did not have Name") + allErrs = append(allErrs, field.Required(fieldPath.Child("Name"), "EtcdMember did not have Name")) } if fi.StringValue(spec.InstanceGroup) == "" { - return field.Required(fieldPath.Child("InstanceGroup"), "EtcdMember did not have InstanceGroup") + allErrs = append(allErrs, field.Required(fieldPath.Child("InstanceGroup"), "EtcdMember did not have InstanceGroup")) } - return nil + return allErrs } // DeepValidate is responsible for validating the instancegroups within the cluster spec func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool) error { - if err := ValidateCluster(c, strict); err != nil { - return err + if errs := ValidateCluster(c, strict); len(errs) != 0 { + return errs.ToAggregate() } if len(groups) == 0 { @@ -756,24 +742,22 @@ func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool) er } for _, g := range groups { - err := CrossValidateInstanceGroup(g, c, strict).ToAggregate() - if err != nil { - return err - } + errs := CrossValidateInstanceGroup(g, c, strict) // Additional cloud-specific validation rules, // such as making sure that identifiers match the expected formats for the given cloud switch kops.CloudProviderID(c.Spec.CloudProvider) { case kops.CloudProviderAWS: - errs := awsValidateInstanceGroup(g) - if len(errs) != 0 { - return errs[0] - } + errs = append(errs, awsValidateInstanceGroup(g)...) default: if len(g.Spec.Volumes) > 0 { - return errors.New("instancegroup volumes are only available with aws at present") + errs = append(errs, field.Forbidden(field.NewPath("spec.volumes"), "instancegroup volumes are only available with aws at present")) } } + + if len(errs) != 0 { + return errs.ToAggregate() + } } return nil diff --git a/pkg/client/simple/vfsclientset/cluster.go b/pkg/client/simple/vfsclientset/cluster.go index 84270be843..1474d07631 100644 --- a/pkg/client/simple/vfsclientset/cluster.go +++ b/pkg/client/simple/vfsclientset/cluster.go @@ -99,8 +99,8 @@ func (c *ClusterVFS) List(options metav1.ListOptions) (*api.ClusterList, error) } func (r *ClusterVFS) Create(c *api.Cluster) (*api.Cluster, error) { - if err := validation.ValidateCluster(c, false); err != nil { - return nil, err + if errs := validation.ValidateCluster(c, false); len(errs) != 0 { + return nil, errs.ToAggregate() } if c.ObjectMeta.CreationTimestamp.IsZero() { diff --git a/upup/pkg/fi/cloudup/populate_cluster_spec.go b/upup/pkg/fi/cloudup/populate_cluster_spec.go index 71a829cbad..2c340447a9 100644 --- a/upup/pkg/fi/cloudup/populate_cluster_spec.go +++ b/upup/pkg/fi/cloudup/populate_cluster_spec.go @@ -87,8 +87,8 @@ func PopulateClusterSpec(clientset simple.Clientset, cluster *api.Cluster, asset // @kris-nova // func (c *populateClusterSpec) run(clientset simple.Clientset) error { - if err := validation.ValidateCluster(c.InputCluster, false); err != nil { - return err + if errs := validation.ValidateCluster(c.InputCluster, false); len(errs) != 0 { + return errs.ToAggregate() } // Copy cluster & instance groups, so we can modify them freely @@ -327,8 +327,8 @@ func (c *populateClusterSpec) run(clientset simple.Clientset) error { fullCluster.Spec = *completed tf.cluster = fullCluster - if err := validation.ValidateCluster(fullCluster, true); err != nil { - return fmt.Errorf("Completed cluster failed validation: %v", err) + if errs := validation.ValidateCluster(fullCluster, true); len(errs) != 0 { + return fmt.Errorf("Completed cluster failed validation: %v", errs.ToAggregate()) } c.fullCluster = fullCluster diff --git a/upup/pkg/fi/cloudup/validation_test.go b/upup/pkg/fi/cloudup/validation_test.go index 6ae2472c1e..bcb287b235 100644 --- a/upup/pkg/fi/cloudup/validation_test.go +++ b/upup/pkg/fi/cloudup/validation_test.go @@ -104,12 +104,12 @@ func buildDefaultCluster(t *testing.T) *api.Cluster { func TestValidateFull_Default_Validates(t *testing.T) { c := buildDefaultCluster(t) - if err := validation.ValidateCluster(c, false); err != nil { + if errs := validation.ValidateCluster(c, false); len(errs) != 0 { klog.Infof("Cluster: %v", c) - t.Fatalf("Validate gave unexpected error (strict=false): %v", err) + t.Fatalf("Validate gave unexpected error (strict=false): %v", errs.ToAggregate()) } - if err := validation.ValidateCluster(c, true); err != nil { - t.Fatalf("Validate gave unexpected error (strict=true): %v", err) + if errs := validation.ValidateCluster(c, true); len(errs) != 0 { + t.Fatalf("Validate gave unexpected error (strict=true): %v", errs.ToAggregate()) } } @@ -196,19 +196,19 @@ func TestValidate_ContainerRegistry_and_ContainerProxy_exclusivity(t *testing.T) } func expectErrorFromValidate(t *testing.T, c *api.Cluster, message string) { - err := validation.ValidateCluster(c, false) - if err == nil { + errs := validation.ValidateCluster(c, false) + if len(errs) == 0 { t.Fatalf("Expected error from Validate") } - actualMessage := fmt.Sprintf("%v", err) + actualMessage := fmt.Sprintf("%v", errs.ToAggregate()) if !strings.Contains(actualMessage, message) { t.Fatalf("Expected error %q, got %q", message, actualMessage) } } func expectNoErrorFromValidate(t *testing.T, c *api.Cluster) { - err := validation.ValidateCluster(c, false) - if err != nil { - t.Fatalf("Unexpected error from Validate: %v", err) + errs := validation.ValidateCluster(c, false) + if len(errs) != 0 { + t.Fatalf("Unexpected error from Validate: %v", errs.ToAggregate()) } }