From aced1cbe1827887d63696bf33c49d71c91739361 Mon Sep 17 00:00:00 2001 From: Jeroen van Erp Date: Wed, 19 Jan 2022 10:12:13 +0100 Subject: [PATCH] Incorporate review comments Signed-off-by: Jeroen van Erp --- docs/cluster_spec.md | 4 +- pkg/apis/kops/validation/validation.go | 12 +++- pkg/model/awsmodel/BUILD.bazel | 1 - pkg/model/awsmodel/api_loadbalancer.go | 63 ++++++++++++++++--- pkg/model/awsmodel/bastion.go | 2 +- pkg/model/awsmodel/external_access.go | 10 +-- pkg/model/awsmodel/utils.go | 42 ------------- upup/pkg/fi/cloudup/apply_cluster.go | 2 +- .../fi/cloudup/awstasks/securitygrouprule.go | 11 ++++ 9 files changed, 82 insertions(+), 65 deletions(-) delete mode 100644 pkg/model/awsmodel/utils.go diff --git a/docs/cluster_spec.md b/docs/cluster_spec.md index f1e19f53f0ac0..0381f1cc60e7a 100644 --- a/docs/cluster_spec.md +++ b/docs/cluster_spec.md @@ -257,7 +257,7 @@ spec: - 12.34.56.78/32 ``` -Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID +Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID. ## kubernetesApiAccess @@ -271,7 +271,7 @@ spec: - 12.34.56.78/32 ``` -Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID +Instead of listing all CIDRs, it is possible to specify a pre-existing [AWS Prefix List](https://docs.aws.amazon.com/vpc/latest/userguide/managed-prefix-lists.html) ID. ## cluster.spec Subnet Keys diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index c4f2d014c82e4..a3cb3e7731475 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -85,21 +85,27 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie // SSHAccess for i, cidr := range spec.SSHAccess { - if !strings.HasPrefix(cidr, "pl-") { + if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("sshAccess").Index(i), cidr, "Prefix List ID only supported for AWS")) + } else { allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("sshAccess").Index(i))...) } } // KubernetesAPIAccess for i, cidr := range spec.KubernetesAPIAccess { - if !strings.HasPrefix(cidr, "pl-") { + if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("kubernetesAPIAccess").Index(i), cidr, "Prefix List ID only supported for AWS")) + } else { allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("kubernetesAPIAccess").Index(i))...) } } // NodePortAccess for i, cidr := range spec.NodePortAccess { - if !strings.HasPrefix(cidr, "pl-") { + if strings.HasPrefix(cidr, "pl-") && kops.CloudProviderID(spec.CloudProvider) != kops.CloudProviderAWS { + allErrs = append(allErrs, field.Invalid(fieldPath.Child("nodePortAccess").Index(i), cidr, "Prefix List ID only supported for AWS")) + } else { allErrs = append(allErrs, validateCIDR(cidr, fieldPath.Child("nodePortAccess").Index(i))...) } } diff --git a/pkg/model/awsmodel/BUILD.bazel b/pkg/model/awsmodel/BUILD.bazel index 44ad14eee1a0f..06360d4a59c43 100644 --- a/pkg/model/awsmodel/BUILD.bazel +++ b/pkg/model/awsmodel/BUILD.bazel @@ -16,7 +16,6 @@ go_library( "oidc_provider.go", "spotinst.go", "sshkey.go", - "utils.go", ], importpath = "k8s.io/kops/pkg/model/awsmodel", visibility = ["//visibility:public"], diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index 47e2318fc1760..aefe24a036f23 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -19,17 +19,27 @@ package awsmodel import ( "fmt" "sort" + "strings" "time" + "github.com/aws/aws-sdk-go/service/ec2" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/dns" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" + "k8s.io/kops/upup/pkg/fi/cloudup/awsup" "k8s.io/kops/upup/pkg/fi/utils" ) +type PrefixListAddressFamily string + +const ( + IPv4AddressFamily PrefixListAddressFamily = "IPv4" + IPv6AddressFamily PrefixListAddressFamily = "IPv6" +) + // LoadBalancerDefaultIdleTimeout is the default idle time for the ELB const LoadBalancerDefaultIdleTimeout = 5 * time.Minute @@ -39,6 +49,7 @@ type APILoadBalancerBuilder struct { Lifecycle fi.Lifecycle SecurityLifecycle fi.Lifecycle + Cloud awsup.AWSCloud } var _ fi.ModelBuilder = &APILoadBalancerBuilder{} @@ -368,7 +379,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { SecurityGroup: lbSG, ToPort: fi.Int64(443), } - LimitIngress(t, cidr) + t.SetCidrOrPrefix(cidr) AddDirectionalGroupRule(c, t) } @@ -414,23 +425,35 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { SecurityGroup: masterGroup.Task, ToPort: fi.Int64(443), } - LimitIngress(t, cidr) + t.SetCidrOrPrefix(cidr) AddDirectionalGroupRule(c, t) } + icmpIpv6 := false + if strings.HasPrefix(cidr, "pl-") { + b, err := b.IsIPv6PrefixList(cidr) + if err != nil { + return err + } + icmpIpv6 = b + } else { + icmpIpv6 = utils.IsIPv6CIDR(cidr) + } + // Allow ICMP traffic required for PMTU discovery - if utils.IsIPv6CIDR(cidr) { - c.AddTask(&awstasks.SecurityGroupRule{ + if icmpIpv6 { + t := &awstasks.SecurityGroupRule{ Name: fi.String("icmpv6-pmtu-api-elb-" + cidr), Lifecycle: b.SecurityLifecycle, - IPv6CIDR: fi.String(cidr), FromPort: fi.Int64(-1), Protocol: fi.String("icmpv6"), SecurityGroup: masterGroup.Task, ToPort: fi.Int64(-1), - }) - } else if utils.IsIPv4CIDR(cidr) { - c.AddTask(&awstasks.SecurityGroupRule{ + } + t.SetCidrOrPrefix(cidr) + c.AddTask(t) + } else { + t := &awstasks.SecurityGroupRule{ Name: fi.String("icmp-pmtu-api-elb-" + cidr), Lifecycle: b.SecurityLifecycle, CIDR: fi.String(cidr), @@ -438,7 +461,9 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { Protocol: fi.String("icmp"), SecurityGroup: masterGroup.Task, ToPort: fi.Int64(4), - }) + } + t.SetCidrOrPrefix(cidr) + c.AddTask(t) } if b.Cluster.Spec.API != nil && b.Cluster.Spec.API.LoadBalancer != nil && b.Cluster.Spec.API.LoadBalancer.SSLCertificate != "" { @@ -451,7 +476,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { SecurityGroup: masterGroup.Task, ToPort: fi.Int64(8443), } - LimitIngress(t, cidr) + t.SetCidrOrPrefix(cidr) c.AddTask(t) } } @@ -524,6 +549,24 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { return nil } +func (b *APILoadBalancerBuilder) IsIPv6PrefixList(prefixListID string) (bool, error) { + out, err := b.Cloud.EC2().DescribeManagedPrefixLists(&ec2.DescribeManagedPrefixListsInput{PrefixListIds: []*string{fi.String(prefixListID)}}) + if err != nil { + return false, fmt.Errorf("error describing prefix list %s: %w", prefixListID, err) + } + + if len(out.PrefixLists) != 1 { + return false, fmt.Errorf("unexpected number of prefix lists: %d", len(out.PrefixLists)) + } + + prefixList := out.PrefixLists[0] + if prefixList.AddressFamily != nil { + return PrefixListAddressFamily(fi.StringValue(prefixList.AddressFamily)) == IPv6AddressFamily, nil + } else { + return false, fmt.Errorf("unknown prefix list address family for prefix list: %s", prefixListID) + } +} + type scoredSubnet struct { score int subnet *kops.ClusterSubnetSpec diff --git a/pkg/model/awsmodel/bastion.go b/pkg/model/awsmodel/bastion.go index 3b060a0fc73a2..e7c7d3a28c9d6 100644 --- a/pkg/model/awsmodel/bastion.go +++ b/pkg/model/awsmodel/bastion.go @@ -218,7 +218,7 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(22), ToPort: fi.Int64(22), } - LimitIngress(t, sshAccess) + t.SetCidrOrPrefix(sshAccess) AddDirectionalGroupRule(c, t) } diff --git a/pkg/model/awsmodel/external_access.go b/pkg/model/awsmodel/external_access.go index 05fd8cba50da3..5aa5a0467da7c 100644 --- a/pkg/model/awsmodel/external_access.go +++ b/pkg/model/awsmodel/external_access.go @@ -70,7 +70,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(22), ToPort: fi.Int64(22), } - LimitIngress(t, sshAccess) + t.SetCidrOrPrefix(sshAccess) AddDirectionalGroupRule(c, t) } @@ -84,7 +84,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(22), ToPort: fi.Int64(22), } - LimitIngress(t, sshAccess) + t.SetCidrOrPrefix(sshAccess) AddDirectionalGroupRule(c, t) } } @@ -107,7 +107,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(int64(nodePortRange.Base)), ToPort: fi.Int64(int64(nodePortRange.Base + nodePortRange.Size - 1)), } - LimitIngress(t, nodePortAccess) + t.SetCidrOrPrefix(nodePortAccess) c.AddTask(t) } { @@ -119,7 +119,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(int64(nodePortRange.Base)), ToPort: fi.Int64(int64(nodePortRange.Base + nodePortRange.Size - 1)), } - LimitIngress(t, nodePortAccess) + t.SetCidrOrPrefix(nodePortAccess) c.AddTask(t) } } @@ -142,7 +142,7 @@ func (b *ExternalAccessModelBuilder) Build(c *fi.ModelBuilderContext) error { FromPort: fi.Int64(443), ToPort: fi.Int64(443), } - LimitIngress(t, apiAccess) + t.SetCidrOrPrefix(apiAccess) AddDirectionalGroupRule(c, t) } } diff --git a/pkg/model/awsmodel/utils.go b/pkg/model/awsmodel/utils.go deleted file mode 100644 index 0622b46bac0c0..0000000000000 --- a/pkg/model/awsmodel/utils.go +++ /dev/null @@ -1,42 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package awsmodel - -import ( - "strings" - - "k8s.io/kops/upup/pkg/fi" - "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" - "k8s.io/kops/upup/pkg/fi/utils" -) - -// IsPrefixListID checks whether the string resembles a prefix list ID -func IsPrefixListID(id string) bool { - return strings.HasPrefix(id, "pl-") -} - -func LimitIngress(t *awstasks.SecurityGroupRule, cidr string) *awstasks.SecurityGroupRule { - if IsPrefixListID(cidr) { - t.PrefixList = fi.String(cidr) - } else if utils.IsIPv6CIDR(cidr) { - t.IPv6CIDR = fi.String(cidr) - } else { - t.CIDR = fi.String(cidr) - } - - return t -} diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index 15d4eedab9215..951d34947b18f 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -536,7 +536,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { } l.Builders = append(l.Builders, - &awsmodel.APILoadBalancerBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle}, + &awsmodel.APILoadBalancerBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle, Cloud: cloud.(awsup.AWSCloud)}, &awsmodel.BastionModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle}, &awsmodel.DNSModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle}, &awsmodel.ExternalAccessModelBuilder{AWSModelContext: awsModelContext, Lifecycle: securityLifecycle}, diff --git a/upup/pkg/fi/cloudup/awstasks/securitygrouprule.go b/upup/pkg/fi/cloudup/awstasks/securitygrouprule.go index c619d102b110a..5362525efb6f0 100644 --- a/upup/pkg/fi/cloudup/awstasks/securitygrouprule.go +++ b/upup/pkg/fi/cloudup/awstasks/securitygrouprule.go @@ -29,6 +29,7 @@ import ( "k8s.io/kops/upup/pkg/fi/cloudup/cloudformation" "k8s.io/kops/upup/pkg/fi/cloudup/terraform" "k8s.io/kops/upup/pkg/fi/cloudup/terraformWriter" + "k8s.io/kops/upup/pkg/fi/utils" ) // +kops:fitask @@ -139,6 +140,16 @@ func (e *SecurityGroupRule) Find(c *fi.Context) (*SecurityGroupRule, error) { return nil, nil } +func (e *SecurityGroupRule) SetCidrOrPrefix(cidr string) { + if strings.HasPrefix(cidr, "pl-") { + e.PrefixList = &cidr + } else if utils.IsIPv6CIDR(cidr) { + e.IPv6CIDR = &cidr + } else { + e.CIDR = &cidr + } +} + func (e *SecurityGroupRule) matches(rule *ec2.SecurityGroupRule) bool { matchFromPort := int64(-1) if e.FromPort != nil {