From 1577b0a54ba40678f0de56efa3b2a404ee8a1e0c Mon Sep 17 00:00:00 2001 From: Timothy Clarke Date: Tue, 16 Feb 2021 16:16:56 +0000 Subject: [PATCH 1/4] Adding Elastic IP Allocations to NLB API --- cloudmock/aws/mockelbv2/loadbalancers.go | 3 + docs/cluster_spec.md | 13 +++++ k8s/crds/kops.k8s.io_clusters.yaml | 4 ++ pkg/apis/kops/cluster.go | 2 + pkg/apis/kops/v1alpha2/cluster.go | 2 + .../kops/v1alpha2/zz_generated.conversion.go | 2 + .../kops/v1alpha2/zz_generated.deepcopy.go | 5 ++ pkg/apis/kops/validation/aws.go | 10 ++++ pkg/apis/kops/validation/aws_test.go | 58 +++++++++++++++++-- pkg/apis/kops/zz_generated.deepcopy.go | 5 ++ pkg/model/awsmodel/api_loadbalancer.go | 3 + .../complex/cloudformation.json | 3 +- .../complex/in-legacy-v1alpha2.yaml | 3 + .../update_cluster/complex/in-v1alpha2.yaml | 5 +- .../update_cluster/complex/kubernetes.tf | 3 +- .../cloudup/awstasks/network_load_balancer.go | 30 ++++++++-- .../pkg/fi/cloudup/awstasks/subnet_mapping.go | 12 +++- 17 files changed, 149 insertions(+), 14 deletions(-) diff --git a/cloudmock/aws/mockelbv2/loadbalancers.go b/cloudmock/aws/mockelbv2/loadbalancers.go index ae0d54b39a..c675d9e812 100644 --- a/cloudmock/aws/mockelbv2/loadbalancers.go +++ b/cloudmock/aws/mockelbv2/loadbalancers.go @@ -97,6 +97,9 @@ func (m *MockELBV2) CreateLoadBalancer(request *elbv2.CreateLoadBalancerInput) ( if subnetMapping.PrivateIPv4Address != nil { lbAddrs = append(lbAddrs, &elbv2.LoadBalancerAddress{PrivateIPv4Address: subnetMapping.PrivateIPv4Address}) } + if subnetMapping.AllocationId != nil { + lbAddrs = append(lbAddrs, &elbv2.LoadBalancerAddress{AllocationId: subnetMapping.AllocationId}) + } zones = append(zones, &elbv2.AvailabilityZone{ SubnetId: subnetMapping.SubnetId, LoadBalancerAddresses: lbAddrs, diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md index b4d01e1f58..555b5bf32b 100644 --- a/docs/cluster_spec.md +++ b/docs/cluster_spec.md @@ -137,6 +137,19 @@ spec: The specified IPv4 addresses must be part of the subnets CIDR. They can not be changed after initial deployment. +If the `type` is `Public` and the `class` is `Network`, you can also specify an Elastic IP allocationID to bind a fixed public IP address per subnet. Pleae note only IPv4 addresses have been tested: +```yaml +spec: + api: + loadBalancer: + type: Public + subnets: + - name: subnet-a + allocationID: eipalloc-222ghi789 +``` + +The specified Allocation ID's must already be created manually or external infrastructure as code, eg Terraform. + If you made a mistake or need to change subnets for any other reason, you're currently forced to manually delete the underlying ELB/NLB and re-run `kops update`. diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml index be73b563c1..d63336d9cf 100644 --- a/k8s/crds/kops.k8s.io_clusters.yaml +++ b/k8s/crds/kops.k8s.io_clusters.yaml @@ -119,6 +119,10 @@ spec: description: LoadBalancerSubnetSpec provides configuration for subnets used for a load balancer properties: + allocationId: + description: AllocationID specifies the Elastic IP Allocation + ID for use by a NLB + type: string name: description: Name specifies the name of the cluster subnet diff --git a/pkg/apis/kops/cluster.go b/pkg/apis/kops/cluster.go index 5adf26d6f1..c146dc091c 100644 --- a/pkg/apis/kops/cluster.go +++ b/pkg/apis/kops/cluster.go @@ -383,6 +383,8 @@ type LoadBalancerSubnetSpec struct { Name string `json:"name,omitempty"` // PrivateIPv4Address specifies the private IPv4 address to use for a NLB PrivateIPv4Address *string `json:"privateIPv4Address,omitempty"` + // AllocationID specifies the Elastic IP Allocation ID for use by a NLB + AllocationID *string `json:"allocationId,omitempty"` } // LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access diff --git a/pkg/apis/kops/v1alpha2/cluster.go b/pkg/apis/kops/v1alpha2/cluster.go index e91611ab72..f5f6413d9f 100644 --- a/pkg/apis/kops/v1alpha2/cluster.go +++ b/pkg/apis/kops/v1alpha2/cluster.go @@ -385,6 +385,8 @@ type LoadBalancerSubnetSpec struct { Name string `json:"name,omitempty"` // PrivateIPv4Address specifies the private IPv4 address to use for a NLB PrivateIPv4Address *string `json:"privateIPv4Address,omitempty"` + // AllocationID specifies the Elastic IP Allocation ID for use by a NLB + AllocationID *string `json:"allocationId,omitempty"` } // LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go index 300a9783ae..4c8da9a529 100644 --- a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go +++ b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go @@ -5181,6 +5181,7 @@ func Convert_kops_LoadBalancerAccessSpec_To_v1alpha2_LoadBalancerAccessSpec(in * func autoConvert_v1alpha2_LoadBalancerSubnetSpec_To_kops_LoadBalancerSubnetSpec(in *LoadBalancerSubnetSpec, out *kops.LoadBalancerSubnetSpec, s conversion.Scope) error { out.Name = in.Name out.PrivateIPv4Address = in.PrivateIPv4Address + out.AllocationID = in.AllocationID return nil } @@ -5192,6 +5193,7 @@ func Convert_v1alpha2_LoadBalancerSubnetSpec_To_kops_LoadBalancerSubnetSpec(in * func autoConvert_kops_LoadBalancerSubnetSpec_To_v1alpha2_LoadBalancerSubnetSpec(in *kops.LoadBalancerSubnetSpec, out *LoadBalancerSubnetSpec, s conversion.Scope) error { out.Name = in.Name out.PrivateIPv4Address = in.PrivateIPv4Address + out.AllocationID = in.AllocationID return nil } diff --git a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go index a543d8f23a..7e23fe4f14 100644 --- a/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go +++ b/pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go @@ -3498,6 +3498,11 @@ func (in *LoadBalancerSubnetSpec) DeepCopyInto(out *LoadBalancerSubnetSpec) { *out = new(string) **out = **in } + if in.AllocationID != nil { + in, out := &in.AllocationID, &out.AllocationID + *out = new(string) + **out = **in + } return } diff --git a/pkg/apis/kops/validation/aws.go b/pkg/apis/kops/validation/aws.go index 14472c7da8..9403082f33 100644 --- a/pkg/apis/kops/validation/aws.go +++ b/pkg/apis/kops/validation/aws.go @@ -241,6 +241,16 @@ func awsValidateLoadBalancerSubnets(fieldPath *field.Path, spec kops.ClusterSpec allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("privateIPv4Address"), "privateIPv4Address only allowed for internal NLBs")) } } + + if subnet.AllocationID != nil { + if *subnet.AllocationID == "" { + allErrs = append(allErrs, field.Required(fieldPath.Index(i).Child("allocationID"), "allocationID can't be empty")) + } + + if lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type == kops.LoadBalancerTypeInternal { + allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("allocationID"), "allocationID only allowed for Public NLBs")) + } + } } return allErrs diff --git a/pkg/apis/kops/validation/aws_test.go b/pkg/apis/kops/validation/aws_test.go index bccf33d784..5c0f8c092e 100644 --- a/pkg/apis/kops/validation/aws_test.go +++ b/pkg/apis/kops/validation/aws_test.go @@ -249,16 +249,18 @@ func TestLoadBalancerSubnets(t *testing.T) { lbSubnets []kops.LoadBalancerSubnetSpec expected []string }{ - { // valid (no privateIPv4Address) + { // valid (no privateIPv4Address, no allocationID) clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: nil, + AllocationID: nil, }, { Name: "b", PrivateIPv4Address: nil, + AllocationID: nil, }, }, }, @@ -268,10 +270,12 @@ func TestLoadBalancerSubnets(t *testing.T) { { Name: "a", PrivateIPv4Address: fi.String("10.0.0.10"), + AllocationID: nil, }, { Name: "b", PrivateIPv4Address: nil, + AllocationID: nil, }, }, }, @@ -281,6 +285,7 @@ func TestLoadBalancerSubnets(t *testing.T) { { Name: "", PrivateIPv4Address: nil, + AllocationID: nil, }, }, expected: []string{"Required value::spec.api.loadBalancer.subnets[0].name"}, @@ -291,62 +296,103 @@ func TestLoadBalancerSubnets(t *testing.T) { { Name: "d", PrivateIPv4Address: nil, + AllocationID: nil, }, }, expected: []string{"Not found::spec.api.loadBalancer.subnets[0].name"}, }, - { // empty privateIPv4Address + { // empty privateIPv4Address, no allocationID clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: fi.String(""), + AllocationID: nil, }, }, expected: []string{"Required value::spec.api.loadBalancer.subnets[0].privateIPv4Address"}, }, - { // invalid privateIPv4Address + { // empty no privateIPv4Address, with allocationID + clusterSubnets: []string{"a", "b", "c"}, + lbSubnets: []kops.LoadBalancerSubnetSpec{ + { + Name: "a", + PrivateIPv4Address: nil, + AllocationID: fi.String(""), + }, + }, + expected: []string{"Required value::spec.api.loadBalancer.subnets[0].allocationID"}, + }, + { // invalid privateIPv4Address, no allocationID clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: fi.String("invalidip"), + AllocationID: nil, }, }, expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"}, }, - { // privateIPv4Address not matching subnet cidr + { // privateIPv4Address not matching subnet cidr, no allocationID clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: fi.String("11.0.0.10"), + AllocationID: nil, }, }, expected: []string{"Invalid value::spec.api.loadBalancer.subnets[0].privateIPv4Address"}, }, - { // invalid class + { // invalid class - with privateIPv4Address, no allocationID class: fi.String(string(kops.LoadBalancerClassClassic)), clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: fi.String("10.0.0.10"), + AllocationID: nil, }, }, expected: []string{"Forbidden::spec.api.loadBalancer.subnets[0].privateIPv4Address"}, }, - { // invalid type + { // invalid class - no privateIPv4Address, with allocationID + class: fi.String(string(kops.LoadBalancerClassClassic)), + clusterSubnets: []string{"a", "b", "c"}, + lbSubnets: []kops.LoadBalancerSubnetSpec{ + { + Name: "a", + PrivateIPv4Address: nil, + AllocationID: fi.String("eipalloc-222ghi789"), + }, + }, + expected: []string{"Forbidden::spec.api.loadBalancer.subnets[0].allocationID"}, + }, + { // invalid type external for private IP lbType: fi.String(string(kops.LoadBalancerTypePublic)), clusterSubnets: []string{"a", "b", "c"}, lbSubnets: []kops.LoadBalancerSubnetSpec{ { Name: "a", PrivateIPv4Address: fi.String("10.0.0.10"), + AllocationID: nil, }, }, expected: []string{"Forbidden::spec.api.loadBalancer.subnets[0].privateIPv4Address"}, }, + { // invalid type Internal for public IP + lbType: fi.String(string(kops.LoadBalancerTypeInternal)), + clusterSubnets: []string{"a", "b", "c"}, + lbSubnets: []kops.LoadBalancerSubnetSpec{ + { + Name: "a", + PrivateIPv4Address: nil, + AllocationID: fi.String("eipalloc-222ghi789"), + }, + }, + expected: []string{"Forbidden::spec.api.loadBalancer.subnets[0].allocationID"}, + }, } for _, test := range tests { diff --git a/pkg/apis/kops/zz_generated.deepcopy.go b/pkg/apis/kops/zz_generated.deepcopy.go index 37dec81945..c7dc4f94cb 100644 --- a/pkg/apis/kops/zz_generated.deepcopy.go +++ b/pkg/apis/kops/zz_generated.deepcopy.go @@ -3696,6 +3696,11 @@ func (in *LoadBalancerSubnetSpec) DeepCopyInto(out *LoadBalancerSubnetSpec) { *out = new(string) **out = **in } + if in.AllocationID != nil { + in, out := &in.AllocationID, &out.AllocationID + *out = new(string) + **out = **in + } return } diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index ad95969f6b..205cb65607 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -79,6 +79,9 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { if subnet.PrivateIPv4Address != nil { nlbSubnetMapping.PrivateIPv4Address = subnet.PrivateIPv4Address } + if subnet.AllocationID != nil { + nlbSubnetMapping.AllocationID = subnet.AllocationID + } nlbSubnetMappings = append(nlbSubnetMappings, nlbSubnetMapping) break } diff --git a/tests/integration/update_cluster/complex/cloudformation.json b/tests/integration/update_cluster/complex/cloudformation.json index a615e17961..2ee5e35d52 100644 --- a/tests/integration/update_cluster/complex/cloudformation.json +++ b/tests/integration/update_cluster/complex/cloudformation.json @@ -1272,7 +1272,8 @@ { "SubnetId": { "Ref": "AWSEC2Subnetustest1acomplexexamplecom" - } + }, + "AllocationId": "eipalloc-012345a678b9cdefa" } ], "Type": "network", diff --git a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml index 201393efaf..a39d22e007 100644 --- a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml @@ -14,6 +14,9 @@ spec: class: Network sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678 sslPolicy: ELBSecurityPolicy-2016-08 + subnets: + - name: us-test-1a + allocationId: eipalloc-012345a678b9cdefa kubernetesApiAccess: - 1.1.1.0/24 - 2001:0:8500::/40 diff --git a/tests/integration/update_cluster/complex/in-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-v1alpha2.yaml index 91f18da4c5..f1e80a900e 100644 --- a/tests/integration/update_cluster/complex/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-v1alpha2.yaml @@ -13,7 +13,10 @@ spec: crossZoneLoadBalancing: true class: Network sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678 - sslPolicy: ELBSecurityPolicy-2016-08 + sslPolicy: ELBSecurityPolicy-2016-08 + subnets: + - name: us-test-1a + allocationId: eipalloc-012345a678b9cdefa kubernetesApiAccess: - 1.1.1.0/24 - 2001:0:8500::/40 diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf index 9ca30c1915..53b552d1d8 100644 --- a/tests/integration/update_cluster/complex/kubernetes.tf +++ b/tests/integration/update_cluster/complex/kubernetes.tf @@ -541,7 +541,8 @@ resource "aws_lb" "api-complex-example-com" { load_balancer_type = "network" name = "api-complex-example-com-vd3t5n" subnet_mapping { - subnet_id = aws_subnet.us-test-1a-complex-example-com.id + allocation_id = "eipalloc-012345a678b9cdefa" + subnet_id = aws_subnet.us-test-1a-complex-example-com.id } tags = { "KubernetesCluster" = "complex.example.com" diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 12a8aab19a..830074c0f3 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -362,6 +362,12 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) } sm.PrivateIPv4Address = a.PrivateIPv4Address } + if a.AllocationId != nil { + if sm.AllocationID != nil { + return nil, fmt.Errorf("NLB has more then one AllocationID per subnet, which is unexpected. This is a bug in kOps, please open a GitHub issue.") + } + sm.AllocationID = a.AllocationId + } } actual.SubnetMappings = append(actual.SubnetMappings, sm) } @@ -528,7 +534,13 @@ func (s *NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) e if len(changes.SubnetMappings) > 0 { expectedSubnets := make(map[string]*string) for _, s := range e.SubnetMappings { - expectedSubnets[*s.Subnet.ID] = s.PrivateIPv4Address + //expectedSubnets[*s.Subnet.ID] = s + if s.AllocationID != nil { + expectedSubnets[*s.Subnet.ID] = s.AllocationID + } + if s.PrivateIPv4Address != nil { + expectedSubnets[*s.Subnet.ID] = s.PrivateIPv4Address + } } for _, s := range a.SubnetMappings { @@ -536,7 +548,7 @@ func (s *NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) e if !ok { return fmt.Errorf("network load balancers do not support detaching subnets") } - if fi.StringValue(eIP) != fi.StringValue(s.PrivateIPv4Address) { + if fi.StringValue(eIP) != fi.StringValue(s.PrivateIPv4Address) || fi.StringValue(eIP) != fi.StringValue(s.AllocationID) { return fmt.Errorf("network load balancers do not support modifying address settings") } } @@ -573,6 +585,7 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne for _, subnetMapping := range e.SubnetMappings { request.SubnetMappings = append(request.SubnetMappings, &elbv2.SubnetMapping{ SubnetId: subnetMapping.Subnet.ID, + AllocationId: subnetMapping.AllocationID, PrivateIPv4Address: subnetMapping.PrivateIPv4Address, }) } @@ -623,18 +636,25 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne if changes.SubnetMappings != nil { actualSubnets := make(map[string]*string) for _, s := range a.SubnetMappings { - actualSubnets[*s.Subnet.ID] = s.PrivateIPv4Address + //actualSubnets[*s.Subnet.ID] = s + if s.AllocationID != nil { + actualSubnets[*s.Subnet.ID] = s.AllocationID + } + if s.PrivateIPv4Address != nil { + actualSubnets[*s.Subnet.ID] = s.PrivateIPv4Address + } } var awsSubnetMappings []*elbv2.SubnetMapping hasChanges := false for _, s := range e.SubnetMappings { aIP, ok := actualSubnets[*s.Subnet.ID] - if !ok || fi.StringValue(s.PrivateIPv4Address) != fi.StringValue(aIP) { + if !ok || (fi.StringValue(s.PrivateIPv4Address) != fi.StringValue(aIP) && fi.StringValue(s.AllocationID) != fi.StringValue(aIP)) { hasChanges = true } awsSubnetMappings = append(awsSubnetMappings, &elbv2.SubnetMapping{ SubnetId: s.Subnet.ID, + AllocationId: s.AllocationID, PrivateIPv4Address: s.PrivateIPv4Address, }) } @@ -747,6 +767,7 @@ func (_ *NetworkLoadBalancer) RenderTerraform(t *terraform.TerraformTarget, a, e for _, subnetMapping := range e.SubnetMappings { nlbTF.SubnetMappings = append(nlbTF.SubnetMappings, terraformNetworkLoadBalancerSubnetMapping{ Subnet: subnetMapping.Subnet.TerraformLink(), + AllocationID: subnetMapping.AllocationID, PrivateIPv4Address: subnetMapping.PrivateIPv4Address, }) } @@ -844,6 +865,7 @@ func (_ *NetworkLoadBalancer) RenderCloudformation(t *cloudformation.Cloudformat for _, subnetMapping := range e.SubnetMappings { nlbCF.SubnetMappings = append(nlbCF.SubnetMappings, &cloudformationSubnetMapping{ Subnet: subnetMapping.Subnet.CloudformationLink(), + AllocationId: subnetMapping.AllocationID, PrivateIPv4Address: subnetMapping.PrivateIPv4Address, }) } diff --git a/upup/pkg/fi/cloudup/awstasks/subnet_mapping.go b/upup/pkg/fi/cloudup/awstasks/subnet_mapping.go index 5854bab7df..1e91c67c17 100644 --- a/upup/pkg/fi/cloudup/awstasks/subnet_mapping.go +++ b/upup/pkg/fi/cloudup/awstasks/subnet_mapping.go @@ -26,6 +26,8 @@ type SubnetMapping struct { // PrivateIPv4Address only valid for NLBs PrivateIPv4Address *string + // AllocationID only valid for NLBs + AllocationID *string } // OrderSubnetsById implements sort.Interface for []Subnet, based on ID @@ -37,7 +39,12 @@ func (a OrderSubnetMappingsByID) Less(i, j int) bool { v1 := fi.StringValue(a[i].Subnet.ID) v2 := fi.StringValue(a[j].Subnet.ID) if v1 == v2 { - return fi.StringValue(a[i].PrivateIPv4Address) < fi.StringValue(a[j].PrivateIPv4Address) + if a[i].PrivateIPv4Address != nil && a[j].PrivateIPv4Address != nil { + return fi.StringValue(a[i].PrivateIPv4Address) < fi.StringValue(a[j].PrivateIPv4Address) + } + if a[i].AllocationID != nil && a[j].AllocationID != nil { + return fi.StringValue(a[i].AllocationID) < fi.StringValue(a[j].AllocationID) + } } return v1 < v2 } @@ -67,6 +74,9 @@ func subnetMappingSlicesEqualIgnoreOrder(l, r []*SubnetMapping) bool { if fi.StringValue(s.PrivateIPv4Address) != fi.StringValue(s2.PrivateIPv4Address) { return false } + if fi.StringValue(s.AllocationID) != fi.StringValue(s2.AllocationID) { + return false + } } return true } From beef09b49489ea31ec49d7bc6f42a1b087f80bcf Mon Sep 17 00:00:00 2001 From: Timothy Clarke Date: Thu, 18 Feb 2021 14:47:10 +0000 Subject: [PATCH 2/4] Updated documentation for EIP Allocation. Must use utility subnets --- docs/cluster_spec.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md index 555b5bf32b..f4d648db73 100644 --- a/docs/cluster_spec.md +++ b/docs/cluster_spec.md @@ -144,11 +144,11 @@ spec: loadBalancer: type: Public subnets: - - name: subnet-a + - name: utility-subnet-a allocationID: eipalloc-222ghi789 ``` -The specified Allocation ID's must already be created manually or external infrastructure as code, eg Terraform. +The specified Allocation ID's must already be created manually or external infrastructure as code, eg Terraform. You will need to place the loadBalanacer in the utility subnets for external connectivity. If you made a mistake or need to change subnets for any other reason, you're currently forced to manually delete the underlying ELB/NLB and re-run `kops update`. From 1a3c675212c4da751496e130c1a45db8ea553be7 Mon Sep 17 00:00:00 2001 From: Timothy Clarke Date: Fri, 19 Feb 2021 10:46:39 +0000 Subject: [PATCH 3/4] Simplifying conditional per code review comments --- pkg/apis/kops/validation/aws.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/pkg/apis/kops/validation/aws.go b/pkg/apis/kops/validation/aws.go index 9403082f33..e71066e25f 100644 --- a/pkg/apis/kops/validation/aws.go +++ b/pkg/apis/kops/validation/aws.go @@ -242,14 +242,8 @@ func awsValidateLoadBalancerSubnets(fieldPath *field.Path, spec kops.ClusterSpec } } - if subnet.AllocationID != nil { - if *subnet.AllocationID == "" { - allErrs = append(allErrs, field.Required(fieldPath.Index(i).Child("allocationID"), "allocationID can't be empty")) - } - - if lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type == kops.LoadBalancerTypeInternal { - allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("allocationID"), "allocationID only allowed for Public NLBs")) - } + if fi.StringValue(subnet.AllocationID) != "" && (lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type == kops.LoadBalancerTypeInternal) { + allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("allocationID"), "allocationID only allowed for Public NLBs")) } } From d59faa329e86484b011cb336caca006a5fde3681 Mon Sep 17 00:00:00 2001 From: Timothy Clarke Date: Fri, 19 Feb 2021 11:19:36 +0000 Subject: [PATCH 4/4] Revert "Simplifying conditional per code review comments" This reverts commit 1a3c675212c4da751496e130c1a45db8ea553be7 as it turned an optional feature subnets[0].allocationID into a required one --- pkg/apis/kops/validation/aws.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/apis/kops/validation/aws.go b/pkg/apis/kops/validation/aws.go index e71066e25f..9403082f33 100644 --- a/pkg/apis/kops/validation/aws.go +++ b/pkg/apis/kops/validation/aws.go @@ -242,8 +242,14 @@ func awsValidateLoadBalancerSubnets(fieldPath *field.Path, spec kops.ClusterSpec } } - if fi.StringValue(subnet.AllocationID) != "" && (lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type == kops.LoadBalancerTypeInternal) { - allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("allocationID"), "allocationID only allowed for Public NLBs")) + if subnet.AllocationID != nil { + if *subnet.AllocationID == "" { + allErrs = append(allErrs, field.Required(fieldPath.Index(i).Child("allocationID"), "allocationID can't be empty")) + } + + if lbSpec.Class != kops.LoadBalancerClassNetwork || lbSpec.Type == kops.LoadBalancerTypeInternal { + allErrs = append(allErrs, field.Forbidden(fieldPath.Index(i).Child("allocationID"), "allocationID only allowed for Public NLBs")) + } } }