Merge pull request #10528 from rifelpet/automated-cherry-pick-of-#105…

…19-origin-release-1.19

Automated cherry pick of #10519: Test that AWS launch templates include wrong SG

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",
     ],
 )

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),

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)
+				}
+			}
+		})
+	}
+}

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"
               ]
             }

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

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

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" {
@@ -307,7 +307,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"
@@ -391,7 +391,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"