Skip to content

Commit

Permalink
Merge pull request #15623 from johngmyers/service-ip-range
Browse files Browse the repository at this point in the history 
Improve validation of PodCIDR and ServiceClusterIPRange
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 19, 2023
2 parents be2cf15 + 36373b1 commit d5c2458
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 54 deletions.
11 changes: 0 additions & 11 deletions pkg/apis/kops/validation/legacy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,17 +244,6 @@ func validateServiceAccountIssuerDiscovery(c *kops.Cluster, said *kops.ServiceAc
return allErrs
}

// validateSubnetCIDR is responsible for validating subnets are part of the CIDRs assigned to the cluster.
func validateSubnetCIDR(networkCIDRs []*net.IPNet, subnetCIDR *net.IPNet) bool {
for _, additionalNetworkCIDR := range networkCIDRs {
if subnet.BelongsTo(additionalNetworkCIDR, subnetCIDR) {
return true
}
}

return false
}

// DeepValidate is responsible for validating the instancegroups within the cluster spec
func DeepValidate(c *kops.Cluster, groups []*kops.InstanceGroup, strict bool, vfsContext *vfs.VFSContext, cloud fi.Cloud) error {
if errs := ValidateCluster(c, strict, vfsContext); len(errs) != 0 {
Expand Down
124 changes: 82 additions & 42 deletions pkg/apis/kops/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -293,23 +293,21 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
}

type cloudProviderConstraints struct {
requiresSubnets bool
requiresNetworkCIDR bool
prohibitsNetworkCIDR bool
prohibitsMultipleNetworkCIDRs bool
requiresNonMasqueradeCIDR bool
requiresServiceClusterSubnetOfNonMasqueradeCIDR bool
requiresSubnetCIDR bool
requiresSubnets bool
requiresNetworkCIDR bool
prohibitsNetworkCIDR bool
prohibitsMultipleNetworkCIDRs bool
requiresNonMasqueradeCIDR bool
requiresSubnetCIDR bool
}

func validateCloudProvider(c *kops.Cluster, provider *kops.CloudProviderSpec, fieldSpec *field.Path) (allErrs field.ErrorList, constraints *cloudProviderConstraints) {
constraints = &cloudProviderConstraints{
requiresSubnets: true,
requiresNetworkCIDR: true,
prohibitsMultipleNetworkCIDRs: true,
requiresNonMasqueradeCIDR: true,
requiresServiceClusterSubnetOfNonMasqueradeCIDR: true,
requiresSubnetCIDR: true,
requiresSubnets: true,
requiresNetworkCIDR: true,
prohibitsMultipleNetworkCIDRs: true,
requiresNonMasqueradeCIDR: true,
requiresSubnetCIDR: true,
}

optionTaken := false
Expand Down Expand Up @@ -342,7 +340,6 @@ func validateCloudProvider(c *kops.Cluster, provider *kops.CloudProviderSpec, fi
constraints.requiresSubnetCIDR = false
constraints.prohibitsNetworkCIDR = true
constraints.requiresNonMasqueradeCIDR = false
constraints.requiresServiceClusterSubnetOfNonMasqueradeCIDR = false
}
if c.Spec.CloudProvider.Hetzner != nil {
if optionTaken {
Expand Down Expand Up @@ -476,7 +473,7 @@ func parseCIDR(fieldPath *field.Path, cidr string) (*net.IPNet, field.ErrorList)
// We recognize the normal CIDR syntax - e.g. `2001:db8::/32`
// We also recognize values like /64#0, meaning "the first available /64 subnet", for dynamic allocations.
// See utils.ParseCIDRNotation for details.
func validateIPv6CIDR(fieldPath *field.Path, cidr string) field.ErrorList {
func validateIPv6CIDR(fieldPath *field.Path, cidr string, serviceClusterIPRange *net.IPNet) field.ErrorList {
allErrs := field.ErrorList{}

if strings.HasPrefix(cidr, "/") {
Expand All @@ -489,11 +486,15 @@ func validateIPv6CIDR(fieldPath *field.Path, cidr string) field.ErrorList {
allErrs = append(allErrs, field.Invalid(fieldPath, cidr, "IPv6 CIDR subnet size must be a value between 0 and 128"))
}
} else {
allErrs = append(allErrs, validateCIDR(fieldPath, cidr)...)
subnetCIDR, errs := parseCIDR(fieldPath, cidr)
allErrs = append(allErrs, errs...)

if !utils.IsIPv6CIDR(cidr) {
allErrs = append(allErrs, field.Invalid(fieldPath, cidr, "Network is not an IPv6 CIDR"))
}
if subnet.Overlap(subnetCIDR, serviceClusterIPRange) {
allErrs = append(allErrs, field.Forbidden(fieldPath, fmt.Sprintf("ipv6CIDR %q must not overlap serviceClusterIPRange %q", cidr, serviceClusterIPRange)))
}
}

return allErrs
Expand Down Expand Up @@ -527,7 +528,7 @@ func validateTopology(c *kops.Cluster, topology *kops.TopologySpec, fieldPath *f
return allErrs
}

func validateSubnets(c *kops.ClusterSpec, subnets []kops.ClusterSubnetSpec, fieldPath *field.Path, strict bool, providerConstraints *cloudProviderConstraints, networkCIDRs []*net.IPNet) field.ErrorList {
func validateSubnets(c *kops.ClusterSpec, subnets []kops.ClusterSubnetSpec, fieldPath *field.Path, strict bool, providerConstraints *cloudProviderConstraints, networkCIDRs []*net.IPNet, podCIDR, serviceClusterIPRange *net.IPNet) field.ErrorList {
allErrs := field.ErrorList{}

if providerConstraints.requiresSubnets && len(subnets) == 0 {
Expand All @@ -537,7 +538,7 @@ func validateSubnets(c *kops.ClusterSpec, subnets []kops.ClusterSubnetSpec, fiel

// Each subnet must be valid
for i := range subnets {
allErrs = append(allErrs, validateSubnet(&subnets[i], c, fieldPath.Index(i), strict, providerConstraints, networkCIDRs)...)
allErrs = append(allErrs, validateSubnet(&subnets[i], c, fieldPath.Index(i), strict, providerConstraints, networkCIDRs, podCIDR, serviceClusterIPRange)...)
}

// cannot duplicate subnet name
Expand Down Expand Up @@ -573,71 +574,89 @@ func validateSubnets(c *kops.ClusterSpec, subnets []kops.ClusterSubnetSpec, fiel
return allErrs
}

func validateSubnet(subnet *kops.ClusterSubnetSpec, c *kops.ClusterSpec, fieldPath *field.Path, strict bool, providerConstraints *cloudProviderConstraints, networkCIDRs []*net.IPNet) field.ErrorList {
func validateSubnet(subnetSpec *kops.ClusterSubnetSpec, c *kops.ClusterSpec, fieldPath *field.Path, strict bool, providerConstraints *cloudProviderConstraints, networkCIDRs []*net.IPNet, podCIDR, serviceClusterIPRange *net.IPNet) field.ErrorList {
allErrs := field.ErrorList{}

// name is required
if subnet.Name == "" {
if subnetSpec.Name == "" {
allErrs = append(allErrs, field.Required(fieldPath.Child("name"), ""))
}

// CIDR
if subnet.CIDR == "" {
if subnetSpec.CIDR == "" {
if providerConstraints.requiresSubnetCIDR && strict {
if !strings.Contains(c.Networking.NonMasqueradeCIDR, ":") || subnet.IPv6CIDR == "" {
if !strings.Contains(c.Networking.NonMasqueradeCIDR, ":") || subnetSpec.IPv6CIDR == "" {
allErrs = append(allErrs, field.Required(fieldPath.Child("cidr"), "subnet does not have a cidr set"))
}
}
} else {
subnetCIDR, errs := parseCIDR(fieldPath.Child("cidr"), subnet.CIDR)
subnetCIDR, errs := parseCIDR(fieldPath.Child("cidr"), subnetSpec.CIDR)
allErrs = append(allErrs, errs...)
if len(networkCIDRs) > 0 && subnetCIDR != nil && !validateSubnetCIDR(networkCIDRs, subnetCIDR) {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("cidr"), fmt.Sprintf("subnet %q had a cidr %q that was not a subnet of the networkCIDR %q", subnet.Name, subnet.CIDR, c.Networking.NetworkCIDR)))
if len(networkCIDRs) > 0 && subnetCIDR != nil {
found := false
for _, networkCIDR := range networkCIDRs {
if subnet.BelongsTo(networkCIDR, subnetCIDR) {
found = true
}
}
if !found {
extraMsg := ""
if len(networkCIDRs) > 1 {
extraMsg = " or an additionalNetworkCIDR"
}
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("cidr"), fmt.Sprintf("subnet %q cidr %q is not a subnet of the networkCIDR %q%s", subnetSpec.Name, subnetSpec.CIDR, c.Networking.NetworkCIDR, extraMsg)))
}
}
if subnet.Overlap(subnetCIDR, podCIDR) {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("cidr"), fmt.Sprintf("subnet %q cidr %q must not overlap podCIDR %q", subnetSpec.Name, subnetSpec.CIDR, podCIDR)))
}
if subnet.Overlap(subnetCIDR, serviceClusterIPRange) {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("cidr"), fmt.Sprintf("subnet %q cidr %q must not overlap serviceClusterIPRange %q", subnetSpec.Name, subnetSpec.CIDR, serviceClusterIPRange)))
}
}

// IPv6CIDR
if subnet.IPv6CIDR != "" {
allErrs = append(allErrs, validateIPv6CIDR(fieldPath.Child("ipv6CIDR"), subnet.IPv6CIDR)...)
if subnetSpec.IPv6CIDR != "" {
allErrs = append(allErrs, validateIPv6CIDR(fieldPath.Child("ipv6CIDR"), subnetSpec.IPv6CIDR, serviceClusterIPRange)...)
}

if subnet.Egress != "" {
egressType := strings.Split(subnet.Egress, "-")[0]
if subnetSpec.Egress != "" {
egressType := strings.Split(subnetSpec.Egress, "-")[0]
if egressType != kops.EgressNatGateway && egressType != kops.EgressElasticIP && egressType != kops.EgressNatInstance && egressType != kops.EgressExternal && egressType != kops.EgressTransitGateway {
allErrs = append(allErrs, field.Invalid(fieldPath.Child("egress"), subnet.Egress,
allErrs = append(allErrs, field.Invalid(fieldPath.Child("egress"), subnetSpec.Egress,
"egress must be of type NAT Gateway, NAT Gateway with existing ElasticIP, NAT EC2 Instance, Transit Gateway, or External"))
}
if subnet.Egress != kops.EgressExternal && subnet.Type != "DualStack" && subnet.Type != "Private" && (subnet.IPv6CIDR == "" || subnet.Type != "Public") {
if subnetSpec.Egress != kops.EgressExternal && subnetSpec.Type != "DualStack" && subnetSpec.Type != "Private" && (subnetSpec.IPv6CIDR == "" || subnetSpec.Type != "Public") {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("egress"), "egress can only be specified for private or IPv6-capable public subnets"))
}
}

allErrs = append(allErrs, IsValidValue(fieldPath.Child("type"), &subnet.Type, []kops.SubnetType{
allErrs = append(allErrs, IsValidValue(fieldPath.Child("type"), &subnetSpec.Type, []kops.SubnetType{
kops.SubnetTypePublic,
kops.SubnetTypePrivate,
kops.SubnetTypeDualStack,
kops.SubnetTypeUtility,
})...)

if subnet.Type == kops.SubnetTypeDualStack && !c.IsIPv6Only() {
if subnetSpec.Type == kops.SubnetTypeDualStack && !c.IsIPv6Only() {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("type"), "subnet type DualStack may only be used in IPv6 clusters"))
}

if c.CloudProvider.Openstack != nil {
if c.CloudProvider.Openstack.Router == nil || c.CloudProvider.Openstack.Router.ExternalNetwork == nil {
if subnet.Type == kops.SubnetTypePublic {
if subnetSpec.Type == kops.SubnetTypePublic {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("type"), "subnet type Public requires an external network"))
}
}
}

if c.CloudProvider.AWS != nil && subnet.AdditionalRoutes != nil {
if len(subnet.ID) > 0 {
if c.CloudProvider.AWS != nil && subnetSpec.AdditionalRoutes != nil {
if len(subnetSpec.ID) > 0 {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("additionalRoutes"), "additional routes cannot be added if the subnet is shared"))
} else if subnet.Type != kops.SubnetTypePrivate {
} else if subnetSpec.Type != kops.SubnetTypePrivate {
allErrs = append(allErrs, field.Forbidden(fieldPath.Child("additionalRoutes"), "additional routes can only be added on private subnets"))
}
allErrs = append(allErrs, awsValidateAdditionalRoutes(fieldPath.Child("additionalRoutes"), subnet.AdditionalRoutes, networkCIDRs)...)
allErrs = append(allErrs, awsValidateAdditionalRoutes(fieldPath.Child("additionalRoutes"), subnetSpec.AdditionalRoutes, networkCIDRs)...)
}

return allErrs
Expand Down Expand Up @@ -991,22 +1010,43 @@ func validateNetworking(cluster *kops.Cluster, v *kops.NetworkingSpec, fldPath *
}
}

var podCIDR *net.IPNet
{
if v.PodCIDR == "" {
if strict && !cluster.Spec.IsKopsControllerIPAM() {
allErrs = append(allErrs, field.Required(fldPath.Child("podCIDR"), "Cluster did not have podCIDR set"))
}
} else {
var errs field.ErrorList
podCIDR, errs = parseCIDR(fldPath.Child("podCIDR"), v.PodCIDR)
allErrs = append(allErrs, errs...)

if podCIDR != nil {
if len(nonMasqueradeCIDRs) > 0 && !subnet.BelongsTo(nonMasqueradeCIDRs[0], podCIDR) {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("podCIDR"), fmt.Sprintf("podCIDR %q must be a subnet of nonMasqueradeCIDR %q", podCIDR, nonMasqueradeCIDRs[0])))
}
}
}
}

var serviceClusterIPRange *net.IPNet
{
if v.ServiceClusterIPRange == "" {
if strict {
allErrs = append(allErrs, field.Required(fldPath.Child("serviceClusterIPRange"), "Cluster did not have serviceClusterIPRange set"))
}
} else {
serviceClusterIPRange, errs := parseCIDR(fldPath.Child("serviceClusterIPRange"), v.ServiceClusterIPRange)
var errs field.ErrorList
serviceClusterIPRange, errs = parseCIDR(fldPath.Child("serviceClusterIPRange"), v.ServiceClusterIPRange)
allErrs = append(allErrs, errs...)

if serviceClusterIPRange != nil && len(nonMasqueradeCIDRs) > 0 && providerConstraints.requiresServiceClusterSubnetOfNonMasqueradeCIDR && !subnet.BelongsTo(nonMasqueradeCIDRs[0], serviceClusterIPRange) {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceClusterIPRange"), fmt.Sprintf("serviceClusterIPRange %q must be a subnet of nonMasqueradeCIDR %q", v.ServiceClusterIPRange, v.NonMasqueradeCIDR)))
if subnet.Overlap(podCIDR, serviceClusterIPRange) {
allErrs = append(allErrs, field.Forbidden(fldPath.Child("serviceClusterIPRange"), fmt.Sprintf("serviceClusterIPRange %q must not overlap podCIDR %q", serviceClusterIPRange, podCIDR)))
}
}
}

allErrs = append(allErrs, validateSubnets(&cluster.Spec, v.Subnets, fldPath.Child("subnets"), strict, providerConstraints, networkCIDRs)...)
allErrs = append(allErrs, validateSubnets(&cluster.Spec, v.Subnets, fldPath.Child("subnets"), strict, providerConstraints, networkCIDRs, podCIDR, serviceClusterIPRange)...)

if v.Topology != nil {
allErrs = append(allErrs, validateTopology(cluster, v.Topology, fldPath.Child("topology"))...)
Expand Down
4 changes: 3 additions & 1 deletion pkg/apis/kops/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ func TestValidateSubnets(t *testing.T) {
},
}
_, ipNet, _ := net.ParseCIDR(cluster.Networking.NetworkCIDR)
errs := validateSubnets(cluster, cluster.Networking.Subnets, field.NewPath("subnets"), true, &cloudProviderConstraints{}, []*net.IPNet{ipNet})
errs := validateSubnets(cluster, cluster.Networking.Subnets, field.NewPath("subnets"), true, &cloudProviderConstraints{}, []*net.IPNet{ipNet}, nil, nil)

testErrors(t, g.Input, errs, g.ExpectedErrors)
}
Expand Down Expand Up @@ -395,6 +395,7 @@ func Test_Validate_Networking_Flannel(t *testing.T) {
Networking: kops.NetworkingSpec{
NetworkCIDR: "10.0.0.0/8",
NonMasqueradeCIDR: "100.64.0.0/10",
PodCIDR: "100.96.0.0/11",
ServiceClusterIPRange: "100.64.0.0/13",
Subnets: []kops.ClusterSubnetSpec{
{
Expand Down Expand Up @@ -461,6 +462,7 @@ func Test_Validate_AdditionalPolicies(t *testing.T) {
Networking: kops.NetworkingSpec{
NetworkCIDR: "10.10.0.0/16",
NonMasqueradeCIDR: "100.64.0.0/10",
PodCIDR: "100.96.0.0/11",
ServiceClusterIPRange: "100.64.0.0/13",
Subnets: []kops.ClusterSubnetSpec{
{
Expand Down
3 changes: 3 additions & 0 deletions pkg/util/subnet/subnet.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ import (

// Overlap checks if two subnets overlap
func Overlap(l, r *net.IPNet) bool {
if l == nil || r == nil {
return false
}
return l.Contains(r.IP) || r.Contains(l.IP)
}

Expand Down

0 comments on commit d5c2458

Please sign in to comment.