Merge pull request #1476 from randomvariable/automated-cherry-pick-of…

…-#1456-upstream-release-0.4

⚠️ ELB uses separate security group

api/v1alpha2/types.go

@@ -311,6 +311,9 @@ var (
 	// SecurityGroupControlPlane defines a Kubernetes control plane node role
 	SecurityGroupControlPlane = SecurityGroupRole("controlplane")
 
+	// SecurityGroupAPIServerLB defines a Kubernetes API Server Load Balancer role
+	SecurityGroupAPIServerLB = SecurityGroupRole("apiserver-lb")
+
 	// SecurityGroupLB defines a container for the cloud provider to inject its load balancer ingress rules
 	SecurityGroupLB = SecurityGroupRole("lb")
 )

pkg/cloud/services/ec2/securitygroups.go

@@ -62,6 +62,7 @@ func (s *Service) reconcileSecurityGroups() error {
 	// Declare all security group roles that the reconcile loop takes care of.
 	roles := []infrav1.SecurityGroupRole{
 		infrav1.SecurityGroupBastion,
+		infrav1.SecurityGroupAPIServerLB,
 		infrav1.SecurityGroupLB,
 		infrav1.SecurityGroupControlPlane,
 		infrav1.SecurityGroupNode,
@@ -378,7 +379,11 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) (
 				Protocol:    infrav1.SecurityGroupProtocolTCP,
 				FromPort:    6443,
 				ToPort:      6443,
-				CidrBlocks:  []string{anyIPv4CidrBlock},
+				SourceSecurityGroupIDs: []string{
+					s.scope.SecurityGroups()[infrav1.SecurityGroupAPIServerLB].ID,
+					s.scope.SecurityGroups()[infrav1.SecurityGroupControlPlane].ID,
+					s.scope.SecurityGroups()[infrav1.SecurityGroupNode].ID,
+				},
 			},
 			{
 				Description:            "etcd",
@@ -458,6 +463,16 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) (
 				},
 			},
 		}, nil
+	case infrav1.SecurityGroupAPIServerLB:
+		return infrav1.IngressRules{
+			{
+				Description: "Kubernetes API",
+				Protocol:    infrav1.SecurityGroupProtocolTCP,
+				FromPort:    s.scope.APIServerPort(),
+				ToPort:      s.scope.APIServerPort(),
+				CidrBlocks:  []string{anyIPv4CidrBlock},
+			},
+		}, nil
 	case infrav1.SecurityGroupLB:
 		// We hand this group off to the in-cluster cloud provider, so these rules aren't used
 		return infrav1.IngressRules{}, nil

pkg/cloud/services/ec2/securitygroups_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/mock/gomock"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha2"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/scope"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/services/ec2/mock_ec2iface"
@@ -106,6 +107,41 @@ func TestReconcileSecurityGroups(t *testing.T) {
 
 				////////////////////////
 
+				securityGroupAPIServerLb := m.CreateSecurityGroup(gomock.Eq(&ec2.CreateSecurityGroupInput{
+					VpcId:       aws.String("vpc-securitygroups"),
+					GroupName:   aws.String("test-cluster-apiserver-lb"),
+					Description: aws.String("Kubernetes cluster test-cluster: apiserver-lb"),
+				})).
+					Return(&ec2.CreateSecurityGroupOutput{GroupId: aws.String("sg-apiserver-lb")}, nil)
+
+				m.CreateTags(matchesTags(&ec2.CreateTagsInput{
+					Resources: []*string{aws.String("sg-apiserver-lb")},
+					Tags: []*ec2.Tag{
+						{
+							Key:   aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
+							Value: aws.String("owned"),
+						},
+						{
+							Key:   aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
+							Value: aws.String("apiserver-lb"),
+						},
+						{
+							Key:   aws.String("Name"),
+							Value: aws.String("test-cluster-apiserver-lb"),
+						},
+					},
+				})).
+					Return(nil, nil).
+					After(securityGroupAPIServerLb)
+
+				m.AuthorizeSecurityGroupIngress(gomock.AssignableToTypeOf(&ec2.AuthorizeSecurityGroupIngressInput{
+					GroupId: aws.String("sg-apiserver-lb"),
+				})).
+					Return(&ec2.AuthorizeSecurityGroupIngressOutput{}, nil).
+					After(securityGroupAPIServerLb)
+
+				////////////////////////
+
 				securityGroupLb := m.CreateSecurityGroup(gomock.Eq(&ec2.CreateSecurityGroupInput{
 					VpcId:       aws.String("vpc-securitygroups"),
 					GroupName:   aws.String("test-cluster-lb"),
@@ -245,6 +281,30 @@ func TestReconcileSecurityGroups(t *testing.T) {
 	}
 }
 
+func TestControlPlaneSecurityGroupNotOpenToAnyCIDR(t *testing.T) {
+	scope, err := scope.NewClusterScope(scope.ClusterScopeParams{
+		Cluster: &clusterv1.Cluster{
+			ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
+		},
+		AWSCluster: &infrav1.AWSCluster{},
+	})
+	if err != nil {
+		t.Fatalf("Failed to create test context: %v", err)
+	}
+
+	s := NewService(scope)
+	rules, err := s.getSecurityGroupIngressRules(infrav1.SecurityGroupControlPlane)
+	if err != nil {
+		t.Fatalf("Failed to lookup controlplane security group ingress rules: %v", err)
+	}
+
+	for _, r := range rules {
+		if sets.NewString(r.CidrBlocks...).Has(anyIPv4CidrBlock) {
+			t.Fatal("Ingress rule allows any CIDR block")
+		}
+	}
+}
+
 func matchesTags(input *ec2.CreateTagsInput) gomock.Matcher {
 	return tagMatcher{input}
 }

pkg/cloud/services/elb/loadbalancer.go

@@ -27,6 +27,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/elb"
 	rgapi "github.com/aws/aws-sdk-go/service/resourcegroupstaggingapi"
 	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/util/sets"
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/api/v1alpha2"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/awserrors"
 	"sigs.k8s.io/cluster-api-provider-aws/pkg/cloud/converters"
@@ -84,6 +85,17 @@ func (s *Service) ReconcileLoadbalancers() error {
 		}
 	}
 
+	// Reconcile the security groups from the spec and the ones currently attached to the load balancer
+	if !sets.NewString(apiELB.SecurityGroupIDs...).Equal(sets.NewString(spec.SecurityGroupIDs...)) {
+		_, err := s.scope.ELB.ApplySecurityGroupsToLoadBalancer(&elb.ApplySecurityGroupsToLoadBalancerInput{
+			LoadBalancerName: &apiELB.Name,
+			SecurityGroups:   aws.StringSlice(spec.SecurityGroupIDs),
+		})
+		if err != nil {
+			return errors.Wrapf(err, "failed to apply security groups to load balancer %q", apiELB.Name)
+		}
+	}
+
 	// TODO(vincepri): check if anything has changed and reconcile as necessary.
 	apiELB.DeepCopyInto(&s.scope.Network().APIServerELB)
 	s.scope.V(4).Info("Control plane load balancer", "api-server-elb", apiELB)
@@ -249,7 +261,7 @@ func (s *Service) getAPIServerClassicELBSpec() (*infrav1.ClassicELB, error) {
 			HealthyThreshold:   5,
 			UnhealthyThreshold: 3,
 		},
-		SecurityGroupIDs: []string{s.scope.SecurityGroups()[infrav1.SecurityGroupControlPlane].ID},
+		SecurityGroupIDs: []string{s.scope.SecurityGroups()[infrav1.SecurityGroupAPIServerLB].ID},
 		Attributes: infrav1.ClassicELBAttributes{
 			IdleTimeout: 10 * time.Minute,
 		},