From 7b7dd7861f2b8beea1d25b3f0bde7e35aa894c1e Mon Sep 17 00:00:00 2001 From: Rudi Chiarito Date: Fri, 25 Mar 2016 13:36:06 -0400 Subject: [PATCH 1/5] Add support for HTTPS->HTTP ELB listeners through annotations Moved listener creation to a separate function, which had the nice side effect of allowing tests (added eight cases). --- pkg/cloudprovider/providers/aws/aws.go | 79 ++++++++++++----- pkg/cloudprovider/providers/aws/aws_test.go | 95 +++++++++++++++++++++ 2 files changed, 154 insertions(+), 20 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index e0cae84b4c6..37032009645 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -69,6 +69,21 @@ const TagNameSubnetPublicELB = "kubernetes.io/role/elb" // This lets us define more advanced semantics in future. const ServiceAnnotationLoadBalancerInternal = "service.beta.kubernetes.io/aws-load-balancer-internal" +// Service annotation requesting a secure listener. Value is [InstanceProtocol=]CertARN +// If InstanceProtocol is `http` (default) or `https`, an HTTPS listener that terminates the connection and parses headers is created. +// If it is set to `ssl` or `tcp`, a "raw" SSL listener is used. +// For more, see http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/elb-listener-config.html +// CertARN is an IAM or CM certificate ARN, e.g. arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012 +const ServiceAnnotationLoadBalancerCertificate = "service.beta.kubernetes.io/aws-load-balancer-certarn" + +// Maps from instance protocol to ELB protocol +var protocolMapping = map[string]string{ + "https": "https", + "http": "https", + "ssl": "ssl", + "tcp": "ssl", +} + // We sometimes read to see if something exists; then try to create it if we didn't find it // This can fail once in a consistent system if done in parallel // In an eventually consistent system, it could fail unboundedly @@ -2099,6 +2114,39 @@ func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) (bool, error) { return false, nil } +func getListener(port api.ServicePort, annotations map[string]string) (*elb.Listener, error) { + loadBalancerPort := int64(port.Port) + instancePort := int64(port.NodePort) + protocol := strings.ToLower(string(port.Protocol)) + instanceProtocol := protocol + + listener := &elb.Listener{} + listener.InstancePort = &instancePort + listener.LoadBalancerPort = &loadBalancerPort + certID := annotations[ServiceAnnotationLoadBalancerCertificate] + if certID != "" { + parts := strings.Split(certID, "=") + if len(parts) == 1 { + protocol = "https" + instanceProtocol = "http" + } else if len(parts) == 2 { + instanceProtocol = strings.ToLower(parts[0]) + protocol = protocolMapping[instanceProtocol] + if protocol == "" { + return nil, fmt.Errorf("Invalid protocol %s in %s", instanceProtocol, certID) + } + certID = parts[1] + } else { + return nil, fmt.Errorf("Invalid certificate annotation %s", certID) + } + listener.SSLCertificateId = &certID + } + listener.Protocol = &protocol + listener.InstanceProtocol = &instanceProtocol + + return listener, nil +} + // EnsureLoadBalancer implements LoadBalancer.EnsureLoadBalancer func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string, annotations map[string]string) (*api.LoadBalancerStatus, error) { glog.V(2).Infof("EnsureLoadBalancer(%v, %v, %v, %v, %v, %v, %v)", @@ -2113,10 +2161,21 @@ func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string, a return nil, fmt.Errorf("requested load balancer with no ports") } + // Figure out what mappings we want on the load balancer + listeners := []*elb.Listener{} for _, port := range apiService.Spec.Ports { if port.Protocol != api.ProtocolTCP { return nil, fmt.Errorf("Only TCP LoadBalancer is supported for AWS ELB") } + if port.NodePort == 0 { + glog.Errorf("Ignoring port without NodePort defined: %v", port) + continue + } + listener, err := getListener(port, annotations) + if err != nil { + return nil, err + } + listeners = append(listeners, listener) } if apiService.Spec.LoadBalancerIP != "" { @@ -2198,26 +2257,6 @@ func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string, a } securityGroupIDs := []string{securityGroupID} - // Figure out what mappings we want on the load balancer - listeners := []*elb.Listener{} - for _, port := range apiService.Spec.Ports { - if port.NodePort == 0 { - glog.Errorf("Ignoring port without NodePort defined: %v", port) - continue - } - instancePort := int64(port.NodePort) - loadBalancerPort := int64(port.Port) - protocol := strings.ToLower(string(port.Protocol)) - - listener := &elb.Listener{} - listener.InstancePort = &instancePort - listener.LoadBalancerPort = &loadBalancerPort - listener.Protocol = &protocol - listener.InstanceProtocol = &protocol - - listeners = append(listeners, listener) - } - // Build the load balancer itself loadBalancer, err := s.ensureLoadBalancer(serviceName, loadBalancerName, listeners, subnetIDs, securityGroupIDs, internalELB) if err != nil { diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 3388865301f..56642e36937 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1199,3 +1199,98 @@ func TestDescribeLoadBalancerOnEnsure(t *testing.T) { c.EnsureLoadBalancer(&api.Service{ObjectMeta: api.ObjectMeta{Name: "myservice", UID: "id"}}, []string{}, map[string]string{}) } + +func TestGetListener(t *testing.T) { + tests := []struct { + name string + + lbPort int64 + instancePort int64 + annotation string + + expectError bool + lbProtocol string + instanceProtocol string + certID string + //listener *elb.Listener + }{ + { + "No annotation, passthrough", + 80, 8000, "", + false, "tcp", "tcp", "", + }, + { + "Invalid cert annotation, no protocol before equal sign", + 443, 8000, "=foo", + true, "tcp", "tcp", "cert", + }, + { + "Invalid cert annotation, bogus protocol before equal sign", + 443, 8000, "bacon=foo", + true, "tcp", "tcp", "cert", + }, + { + "Invalid cert annotation, too many equal signs", + 443, 8000, "==", + true, "tcp", "tcp", "cert", + }, + { + "HTTPS->HTTPS", + 443, 8000, "https=cert", + false, "https", "https", "cert", + }, + { + "HTTPS->HTTP", + 443, 8000, "http=cert", + false, "https", "http", "cert", + }, + { + "SSL->SSL", + 443, 8000, "ssl=cert", + false, "ssl", "ssl", "cert", + }, + { + "SSL->TCP", + 443, 8000, "tcp=cert", + false, "ssl", "tcp", "cert", + }, + } + + for _, test := range tests { + t.Logf("Running test case %s", test.name) + annotations := make(map[string]string) + if test.annotation != "" { + annotations[ServiceAnnotationLoadBalancerCertificate] = test.annotation + } + l, err := getListener(api.ServicePort{ + NodePort: int(test.instancePort), + Port: int(test.lbPort), + Protocol: api.Protocol("tcp"), + }, annotations) + if test.expectError { + if err == nil { + t.Errorf("Should error for case %s", test.name) + } + } else { + if err != nil { + t.Errorf("Should succeed for case: %s, got %v", test.name, err) + } else { + var cert *string + if test.certID != "" { + cert = &test.certID + } + expected := &elb.Listener{ + InstancePort: &test.instancePort, + InstanceProtocol: &test.instanceProtocol, + LoadBalancerPort: &test.lbPort, + Protocol: &test.lbProtocol, + SSLCertificateId: cert, + } + if !reflect.DeepEqual(l, expected) { + t.Errorf("Incorrect listener (%v vs %v) for case: %s", + l, expected, test.name) + } + } + } + } +} From 61471965d8ddc4587af781da1a0a607b29d619bf Mon Sep 17 00:00:00 2001 From: Rudi Chiarito Date: Mon, 2 May 2016 17:43:26 -0400 Subject: [PATCH 2/5] Split annotation in two --- pkg/cloudprovider/providers/aws/aws.go | 33 ++++++++------- pkg/cloudprovider/providers/aws/aws_test.go | 47 +++++++++++---------- 2 files changed, 42 insertions(+), 38 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 37032009645..22486ff247d 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -69,15 +69,20 @@ const TagNameSubnetPublicELB = "kubernetes.io/role/elb" // This lets us define more advanced semantics in future. const ServiceAnnotationLoadBalancerInternal = "service.beta.kubernetes.io/aws-load-balancer-internal" -// Service annotation requesting a secure listener. Value is [InstanceProtocol=]CertARN -// If InstanceProtocol is `http` (default) or `https`, an HTTPS listener that terminates the connection and parses headers is created. -// If it is set to `ssl` or `tcp`, a "raw" SSL listener is used. +// Service annotation requesting a secure listener. Value is a valid certificate ARN. // For more, see http://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/elb-listener-config.html // CertARN is an IAM or CM certificate ARN, e.g. arn:aws:acm:us-east-1:123456789012:certificate/12345678-1234-1234-1234-123456789012 -const ServiceAnnotationLoadBalancerCertificate = "service.beta.kubernetes.io/aws-load-balancer-certarn" +const ServiceAnnotationLoadBalancerCertificate = "service.beta.kubernetes.io/aws-load-balancer-ssl-cert" -// Maps from instance protocol to ELB protocol -var protocolMapping = map[string]string{ +// Service annotation specifying the protocol spoken by the backend (pod) behind a secure listener. +// Only inspected when `aws-load-balancer-ssl-cert` is used. +// If `http` (default) or `https`, an HTTPS listener that terminates the connection and parses headers is created. +// If set to `ssl` or `tcp`, a "raw" SSL listener is used. + +const ServiceAnnotationLoadBalancerBEProtocol = "service.beta.kubernetes.io/aws-load-balancer-backend-protocol" + +// Maps from backend protocol to ELB protocol +var backendProtocolMapping = map[string]string{ "https": "https", "http": "https", "ssl": "ssl", @@ -2125,19 +2130,15 @@ func getListener(port api.ServicePort, annotations map[string]string) (*elb.List listener.LoadBalancerPort = &loadBalancerPort certID := annotations[ServiceAnnotationLoadBalancerCertificate] if certID != "" { - parts := strings.Split(certID, "=") - if len(parts) == 1 { + instanceProtocol = annotations[ServiceAnnotationLoadBalancerBEProtocol] + if instanceProtocol == "" { protocol = "https" instanceProtocol = "http" - } else if len(parts) == 2 { - instanceProtocol = strings.ToLower(parts[0]) - protocol = protocolMapping[instanceProtocol] - if protocol == "" { - return nil, fmt.Errorf("Invalid protocol %s in %s", instanceProtocol, certID) - } - certID = parts[1] } else { - return nil, fmt.Errorf("Invalid certificate annotation %s", certID) + protocol = backendProtocolMapping[instanceProtocol] + if protocol == "" { + return nil, fmt.Errorf("Invalid backend protocol %s in %s", instanceProtocol, certID) + } } listener.SSLCertificateId = &certID } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 56642e36937..aaf96e46bce 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1204,54 +1204,54 @@ func TestGetListener(t *testing.T) { tests := []struct { name string - lbPort int64 - instancePort int64 - annotation string + lbPort int64 + instancePort int64 + backendProtocolAnnotation string + certAnnotation string expectError bool lbProtocol string instanceProtocol string certID string - //listener *elb.Listener }{ { - "No annotation, passthrough", - 80, 8000, "", + "No cert or BE protocol annotation, passthrough", + 80, 8000, "", "", false, "tcp", "tcp", "", }, { - "Invalid cert annotation, no protocol before equal sign", - 443, 8000, "=foo", + "BE protocol without cert annotation, passthrough", + 443, 8001, "https", "", + false, "tcp", "tcp", "", + }, + { + "Invalid cert annotation, bogus backend protocol", + 443, 8002, "bacon", "foo", true, "tcp", "tcp", "cert", }, { - "Invalid cert annotation, bogus protocol before equal sign", - 443, 8000, "bacon=foo", - true, "tcp", "tcp", "cert", - }, - { - "Invalid cert annotation, too many equal signs", - 443, 8000, "==", + "Invalid cert annotation, protocol followed by equal sign", + 443, 8003, "http=", "=", true, "tcp", "tcp", "cert", }, { "HTTPS->HTTPS", - 443, 8000, "https=cert", + 443, 8004, "https", "cert", false, "https", "https", "cert", }, { "HTTPS->HTTP", - 443, 8000, "http=cert", + 443, 8005, "http", "cert", false, "https", "http", "cert", }, { "SSL->SSL", - 443, 8000, "ssl=cert", + 443, 8006, "ssl", "cert", false, "ssl", "ssl", "cert", }, { "SSL->TCP", - 443, 8000, "tcp=cert", + 443, 8007, "tcp", "cert", false, "ssl", "tcp", "cert", }, } @@ -1259,8 +1259,11 @@ func TestGetListener(t *testing.T) { for _, test := range tests { t.Logf("Running test case %s", test.name) annotations := make(map[string]string) - if test.annotation != "" { - annotations[ServiceAnnotationLoadBalancerCertificate] = test.annotation + if test.backendProtocolAnnotation != "" { + annotations[ServiceAnnotationLoadBalancerBEProtocol] = test.backendProtocolAnnotation + } + if test.certAnnotation != "" { + annotations[ServiceAnnotationLoadBalancerCertificate] = test.certAnnotation } l, err := getListener(api.ServicePort{ NodePort: int(test.instancePort), @@ -1287,7 +1290,7 @@ func TestGetListener(t *testing.T) { SSLCertificateId: cert, } if !reflect.DeepEqual(l, expected) { - t.Errorf("Incorrect listener (%v vs %v) for case: %s", + t.Errorf("Incorrect listener (%v vs expected %v) for case: %s", l, expected, test.name) } } From 898df1f52b1fdabacb6c6b0f7faaf0e2691fad90 Mon Sep 17 00:00:00 2001 From: Rudi Chiarito Date: Mon, 2 May 2016 19:20:50 -0400 Subject: [PATCH 3/5] Fix API fields to use new int32 sizes --- pkg/cloudprovider/providers/aws/aws_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index aaf96e46bce..b2207093219 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1266,8 +1266,8 @@ func TestGetListener(t *testing.T) { annotations[ServiceAnnotationLoadBalancerCertificate] = test.certAnnotation } l, err := getListener(api.ServicePort{ - NodePort: int(test.instancePort), - Port: int(test.lbPort), + NodePort: int32(test.instancePort), + Port: int32(test.lbPort), Protocol: api.Protocol("tcp"), }, annotations) if test.expectError { From e19c069b9dcdd446a730e0bd063bb40ddad031df Mon Sep 17 00:00:00 2001 From: Rudi Chiarito Date: Tue, 10 May 2016 11:40:34 -0400 Subject: [PATCH 4/5] Add comment, rename getListener to buildListener --- pkg/cloudprovider/providers/aws/aws.go | 7 ++++--- pkg/cloudprovider/providers/aws/aws_test.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 22486ff247d..4bee1105cd4 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -78,7 +78,6 @@ const ServiceAnnotationLoadBalancerCertificate = "service.beta.kubernetes.io/aws // Only inspected when `aws-load-balancer-ssl-cert` is used. // If `http` (default) or `https`, an HTTPS listener that terminates the connection and parses headers is created. // If set to `ssl` or `tcp`, a "raw" SSL listener is used. - const ServiceAnnotationLoadBalancerBEProtocol = "service.beta.kubernetes.io/aws-load-balancer-backend-protocol" // Maps from backend protocol to ELB protocol @@ -2119,7 +2118,9 @@ func isSubnetPublic(rt []*ec2.RouteTable, subnetID string) (bool, error) { return false, nil } -func getListener(port api.ServicePort, annotations map[string]string) (*elb.Listener, error) { +// buildListener creates a new listener from the given port, adding an SSL certificate +// if indicated by the appropriate annotations. +func buildListener(port api.ServicePort, annotations map[string]string) (*elb.Listener, error) { loadBalancerPort := int64(port.Port) instancePort := int64(port.NodePort) protocol := strings.ToLower(string(port.Protocol)) @@ -2172,7 +2173,7 @@ func (s *AWSCloud) EnsureLoadBalancer(apiService *api.Service, hosts []string, a glog.Errorf("Ignoring port without NodePort defined: %v", port) continue } - listener, err := getListener(port, annotations) + listener, err := buildListener(port, annotations) if err != nil { return nil, err } diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index b2207093219..aaddcf65754 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1200,7 +1200,7 @@ func TestDescribeLoadBalancerOnEnsure(t *testing.T) { c.EnsureLoadBalancer(&api.Service{ObjectMeta: api.ObjectMeta{Name: "myservice", UID: "id"}}, []string{}, map[string]string{}) } -func TestGetListener(t *testing.T) { +func TestBuildListener(t *testing.T) { tests := []struct { name string From 59334408a6ff48c358e83e77fe5f8e132239da25 Mon Sep 17 00:00:00 2001 From: Rudi Chiarito Date: Tue, 10 May 2016 11:53:44 -0400 Subject: [PATCH 5/5] Change default when no BE proto given, add test for that Also improve error message when BE proto is wrong --- pkg/cloudprovider/providers/aws/aws.go | 6 +++--- pkg/cloudprovider/providers/aws/aws_test.go | 9 +++++++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index 4bee1105cd4..1ecf9a7e74c 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -2133,12 +2133,12 @@ func buildListener(port api.ServicePort, annotations map[string]string) (*elb.Li if certID != "" { instanceProtocol = annotations[ServiceAnnotationLoadBalancerBEProtocol] if instanceProtocol == "" { - protocol = "https" - instanceProtocol = "http" + protocol = "ssl" + instanceProtocol = "tcp" } else { protocol = backendProtocolMapping[instanceProtocol] if protocol == "" { - return nil, fmt.Errorf("Invalid backend protocol %s in %s", instanceProtocol, certID) + return nil, fmt.Errorf("Invalid backend protocol %s for %s in %s", instanceProtocol, certID, ServiceAnnotationLoadBalancerBEProtocol) } } listener.SSLCertificateId = &certID diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index aaddcf65754..9e8df8103eb 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/pkg/cloudprovider/providers/aws/aws_test.go @@ -1216,9 +1216,14 @@ func TestBuildListener(t *testing.T) { }{ { "No cert or BE protocol annotation, passthrough", - 80, 8000, "", "", + 80, 7999, "", "", false, "tcp", "tcp", "", }, + { + "Cert annotation without BE protocol specified, SSL->TCP", + 80, 8000, "", "cert", + false, "ssl", "tcp", "cert", + }, { "BE protocol without cert annotation, passthrough", 443, 8001, "https", "", @@ -1265,7 +1270,7 @@ func TestBuildListener(t *testing.T) { if test.certAnnotation != "" { annotations[ServiceAnnotationLoadBalancerCertificate] = test.certAnnotation } - l, err := getListener(api.ServicePort{ + l, err := buildListener(api.ServicePort{ NodePort: int32(test.instancePort), Port: int32(test.lbPort), Protocol: api.Protocol("tcp"),