addressing review comments

cloudmock/aws/mockautoscaling/group.go

@@ -126,6 +126,16 @@ func (m *MockAutoscaling) EnableMetricsCollection(request *autoscaling.EnableMet
 	return response, nil
 }
 
+func (m *MockAutoscaling) DescribeLoadBalancerTargetGroups(*autoscaling.DescribeLoadBalancerTargetGroupsInput) (*autoscaling.DescribeLoadBalancerTargetGroupsOutput, error) {
+	//klog.Fatalf("Not implemented")
+	return nil, nil
+}
+
+func (m *MockAutoscaling) DescribeLoadBalancers(*autoscaling.DescribeLoadBalancersInput) (*autoscaling.DescribeLoadBalancersOutput, error) {
+	//klog.Fatalf("Not implemented")
+	return nil, nil
+}
+
 func (m *MockAutoscaling) DescribeAutoScalingGroups(input *autoscaling.DescribeAutoScalingGroupsInput) (*autoscaling.DescribeAutoScalingGroupsOutput, error) {
 	m.mutex.Lock()
 	defer m.mutex.Unlock()

docs/cli/kops_create_cluster.md

@@ -66,6 +66,7 @@ kops create cluster [flags]
 
 ```
       --admin-access strings             Restrict API access to this CIDR.  If not set, access will not be restricted by IP. (default [0.0.0.0/0])
+      --api-loadbalancer-class string    Currently only supported in AWS. Sets the API loadbalancer class to eiether 'classic' or 'network'
       --api-loadbalancer-type string     Sets the API loadbalancer type to either 'public' or 'internal'
       --api-ssl-certificate string       Currently only supported in AWS. Sets the ARN of the SSL Certificate to use for the API server loadbalancer.
       --associate-public-ip              Specify --associate-public-ip=[true|false] to enable/disable association of public IP for master ASG and nodes. Default is 'true'.

docs/cluster_spec.md

@@ -81,14 +81,15 @@ spec:
 ```
 
 *AWS only*
-You can choose to have a Network Load Balancer instead of a Classsic Load Balancer.  The `class`
+You can choose to have a Network Load Balancer instead of a Classic Load Balancer.  The `class`
 field should be either `Network` or `Classic` (default). Note: changing the class of load balancer in an existing
 cluster is a disruptive operation. Until the masters have gone through a rolling update, new connections to the apiserver will fail due to the old master's TLS certificates containing the old load balancer's IP address.
 ```yaml
 spec:
   api:
     loadBalancer:
-      class : Network
+      class: Network
+      type: Public
 ```
 
 ## etcdClusters

go.mod

@@ -116,6 +116,7 @@ require (
 	k8s.io/component-base v0.19.0
 	k8s.io/gengo v0.0.0-20200710205751-c0d492a0f3ca
 	k8s.io/helm v2.9.0+incompatible
+	k8s.io/klog v1.0.0
 	k8s.io/klog/v2 v2.3.0
 	k8s.io/kubectl v0.0.0
 	k8s.io/legacy-cloud-providers v0.0.0

k8s/crds/kops.k8s.io_clusters.yaml

@@ -74,8 +74,7 @@ spec:
                           type: string
                         type: array
                       class:
-                        description: 'Class determines the type of API Loadbalancer.
-                          Valid values:   ''network'' use aws nlb or ''classic'' use aws elb (default)'
+                        description: 'LoadBalancerClass specifies the class of load balancer to create: classic, network'
                         type: string
                       crossZoneLoadBalancing:
                         description: CrossZoneLoadBalancing allows you to enable the cross zone load balancing

pkg/commands/BUILD.bazel

@@ -29,6 +29,8 @@ go_library(
         "//upup/pkg/fi/cloudup/openstack:go_default_library",
         "//util/pkg/reflectutils:go_default_library",
         "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library",
+        "//vendor/github.com/aws/aws-sdk-go/service/elb:go_default_library",
+        "//vendor/github.com/aws/aws-sdk-go/service/elbv2:go_default_library",
         "//vendor/github.com/spf13/cobra:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/util/validation/field:go_default_library",

pkg/commands/status_discovery.go

@@ -62,55 +62,34 @@ func (s *CloudDiscoveryStatusStore) GetApiIngressStatus(cluster *kops.Cluster) (
 	if awsCloud, ok := cloud.(awsup.AWSCloud); ok {
 		name := "api." + cluster.Name
 
-		ELB, NLB := false, false
-
-		lbSpec := cluster.Spec.API.LoadBalancer
-		switch lbSpec.Class {
-		case kops.LoadBalancerClassClassic, "":
-			ELB = true
-		case kops.LoadBalancerClassNetwork:
-			NLB = true
-		default:
-			return nil, fmt.Errorf("Unknown Cluster.Spec.API.LoadBalancer.Class : %v", lbSpec.Class)
-		}
-
+		var lbDnsName string
 		var lb interface{}
 
-		if ELB {
-			lb, err = awstasks.FindLoadBalancerByNameTag(awsCloud, name)
-			if isNil(lb) {
-				return nil, nil
-			}
+		lb, err = awstasks.FindLoadBalancerByNameTag(awsCloud, name)
+		if err != nil {
+			return nil, fmt.Errorf("error looking for AWS ELB: %v", err)
+		}
+		if !isNil(lb) {
+			lbDnsName = aws.StringValue(lb.(*elb.LoadBalancerDescription).DNSName)
+		} else {
+			lb, err = awstasks.FindNetworkLoadBalancerByNameTag(awsCloud, name)
 			if err != nil {
-				return nil, fmt.Errorf("error looking for AWS ELB: %v", err)
+				return nil, fmt.Errorf("error looking for AWS NLB: %v", err)
 			}
-		} else if NLB {
-			lb, err = awstasks.FindNetworkLoadBalancerByNameTag(awsCloud, name)
 			if isNil(lb) {
 				return nil, nil
 			}
-			if err != nil {
-				return nil, fmt.Errorf("error looking for AWS ELB: %v", err)
-			}
-
+			lbDnsName = aws.StringValue(lb.(*elbv2.LoadBalancer).DNSName)
 		}
-		var ingresses []kops.ApiIngressStatus
 
-		if !isNil(lb) {
-			var lbDnsName string
-			if ELB {
-				lbDnsName = aws.StringValue(lb.(*elb.LoadBalancerDescription).DNSName)
-			} else if NLB {
-				lbDnsName = aws.StringValue(lb.(*elbv2.LoadBalancer).DNSName)
-			}
-			//lbDnsName := aws.StringValue(lb.DNSName)
-			if lbDnsName == "" {
-				return nil, fmt.Errorf("found api LB %q, but it did not have a DNSName", name)
-			}
+		var ingresses []kops.ApiIngressStatus
 
-			ingresses = append(ingresses, kops.ApiIngressStatus{Hostname: lbDnsName})
+		if lbDnsName == "" {
+			return nil, fmt.Errorf("found api LB %q, but it did not have a DNSName", name)
 		}
 
+		ingresses = append(ingresses, kops.ApiIngressStatus{Hostname: lbDnsName})
+
 		return ingresses, nil
 	}
 

pkg/model/awsmodel/api_loadbalancer.go

@@ -45,18 +45,11 @@ var _ fi.ModelBuilder = &APILoadBalancerBuilder{}
 
 // Build is responsible for building the KubeAPI tasks for the aws model
 func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
-	NLB, ELB := false, false //TOOD: change ot using lbSpec.Type == kops.LoadBalancerClassClassic && lbSpec.Type == kops.LoadBalancerClassNetwork below
 	{
+		var Class *string
+
 		if b.UseLoadBalancerForAPI() {
-			lbSpec := b.Cluster.Spec.API.LoadBalancer
-			switch lbSpec.Class {
-			case kops.LoadBalancerClassClassic, "":
-				ELB = true
-			case kops.LoadBalancerClassNetwork:
-				NLB = true
-			default:
-				return fmt.Errorf("Unknown Cluster.Spec.API.LoadBalancer.Class : %v", lbSpec.Class)
-			}
+			Class = fi.String(string(b.Cluster.Spec.API.LoadBalancer.Class))
 		}
 
 		var agNames []*string
@@ -68,18 +61,16 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 
 		cleanup := &awstasks.LoadBalancerCleanup{
-			Name:         fi.String("cleanup.api." + b.ClusterName()),
-			AgNames:      agNames,
-			Lifecycle:    b.Lifecycle, //what does this even do?
-			UseELBForAPI: fi.Bool(ELB),
-			UseNLBForAPI: fi.Bool(NLB),
-			NLBName:      fi.String("api." + b.ClusterName()),
-			ELBName:      fi.String("api." + b.ClusterName()), //TODO: think about changing their names?
+			Name:                  fi.String("cleanup.api." + b.ClusterName()),
+			AgNames:               agNames,
+			Lifecycle:             b.Lifecycle,
+			LoadBalancerSpecClass: Class,
+			LBName:                fi.String("api." + b.ClusterName()),
 		}
 		c.AddTask(cleanup)
 	}
 
-	// Configuration where an ELB fronts the API
+	// Configuration where an LB fronts the API
 	if !b.UseLoadBalancerForAPI() {
 		return nil
 	}
@@ -230,12 +221,12 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 			elb.Scheme = nil
 			nlb.Scheme = nil
 		default:
-			return fmt.Errorf("unknown elb/nlb Type: %q", lbSpec.Type)
+			return fmt.Errorf("unknown load balancer Type: %q", lbSpec.Type)
 		}
 
-		if ELB {
+		if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 			c.AddTask(elb)
-		} else if NLB {
+		} else if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork {
 			c.AddTask(nlb)
 		}
 
@@ -267,7 +258,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 	}
 
 	// Allow traffic from ELB to egress freely
-	if ELB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 		t := &awstasks.SecurityGroupRule{
 			Name:          fi.String("api-elb-egress"),
 			Lifecycle:     b.SecurityLifecycle,
@@ -279,7 +270,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 	}
 
 	// Allow traffic into the ELB from KubernetesAPIAccess CIDRs
-	if ELB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 		for _, cidr := range b.Cluster.Spec.KubernetesAPIAccess {
 			t := &awstasks.SecurityGroupRule{
 				Name:          fi.String("https-api-elb-" + cidr),
@@ -310,13 +301,14 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		return err
 	}
 
-	if NLB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork {
 		for _, cidr := range b.Cluster.Spec.KubernetesAPIAccess {
 
 			for _, masterGroup := range masterGroups {
 				suffix := masterGroup.Suffix
+				//TODO: In a recent test, I did not see this security group on the master nodes: Verify why Security group on master nodes does not have the VPC CIDR
 				t := &awstasks.SecurityGroupRule{
-					Name:          fi.String(fmt.Sprintf("https-nlb-to-master%s-%s", suffix, cidr)), //TODO: what is a good name for this?
+					Name:          fi.String(fmt.Sprintf("https-nlb-to-master%s-%s", suffix, cidr)),
 					Lifecycle:     b.SecurityLifecycle,
 					CIDR:          fi.String(cidr),
 					FromPort:      fi.Int64(443),
@@ -340,9 +332,8 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
-	//TODO: not implemented for NLB, would the security groups go to the master nodes?
 	// Add precreated additional security groups to the ELB
-	if ELB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 		for _, id := range b.Cluster.Spec.API.LoadBalancer.AdditionalSecurityGroups {
 			t := &awstasks.SecurityGroup{
 				Name:      fi.String(id),
@@ -357,8 +348,10 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
+	//TODO: Add precreated additional security groups to the Master nodes in case of NLB
+
 	// Allow HTTPS to the master instances from the ELB
-	if ELB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 		for _, masterGroup := range masterGroups {
 			suffix := masterGroup.Suffix
 			c.AddTask(&awstasks.SecurityGroupRule{
@@ -373,7 +366,8 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
-	if NLB {
+	if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork {
+		//TODO: resarch if the NLB's nodes will have constant IP's.  If so, consider the following as a TODO:
 		// Can tighten security by allowing only https access from the private ip's of the eni's associated with the nlb's nodes in each availability zone.
 		// Recommended approach is the whole vpc cidr https://docs.aws.amazon.com/elasticloadbalancing/latest/network/target-group-register-targets.html#target-security-groups
 		// work around suggested here https://forums.aws.amazon.com/thread.jspa?threadID=263245&start=0&tstart=0
@@ -395,11 +389,10 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 	if dns.IsGossipHostname(b.Cluster.Name) || b.UsePrivateDNS() {
 		// Ensure the ELB hostname is included in the TLS certificate,
 		// if we're not going to use an alias for it
-		if ELB {
+		if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 			elb.ForAPIServer = true
-		} else if NLB {
-			fmt.Println("not yet implemented")
-			//nlb.ForAPIServer = true
+		} else if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork {
+			nlb.ForAPIServer = true
 		}
 
 	}
@@ -409,15 +402,15 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 	// is already done as part of the Elastigroup's creation, if needed.
 	if !featureflag.Spotinst.Enabled() {
 		for _, ig := range b.MasterInstanceGroups() {
-			if ELB {
+			if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassClassic {
 				c.AddTask(&awstasks.LoadBalancerAttachment{
 					Name:             fi.String("api-" + ig.ObjectMeta.Name),
 					Lifecycle:        b.Lifecycle,
 					AutoscalingGroup: b.LinkToAutoscalingGroup(ig),
 					LoadBalancer:     b.LinkToELB("api"),
 				})
 			}
-			if NLB {
+			if b.Cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork {
 				c.AddTask(&awstasks.NetworkLoadBalancerAttachment{
 					Name:             fi.String("api-" + ig.ObjectMeta.Name),
 					Lifecycle:        b.Lifecycle,

pkg/model/dns.go

@@ -91,27 +91,16 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
-	//TODO: use interface (but what about bastion?), should I create a new member instead of targetLoadBalancer for the interface?
-
-	ELB, NLB := false, false
-	lbSpec := b.Cluster.Spec.API.LoadBalancer
-	switch lbSpec.Class {
-	case kops.LoadBalancerClassClassic, "":
-		ELB = true
-	case kops.LoadBalancerClassNetwork:
-		NLB = true
-	default:
-		return fmt.Errorf("Unknown Cluster.Spec.API.LoadBalancer.Class : %v", lbSpec.Class)
-	}
-
-	var targetLoadBalancer *awstasks.LoadBalancer
-	var targetNetworkLoadBalancer *awstasks.NetworkLoadBalancer
-
-	if ELB {
-		targetLoadBalancer = b.LinkToELB("api")
-	}
-	if NLB {
-		targetNetworkLoadBalancer = b.LinkToNLB("api")
+	var targetLoadBalancer awstasks.DNSTarget
+
+	if b.UseLoadBalancerForAPI() || b.UseLoadBalancerForInternalAPI() {
+		lbSpec := b.Cluster.Spec.API.LoadBalancer
+		switch lbSpec.Class {
+		case kops.LoadBalancerClassClassic, "":
+			targetLoadBalancer = awstasks.DNSTarget(b.LinkToELB("api"))
+		case kops.LoadBalancerClassNetwork:
+			targetLoadBalancer = awstasks.DNSTarget(b.LinkToNLB("api"))
+		}
 	}
 
 	if b.UseLoadBalancerForAPI() {
@@ -124,12 +113,11 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error {
 			}
 
 			apiDnsName := &awstasks.DNSName{
-				Name:                      s(b.Cluster.Spec.MasterPublicName),
-				Lifecycle:                 b.Lifecycle,
-				Zone:                      b.LinkToDNSZone(),
-				ResourceType:              s("A"),
-				TargetLoadBalancer:        awstasks.DNSTarget(b.LinkToELB("api")),
-				TargetNetworkLoadBalancer: targetNetworkLoadBalancer,
+				Name:               s(b.Cluster.Spec.MasterPublicName),
+				Lifecycle:          b.Lifecycle,
+				Zone:               b.LinkToDNSZone(),
+				ResourceType:       s("A"),
+				TargetLoadBalancer: targetLoadBalancer,
 			}
 			c.AddTask(apiDnsName)
 		}
@@ -145,12 +133,11 @@ func (b *DNSModelBuilder) Build(c *fi.ModelBuilderContext) error {
 			}
 
 			internalApiDnsName := &awstasks.DNSName{
-				Name:                      s(b.Cluster.Spec.MasterInternalName),
-				Lifecycle:                 b.Lifecycle,
-				Zone:                      b.LinkToDNSZone(),
-				ResourceType:              s("A"),
-				TargetLoadBalancer:        targetLoadBalancer,
-				TargetNetworkLoadBalancer: targetNetworkLoadBalancer,
+				Name:               s(b.Cluster.Spec.MasterInternalName),
+				Lifecycle:          b.Lifecycle,
+				Zone:               b.LinkToDNSZone(),
+				ResourceType:       s("A"),
+				TargetLoadBalancer: targetLoadBalancer,
 			}
 			// Using EnsureTask as MasterInternalName and MasterPublicName could be the same
 			c.EnsureTask(internalApiDnsName)