-
Notifications
You must be signed in to change notification settings - Fork 39.3k
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
refactor: disable insecure serving in kube-scheduler #96345
refactor: disable insecure serving in kube-scheduler #96345
Conversation
@ingvagabund: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
@knight42 I have followed your changes in #96216 combined with what I found in https://github.com/kubernetes/kubernetes/pull/95522/files. I have copied |
@ingvagabund Thanks for your effort! Now the major obstacle is how to make Prometheus fetch metrics from the secure port of controller-manager/scheduler in the job |
fb21bd9
to
fe3e2a9
Compare
fe3e2a9
to
e5692e3
Compare
Are you targeting 1.20 or 1.21? |
1.20 would be great, though with 3 days left before the code freeze, finishing the PR might be too rushed |
@x13n does any metric pipeline rely on the insecure serving for kube-scheduler? |
ee3f679
to
15b9ac7
Compare
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.
minor comments.
) | ||
fs.IPVar(&bindAddr, "address", bindAddr, | ||
"The IP address on which to serve the insecure --port (set to 0.0.0.0 for all IPv4 interfaces and :: for all IPv6 interfaces).") | ||
fs.MarkDeprecated("address", "This flag has no effect now and will be removed in v1.24.") |
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.
Mention the alternative: --bind-address
fs.MarkDeprecated("address", "This flag has no effect now and will be removed in v1.24.") | ||
|
||
fs.IntVar(&bindPort, "port", bindPort, "The port on which to serve unsecured, unauthenticated access. Set to 0 to disable.") | ||
fs.MarkDeprecated("port", "This flag has no effect now and will be removed in v1.24.") |
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.
Mention the alternative: --secure-port
cmd/kube-scheduler/app/server.go
Outdated
@@ -118,6 +131,11 @@ func runCommand(cmd *cobra.Command, opts *options.Options, registryOptions ...Op | |||
verflag.PrintAndExitIfRequested() | |||
cliflag.PrintFlags(cmd.Flags()) | |||
|
|||
err := checkNonZeroInsecurePort(cmd.Flags()) |
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.
do this in Deprecated.Validate
.
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.
If you mean DeprecatedOptions
, the type does not provide the port flag through it. Also, adding a new Port field would require changing other places.
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 it that many places to add it?
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.
Actually, not too many after all. Updated
// HealthzBindAddress is the IP address and port for the health check server to serve on, | ||
// defaulting to 0.0.0.0:10251 | ||
|
||
// Note: Both HealthzBindAddress and MetricsBindAddress fields are deprecated |
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.
Put the deprecated note in each field's go comment. And say that only empty or port 0 is allowed.
// HealthzBindAddress is the IP address and port for the health check server to serve on, | ||
// defaulting to 0.0.0.0:10251 | ||
|
||
// Note: Both HealthzBindAddress and MetricsBindAddress fields are deprecated |
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.
Same here
4aca9d4
to
ec206eb
Compare
ec206eb
to
07af669
Compare
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.
Thank you for working on this @ingvagabund
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, alculquicondor, dims, ingvagabund, JAORMX, knight42, liggitt, ravisantoshgudimetla, sttts 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:
Approvers can indicate their approval by writing |
/hold cancel |
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.
/lgtm
From logs: Flag --port has been deprecated, This flag has no effect now and will be removed in v1.24. So that's what we do. We had this flag only set to disable insecure serving, and insecure serving has been removed in upstream, thereby rendering the use of this flag a no-op. Controller-manager PR: kubernetes/kubernetes#96216 Scheduler PR: kubernetes/kubernetes#96345 Change-Id: If9009aa6f7c72a5ec8b7baf2326964167059c0a1 Reviewed-on: https://review.monogon.dev/c/monogon/+/665 Reviewed-by: Lorenz Brun <lorenz@monogon.tech>
What type of PR is this?
/kind deprecation
/kind cleanup
What this PR does / why we need it:
Disable insecure serving in kube-scheduler
Which issue(s) this PR fixes:
xref #91506
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: