-
Notifications
You must be signed in to change notification settings - Fork 223
NE-408: Allow configuring ELB connection idle timeout #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ba34922
db2f6de
28eff04
e03b102
b45521c
a635566
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -414,7 +414,7 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig | |
| // operator can safely update. | ||
| switch effectiveStrategy.Type { | ||
| case operatorv1.LoadBalancerServiceStrategyType: | ||
| // Update if GCP LB provider parameters changed. | ||
| // Update if LB provider parameters changed. | ||
| statusLB := ic.Status.EndpointPublishingStrategy.LoadBalancer | ||
| specLB := effectiveStrategy.LoadBalancer | ||
| if specLB != nil && statusLB != nil { | ||
|
|
@@ -426,19 +426,70 @@ func setDefaultPublishingStrategy(ic *operatorv1.IngressController, infraConfig | |
| changed = true | ||
| } | ||
|
|
||
| // If the ProviderParameters field does not exist for spec or status, | ||
| // just propagate (or remove) ProviderParameters in its entirety | ||
| // (as long as GCP parameters are specified one way or the other). | ||
| if specLB.ProviderParameters == nil && statusLB.ProviderParameters != nil && statusLB.ProviderParameters.GCP != nil || | ||
| specLB.ProviderParameters != nil && specLB.ProviderParameters.GCP != nil && statusLB.ProviderParameters == nil { | ||
| ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters = specLB.ProviderParameters | ||
| changed = true | ||
| // Detect changes to provider-specific parameters. | ||
| // Currently the only platforms with configurable | ||
| // provider-specific parameters are AWS and GCP. | ||
| var lbType operatorv1.LoadBalancerProviderType | ||
| if specLB.ProviderParameters != nil { | ||
| lbType = specLB.ProviderParameters.Type | ||
| } | ||
|
|
||
| if specLB.ProviderParameters != nil && statusLB.ProviderParameters != nil && | ||
| specLB.ProviderParameters.GCP != statusLB.ProviderParameters.GCP { | ||
| ic.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP = specLB.ProviderParameters.GCP | ||
| changed = true | ||
| switch lbType { | ||
| case operatorv1.AWSLoadBalancerProvider: | ||
| // The only provider parameter that is supported | ||
| // for AWS is the connection idle timeout for | ||
| // classic ELBs. | ||
| var specIdleTimeout, statusIdleTimeout metav1.Duration | ||
| if specLB.ProviderParameters != nil && specLB.ProviderParameters.AWS != nil && specLB.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil { | ||
| specIdleTimeout = specLB.ProviderParameters.AWS.ClassicLoadBalancerParameters.ConnectionIdleTimeout | ||
| } | ||
| if statusLB.ProviderParameters != nil && statusLB.ProviderParameters.AWS != nil && statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters != nil { | ||
| statusIdleTimeout = statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters.ConnectionIdleTimeout | ||
| } | ||
| if specIdleTimeout != statusIdleTimeout { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this also do some small validation on the timeout value to make sure a good value doesn't get changed into a bad value (e.g., like -1s) ? I know for durations, there is no validation in the api.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point, we might as well check for invalid values here. I've changed the check below from |
||
| if statusLB.ProviderParameters == nil { | ||
| statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{} | ||
| } | ||
| if len(statusLB.ProviderParameters.Type) == 0 { | ||
| statusLB.ProviderParameters.Type = operatorv1.AWSLoadBalancerProvider | ||
| } | ||
| if statusLB.ProviderParameters.AWS == nil { | ||
| statusLB.ProviderParameters.AWS = &operatorv1.AWSLoadBalancerParameters{} | ||
| } | ||
| if len(statusLB.ProviderParameters.AWS.Type) == 0 { | ||
| statusLB.ProviderParameters.AWS.Type = operatorv1.AWSClassicLoadBalancer | ||
| } | ||
| if statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters == nil { | ||
| statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters = &operatorv1.AWSClassicLoadBalancerParameters{} | ||
| } | ||
| var v metav1.Duration | ||
| if specIdleTimeout.Duration > 0 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, it isn't a pointer type—it's declared as (BTW, the line number that you are asking about is also the number of this PR!) |
||
| v = specIdleTimeout | ||
| } | ||
| statusLB.ProviderParameters.AWS.ClassicLoadBalancerParameters.ConnectionIdleTimeout = v | ||
| changed = true | ||
| } | ||
| case operatorv1.GCPLoadBalancerProvider: | ||
| // The only provider parameter that is supported | ||
| // for GCP is the ClientAccess parameter. | ||
| var specClientAccess, statusClientAccess operatorv1.GCPClientAccess | ||
| if specLB.ProviderParameters != nil && specLB.ProviderParameters.GCP != nil { | ||
| specClientAccess = specLB.ProviderParameters.GCP.ClientAccess | ||
| } | ||
| if statusLB.ProviderParameters != nil && statusLB.ProviderParameters.GCP != nil { | ||
| statusClientAccess = statusLB.ProviderParameters.GCP.ClientAccess | ||
| } | ||
| if specClientAccess != statusClientAccess { | ||
| if statusLB.ProviderParameters == nil { | ||
| statusLB.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ | ||
| Type: operatorv1.GCPLoadBalancerProvider, | ||
| } | ||
| } | ||
| if statusLB.ProviderParameters.GCP == nil { | ||
| statusLB.ProviderParameters.GCP = &operatorv1.GCPLoadBalancerParameters{} | ||
| } | ||
| statusLB.ProviderParameters.GCP.ClientAccess = specClientAccess | ||
| changed = true | ||
| } | ||
| } | ||
|
|
||
| return changed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,14 @@ package ingress | |
| import ( | ||
| "reflect" | ||
| "testing" | ||
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| operatorv1 "github.com/openshift/api/operator/v1" | ||
|
|
||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| // TestSetDefaultDomain verifies that setDefaultDomain behaves correctly. | ||
|
|
@@ -267,6 +270,30 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { | |
| } | ||
| return eps | ||
| } | ||
| elbWithNullParameters = func() *operatorv1.EndpointPublishingStrategy { | ||
| eps := lb(operatorv1.ExternalLoadBalancer) | ||
| eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ | ||
| Type: operatorv1.AWSLoadBalancerProvider, | ||
| AWS: &operatorv1.AWSLoadBalancerParameters{ | ||
| Type: operatorv1.AWSClassicLoadBalancer, | ||
| ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{}, | ||
| }, | ||
| } | ||
| return eps | ||
| } | ||
| elbWithIdleTimeout = func(timeout metav1.Duration) *operatorv1.EndpointPublishingStrategy { | ||
| eps := lb(operatorv1.ExternalLoadBalancer) | ||
| eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ | ||
| Type: operatorv1.AWSLoadBalancerProvider, | ||
| AWS: &operatorv1.AWSLoadBalancerParameters{ | ||
| Type: operatorv1.AWSClassicLoadBalancer, | ||
| ClassicLoadBalancerParameters: &operatorv1.AWSClassicLoadBalancerParameters{ | ||
| ConnectionIdleTimeout: timeout, | ||
| }, | ||
| }, | ||
| } | ||
| return eps | ||
| } | ||
| nlb = func() *operatorv1.EndpointPublishingStrategy { | ||
| eps := lb(operatorv1.ExternalLoadBalancer) | ||
| eps.LoadBalancer.ProviderParameters = &operatorv1.ProviderLoadBalancerParameters{ | ||
|
|
@@ -359,6 +386,30 @@ func TestSetDefaultPublishingStrategyHandlesUpdates(t *testing.T) { | |
| expectedResult: false, | ||
| expectedIC: makeIC(spec(elb()), status(nlb())), | ||
| }, | ||
| { | ||
| name: "loadbalancer ELB connection idle timeout changed from unset with null provider parameters to 2m", | ||
| ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithNullParameters())), | ||
| expectedResult: true, | ||
| expectedIC: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute}))), | ||
| }, | ||
| { | ||
| name: "loadbalancer ELB connection idle timeout changed from unset with empty provider parameters to 2m", | ||
| ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithIdleTimeout(metav1.Duration{}))), | ||
| expectedResult: true, | ||
| expectedIC: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute})), status(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute}))), | ||
| }, | ||
| { | ||
| name: "loadbalancer ELB connection idle timeout changed from unset to -1s", | ||
| ic: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: -1 * time.Second})), status(elb())), | ||
| expectedResult: true, | ||
| expectedIC: makeIC(spec(elbWithIdleTimeout(metav1.Duration{Duration: -1 * time.Second})), status(elbWithIdleTimeout(metav1.Duration{}))), | ||
| }, | ||
| { | ||
| name: "loadbalancer ELB connection idle timeout changed from 2m to unset", | ||
| ic: makeIC(spec(elb()), status(elbWithIdleTimeout(metav1.Duration{Duration: 2 * time.Minute}))), | ||
| expectedResult: true, | ||
| expectedIC: makeIC(spec(elb()), status(elbWithIdleTimeout(metav1.Duration{}))), | ||
| }, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add a test for a bad value to see what happens.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the "loadbalancer ELB connection idle timeout changed from unset to -1s" test what you had in mind? |
||
| { | ||
| name: "loadbalancer GCP Global Access changed from unset to global", | ||
| ic: makeIC(spec(gcpLB(operatorv1.GCPGlobalAccess)), status(lb(operatorv1.ExternalLoadBalancer))), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ import ( | |
| "fmt" | ||
| "strconv" | ||
| "strings" | ||
| "time" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| "github.com/google/go-cmp/cmp/cmpopts" | ||
|
|
@@ -80,6 +81,10 @@ const ( | |
| awsLBHealthCheckHealthyThresholdAnnotation = "service.beta.kubernetes.io/aws-load-balancer-healthcheck-healthy-threshold" | ||
| awsLBHealthCheckHealthyThresholdDefault = "2" | ||
|
|
||
| // awsELBConnectionIdleTimeoutAnnotation specifies the timeout for idle | ||
| // connections for a Classic ELB. | ||
| awsELBConnectionIdleTimeoutAnnotation = "service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout" | ||
|
|
||
| // iksLBScopeAnnotation is the annotation used on a service to specify an IBM | ||
| // load balancer IP type. | ||
| iksLBScopeAnnotation = "service.kubernetes.io/ibm-load-balancer-cloud-provider-ip-type" | ||
|
|
@@ -215,6 +220,8 @@ var ( | |
| // AWS LB health check interval annotation (see | ||
| // <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>). | ||
| awsLBHealthCheckIntervalAnnotation, | ||
| // AWS connection idle timeout annotation. | ||
| awsELBConnectionIdleTimeoutAnnotation, | ||
| // GCP Global Access internal Load Balancer annotation | ||
| // (see <https://issues.redhat.com/browse/NE-447>). | ||
| GCPGlobalAccessAnnotation, | ||
|
|
@@ -336,7 +343,8 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef | |
|
|
||
| service.Spec.Selector = controller.IngressControllerDeploymentPodSelector(ci).MatchLabels | ||
|
|
||
| isInternal := ci.Status.EndpointPublishingStrategy.LoadBalancer != nil && ci.Status.EndpointPublishingStrategy.LoadBalancer.Scope == operatorv1.InternalLoadBalancer | ||
| lb := ci.Status.EndpointPublishingStrategy.LoadBalancer | ||
| isInternal := lb != nil && lb.Scope == operatorv1.InternalLoadBalancer | ||
|
|
||
| if service.Annotations == nil { | ||
| service.Annotations = map[string]string{} | ||
|
|
@@ -356,10 +364,10 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef | |
|
|
||
| // Set the GCP Global Access annotation for internal load balancers on GCP only | ||
| if platform.Type == configv1.GCPPlatformType { | ||
| if ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP != nil { | ||
| globalAccessEnabled := ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess | ||
| if lb != nil && lb.ProviderParameters != nil && | ||
| lb.ProviderParameters.Type == operatorv1.GCPLoadBalancerProvider && | ||
| lb.ProviderParameters.GCP != nil { | ||
| globalAccessEnabled := lb.ProviderParameters.GCP.ClientAccess == operatorv1.GCPGlobalAccess | ||
| service.Annotations[GCPGlobalAccessAnnotation] = strconv.FormatBool(globalAccessEnabled) | ||
| } | ||
| } | ||
|
|
@@ -371,16 +379,23 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef | |
| } | ||
| switch platform.Type { | ||
| case configv1.AWSPlatformType: | ||
| if ci.Status.EndpointPublishingStrategy.LoadBalancer != nil && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters != nil && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS != nil && | ||
| ci.Status.EndpointPublishingStrategy.LoadBalancer.ProviderParameters.AWS.Type == operatorv1.AWSNetworkLoadBalancer { | ||
| service.Annotations[AWSLBTypeAnnotation] = AWSNLBAnnotation | ||
| // NLBs require a different health check interval than CLBs | ||
| service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalNLB | ||
| } else { | ||
| service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault | ||
| service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalDefault | ||
| if lb != nil && lb.ProviderParameters != nil { | ||
| if aws := lb.ProviderParameters.AWS; aws != nil && lb.ProviderParameters.Type == operatorv1.AWSLoadBalancerProvider { | ||
| switch aws.Type { | ||
| case operatorv1.AWSNetworkLoadBalancer: | ||
| service.Annotations[AWSLBTypeAnnotation] = AWSNLBAnnotation | ||
| // NLBs require a different health check interval than CLBs. | ||
| // See <https://bugzilla.redhat.com/show_bug.cgi?id=1908758>. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit - scattering bug numbers around feels like it's going to be a problem once Bugzilla is gone.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many other projects use Bugzilla outside of OpenShift. I doubt Bugzilla will ever go away (and I would be very disappointed if it did). |
||
| service.Annotations[awsLBHealthCheckIntervalAnnotation] = awsLBHealthCheckIntervalNLB | ||
| case operatorv1.AWSClassicLoadBalancer: | ||
| if aws.ClassicLoadBalancerParameters != nil { | ||
| if v := aws.ClassicLoadBalancerParameters.ConnectionIdleTimeout; v.Duration > 0 { | ||
| service.Annotations[awsELBConnectionIdleTimeoutAnnotation] = strconv.FormatUint(uint64(v.Round(time.Second).Seconds()), 10) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if platform.AWS != nil && len(platform.AWS.ResourceTags) > 0 { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the change to incorporate
GCP.ClientAccesdto fix a bug? Can you add a comment here, to replace the one you removed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention wasn't to fix bugs, but it probably does prevent some weird behavior and could prevent future bugs. I've added some comments and rewritten the commit message to elaborate on this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does commit "rework GCP logic" belong to this PR? Can it go in the upcoming commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. Are you suggesting I merge the commits? I thought it would be much simpler to understand the changes if I first refactored to simplify the existing logic and then added the new logic.