Do not add PMTU ICMP Security group rules when using a prefix list

kubernetes · Feb 15, 2022 · 11c1137 · 11c1137
1 parent c0dab57
commit 11c1137
Show file tree

Hide file tree

Showing 5 changed files with 6 additions and 70 deletions.
diff --git a/pkg/model/awsmodel/BUILD.bazel b/pkg/model/awsmodel/BUILD.bazel
diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go
@@ -22,25 +22,15 @@ import (
 	"strings"
 	"time"
 
-	"github.com/aws/aws-sdk-go/aws/awserr"
-	"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
 
@@ -50,7 +40,6 @@ type APILoadBalancerBuilder struct {
 
 	Lifecycle         fi.Lifecycle
 	SecurityLifecycle fi.Lifecycle
-	Cloud             awsup.AWSCloud
 }
 
 var _ fi.ModelBuilder = &APILoadBalancerBuilder{}
@@ -430,19 +419,11 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 					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 icmpIpv6 {
+					// In case of a prefix list we do not add a rule for ICMP traffic for PMTU discovery.
+					// This would require calling out to AWS to check whether the prefix list is IPv4 or IPv6.
+				} else if utils.IsIPv6CIDR(cidr) {
+					// Allow ICMP traffic required for PMTU discovery
 					t := &awstasks.SecurityGroupRule{
 						Name:          fi.String("icmpv6-pmtu-api-elb-" + cidr),
 						Lifecycle:     b.SecurityLifecycle,
@@ -453,7 +434,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 					}
 					t.SetCidrOrPrefix(cidr)
 					c.AddTask(t)
-				} else {
+				} else if utils.IsIPv4CIDR(cidr) {
 					t := &awstasks.SecurityGroupRule{
 						Name:          fi.String("icmp-pmtu-api-elb-" + cidr),
 						Lifecycle:     b.SecurityLifecycle,
@@ -549,29 +530,6 @@ 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 {
-		if awsErr, ok := err.(awserr.Error); ok {
-			if awsErr.Code() == "InvalidPrefixListID.NotFound" {
-				return false, fmt.Errorf("Prefix list %q not found", prefixListID)
-			}
-		}
-		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/tests/integration/update_cluster/complex/cloudformation.json b/tests/integration/update_cluster/complex/cloudformation.json
@@ -939,18 +939,6 @@
         "CidrIp": "1.1.1.0/24"
       }
     },
-    "AWSEC2SecurityGroupIngressicmppmtuapielbpl44444444": {
-      "Type": "AWS::EC2::SecurityGroupIngress",
-      "Properties": {
-        "GroupId": {
-          "Ref": "AWSEC2SecurityGroupmasterscomplexexamplecom"
-        },
-        "FromPort": 3,
-        "ToPort": 4,
-        "IpProtocol": "icmp",
-        "SourcePrefixListId": "pl-44444444"
-      }
-    },
     "AWSEC2SecurityGroupIngressnodeporttcpexternaltonode102030024": {
       "Type": "AWS::EC2::SecurityGroupIngress",
       "Properties": {

diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf
@@ -1046,15 +1046,6 @@ resource "aws_security_group_rule" "icmp-pmtu-api-elb-1-1-1-0--24" {
   type              = "ingress"
 }
 
-resource "aws_security_group_rule" "icmp-pmtu-api-elb-pl-44444444" {
-  from_port         = 3
-  prefix_list_ids   = ["pl-44444444"]
-  protocol          = "icmp"
-  security_group_id = aws_security_group.masters-complex-example-com.id
-  to_port           = 4
-  type              = "ingress"
-}
-
 resource "aws_security_group_rule" "nodeport-tcp-external-to-node-1-2-3-4--32" {
   cidr_blocks       = ["1.2.3.4/32"]
   from_port         = 28000

diff --git 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, Cloud: cloud.(awsup.AWSCloud)},
+				&awsmodel.APILoadBalancerBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle},
 				&awsmodel.BastionModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle, SecurityLifecycle: securityLifecycle},
 				&awsmodel.DNSModelBuilder{AWSModelContext: awsModelContext, Lifecycle: clusterLifecycle},
 				&awsmodel.ExternalAccessModelBuilder{AWSModelContext: awsModelContext, Lifecycle: securityLifecycle},