diff --git a/pkg/cloudprovider/providers/aws/BUILD b/pkg/cloudprovider/providers/aws/BUILD index 5e4a1af651d..5936b00945d 100644 --- a/pkg/cloudprovider/providers/aws/BUILD +++ b/pkg/cloudprovider/providers/aws/BUILD @@ -81,6 +81,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", diff --git a/pkg/cloudprovider/providers/aws/aws.go b/pkg/cloudprovider/providers/aws/aws.go index b29199072a3..292210bacb1 100644 --- a/pkg/cloudprovider/providers/aws/aws.go +++ b/pkg/cloudprovider/providers/aws/aws.go @@ -172,6 +172,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" @@ -3375,7 +3394,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) } @@ -3391,7 +3410,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 } diff --git a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go index d494a0156f4..6baaca2ac6c 100644 --- a/pkg/cloudprovider/providers/aws/aws_loadbalancer.go +++ b/pkg/cloudprovider/providers/aws/aws_loadbalancer.go @@ -34,9 +34,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) +) func isNLB(annotations map[string]string) bool { if annotations[ServiceAnnotationLoadBalancerType] == "nlb" { @@ -1173,44 +1183,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 diff --git a/pkg/cloudprovider/providers/aws/aws_test.go b/pkg/cloudprovider/providers/aws/aws_test.go index 7042bfabc01..ded45896a65 100644 --- a/pkg/cloudprovider/providers/aws/aws_test.go +++ b/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: ¤tHC} + 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)}