From 61763d488a4ab5d76aac12ece983e451f726e18b Mon Sep 17 00:00:00 2001 From: Simone Sciarrati Date: Fri, 27 Aug 2021 11:32:14 +0200 Subject: [PATCH] Add option to create an internal load balancer for the bastion --- docs/bastion.md | 13 ++ k8s/crds/kops.k8s.io_clusters.yaml | 4 + pkg/apis/kops/bastion.go | 2 + pkg/apis/kops/v1alpha2/bastion.go | 2 + .../kops/v1alpha2/zz_generated.conversion.go | 2 + pkg/model/awsmodel/bastion.go | 121 ++++++++++++++++-- .../privatekopeio/kubernetes.tf | 2 +- .../update_cluster/unmanaged/kubernetes.tf | 2 +- upup/pkg/fi/cloudup/new_cluster.go | 3 + 9 files changed, 135 insertions(+), 16 deletions(-) diff --git a/docs/bastion.md b/docs/bastion.md index 0d748c7d21..db9933f2d0 100644 --- a/docs/bastion.md +++ b/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') }} diff --git a/k8s/crds/kops.k8s.io_clusters.yaml b/k8s/crds/kops.k8s.io_clusters.yaml index 8a879536c8..5f0e56b26d 100644 --- a/k8s/crds/kops.k8s.io_clusters.yaml +++ b/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: diff --git a/pkg/apis/kops/bastion.go b/pkg/apis/kops/bastion.go index b8723b02e4..4033ffc267 100644 --- a/pkg/apis/kops/bastion.go +++ b/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"` } diff --git a/pkg/apis/kops/v1alpha2/bastion.go b/pkg/apis/kops/v1alpha2/bastion.go index b4197d12e1..2a8bd287ec 100644 --- a/pkg/apis/kops/v1alpha2/bastion.go +++ b/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"` } diff --git a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go b/pkg/apis/kops/v1alpha2/zz_generated.conversion.go index 3be1e3b1b2..dd36834c2e 100644 --- a/pkg/apis/kops/v1alpha2/zz_generated.conversion.go +++ b/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 } diff --git a/pkg/model/awsmodel/bastion.go b/pkg/model/awsmodel/bastion.go index a6567e9786..1975049645 100644 --- a/pkg/model/awsmodel/bastion.go +++ b/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() - for _, ig := range bastionInstanceGroups { - subnets, err := b.GatherSubnets(ig) - if err != nil { - return err - } - for _, s := range subnets { - zones.Insert(s.Zone) + // Compute the subnets - only one per zone, and then break ties based on chooseBestSubnetForELB + subnetsByZone := make(map[string][]*kops.ClusterSubnetSpec) + for i := range b.Cluster.Spec.Subnets { + subnet := &b.Cluster.Spec.Subnets[i] + + switch subnet.Type { + case kops.SubnetTypePublic, kops.SubnetTypeUtility: + 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 { - utilitySubnet, err := b.LinkToUtilitySubnetInZone(zoneName) - if err != nil { - return err - } - elbSubnets = append(elbSubnets, utilitySubnet) + for zone, subnets := range subnetsByZone { + subnet := b.chooseBestSubnetForELB(zone, subnets) + + elbSubnet := b.LinkToSubnet(subnet) + elbSubnets = append(elbSubnets, elbSubnet) } } @@ -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 +} diff --git a/tests/integration/update_cluster/privatekopeio/kubernetes.tf b/tests/integration/update_cluster/privatekopeio/kubernetes.tf index ce3803ca24..330afea752 100644 --- a/tests/integration/update_cluster/privatekopeio/kubernetes.tf +++ b/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" diff --git a/tests/integration/update_cluster/unmanaged/kubernetes.tf b/tests/integration/update_cluster/unmanaged/kubernetes.tf index 3ffd3e90c5..7ec96d676c 100644 --- a/tests/integration/update_cluster/unmanaged/kubernetes.tf +++ b/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" diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index 74b7f38864..caba31949f 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/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