Repository: kubernetes/kops (go)

Issue:
EC2 security groups meant only for NLB-fronted API server are attached to all machines in the cluster
**1. What `kops` version are you running? The command `kops version`, will display
 this information.**

1.19.0-beta.3

**1. What Kubernetes version are you running? `kubectl version` will print the
 version if a cluster is running or provide the Kubernetes version specified as
 a `kops` flag.**

`version.Info{Major:"1", Minor:"19", GitVersion:"v1.19.4", GitCommit:"d360454c9bcd1634cf4cc52d1867af5491dc9c5f", GitTreeState:"clean", BuildDate:"2020-11-11T13:09:17Z", GoVersion:"go1.15.2", Compiler:"gc", Platform:"linux/amd64"}`

**3. What cloud provider are you using?**

AWS

**4. What commands did you run?  What is the simplest way to reproduce this issue?**

1. Create a cluster with the following _Cluster_ manifest fields set, optionally via _kops edit cluster_:  
  - _spec.api.loadBalancer.class_  
     Set this to "Network"
  - _spec.api.loadBalancer.additionalSecurityGroups_  
    Include at least one security group ID here.
1. _kops update cluster --out=. --target=terraform --create-kube-config=false_
1. _terraform apply -auto-approve_
1. Inspect the resulting ASG launch templates using the _aws_ tool or the AWS Web console

**1. What happened after the commands executed?**

The security group nominated by its ID in the "spec.api.loadBalancer.additionalSecurityGroups" _Cluster_ manifest is applied to **all** the ASGs involved in the cluster: master machines, worker machines, and bastion machines.

**1. What did you expect to happen?**

The security group nominated by its ID in the "spec.api.loadBalancer.additionalSecurityGroups" _Cluster_ manifest should be applied to only the ASGs involved in hosting the cluster's API servers: namely, the master machines. Neither the worker machines nor the bastion machines should use this security group.

**1. Anything else do we need to know?**

This topic [came up in discussion in the "kops-users" channel](https://kubernetes.slack.com/archives/C3QUFP0QM/p1609441614043100) of the "Kubernetes" Slack workspace. There, @rifelpet noted that the defect lies in the `(*AutoscalingGroupModelBuilder).buildLaunchConfigurationTask` method, where [it builds up the security group-related configuration for ASGs in clusters using an NLB in front of the API servers](https://github.com/kubernetes/kops/blob/66fe512b69b0d69853ad4c8abae894f85497b7e2/pkg/model/awsmodel/autoscalinggroup.go#L230-L241). It looks like we should skip that step if [the target _InstanceGroup_ role is not "Master."](https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/instancegroup.go#L56-L57)

Pull request description:
Only include API server additional security groups in InstanceGroups for masters
When using an AWS NLB in front of the Kubernetes API servers, we can't attach the EC2 security groups nominated in the _Cluster_ "spec.api.loadBalancer.additionalSecurityGroups" field directly to the load balancer, as NLBs don't have associated security groups. Instead, we intend to attach those nominated security groups to the machines that will receive network traffic forwarded from the NLB's listeners. For the API servers, since that program runs only on the master or control plane machines, we need only attach those security groups to the machines that will host the _kube-apiserver_ program, by way of the ASG launch templates that come from _kOps_ _InstanceGroups_ of role "master."

We were mistakenly including these security groups in launch templates derived from _InstanceGroups_ of all of our three current roles: "bastion," "master," and "node." Instead, skip _InstanceGroups_ of the "bastion" and "node" roles and only target those of role "master."

Fixes #10517.


Patch:
diff --git a/pkg/model/awsmodel/BUILD.bazel b/pkg/model/awsmodel/BUILD.bazel
index 639a221b09..6cb1ebd2d1 100644
--- a/pkg/model/awsmodel/BUILD.bazel
+++ b/pkg/model/awsmodel/BUILD.bazel
@@ -39,5 +39,6 @@ go_test(
         "//pkg/testutils:go_default_library",
         "//upup/pkg/fi:go_default_library",
         "//upup/pkg/fi/cloudup/awstasks:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
     ],
 )
diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go
index 365e7b24a8..3c7f8486ac 100644
--- a/pkg/model/awsmodel/autoscalinggroup.go
+++ b/pkg/model/awsmodel/autoscalinggroup.go
@@ -199,7 +199,7 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchConfigurationTask(c *fi.ModelB
 	// @step: add the iam instance profile
 	link, err := b.LinkToIAMInstanceProfile(ig)
 	if err != nil {
-		return nil, fmt.Errorf("unable to find iam profile link for instance group %q: %v", ig.ObjectMeta.Name, err)
+		return nil, fmt.Errorf("unable to find IAM profile link for instance group %q: %w", ig.ObjectMeta.Name, err)
 	}
 
 	t := &awstasks.LaunchConfiguration{
@@ -226,7 +226,8 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchConfigurationTask(c *fi.ModelB
 		t.HTTPPutResponseHopLimit = ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit
 	}
 
-	if b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork {
+	if ig.Spec.Role == kops.InstanceGroupRoleMaster &&
+		b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork {
 		for _, id := range b.Cluster.Spec.API.LoadBalancer.AdditionalSecurityGroups {
 			sgTask := &awstasks.SecurityGroup{
 				ID:        fi.String(id),


Test patch:
diff --git a/pkg/model/awsmodel/autoscalinggroup_test.go b/pkg/model/awsmodel/autoscalinggroup_test.go
index 9a0070c679..475f2da321 100644
--- a/pkg/model/awsmodel/autoscalinggroup_test.go
+++ b/pkg/model/awsmodel/autoscalinggroup_test.go
@@ -17,8 +17,10 @@ limitations under the License.
 package awsmodel
 
 import (
+	"fmt"
 	"testing"
 
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/pkg/model"
 	"k8s.io/kops/pkg/model/iam"
@@ -27,6 +29,10 @@ import (
 	"k8s.io/kops/upup/pkg/fi/cloudup/awstasks"
 )
 
+const (
+	sshPublicKeyEntry = "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCySdqIU+FhCWl3BNrAvPaOe5VfL2aCARUWwy91ZP+T7LBwFa9lhdttfjp/VX1D1/PVwntn2EhN079m8c2kfdmiZ/iCHqrLyIGSd+BOiCz0lT47znvANSfxYjLUuKrWWWeaXqerJkOsAD4PHchRLbZGPdbfoBKwtb/WT4GMRQmb9vmiaZYjsfdPPM9KkWI9ECoWFGjGehA8D+iYIPR711kRacb1xdYmnjHqxAZHFsb5L8wDWIeAyhy49cBD+lbzTiioq2xWLorXuFmXh6Do89PgzvHeyCLY6816f/kCX6wIFts8A2eaEHFL4rAOsuh6qHmSxGCR9peSyuRW8DxV725x justin@test"
+)
+
 func buildMinimalCluster() *kops.Cluster {
 	return testutils.BuildMinimalCluster("testcluster.test.com")
 
@@ -48,7 +54,7 @@ func TestRootVolumeOptimizationFlag(t *testing.T) {
 	ig.Spec.RootVolumeOptimization = fi.Bool(true)
 
 	k := [][]byte{}
-	k = append(k, []byte("ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQCySdqIU+FhCWl3BNrAvPaOe5VfL2aCARUWwy91ZP+T7LBwFa9lhdttfjp/VX1D1/PVwntn2EhN079m8c2kfdmiZ/iCHqrLyIGSd+BOiCz0lT47znvANSfxYjLUuKrWWWeaXqerJkOsAD4PHchRLbZGPdbfoBKwtb/WT4GMRQmb9vmiaZYjsfdPPM9KkWI9ECoWFGjGehA8D+iYIPR711kRacb1xdYmnjHqxAZHFsb5L8wDWIeAyhy49cBD+lbzTiioq2xWLorXuFmXh6Do89PgzvHeyCLY6816f/kCX6wIFts8A2eaEHFL4rAOsuh6qHmSxGCR9peSyuRW8DxV725x justin@test"))
+	k = append(k, []byte(sshPublicKeyEntry))
 
 	igs := []*kops.InstanceGroup{}
 	igs = append(igs, ig)
@@ -75,3 +81,121 @@ func TestRootVolumeOptimizationFlag(t *testing.T) {
 		t.Fatalf("RootVolumeOptimization was expected to be true, but was false")
 	}
 }
+
+func TestAPIServerAdditionalSecurityGroupsWithNLB(t *testing.T) {
+	const sgIDAPIServer = "sg-01234567890abcdef"
+
+	cluster := buildMinimalCluster()
+	cluster.Spec.API = &kops.AccessSpec{
+		LoadBalancer: &kops.LoadBalancerAccessSpec{
+			Class:                    kops.LoadBalancerClassNetwork,
+			AdditionalSecurityGroups: []string{sgIDAPIServer},
+		},
+	}
+
+	const (
+		roleBastion = iota
+		roleMaster
+		roleNode
+		_roleCount
+	)
+	igs := make([]*kops.InstanceGroup, _roleCount)
+	// NB: (*AutoscalingGroupModelBuilder).buildLaunchConfigurationTask expects there to be at least
+	// one subnet specified in each InstanceGroup.
+	subnets := []string{cluster.Spec.Subnets[0].Name}
+	igs[roleBastion] = &kops.InstanceGroup{
+		ObjectMeta: v1.ObjectMeta{
+			Name: "bastion1",
+		},
+		Spec: kops.InstanceGroupSpec{
+			Role:                     kops.InstanceGroupRoleBastion,
+			Subnets:                  subnets,
+			AdditionalSecurityGroups: []string{"sg-1234567890abcdef0"},
+		},
+	}
+	igs[roleMaster] = &kops.InstanceGroup{
+		ObjectMeta: v1.ObjectMeta{
+			Name: "master1",
+		},
+		Spec: kops.InstanceGroupSpec{
+			Role:                     kops.InstanceGroupRoleMaster,
+			Subnets:                  subnets,
+			AdditionalSecurityGroups: []string{"sg-234567890abcdef01"},
+		},
+	}
+	igs[roleNode] = &kops.InstanceGroup{
+		ObjectMeta: v1.ObjectMeta{
+			Name: "node1",
+		},
+		Spec: kops.InstanceGroupSpec{
+			Role:                     kops.InstanceGroupRoleNode,
+			Subnets:                  subnets,
+			AdditionalSecurityGroups: []string{"sg-34567890abcdef012"},
+		},
+	}
+
+	b := AutoscalingGroupModelBuilder{
+		AWSModelContext: &AWSModelContext{
+			KopsModelContext: &model.KopsModelContext{
+				IAMModelContext: iam.IAMModelContext{Cluster: cluster},
+				SSHPublicKeys:   [][]byte{[]byte(sshPublicKeyEntry)},
+				InstanceGroups:  igs,
+			},
+		},
+	}
+
+	c := &fi.ModelBuilderContext{
+		Tasks: make(map[string]fi.Task),
+	}
+
+	b.Build(c)
+
+	hasSecurityGroup := func(lt *awstasks.LaunchTemplate, id string) bool {
+		for _, sg := range lt.SecurityGroups {
+			if sg.ID != nil && *sg.ID == id {
+				return true
+			}
+		}
+		return false
+	}
+	hasDesignatedSecurityGroup := func(lt *awstasks.LaunchTemplate) bool {
+		return hasSecurityGroup(lt, sgIDAPIServer)
+	}
+	launchTemplateForGroup := func(t *testing.T, ig *kops.InstanceGroup) *awstasks.LaunchTemplate {
+		t.Helper()
+		subdomain := ig.Name
+		if ig.Spec.Role == kops.InstanceGroupRoleMaster {
+			subdomain = ig.Name + ".masters"
+		}
+		task, ok := c.Tasks[fmt.Sprintf("LaunchTemplate/%s.%s", subdomain, cluster.Name)]
+		if !ok {
+			t.Fatalf("No task available in model build context for InstanceGroup %q", ig.Name)
+		}
+		if task == nil {
+			t.Fatalf("Task pointer in model build context for InstanceGroup %q is nil", ig.Name)
+		}
+		return task.(*awstasks.LaunchTemplate)
+	}
+	tests := []struct {
+		ig          *kops.InstanceGroup
+		expectHasSG bool
+	}{
+		{igs[roleBastion], false},
+		{igs[roleMaster], true},
+		{igs[roleNode], false},
+	}
+	for _, test := range tests {
+		role := test.ig.Spec.Role
+		t.Run(string(role), func(t *testing.T) {
+			lt := launchTemplateForGroup(t, test.ig)
+			if want, got := test.expectHasSG, hasDesignatedSecurityGroup(lt); got != want {
+				t.Errorf("%q (role %q): launch template includes API server security group: want %t, got %t", test.ig.Name, role, want, got)
+			}
+			for _, sg := range test.ig.Spec.AdditionalSecurityGroups {
+				if want, got := true, hasSecurityGroup(lt, sg); got != want {
+					t.Errorf("%q (role %q): launch template includes additional security group %q: want %t, got %t", test.ig.Name, role, sg, want, got)
+				}
+			}
+		})
+	}
+}
diff --git a/tests/integration/update_cluster/complex/cloudformation.json b/tests/integration/update_cluster/complex/cloudformation.json
index 9e2e0a5c95..3e83851c40 100644
--- a/tests/integration/update_cluster/complex/cloudformation.json
+++ b/tests/integration/update_cluster/complex/cloudformation.json
@@ -279,8 +279,8 @@
                 {
                   "Ref": "AWSEC2SecurityGroupmasterscomplexexamplecom"
                 },
-                "sg-exampleid3",
-                "sg-exampleid4"
+                "sg-exampleid5",
+                "sg-exampleid6"
               ]
             }
           ],
@@ -422,8 +422,6 @@
                   "Ref": "AWSEC2SecurityGroupnodescomplexexamplecom"
                 },
                 "sg-exampleid3",
-                "sg-exampleid3",
-                "sg-exampleid4",
                 "sg-exampleid4"
               ]
             }
diff --git a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml
index b8e55435f7..ff53856422 100644
--- a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml
+++ b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml
@@ -8,8 +8,8 @@ spec:
     loadBalancer:
       type: Public
       additionalSecurityGroups:
-      - sg-exampleid3
-      - sg-exampleid4
+      - sg-exampleid5
+      - sg-exampleid6
       crossZoneLoadBalancing: true
       class: Network
       sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678
diff --git a/tests/integration/update_cluster/complex/in-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-v1alpha2.yaml
index e974f16b7d..fd61b783a5 100644
--- a/tests/integration/update_cluster/complex/in-v1alpha2.yaml
+++ b/tests/integration/update_cluster/complex/in-v1alpha2.yaml
@@ -8,8 +8,8 @@ spec:
     loadBalancer:
       type: Public
       additionalSecurityGroups:
-      - sg-exampleid3
-      - sg-exampleid4
+      - sg-exampleid5
+      - sg-exampleid6
       crossZoneLoadBalancing: true
       class: Network
       sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678
diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf
index ddb06041bf..c71f3d2380 100644
--- a/tests/integration/update_cluster/complex/kubernetes.tf
+++ b/tests/integration/update_cluster/complex/kubernetes.tf
@@ -1,11 +1,11 @@
 locals {
   cluster_name                 = "complex.example.com"
   master_autoscaling_group_ids = [aws_autoscaling_group.master-us-test-1a-masters-complex-example-com.id]
-  master_security_group_ids    = [aws_security_group.masters-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
+  master_security_group_ids    = [aws_security_group.masters-complex-example-com.id, "sg-exampleid5", "sg-exampleid6"]
   masters_role_arn             = aws_iam_role.masters-complex-example-com.arn
   masters_role_name            = aws_iam_role.masters-complex-example-com.name
   node_autoscaling_group_ids   = [aws_autoscaling_group.nodes-complex-example-com.id]
-  node_security_group_ids      = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid3", "sg-exampleid4", "sg-exampleid4"]
+  node_security_group_ids      = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
   node_subnet_ids              = [aws_subnet.us-test-1a-complex-example-com.id]
   nodes_role_arn               = aws_iam_role.nodes-complex-example-com.arn
   nodes_role_name              = aws_iam_role.nodes-complex-example-com.name
@@ -25,7 +25,7 @@ output "master_autoscaling_group_ids" {
 }
 
 output "master_security_group_ids" {
-  value = [aws_security_group.masters-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
+  value = [aws_security_group.masters-complex-example-com.id, "sg-exampleid5", "sg-exampleid6"]
 }
 
 output "masters_role_arn" {
@@ -41,7 +41,7 @@ output "node_autoscaling_group_ids" {
 }
 
 output "node_security_group_ids" {
-  value = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid3", "sg-exampleid4", "sg-exampleid4"]
+  value = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
 }
 
 output "node_subnet_ids" {
@@ -321,7 +321,7 @@ resource "aws_launch_template" "master-us-test-1a-masters-complex-example-com" {
   network_interfaces {
     associate_public_ip_address = true
     delete_on_termination       = true
-    security_groups             = [aws_security_group.masters-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
+    security_groups             = [aws_security_group.masters-complex-example-com.id, "sg-exampleid5", "sg-exampleid6"]
   }
   tag_specifications {
     resource_type = "instance"
@@ -405,7 +405,7 @@ resource "aws_launch_template" "nodes-complex-example-com" {
   network_interfaces {
     associate_public_ip_address = true
     delete_on_termination       = true
-    security_groups             = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid3", "sg-exampleid4", "sg-exampleid4"]
+    security_groups             = [aws_security_group.nodes-complex-example-com.id, "sg-exampleid3", "sg-exampleid4"]
   }
   tag_specifications {
     resource_type = "instance"