Merge pull request #4036 from almariah/feature-api-elb-security-groups

Automatic merge from submit-queue.

Allow additional SGs to be added to API loadbalancer

Allow adding precreated additional security groups to the API loadbalancer using cluster spec:
```yaml
spec:
  api:
    loadBalancer:
      type: Public
      additionalSecurityGroups:
      - sg-exampleid3
      - sg-exampleid4
```

- [x] Adding additionalSecurityGroups cluster spec
- [x] Adding validation for repeated security groups
- [x] Adding validation for API loadbalancer security groups
- [x] Integration test for API loadbalancer and its security groups
- [x] Update API docs and cluster.spec docs

docs/cluster_spec.md

@@ -23,6 +23,18 @@ spec:
 When configuring a LoadBalancer, you can also choose to have a public ELB or an internal (VPC only) ELB.  The `type`
 field should be `Public` or `Internal`.
 
+Also, you can add precreated additional security groups to the load balancer by setting `additionalSecurityGroups`.
+
+```yaml
+spec:
+  api:
+    loadBalancer:
+      type: Public
+      additionalSecurityGroups:
+      - sg-xxxxxxxx
+      - sg-xxxxxxxx
+```
+
 Additionally, you can increase idle timeout of the load balancer by setting its `idleTimeoutSeconds`. The default idle timeout is 5 minutes, with a maximum of 3600 seconds (60 minutes) being allowed by AWS.
 For more information see [configuring idle timeouts](http://docs.aws.amazon.com/elasticloadbalancing/latest/classic/config-idle-timeout.html).
 

pkg/apis/kops/cluster.go

@@ -267,9 +267,11 @@ const (
 	LoadBalancerTypeInternal LoadBalancerType = "Internal"
 )
 
+// LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access
 type LoadBalancerAccessSpec struct {
-	Type               LoadBalancerType `json:"type,omitempty"`
-	IdleTimeoutSeconds *int64           `json:"idleTimeoutSeconds,omitempty"`
+	Type                     LoadBalancerType `json:"type,omitempty"`
+	IdleTimeoutSeconds       *int64           `json:"idleTimeoutSeconds,omitempty"`
+	AdditionalSecurityGroups []string         `json:"additionalSecurityGroups,omitempty"`
 }
 
 // KubeDNSConfig defines the kube dns configuration

pkg/apis/kops/v1alpha1/cluster.go

@@ -266,9 +266,11 @@ const (
 	LoadBalancerTypeInternal LoadBalancerType = "Internal"
 )
 
+// LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access
 type LoadBalancerAccessSpec struct {
-	Type               LoadBalancerType `json:"type,omitempty"`
-	IdleTimeoutSeconds *int64           `json:"idleTimeoutSeconds,omitempty"`
+	Type                     LoadBalancerType `json:"type,omitempty"`
+	IdleTimeoutSeconds       *int64           `json:"idleTimeoutSeconds,omitempty"`
+	AdditionalSecurityGroups []string         `json:"additionalSecurityGroups,omitempty"`
 }
 
 // KubeDNSConfig defines the kube dns configuration

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

@@ -2283,6 +2283,7 @@ func Convert_kops_LeaderElectionConfiguration_To_v1alpha1_LeaderElectionConfigur
 func autoConvert_v1alpha1_LoadBalancerAccessSpec_To_kops_LoadBalancerAccessSpec(in *LoadBalancerAccessSpec, out *kops.LoadBalancerAccessSpec, s conversion.Scope) error {
 	out.Type = kops.LoadBalancerType(in.Type)
 	out.IdleTimeoutSeconds = in.IdleTimeoutSeconds
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	return nil
 }
 
@@ -2294,6 +2295,7 @@ func Convert_v1alpha1_LoadBalancerAccessSpec_To_kops_LoadBalancerAccessSpec(in *
 func autoConvert_kops_LoadBalancerAccessSpec_To_v1alpha1_LoadBalancerAccessSpec(in *kops.LoadBalancerAccessSpec, out *LoadBalancerAccessSpec, s conversion.Scope) error {
 	out.Type = LoadBalancerType(in.Type)
 	out.IdleTimeoutSeconds = in.IdleTimeoutSeconds
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	return nil
 }
 

pkg/apis/kops/v1alpha1/zz_generated.deepcopy.go

@@ -2622,6 +2622,11 @@ func (in *LoadBalancerAccessSpec) DeepCopyInto(out *LoadBalancerAccessSpec) {
 			**out = **in
 		}
 	}
+	if in.AdditionalSecurityGroups != nil {
+		in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	return
 }
 

pkg/apis/kops/v1alpha2/cluster.go

@@ -267,9 +267,11 @@ const (
 	LoadBalancerTypeInternal LoadBalancerType = "Internal"
 )
 
+// LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access
 type LoadBalancerAccessSpec struct {
-	Type               LoadBalancerType `json:"type,omitempty"`
-	IdleTimeoutSeconds *int64           `json:"idleTimeoutSeconds,omitempty"`
+	Type                     LoadBalancerType `json:"type,omitempty"`
+	IdleTimeoutSeconds       *int64           `json:"idleTimeoutSeconds,omitempty"`
+	AdditionalSecurityGroups []string         `json:"additionalSecurityGroups,omitempty"`
 }
 
 type KubeDNSConfig struct {

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

@@ -2545,6 +2545,7 @@ func Convert_kops_LeaderElectionConfiguration_To_v1alpha2_LeaderElectionConfigur
 func autoConvert_v1alpha2_LoadBalancerAccessSpec_To_kops_LoadBalancerAccessSpec(in *LoadBalancerAccessSpec, out *kops.LoadBalancerAccessSpec, s conversion.Scope) error {
 	out.Type = kops.LoadBalancerType(in.Type)
 	out.IdleTimeoutSeconds = in.IdleTimeoutSeconds
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	return nil
 }
 
@@ -2556,6 +2557,7 @@ func Convert_v1alpha2_LoadBalancerAccessSpec_To_kops_LoadBalancerAccessSpec(in *
 func autoConvert_kops_LoadBalancerAccessSpec_To_v1alpha2_LoadBalancerAccessSpec(in *kops.LoadBalancerAccessSpec, out *LoadBalancerAccessSpec, s conversion.Scope) error {
 	out.Type = LoadBalancerType(in.Type)
 	out.IdleTimeoutSeconds = in.IdleTimeoutSeconds
+	out.AdditionalSecurityGroups = in.AdditionalSecurityGroups
 	return nil
 }
 

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

@@ -2748,6 +2748,11 @@ func (in *LoadBalancerAccessSpec) DeepCopyInto(out *LoadBalancerAccessSpec) {
 			**out = **in
 		}
 	}
+	if in.AdditionalSecurityGroups != nil {
+		in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	return
 }
 

pkg/apis/kops/validation/aws.go

@@ -19,13 +19,22 @@ package validation
 import (
 	"strings"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 )
 
 func awsValidateCluster(c *kops.Cluster) field.ErrorList {
-	return nil
+	allErrs := field.ErrorList{}
+
+	if c.Spec.API != nil {
+		if c.Spec.API.LoadBalancer != nil {
+			allErrs = append(allErrs, awsValidateAdditionalSecurityGroups(field.NewPath("spec", "api", "loadBalancer", "additionalSecurityGroups"), c.Spec.API.LoadBalancer.AdditionalSecurityGroups)...)
+		}
+	}
+
+	return allErrs
 }
 
 func awsValidateInstanceGroup(ig *kops.InstanceGroup) field.ErrorList {
@@ -41,7 +50,12 @@ func awsValidateInstanceGroup(ig *kops.InstanceGroup) field.ErrorList {
 func awsValidateAdditionalSecurityGroups(fieldPath *field.Path, groups []string) field.ErrorList {
 	allErrs := field.ErrorList{}
 
+	names := sets.NewString()
 	for i, s := range groups {
+		if names.Has(s) {
+			allErrs = append(allErrs, field.Invalid(fieldPath.Index(i), s, "security groups with duplicate name found"))
+		}
+		names.Insert(s)
 		if strings.TrimSpace(s) == "" {
 			allErrs = append(allErrs, field.Invalid(fieldPath.Index(i), s, "security group cannot be empty, if specified"))
 			continue

pkg/apis/kops/zz_generated.deepcopy.go

@@ -2983,6 +2983,11 @@ func (in *LoadBalancerAccessSpec) DeepCopyInto(out *LoadBalancerAccessSpec) {
 			**out = **in
 		}
 	}
+	if in.AdditionalSecurityGroups != nil {
+		in, out := &in.AdditionalSecurityGroups, &out.AdditionalSecurityGroups
+		*out = make([]string, len(*in))
+		copy(*out, *in)
+	}
 	return
 }
 

pkg/model/awsmodel/api_loadbalancer.go

@@ -184,6 +184,21 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}
 
+	// Add precreated additional security groups to the ELB
+	{
+		for _, id := range b.Cluster.Spec.API.LoadBalancer.AdditionalSecurityGroups {
+			t := &awstasks.SecurityGroup{
+				Name:   fi.String(id),
+				ID:     fi.String(id),
+				Shared: fi.Bool(true),
+			}
+			if err := c.EnsureTask(t); err != nil {
+				return err
+			}
+			elb.SecurityGroups = append(elb.SecurityGroups, t)
+		}
+	}
+
 	// Allow HTTPS to the master instances from the ELB
 	{
 		t := &awstasks.SecurityGroupRule{

tests/integration/update_cluster/complex/in-v1alpha2.yaml

@@ -4,6 +4,12 @@ metadata:
   creationTimestamp: "2016-12-10T22:42:27Z"
   name: complex.example.com
 spec:
+  api:
+    loadBalancer:
+      type: Public
+      additionalSecurityGroups:
+      - sg-exampleid3
+      - sg-exampleid4
   kubernetesApiAccess:
   - 0.0.0.0/0
   channel: stable
@@ -84,5 +90,3 @@ spec:
   role: Master
   subnets:
   - us-test-1a
-
-

tests/integration/update_cluster/complex/kubernetes.tf

@@ -42,6 +42,11 @@ provider "aws" {
   region = "us-test-1"
 }
 
+resource "aws_autoscaling_attachment" "master-us-test-1a-masters-complex-example-com" {
+  elb                    = "${aws_elb.api-complex-example-com.id}"
+  autoscaling_group_name = "${aws_autoscaling_group.master-us-test-1a-masters-complex-example-com.id}"
+}
+
 resource "aws_autoscaling_group" "master-us-test-1a-masters-complex-example-com" {
   name                 = "master-us-test-1a.masters.complex.example.com"
   launch_configuration = "${aws_launch_configuration.master-us-test-1a-masters-complex-example-com.id}"
@@ -150,6 +155,35 @@ resource "aws_ebs_volume" "us-test-1a-etcd-main-complex-example-com" {
   }
 }
 
+resource "aws_elb" "api-complex-example-com" {
+  name = "api-complex-example-com-vd3t5n"
+
+  listener = {
+    instance_port     = 443
+    instance_protocol = "TCP"
+    lb_port           = 443
+    lb_protocol       = "TCP"
+  }
+
+  security_groups = ["${aws_security_group.api-elb-complex-example-com.id}", "sg-exampleid3", "sg-exampleid4"]
+  subnets         = ["${aws_subnet.us-test-1a-complex-example-com.id}"]
+
+  health_check = {
+    target              = "SSL:443"
+    healthy_threshold   = 2
+    unhealthy_threshold = 2
+    interval            = 10
+    timeout             = 5
+  }
+
+  idle_timeout = 300
+
+  tags = {
+    KubernetesCluster = "complex.example.com"
+    Name              = "api.complex.example.com"
+  }
+}
+
 resource "aws_iam_instance_profile" "masters-complex-example-com" {
   name = "masters.complex.example.com"
   role = "${aws_iam_role.masters-complex-example-com.name}"
@@ -249,6 +283,19 @@ resource "aws_route" "0-0-0-0--0" {
   gateway_id             = "${aws_internet_gateway.complex-example-com.id}"
 }
 
+resource "aws_route53_record" "api-complex-example-com" {
+  name = "api.complex.example.com"
+  type = "A"
+
+  alias = {
+    name                   = "${aws_elb.api-complex-example-com.dns_name}"
+    zone_id                = "${aws_elb.api-complex-example-com.zone_id}"
+    evaluate_target_health = false
+  }
+
+  zone_id = "/hostedzone/Z1AFAKE1ZON3YO"
+}
+
 resource "aws_route_table" "complex-example-com" {
   vpc_id = "${aws_vpc.complex-example-com.id}"
 
@@ -263,6 +310,17 @@ resource "aws_route_table_association" "us-test-1a-complex-example-com" {
   route_table_id = "${aws_route_table.complex-example-com.id}"
 }
 
+resource "aws_security_group" "api-elb-complex-example-com" {
+  name        = "api-elb.complex.example.com"
+  vpc_id      = "${aws_vpc.complex-example-com.id}"
+  description = "Security group for api ELB"
+
+  tags = {
+    KubernetesCluster = "complex.example.com"
+    Name              = "api-elb.complex.example.com"
+  }
+}
+
 resource "aws_security_group" "masters-complex-example-com" {
   name        = "masters.complex.example.com"
   vpc_id      = "${aws_vpc.complex-example-com.id}"
@@ -312,15 +370,33 @@ resource "aws_security_group_rule" "all-node-to-node" {
   protocol                 = "-1"
 }
 
-resource "aws_security_group_rule" "https-external-to-master-0-0-0-0--0" {
+resource "aws_security_group_rule" "api-elb-egress" {
+  type              = "egress"
+  security_group_id = "${aws_security_group.api-elb-complex-example-com.id}"
+  from_port         = 0
+  to_port           = 0
+  protocol          = "-1"
+  cidr_blocks       = ["0.0.0.0/0"]
+}
+
+resource "aws_security_group_rule" "https-api-elb-0-0-0-0--0" {
   type              = "ingress"
-  security_group_id = "${aws_security_group.masters-complex-example-com.id}"
+  security_group_id = "${aws_security_group.api-elb-complex-example-com.id}"
   from_port         = 443
   to_port           = 443
   protocol          = "tcp"
   cidr_blocks       = ["0.0.0.0/0"]
 }
 
+resource "aws_security_group_rule" "https-elb-to-master" {
+  type                     = "ingress"
+  security_group_id        = "${aws_security_group.masters-complex-example-com.id}"
+  source_security_group_id = "${aws_security_group.api-elb-complex-example-com.id}"
+  from_port                = 443
+  to_port                  = 443
+  protocol                 = "tcp"
+}
+
 resource "aws_security_group_rule" "master-egress" {
   type              = "egress"
   security_group_id = "${aws_security_group.masters-complex-example-com.id}"