allow ELB HC configuration via Service annotations

pkg/cloudprovider/providers/aws/BUILD

@@ -80,6 +80,7 @@ go_test(
         "//vendor/github.com/aws/aws-sdk-go/service/elb:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/mock:go_default_library",
+        "//vendor/github.com/stretchr/testify/require:go_default_library",
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/types:go_default_library",

pkg/cloudprovider/providers/aws/aws.go

@@ -154,6 +154,25 @@ const ServiceAnnotationLoadBalancerBEProtocol = "service.beta.kubernetes.io/aws-
 // For example: "Key1=Val1,Key2=Val2,KeyNoVal1=,KeyNoVal2"
 const ServiceAnnotationLoadBalancerAdditionalTags = "service.beta.kubernetes.io/aws-load-balancer-additional-resource-tags"
 
+// ServiceAnnotationLoadBalancerHCHealthyThreshold is the annotation used on
+// the service to specify the number of successive successful health checks
+// required for a backend to be considered healthy for traffic.
+const ServiceAnnotationLoadBalancerHCHealthyThreshold = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-healthy-threshold"
+
+// ServiceAnnotationLoadBalancerHCUnhealthyThreshold is the annotation used
+// on the service to specify the number of unsuccessful health checks
+// required for a backend to be considered unhealthy for traffic
+const ServiceAnnotationLoadBalancerHCUnhealthyThreshold = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-unhealthy-threshold"
+
+// ServiceAnnotationLoadBalancerHCTimeout is the annotation used on the
+// service to specify, in seconds, how long to wait before marking a health
+// check as failed.
+const ServiceAnnotationLoadBalancerHCTimeout = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-timeout"
+
+// ServiceAnnotationLoadBalancerHCInterval is the annotation used on the
+// service to specify, in seconds, the interval between health checks.
+const ServiceAnnotationLoadBalancerHCInterval = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-interval"
+
 // Event key when a volume is stuck on attaching state when being attached to a volume
 const volumeAttachmentStuck = "VolumeAttachmentStuck"
 
@@ -3195,7 +3214,7 @@ func (c *Cloud) EnsureLoadBalancer(clusterName string, apiService *v1.Service, n
 
 	if path, healthCheckNodePort := service.GetServiceHealthCheckPathPort(apiService); path != "" {
 		glog.V(4).Infof("service %v (%v) needs health checks on :%d%s)", apiService.Name, loadBalancerName, healthCheckNodePort, path)
-		err = c.ensureLoadBalancerHealthCheck(loadBalancer, "HTTP", healthCheckNodePort, path)
+		err = c.ensureLoadBalancerHealthCheck(loadBalancer, "HTTP", healthCheckNodePort, path, annotations)
 		if err != nil {
 			return nil, fmt.Errorf("Failed to ensure health check for localized service %v on node port %v: %q", loadBalancerName, healthCheckNodePort, err)
 		}
@@ -3211,7 +3230,7 @@ func (c *Cloud) EnsureLoadBalancer(clusterName string, apiService *v1.Service, n
 			break
 		}
 		// there must be no path on TCP health check
-		err = c.ensureLoadBalancerHealthCheck(loadBalancer, "TCP", tcpHealthCheckPort, "")
+		err = c.ensureLoadBalancerHealthCheck(loadBalancer, "TCP", tcpHealthCheckPort, "", annotations)
 		if err != nil {
 			return nil, err
 		}

pkg/cloudprovider/providers/aws/aws_loadbalancer.go

@@ -32,9 +32,19 @@ import (
 	"k8s.io/apimachinery/pkg/util/sets"
 )
 
-const ProxyProtocolPolicyName = "k8s-proxyprotocol-enabled"
+const (
+	ProxyProtocolPolicyName = "k8s-proxyprotocol-enabled"
 
-const SSLNegotiationPolicyNameFormat = "k8s-SSLNegotiationPolicy-%s"
+	SSLNegotiationPolicyNameFormat = "k8s-SSLNegotiationPolicy-%s"
+)
+
+var (
+	// Defaults for ELB Healthcheck
+	defaultHCHealthyThreshold   = int64(2)
+	defaultHCUnhealthyThreshold = int64(6)
+	defaultHCTimeout            = int64(5)
+	defaultHCInterval           = int64(10)
+)
 
 // getLoadBalancerAdditionalTags converts the comma separated list of key-value
 // pairs in the ServiceAnnotationLoadBalancerAdditionalTags annotation and returns
@@ -389,44 +399,72 @@ func awsArnEquals(l, r *string) bool {
 	return strings.EqualFold(aws.StringValue(l), aws.StringValue(r))
 }
 
+// getExpectedHealthCheck returns an elb.Healthcheck for the provided target
+// and using either sensible defaults or overrides via Service annotations
+func (c *Cloud) getExpectedHealthCheck(target string, annotations map[string]string) (*elb.HealthCheck, error) {
+	healthcheck := &elb.HealthCheck{Target: &target}
+	getOrDefault := func(annotation string, defaultValue int64) (*int64, error) {
+		i64 := defaultValue
+		var err error
+		if s, ok := annotations[annotation]; ok {
+			i64, err = strconv.ParseInt(s, 10, 0)
+			if err != nil {
+				return nil, fmt.Errorf("failed parsing health check annotation value: %v", err)
+			}
+		}
+		return &i64, nil
+	}
+	var err error
+	healthcheck.HealthyThreshold, err = getOrDefault(ServiceAnnotationLoadBalancerHCHealthyThreshold, defaultHCHealthyThreshold)
+	if err != nil {
+		return nil, err
+	}
+	healthcheck.UnhealthyThreshold, err = getOrDefault(ServiceAnnotationLoadBalancerHCUnhealthyThreshold, defaultHCUnhealthyThreshold)
+	if err != nil {
+		return nil, err
+	}
+	healthcheck.Timeout, err = getOrDefault(ServiceAnnotationLoadBalancerHCTimeout, defaultHCTimeout)
+	if err != nil {
+		return nil, err
+	}
+	healthcheck.Interval, err = getOrDefault(ServiceAnnotationLoadBalancerHCInterval, defaultHCInterval)
+	if err != nil {
+		return nil, err
+	}
+	if err = healthcheck.Validate(); err != nil {
+		return nil, fmt.Errorf("some of the load balancer health check parameters are invalid: %v", err)
+	}
+	return healthcheck, nil
+}
+
 // Makes sure that the health check for an ELB matches the configured health check node port
-func (c *Cloud) ensureLoadBalancerHealthCheck(loadBalancer *elb.LoadBalancerDescription, protocol string, port int32, path string) error {
+func (c *Cloud) ensureLoadBalancerHealthCheck(loadBalancer *elb.LoadBalancerDescription, protocol string, port int32, path string, annotations map[string]string) error {
 	name := aws.StringValue(loadBalancer.LoadBalancerName)
 
 	actual := loadBalancer.HealthCheck
-
-	// Default AWS settings
-	expectedHealthyThreshold := int64(2)
-	expectedUnhealthyThreshold := int64(6)
-	expectedTimeout := int64(5)
-	expectedInterval := int64(10)
-
 	expectedTarget := protocol + ":" + strconv.FormatInt(int64(port), 10) + path
+	expected, err := c.getExpectedHealthCheck(expectedTarget, annotations)
+	if err != nil {
+		return fmt.Errorf("cannot update health check for load balancer %q: %q", name, err)
+	}
 
-	if expectedTarget == aws.StringValue(actual.Target) &&
-		expectedHealthyThreshold == orZero(actual.HealthyThreshold) &&
-		expectedUnhealthyThreshold == orZero(actual.UnhealthyThreshold) &&
-		expectedTimeout == orZero(actual.Timeout) &&
-		expectedInterval == orZero(actual.Interval) {
+	// comparing attributes 1 by 1 to avoid breakage in case a new field is
+	// added to the HC which breaks the equality
+	if aws.StringValue(expected.Target) == aws.StringValue(actual.Target) &&
+		aws.Int64Value(expected.HealthyThreshold) == aws.Int64Value(actual.HealthyThreshold) &&
+		aws.Int64Value(expected.UnhealthyThreshold) == aws.Int64Value(actual.UnhealthyThreshold) &&
+		aws.Int64Value(expected.Interval) == aws.Int64Value(actual.Interval) &&
+		aws.Int64Value(expected.Timeout) == aws.Int64Value(actual.Timeout) {
 		return nil
 	}
 
-	glog.V(2).Infof("Updating load-balancer health-check for %q", name)
-
-	healthCheck := &elb.HealthCheck{}
-	healthCheck.HealthyThreshold = &expectedHealthyThreshold
-	healthCheck.UnhealthyThreshold = &expectedUnhealthyThreshold
-	healthCheck.Timeout = &expectedTimeout
-	healthCheck.Interval = &expectedInterval
-	healthCheck.Target = &expectedTarget
-
 	request := &elb.ConfigureHealthCheckInput{}
-	request.HealthCheck = healthCheck
+	request.HealthCheck = expected
 	request.LoadBalancerName = loadBalancer.LoadBalancerName
 
-	_, err := c.elb.ConfigureHealthCheck(request)
+	_, err = c.elb.ConfigureHealthCheck(request)
 	if err != nil {
-		return fmt.Errorf("error configuring load-balancer health-check for %q: %q", name, err)
+		return fmt.Errorf("error configuring load balancer health check for %q: %q", name, err)
 	}
 
 	return nil

pkg/cloudprovider/providers/aws/aws_test.go

@@ -32,6 +32,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/types"
 	kubeletapis "k8s.io/kubernetes/pkg/kubelet/apis"
 )
@@ -87,6 +88,24 @@ func (m *MockedFakeELB) AddTags(input *elb.AddTagsInput) (*elb.AddTagsOutput, er
 	return args.Get(0).(*elb.AddTagsOutput), nil
 }
 
+func (m *MockedFakeELB) ConfigureHealthCheck(input *elb.ConfigureHealthCheckInput) (*elb.ConfigureHealthCheckOutput, error) {
+	args := m.Called(input)
+	if args.Get(0) == nil {
+		return nil, args.Error(1)
+	}
+	return args.Get(0).(*elb.ConfigureHealthCheckOutput), args.Error(1)
+}
+
+func (m *MockedFakeELB) expectConfigureHealthCheck(loadBalancerName *string, expectedHC *elb.HealthCheck, returnErr error) {
+	expected := &elb.ConfigureHealthCheckInput{HealthCheck: expectedHC, LoadBalancerName: loadBalancerName}
+	call := m.On("ConfigureHealthCheck", expected)
+	if returnErr != nil {
+		call.Return(nil, returnErr)
+	} else {
+		call.Return(&elb.ConfigureHealthCheckOutput{}, nil)
+	}
+}
+
 func TestReadAWSCloudConfig(t *testing.T) {
 	tests := []struct {
 		name string
@@ -1160,6 +1179,121 @@ func TestAddLoadBalancerTags(t *testing.T) {
 	awsServices.elb.(*MockedFakeELB).AssertExpectations(t)
 }
 
+func TestEnsureLoadBalancerHealthCheck(t *testing.T) {
+
+	tests := []struct {
+		name                string
+		annotations         map[string]string
+		overriddenFieldName string
+		overriddenValue     int64
+	}{
+		{"falls back to HC defaults", map[string]string{}, "", int64(0)},
+		{"healthy threshold override", map[string]string{ServiceAnnotationLoadBalancerHCHealthyThreshold: "7"}, "HealthyThreshold", int64(7)},
+		{"unhealthy threshold override", map[string]string{ServiceAnnotationLoadBalancerHCUnhealthyThreshold: "7"}, "UnhealthyThreshold", int64(7)},
+		{"timeout override", map[string]string{ServiceAnnotationLoadBalancerHCTimeout: "7"}, "Timeout", int64(7)},
+		{"interval override", map[string]string{ServiceAnnotationLoadBalancerHCInterval: "7"}, "Interval", int64(7)},
+	}
+	lbName := "myLB"
+	// this HC will always differ from the expected HC and thus it is expected an
+	// API call will be made to update it
+	currentHC := &elb.HealthCheck{}
+	elbDesc := &elb.LoadBalancerDescription{LoadBalancerName: &lbName, HealthCheck: currentHC}
+	defaultHealthyThreshold := int64(2)
+	defaultUnhealthyThreshold := int64(6)
+	defaultTimeout := int64(5)
+	defaultInterval := int64(10)
+	protocol, path, port := "tcp", "", int32(8080)
+	target := "tcp:8080"
+	defaultHC := &elb.HealthCheck{
+		HealthyThreshold:   &defaultHealthyThreshold,
+		UnhealthyThreshold: &defaultUnhealthyThreshold,
+		Timeout:            &defaultTimeout,
+		Interval:           &defaultInterval,
+		Target:             &target,
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			awsServices := newMockedFakeAWSServices(TestClusterId)
+			c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+			assert.Nil(t, err, "Error building aws cloud: %v", err)
+			expectedHC := *defaultHC
+			if test.overriddenFieldName != "" { // cater for test case with no overrides
+				value := reflect.ValueOf(&test.overriddenValue)
+				reflect.ValueOf(&expectedHC).Elem().FieldByName(test.overriddenFieldName).Set(value)
+			}
+			awsServices.elb.(*MockedFakeELB).expectConfigureHealthCheck(&lbName, &expectedHC, nil)
+
+			err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, test.annotations)
+
+			require.Nil(t, err)
+			awsServices.elb.(*MockedFakeELB).AssertExpectations(t)
+		})
+	}
+
+	t.Run("does not make an API call if the current health check is the same", func(t *testing.T) {
+		awsServices := newMockedFakeAWSServices(TestClusterId)
+		c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+		assert.Nil(t, err, "Error building aws cloud: %v", err)
+		expectedHC := *defaultHC
+		timeout := int64(3)
+		expectedHC.Timeout = &timeout
+		annotations := map[string]string{ServiceAnnotationLoadBalancerHCTimeout: "3"}
+		var currentHC elb.HealthCheck
+		currentHC = expectedHC
+
+		// NOTE no call expectations are set on the ELB mock
+		// test default HC
+		elbDesc := &elb.LoadBalancerDescription{LoadBalancerName: &lbName, HealthCheck: defaultHC}
+		err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, map[string]string{})
+		assert.Nil(t, err)
+		// test HC with override
+		elbDesc = &elb.LoadBalancerDescription{LoadBalancerName: &lbName, HealthCheck: &currentHC}
+		err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, annotations)
+		assert.Nil(t, err)
+	})
+
+	t.Run("validates resulting expected health check before making an API call", func(t *testing.T) {
+		awsServices := newMockedFakeAWSServices(TestClusterId)
+		c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+		assert.Nil(t, err, "Error building aws cloud: %v", err)
+		expectedHC := *defaultHC
+		invalidThreshold := int64(1)
+		expectedHC.HealthyThreshold = &invalidThreshold
+		require.Error(t, expectedHC.Validate()) // confirm test precondition
+		annotations := map[string]string{ServiceAnnotationLoadBalancerHCTimeout: "1"}
+
+		// NOTE no call expectations are set on the ELB mock
+		err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, annotations)
+
+		require.Error(t, err)
+	})
+
+	t.Run("handles invalid override values", func(t *testing.T) {
+		awsServices := newMockedFakeAWSServices(TestClusterId)
+		c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+		assert.Nil(t, err, "Error building aws cloud: %v", err)
+		annotations := map[string]string{ServiceAnnotationLoadBalancerHCTimeout: "3.3"}
+
+		// NOTE no call expectations are set on the ELB mock
+		err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, annotations)
+
+		require.Error(t, err)
+	})
+
+	t.Run("returns error when updating the health check fails", func(t *testing.T) {
+		awsServices := newMockedFakeAWSServices(TestClusterId)
+		c, err := newAWSCloud(strings.NewReader("[global]"), awsServices)
+		assert.Nil(t, err, "Error building aws cloud: %v", err)
+		returnErr := fmt.Errorf("throttling error")
+		awsServices.elb.(*MockedFakeELB).expectConfigureHealthCheck(&lbName, defaultHC, returnErr)
+
+		err = c.ensureLoadBalancerHealthCheck(elbDesc, protocol, port, path, map[string]string{})
+
+		require.Error(t, err)
+		awsServices.elb.(*MockedFakeELB).AssertExpectations(t)
+	})
+}
+
 func newMockedFakeAWSServices(id string) *FakeAWSServices {
 	s := NewFakeAWSServices(id)
 	s.ec2 = &MockedFakeEC2{FakeEC2Impl: s.ec2.(*FakeEC2Impl)}