fix nil function invocation in client-go/tools/leaderelection #78778

hbagdi · 2019-06-06T21:58:52Z

It should be possible for the user of leaderelection API to not provide
callbacks for stopped and started leading conditions. Currnetly, the
code results in fatal error and exits.

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change

/kind bug

/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-06-06T21:58:53Z

Welcome @hbagdi!

It looks like this is your first PR to kubernetes/kubernetes 🎉. 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/kubernetes 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 · 2019-06-06T21:58:56Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2019-06-06T21:59:00Z

Hi @hbagdi. Thanks for your PR.

I'm waiting for a kubernetes 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.

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/test-infra repository.

hbagdi · 2019-06-06T22:06:25Z

I signed it

neolit123

thank you for the PR, i think this make sense, but needs approval by the maintainers.
the 1.15 code freeze will be lifted after 11th of june.
try pinging #sig-api-machinery on slack if needed after that.

/ok-to-test
/priority backlog

fedebongio · 2019-06-10T21:05:14Z

/assign @cheftako @roycaihw

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

cheftako

/lgtm

roycaihw · 2019-06-18T23:45:58Z

looks good

/assign @mikedanese
for approval

mikedanese · 2019-06-19T03:27:05Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

 	}()
 	if !le.acquire(ctx) {
 		return // ctx signalled done
 	}
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
-	go le.config.Callbacks.OnStartedLeading(ctx)
+	if le.config.Callbacks.OnStartedLeading != nil {


The other one is fine, but if this is nil, we should probably panic. There's no point to this client if you don't do anything when you get leadership.

A client could potentially not actively want to be notified of it being a leader and but choose to check the leader status using IsLeader() function whenever it is actually going to take up any action.

You can do this by providing an empty OnStartedLeading callback. I am hesitant to further facilitate this behavior.

Mike has a good point. That having been said I personally would rather an explicit panic which explained the issue (eg. "Leader elect called without a proper started leading method") than a fatal error about referencing invalid memory.

You can do this by providing an empty OnStartedLeading callback. I am hesitant to further facilitate this behavior.

My take is that zero values in Go shouldn't panic in such a case.

Please let me know the exact change that should be made to move this forward. You guys are more familiar with the API style and end-user expectations here so I'll make a change that works for everyone.

mikedanese · 2019-06-21T02:38:13Z

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go

@@ -190,14 +190,18 @@ type LeaderElector struct {
 func (le *LeaderElector) Run(ctx context.Context) {
 	defer func() {
 		runtime.HandleCrash()
-		le.config.Callbacks.OnStoppedLeading()


Sorry I misread this originally. This is intended to crash.

mikedanese · 2019-06-21T02:40:29Z

Can you validate that OnStart and OnFinish callbacks are non-nil in NewLeaderElector, else return an error? The interface is designed so that these are explicitly required. If you choose to ignore them (very unadvised), you must do so explicitly.

hbagdi · 2019-07-29T18:37:46Z

@mikedanese updated. Please re-review.

If user specifies a nil callback, then error out rather than crashing at runtime due to a nil reference.

mikedanese · 2019-07-30T14:32:56Z

/lgtm
/approve

k8s-ci-robot · 2019-07-30T14:33:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hbagdi, mikedanese

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/client-go/tools/leaderelection/OWNERS~~ [mikedanese]

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

hbagdi · 2019-07-30T14:56:46Z

How do we fix the failing CI? 🤔

mikedanese · 2019-07-30T15:16:07Z

/retest

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 6, 2019

hbagdi changed the title ~~fix nil function invocation in client-go/tools/leaderlection~~ fix nil function invocation in client-go/tools/leaderelection Jun 6, 2019

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 6, 2019

k8s-ci-robot requested review from eparis and resouer June 6, 2019 22:00

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 6, 2019

neolit123 reviewed Jun 8, 2019

View reviewed changes

k8s-ci-robot assigned cheftako and roycaihw Jun 10, 2019

cheftako reviewed Jun 12, 2019

View reviewed changes

staging/src/k8s.io/client-go/tools/leaderelection/leaderelection.go Outdated Show resolved Hide resolved

hbagdi force-pushed the fix/nil-functions branch from b1cd519 to 2e2a40c Compare June 12, 2019 03:52

cheftako reviewed Jun 12, 2019

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2019

k8s-ci-robot assigned mikedanese Jun 18, 2019

mikedanese reviewed Jun 19, 2019

View reviewed changes

mikedanese reviewed Jun 21, 2019

View reviewed changes

hbagdi force-pushed the fix/nil-functions branch from 2e2a40c to 906edfd Compare July 29, 2019 18:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2019

fix: do not allow nil Callbacks functions

9dbbc65

If user specifies a nil callback, then error out rather than crashing at runtime due to a nil reference.

hbagdi force-pushed the fix/nil-functions branch from 906edfd to 9dbbc65 Compare July 29, 2019 20:18

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 30, 2019

k8s-ci-robot merged commit 758af11 into kubernetes:master Jul 30, 2019

k8s-ci-robot added this to the v1.16 milestone Jul 30, 2019

hbagdi deleted the fix/nil-functions branch July 30, 2019 17:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix nil function invocation in client-go/tools/leaderelection #78778

fix nil function invocation in client-go/tools/leaderelection #78778

hbagdi commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

hbagdi commented Jun 6, 2019

neolit123 left a comment

fedebongio commented Jun 10, 2019

cheftako left a comment

roycaihw commented Jun 18, 2019

mikedanese Jun 19, 2019

hbagdi Jun 19, 2019

mikedanese Jun 19, 2019

cheftako Jun 20, 2019

hbagdi Jun 20, 2019

mikedanese Jun 21, 2019

mikedanese commented Jun 21, 2019 •

edited

hbagdi commented Jul 29, 2019

mikedanese commented Jul 30, 2019

k8s-ci-robot commented Jul 30, 2019

hbagdi commented Jul 30, 2019

mikedanese commented Jul 30, 2019

fix nil function invocation in client-go/tools/leaderelection #78778

fix nil function invocation in client-go/tools/leaderelection #78778

Conversation

hbagdi commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

k8s-ci-robot commented Jun 6, 2019

hbagdi commented Jun 6, 2019

neolit123 left a comment

Choose a reason for hiding this comment

fedebongio commented Jun 10, 2019

cheftako left a comment

Choose a reason for hiding this comment

roycaihw commented Jun 18, 2019

mikedanese Jun 19, 2019

Choose a reason for hiding this comment

hbagdi Jun 19, 2019

Choose a reason for hiding this comment

mikedanese Jun 19, 2019

Choose a reason for hiding this comment

cheftako Jun 20, 2019

Choose a reason for hiding this comment

hbagdi Jun 20, 2019

Choose a reason for hiding this comment

mikedanese Jun 21, 2019

Choose a reason for hiding this comment

mikedanese commented Jun 21, 2019 • edited

hbagdi commented Jul 29, 2019

mikedanese commented Jul 30, 2019

k8s-ci-robot commented Jul 30, 2019

hbagdi commented Jul 30, 2019

mikedanese commented Jul 30, 2019

mikedanese commented Jun 21, 2019 •

edited