Add option to create an internal load balancer for the bastion

docs/bastion.md

@@ -72,6 +72,19 @@ spec:
       bastionPublicName: bastion.mycluster.example.com
 ```
 
+### Using an internal (VPC only) load balancer 
+{{ kops_feature_table(kops_added_default='1.22') }}
+
+When configuring a LoadBalancer, you can also choose to have a public load balancer or an internal (VPC only) load balancer. The `type` field should be `Public` or `Internal` (defaults to `Public` if omitted).
+
+```yaml
+spec:
+  topology:
+    bastion:
+      loadBalancer:
+        type: "Internal"
+```
+
 ### Additional security groups to ELB
 {{ kops_feature_table(kops_added_default='1.18') }}
 

k8s/crds/kops.k8s.io_clusters.yaml

@@ -4451,6 +4451,10 @@ spec:
                             items:
                               type: string
                             type: array
+                          type:
+                            description: Type of load balancer to create, it can be
+                              Public or Internal.
+                            type: string
                         type: object
                     type: object
                   dns:

pkg/apis/kops/bastion.go

@@ -26,4 +26,6 @@ type BastionSpec struct {
 
 type BastionLoadBalancerSpec struct {
 	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
+	// Type of load balancer to create, it can be Public or Internal.
+	Type LoadBalancerType `json:"type,omitempty"`
 }

pkg/apis/kops/v1alpha2/bastion.go

@@ -25,4 +25,6 @@ type BastionSpec struct {
 
 type BastionLoadBalancerSpec struct {
 	AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"`
+	// Type of load balancer to create, it can be Public or Internal.
+	Type LoadBalancerType `json:"type,omitempty"`
 }

pkg/apis/kops/v1alpha2/zz_generated.conversion.go

@@ -1568,6 +1568,7 @@ func Convert_kops_AzureConfiguration_To_v1alpha2_AzureConfiguration(in *kops.Azu
 
 func autoConvert_v1alpha2_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in *BastionLoadBalancerSpec, out *kops.BastionLoadBalancerSpec, s conversion.Scope) error {
 	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
+	out.Type = kops.LoadBalancerType(in.Type)
 	return nil
 }
 
@@ -1578,6 +1579,7 @@ func Convert_v1alpha2_BastionLoadBalancerSpec_To_kops_BastionLoadBalancerSpec(in
 
 func autoConvert_kops_BastionLoadBalancerSpec_To_v1alpha2_BastionLoadBalancerSpec(in *kops.BastionLoadBalancerSpec, out *BastionLoadBalancerSpec, s conversion.Scope) error {
 	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
+	out.Type = LoadBalancerType(in.Type)
 	return nil
 }
 

pkg/model/awsmodel/bastion.go

@@ -17,9 +17,12 @@ limitations under the License.
 package awsmodel
 
 import (
+	"fmt"
+	"sort"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/klog/v2"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
@@ -100,6 +103,30 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
+	var bastionLoadBalancerType kops.LoadBalancerType
+	{
+		// Check if we requested a public or internal ELB
+		if b.Cluster.Spec.Topology != nil && b.Cluster.Spec.Topology.Bastion != nil && b.Cluster.Spec.Topology.Bastion.LoadBalancer != nil {
+			if b.Cluster.Spec.Topology.Bastion.LoadBalancer.Type != "" {
+				switch b.Cluster.Spec.Topology.Bastion.LoadBalancer.Type {
+				case kops.LoadBalancerTypeInternal:
+					bastionLoadBalancerType = "Internal"
+				case kops.LoadBalancerTypePublic:
+					bastionLoadBalancerType = "Public"
+				default:
+					return fmt.Errorf("unhandled bastion LoadBalancer type %q", b.Cluster.Spec.Topology.Bastion.LoadBalancer.Type)
+				}
+			} else {
+				// Default to Public
+				b.Cluster.Spec.Topology.Bastion.LoadBalancer.Type = kops.LoadBalancerTypePublic
+				bastionLoadBalancerType = "Public"
+			}
+		} else {
+			// Default to Public
+			bastionLoadBalancerType = "Public"
+		}
+	}
+
 	// Allow incoming SSH traffic to bastions, through the ELB
 	// TODO: Could we get away without an ELB here?  Tricky to fix if dns-controller breaks though...
 	for _, dest := range bastionGroups {
@@ -202,23 +229,34 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error {
 
 	var elbSubnets []*awstasks.Subnet
 	{
-		zones := sets.NewString()
+		// Compute the subnets - only one per zone, and then break ties based on chooseBestSubnetForELB
-		for _, ig := range bastionInstanceGroups {
+		subnetsByZone := make(map[string][]*kops.ClusterSubnetSpec)
-			subnets, err := b.GatherSubnets(ig)
+		for i := range b.Cluster.Spec.Subnets {
-			if err != nil {
+			subnet := &b.Cluster.Spec.Subnets[i]
-				return err
+
-			}
+			switch subnet.Type {
-			for _, s := range subnets {
+			case kops.SubnetTypePublic, kops.SubnetTypeUtility:
-				zones.Insert(s.Zone)
+				if bastionLoadBalancerType != kops.LoadBalancerTypePublic {
+					continue
+				}
+
+			case kops.SubnetTypePrivate:
+				if bastionLoadBalancerType != kops.LoadBalancerTypeInternal {
+					continue
+				}
+
+			default:
+				return fmt.Errorf("subnet %q had unknown type %q", subnet.Name, subnet.Type)
 			}
+
+			subnetsByZone[subnet.Zone] = append(subnetsByZone[subnet.Zone], subnet)
 		}
 
-		for zoneName := range zones {
+		for zone, subnets := range subnetsByZone {
-			utilitySubnet, err := b.LinkToUtilitySubnetInZone(zoneName)
+			subnet := b.chooseBestSubnetForELB(zone, subnets)
-			if err != nil {
+
-				return err
+			elbSubnet := b.LinkToSubnet(subnet)
-			}
+			elbSubnets = append(elbSubnets, elbSubnet)
-			elbSubnets = append(elbSubnets, utilitySubnet)
 		}
 	}
 
@@ -281,6 +319,15 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error {
 				elb.SecurityGroups = append(elb.SecurityGroups, t)
 			}
 		}
+		// Set the elb Scheme according to load balancer Type
+		switch bastionLoadBalancerType {
+		case kops.LoadBalancerTypeInternal:
+			elb.Scheme = fi.String("internal")
+		case kops.LoadBalancerTypePublic:
+			elb.Scheme = nil
+		default:
+			return fmt.Errorf("unhandled bastion LoadBalancer type %q", bastionLoadBalancerType)
+		}
 
 		c.AddTask(elb)
 	}
@@ -306,3 +353,49 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	}
 	return nil
 }
+
+// Choose between subnets in a zone.
+// We have already applied the rules to match internal subnets to internal ELBs and vice-versa for public-facing ELBs.
+// For internal ELBs: we prefer the master subnets
+// For public facing ELBs: we prefer the utility subnets
+func (b *BastionModelBuilder) chooseBestSubnetForELB(zone string, subnets []*kops.ClusterSubnetSpec) *kops.ClusterSubnetSpec {
+	if len(subnets) == 0 {
+		return nil
+	}
+	if len(subnets) == 1 {
+		return subnets[0]
+	}
+
+	migSubnets := sets.NewString()
+	for _, ig := range b.MasterInstanceGroups() {
+		for _, subnet := range ig.Spec.Subnets {
+			migSubnets.Insert(subnet)
+		}
+	}
+
+	var scoredSubnets []*scoredSubnet
+	for _, subnet := range subnets {
+		score := 0
+
+		if migSubnets.Has(subnet.Name) {
+			score += 1
+		}
+
+		if subnet.Type == kops.SubnetTypeUtility {
+			score += 1
+		}
+
+		scoredSubnets = append(scoredSubnets, &scoredSubnet{
+			score:  score,
+			subnet: subnet,
+		})
+	}
+
+	sort.Sort(ByScoreDescending(scoredSubnets))
+
+	if scoredSubnets[0].score == scoredSubnets[1].score {
+		klog.V(2).Infof("Making arbitrary choice between subnets in zone %q to attach to ELB (%q vs %q)", zone, scoredSubnets[0].subnet.Name, scoredSubnets[1].subnet.Name)
+	}
+
+	return scoredSubnets[0].subnet
+}

tests/integration/update_cluster/privatekopeio/kubernetes.tf

@@ -369,7 +369,7 @@ resource "aws_elb" "bastion-privatekopeio-example-com" {
   }
   name            = "bastion-privatekopeio-exa-d8ef8e"
   security_groups = [aws_security_group.bastion-elb-privatekopeio-example-com.id]
-  subnets         = [aws_subnet.utility-us-test-1a-privatekopeio-example-com.id]
+  subnets         = [aws_subnet.utility-us-test-1a-privatekopeio-example-com.id, aws_subnet.utility-us-test-1b-privatekopeio-example-com.id]
   tags = {
     "KubernetesCluster"                               = "privatekopeio.example.com"
     "Name"                                            = "bastion.privatekopeio.example.com"

tests/integration/update_cluster/unmanaged/kubernetes.tf

@@ -349,7 +349,7 @@ resource "aws_elb" "bastion-unmanaged-example-com" {
   }
   name            = "bastion-unmanaged-example-d7bn3d"
   security_groups = [aws_security_group.bastion-elb-unmanaged-example-com.id]
-  subnets         = [aws_subnet.utility-us-test-1a-unmanaged-example-com.id]
+  subnets         = [aws_subnet.utility-us-test-1a-unmanaged-example-com.id, aws_subnet.utility-us-test-1b-unmanaged-example-com.id]
   tags = {
     "KubernetesCluster"                           = "unmanaged.example.com"
     "Name"                                        = "bastion.unmanaged.example.com"

upup/pkg/fi/cloudup/new_cluster.go

@@ -124,6 +124,9 @@ type NewClusterOptions struct {
 	NodeCount int32
 	// Bastion enables the creation of a Bastion instance.
 	Bastion bool
+	// BastionLoadBalancerType is the bastion loadbalancer type to use; "public" or "internal".
+	// Defaults to "public".
+	BastionLoadBalancerType string
 
 	// Networking is the networking provider/node to use.
 	Networking string