Skip to content

fix: Fix panic in descheduler when using --secure-port=0 #1647

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix: Fix panic in descheduler when using --secure-port=0 #1647

wants to merge 3 commits into from
+16 −8

Conversation

dongjiang1989
Copy link
Contributor

@dongjiang1989 dongjiang1989 commented Mar 12, 2025 •
edited
Loading

Fix panic in descheduler when using --secure-port=0:

$ ./descheduler --secure-port=0
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x70 pc=0x2a81a23]

goroutine 1 [running]:
sigs.k8s.io/descheduler/cmd/descheduler/app.NewDeschedulerCommand.func2(0xc0003a0500?, {0x2af01b3?, 0x4?, 0x2af01b7?})
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/cmd/descheduler/app/server.go:78 +0x83
github.com/spf13/cobra.(*Command).execute(0xc0002d6008, {0xc00004e150, 0x1, 0x1})
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/vendor/github.com/spf13/cobra/command.go:983 +0xaaa
github.com/spf13/cobra.(*Command).ExecuteC(0xc0002d6008)
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/vendor/github.com/spf13/cobra/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/vendor/github.com/spf13/cobra/command.go:1039
k8s.io/component-base/cli.run(0xc0002d6008)
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/vendor/k8s.io/component-base/cli/run.go:146 +0x290
k8s.io/component-base/cli.Run(0xc0002d6008?)
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/vendor/k8s.io/component-base/cli/run.go:46 +0x17
main.main()
	/Users/dongjiang/Documents/go/src/github.com/kubernetes-sigs/descheduler/cmd/descheduler/descheduler.go:31 +0xa5

Fixed:

 $ ./descheduler --secure-port=0 
E0312 18:00:53.239630   95679 server.go:73] "failed to run descheduler server" err="unable to create config: unable to build in cluster config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
E0312 18:00:53.240747   95679 run.go:72] "command failed" err="unable to create config: unable to build in cluster config: unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"

Close #1639

@dongjiang1989
fix panic
581d997 
Signed-off-by: dongjiang <[email protected]>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 12, 2025
@k8s-ci-robot k8s-ci-robot requested a review from damemi March 12, 2025 10:14
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

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 k8s-ci-robot requested a review from JaneLiuL March 12, 2025 10:14
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 12, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @dongjiang1989. Thanks for your PR.

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

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 12, 2025
@dongjiang1989
Copy link
Contributor Author

dongjiang1989 commented Apr 1, 2025 •
edited
Loading

/cc @damemi @JaneLiuL /PTAL

@k8s-ci-robot
Copy link
Contributor

@dongjiang1989: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @damemi @JaneLiuL PTAL

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.

@googs1025
Copy link
Member

/cc
I will review this. These days

@k8s-ci-robot k8s-ci-robot requested a review from googs1025 April 1, 2025 06:19
@googs1025
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 1, 2025
@dongjiang1989 dongjiang1989 reopened this Apr 2, 2025
@ricardomaraschini
Copy link
Contributor

/retest

ricardomaraschini
ricardomaraschini reviewed Apr 10, 2025
View reviewed changes
Copy link
Contributor

@ricardomaraschini ricardomaraschini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a concern about this argument, its help reads:

The port on which to serve HTTPS with authentication and authorization. If 0, don't serve HTTPS at all. (default 10258)

I don't think we implement authentication and authorization.

@dongjiang1989
Copy link
Contributor Author

dongjiang1989 commented Apr 10, 2025 •
edited
Loading

I have a concern about this argument, its help reads:

The port on which to serve HTTPS with authentication and authorization. If 0, don't serve HTTPS at all. (default 10258)

I don't think we implement authentication and authorization.

Thanks @ricardomaraschini

Hmmm.... Or we can override this secure-port parameter definition? 🤔
Maybe more than one parameter inheritance description is incorrect

@ricardomaraschini
Copy link
Contributor

Hmmm.... Or we can override this secure-port parameter definition? 🤔 Maybe more than one parameter inheritance description is incorrect

You are probably right. Those flags seem to come out directly from a different project and have been reused here. This may be orthogonal to what we are trying to solve here but I would like us to evaluate all the flags and try to consolidate what is really applicable here.

This one solves the panic and that is good.

@dongjiang1989
Copy link
Contributor Author

Thanks @ricardomaraschini

@dongjiang1989
Copy link
Contributor Author

Needs #1675

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ricardomaraschini ricardomaraschini ricardomaraschini left review comments

@damemi damemi Awaiting requested review from damemi

@JaneLiuL JaneLiuL Awaiting requested review from JaneLiuL

@googs1025 googs1025 Awaiting requested review from googs1025

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disabling secure port is not working
4 participants
@dongjiang1989 @k8s-ci-robot @googs1025 @ricardomaraschini