Add configurable connection draining timeout for L4 LoadBalancers

[mattrobenolt]

Adds networking.gke.io/connection-draining-timeout-sec annotation to configure connection draining timeout for L4 LoadBalancers with RBS enabled.

Currently, L4 LoadBalancers use a hardcoded 30 second timeout. For workloads with long-lived connections, this is quite insufficient. GCE supports up to 3600 seconds, but there is no way to configure this in the Service definition.

Note: for disclosure, I used the assistance of Claude Code to help implement this PR, but I have reviewed everything personally to the best of my knowledge.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mattrobenolt
Once this PR has been reviewed and has the lgtm label, please assign felipeyepez for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Welcome @mattrobenolt!

It looks like this is your first PR to kubernetes/ingress-gce 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-gce has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @mattrobenolt. Thanks for your PR.

I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[TortillaZHawaii]

Hi, thanks for the PR.

I would like to understand what happens, if the user has modified the timeout on the created Backend Service resource through Cloud Console or CLI. Unfortunately we did somewhat allow this to happen (even if it wasn't supported) and this PR would break the expectations for those users.

It's also not clear what the implications are for the rest of the stack, I'm not sure if there won't be any issues somewhere unexpected.

/hold
/ok-to-test

[TortillaZHawaii]

We could use "networking.gke.io/connection-draining-timeout" and use time.Duration to allow specifying time with unit, like in https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#configurable-container-restart-delay

[mattrobenolt]

I'm not entirely sure that makes sense in this context when GCP only accepts seconds as input in the API.

So what would it mean for a spec to say networking.gke.io/connection-draining-timeout = 100ms?

We could error on anything that is sub-second, and that'd enable us to use 5m or something else?

Is that more what you were thinking?

[TortillaZHawaii]

I'm not entirely sure that makes sense in this context when GCP only accepts seconds as input in the API.
So what would it mean for a spec to say networking.gke.io/connection-draining-timeout = 100ms?

We need validation regardless of using int or duration.

and that'd enable us to use 5m or something else?

yes exactly

[mattrobenolt]

Done, I swapped this to just connection-draining-timeout with some validation for the acceptable range.

[mattrobenolt]

I would like to understand what happens, if the user has modified the timeout on the created Backend Service resource through Cloud Console or CLI. Unfortunately we did somewhat allow this to happen (even if it wasn't supported) and this PR would break the expectations for those users.

The expected behavior with this PR is that value is preserved, and that's exactly our setup. We create the LoadBalancer, then need to modify it after with gcloud to change this attribute. pkg/backends/backends.go:472

[mattrobenolt]

@TortillaZHawaii if you'd like to take another look, I think everything is addressed. Also added more testing around pre-existing values not being clobbered.

[k8s-ci-robot]

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.