From 3656924bc568cfab884c2fac2362b0242453807c Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Sat, 21 Jan 2023 16:08:39 -0800 Subject: [PATCH 1/4] Modify complex integration test to have subnet out of additionalNetworkCIDRs --- .../integration/update_cluster/complex/in-legacy-v1alpha2.yaml | 2 +- tests/integration/update_cluster/complex/in-v1alpha2.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml index e942243457..05eaf207f2 100644 --- a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml @@ -84,7 +84,7 @@ spec: name: us-test-1a type: Public zone: us-test-1a - - cidr: 172.20.64.0/19 + - cidr: 10.1.64.0/19 name: us-east-1a-private type: Private zone: us-test-1a diff --git a/tests/integration/update_cluster/complex/in-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-v1alpha2.yaml index b8c61a4af8..8b248c1959 100644 --- a/tests/integration/update_cluster/complex/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-v1alpha2.yaml @@ -85,7 +85,7 @@ spec: name: us-test-1a type: Public zone: us-test-1a - - cidr: 172.20.64.0/19 + - cidr: 10.1.64.0/19 name: us-east-1a-private type: Private zone: us-test-1a From 045647f9bb732e084c2cabdd8214999656522ed1 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Sat, 21 Jan 2023 16:11:59 -0800 Subject: [PATCH 2/4] hack/update-expected.sh --- .../complex/data/aws_s3_object_cluster-completed.spec_content | 2 +- tests/integration/update_cluster/complex/kubernetes.tf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/update_cluster/complex/data/aws_s3_object_cluster-completed.spec_content b/tests/integration/update_cluster/complex/data/aws_s3_object_cluster-completed.spec_content index 0c5a58a7a2..1883b94d78 100644 --- a/tests/integration/update_cluster/complex/data/aws_s3_object_cluster-completed.spec_content +++ b/tests/integration/update_cluster/complex/data/aws_s3_object_cluster-completed.spec_content @@ -238,7 +238,7 @@ spec: - additionalRoutes: - cidr: 192.168.1.10/32 target: tgw-0123456 - cidr: 172.20.64.0/19 + cidr: 10.1.64.0/19 egress: tgw-123456 name: us-east-1a-private type: Private diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf index c59736dabd..b082ac0b5f 100644 --- a/tests/integration/update_cluster/complex/kubernetes.tf +++ b/tests/integration/update_cluster/complex/kubernetes.tf @@ -1135,7 +1135,7 @@ resource "aws_security_group_rule" "tcp-api-pl-44444444" { resource "aws_subnet" "us-east-1a-private-complex-example-com" { availability_zone = "us-test-1a" - cidr_block = "172.20.64.0/19" + cidr_block = "10.1.64.0/19" enable_resource_name_dns_a_record_on_launch = true private_dns_hostname_type_on_launch = "resource-name" tags = { From 7d3c20d0365853c9d0f441019bfc9d28b89dc0ed Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Sat, 21 Jan 2023 18:42:58 -0800 Subject: [PATCH 3/4] Validate additionalRoutes against additionalNetworkCIDRs --- pkg/apis/kops/validation/aws.go | 56 +++++------ pkg/apis/kops/validation/aws_test.go | 128 ++++++++++++++++--------- pkg/apis/kops/validation/validation.go | 9 ++ 3 files changed, 113 insertions(+), 80 deletions(-) diff --git a/pkg/apis/kops/validation/aws.go b/pkg/apis/kops/validation/aws.go index b1f42fda8a..84f71f0c90 100644 --- a/pkg/apis/kops/validation/aws.go +++ b/pkg/apis/kops/validation/aws.go @@ -58,18 +58,6 @@ func awsValidateCluster(c *kops.Cluster) field.ErrorList { allErrs = append(allErrs, awsValidateIAMAuthenticator(field.NewPath("spec", "authentication", "aws"), c.Spec.Authentication.AWS)...) } - for i, subnet := range c.Spec.Networking.Subnets { - f := field.NewPath("spec", "networking", "subnets").Index(i) - if subnet.AdditionalRoutes != nil { - if len(subnet.ID) > 0 { - allErrs = append(allErrs, field.Invalid(f, subnet, "additional routes cannot be added if the subnet is shared")) - } else if subnet.Type != kops.SubnetTypePrivate { - allErrs = append(allErrs, field.Invalid(f, subnet, "additional routes can only be added on private subnets")) - } - allErrs = append(allErrs, awsValidateAdditionalRoutes(f.Child("additionalRoutes"), subnet.AdditionalRoutes, c.Spec.Networking.NetworkCIDR)...) - } - } - return allErrs } @@ -416,31 +404,29 @@ func hasAWSEBSCSIDriver(c kops.ClusterSpec) bool { return *c.CloudProvider.AWS.EBSCSIDriver.Enabled } -func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec, cidr string) field.ErrorList { +func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec, networkCIDRs []*net.IPNet) field.ErrorList { allErrs := field.ErrorList{} - _, clusterNet, errClusterNet := net.ParseCIDR(cidr) - if errClusterNet != nil { - allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "networking", "networkCIDR"), cidr, "Invalid cluster cidr")) - } else { - for i, r := range routes { - f := fieldPath.Index(i) + for i, r := range routes { + f := fieldPath.Index(i) - // Check if target is a known type - if !strings.HasPrefix(r.Target, "pcx-") && - !strings.HasPrefix(r.Target, "i-") && - !strings.HasPrefix(r.Target, "nat-") && - !strings.HasPrefix(r.Target, "tgw-") && - !strings.HasPrefix(r.Target, "igw-") && - !strings.HasPrefix(r.Target, "eigw-") { - allErrs = append(allErrs, field.Invalid(f.Child("target"), r, "unknown target type for route")) - } + // Check if target is a known type + if !strings.HasPrefix(r.Target, "pcx-") && + !strings.HasPrefix(r.Target, "i-") && + !strings.HasPrefix(r.Target, "nat-") && + !strings.HasPrefix(r.Target, "tgw-") && + !strings.HasPrefix(r.Target, "igw-") && + !strings.HasPrefix(r.Target, "eigw-") { + allErrs = append(allErrs, field.Invalid(f.Child("target"), r, "unknown target type for route")) + } - ipRoute, _, e := net.ParseCIDR(r.CIDR) - if e != nil { - allErrs = append(allErrs, field.Invalid(f.Child("cidr"), r, "invalid cidr")) - } else if clusterNet.Contains(ipRoute) && strings.HasPrefix(r.Target, "pcx-") { - allErrs = append(allErrs, field.Forbidden(f.Child("target"), "target is more specific than cluster CIDR block. This route can target only an interface or an instance.")) + var routeCIDRs []*net.IPNet + allErrs = append(allErrs, validateCIDR(r.CIDR, f.Child("cidr"), &routeCIDRs)...) + if len(routeCIDRs) > 0 { + for _, clusterNet := range networkCIDRs { + if clusterNet.Contains(routeCIDRs[0].IP) && strings.HasPrefix(r.Target, "pcx-") { + allErrs = append(allErrs, field.Forbidden(f.Child("target"), "target is more specific than a network CIDR block. This route can target only an interface or an instance.")) + } } } } @@ -448,7 +434,9 @@ func awsValidateAdditionalRoutes(fieldPath *field.Path, routes []kops.RouteSpec, // Check for duplicated CIDR { cidrs := sets.NewString() - cidrs.Insert(cidr) + for _, cidr := range networkCIDRs { + cidrs.Insert(cidr.String()) + } for i := range routes { rCidr := routes[i].CIDR if cidrs.Has(rCidr) { diff --git a/pkg/apis/kops/validation/aws_test.go b/pkg/apis/kops/validation/aws_test.go index c49a1355ab..d2e993476f 100644 --- a/pkg/apis/kops/validation/aws_test.go +++ b/pkg/apis/kops/validation/aws_test.go @@ -22,6 +22,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/kops/cloudmock/aws/mockec2" "k8s.io/kops/upup/pkg/fi" @@ -676,14 +677,17 @@ func TestAWSAuthentication(t *testing.T) { func TestAWSAdditionalRoutes(t *testing.T) { tests := []struct { - clusterCidr string - providerId string - subnetType kops.SubnetType - route []kops.RouteSpec - expected []string + name string + clusterCIDR string + additionalClusterCIDRs []string + providerId string + subnetType kops.SubnetType + route []kops.RouteSpec + expected []string }{ - { // valid pcx - clusterCidr: "100.64.0.0/10", + { + name: "valid pcx", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -692,8 +696,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // valid instance - clusterCidr: "100.64.0.0/10", + { + name: "valid instance", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -702,8 +707,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // valid nat - clusterCidr: "100.64.0.0/10", + { + name: "valid nat", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -712,8 +718,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // valid transit gateway - clusterCidr: "100.64.0.0/10", + { + name: "valid transit gateway", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -722,8 +729,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // valid internet gateway - clusterCidr: "100.64.0.0/10", + { + name: "valid internet gateway", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -732,8 +740,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // valid egress only internet gateway - clusterCidr: "100.64.0.0/10", + { + name: "valid egress only internet gateway", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -742,8 +751,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, }, }, - { // bad cluster cidr - clusterCidr: "not cidr", + { + name: "bad cluster cidr", + clusterCIDR: "not cidr", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -753,8 +763,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, expected: []string{"Invalid value::spec.networking.networkCIDR"}, }, - { // bad cidr - clusterCidr: "100.64.0.0/10", + { + name: "bad cidr", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -764,8 +775,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, expected: []string{"Invalid value::spec.networking.subnets[0].additionalRoutes[0].cidr"}, }, - { // bad target - clusterCidr: "100.64.0.0/10", + { + name: "bad target", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -775,8 +787,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, expected: []string{"Invalid value::spec.networking.subnets[0].additionalRoutes[0].target"}, }, - { // target more specific - clusterCidr: "100.64.0.0/10", + { + name: "target more specific", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -786,8 +799,22 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, expected: []string{"Forbidden::spec.networking.subnets[0].additionalRoutes[0].target"}, }, - { // duplicates cidr - clusterCidr: "100.64.0.0/10", + { + name: "target more specific additionalCIDR", + clusterCIDR: "100.64.0.0/16", + additionalClusterCIDRs: []string{"100.66.0.0/16", "100.67.0.0/16"}, + subnetType: kops.SubnetTypePrivate, + route: []kops.RouteSpec{ + { + CIDR: "100.66.2.0/24", + Target: "pcx-abcdef", + }, + }, + expected: []string{"Forbidden::spec.networking.subnets[0].additionalRoutes[0].target"}, + }, + { + name: "duplicates cidr", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, route: []kops.RouteSpec{ { @@ -801,8 +828,9 @@ func TestAWSAdditionalRoutes(t *testing.T) { }, expected: []string{"Duplicate value::spec.networking.subnets[0].additionalRoutes[1].cidr"}, }, - { // shared subnet - clusterCidr: "100.64.0.0/10", + { + name: "shared subnet", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePrivate, providerId: "123456", route: []kops.RouteSpec{ @@ -811,10 +839,11 @@ func TestAWSAdditionalRoutes(t *testing.T) { Target: "pcx-abcdef", }, }, - expected: []string{"Invalid value::spec.networking.subnets[0]"}, + expected: []string{"Forbidden::spec.networking.subnets[0].additionalRoutes"}, }, - { // not a private subnet - clusterCidr: "100.64.0.0/10", + { + name: "not a private subnet", + clusterCIDR: "100.64.0.0/10", subnetType: kops.SubnetTypePublic, route: []kops.RouteSpec{ { @@ -822,26 +851,33 @@ func TestAWSAdditionalRoutes(t *testing.T) { Target: "pcx-abcdef", }, }, - expected: []string{"Invalid value::spec.networking.subnets[0]"}, + expected: []string{"Forbidden::spec.networking.subnets[0].additionalRoutes"}, }, } for _, test := range tests { - cluster := kops.Cluster{ - Spec: kops.ClusterSpec{ - Networking: kops.NetworkingSpec{ - NetworkCIDR: test.clusterCidr, - Subnets: []kops.ClusterSubnetSpec{ - { - ID: test.providerId, - Type: test.subnetType, - AdditionalRoutes: test.route, + t.Run(test.name, func(t *testing.T) { + cluster := kops.Cluster{ + Spec: kops.ClusterSpec{ + CloudProvider: kops.CloudProviderSpec{ + AWS: &kops.AWSSpec{}, + }, + Networking: kops.NetworkingSpec{ + NetworkCIDR: test.clusterCIDR, + AdditionalNetworkCIDRs: test.additionalClusterCIDRs, + Subnets: []kops.ClusterSubnetSpec{ + { + Name: "us-east-1a", + ID: test.providerId, + Type: test.subnetType, + AdditionalRoutes: test.route, + }, }, }, }, - }, - } - errs := awsValidateCluster(&cluster) - testErrors(t, test, errs, test.expected) + } + errs := validateNetworking(&cluster, &cluster.Spec.Networking, field.NewPath("spec", "networking"), false, &cloudProviderConstraints{}) + testErrors(t, test, errs, test.expected) + }) } } diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 28071d1b1e..ec1afbcc4a 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -624,6 +624,15 @@ func validateSubnet(subnet *kops.ClusterSubnetSpec, c *kops.ClusterSpec, fieldPa } } + if c.CloudProvider.AWS != nil && subnet.AdditionalRoutes != nil { + if len(subnet.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 { + 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)...) + } + return allErrs } From c905c4f8272950bc12cc504f84ff4ed51189538c Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Sat, 21 Jan 2023 18:52:05 -0800 Subject: [PATCH 4/4] Include AdditionalNetworkCIDRs in default proxy excludes --- upup/pkg/fi/cloudup/defaults.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/upup/pkg/fi/cloudup/defaults.go b/upup/pkg/fi/cloudup/defaults.go index c6e5c180d6..27dacb8095 100644 --- a/upup/pkg/fi/cloudup/defaults.go +++ b/upup/pkg/fi/cloudup/defaults.go @@ -230,6 +230,12 @@ func assignProxy(cluster *kops.Cluster) (*kops.EgressProxySpec, error) { klog.Warningf("No NetworkCIDR defined (yet), not adding to egressProxy.excludes") } + for _, cidr := range cluster.Spec.Networking.AdditionalNetworkCIDRs { + if !strings.Contains(cluster.Spec.Networking.EgressProxy.ProxyExcludes, cidr) { + egressSlice = append(egressSlice, cidr) + } + } + egressProxy.ProxyExcludes = strings.Join(egressSlice, ",") klog.V(8).Infof("Completed setting up Proxy excludes as follows: %q", egressProxy.ProxyExcludes) } else {