Skip to content
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

kube-proxy: add a flag to disable nodePortOnLocalhost #108250

Merged
merged 1 commit into from Nov 4, 2022
Merged

kube-proxy: add a flag to disable nodePortOnLocalhost #108250

merged 1 commit into from Nov 4, 2022

Conversation

cyclinder
Copy link
Contributor

@cyclinder cyclinder commented Feb 21, 2022 •
edited

Signed-off-by: cyclinder qifeng.guo@daocloud.io

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adding a flag to disable NodePort services can be accessed on localhost. this will reduce security risks.

Which issue(s) this PR fixes:

refer to:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a kube-proxy flag (--iptables-localhost-nodeports, default true) to allow disabling NodePort services on loopback addresses. Note: this only applies to iptables mode and ipv4.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 21, 2022
@cyclinder
Copy link
Contributor Author

/retest
/sig network

@k8s-ci-robot k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Mar 1, 2022
@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@cyclinder cyclinder force-pushed the add_flag_in_proxy branch 3 times, most recently from 3039fef to f8033e0 Compare March 8, 2022 15:08
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/cloudprovider area/kubelet area/test sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 8, 2022
thockin
thockin reviewed Oct 19, 2022
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

Overall LGTM, small picks

cmd/kube-proxy/app/server.go Outdated Show resolved Hide resolved
pkg/proxy/iptables/proxier.go Outdated Show resolved Hide resolved
@thockin
Copy link
Member

thockin commented Oct 19, 2022

@danwinship you have been most active in here - do you want final LGTM?

@BenTheElder
Copy link
Member

Release note still refers to this as legacy behavior:

Add a flag(localhost-nodePorts) to disables the legacy behavior of allowing NodePort services to be accessed via localhost. By default, Users can still access the nodeport service via localhost, which is consistent with the current behavior. You can disable this legacy behavior with this flag. Note this only works for iptables mode and IPv4.

suggest:

Add a kube-proxy flag (--iptables-localhost-nodeports, default true) to allow disabling NodePort services on loopback addresses. Note: this only applies to iptables mode

(Along with thockin's comment about renaming the flag)

also, perhaps it makes more sense to have --disable-iptables-nodeport-loopback (default false)?

I feel --boolean-flag=false is a bit awkward

@cyclinder
Copy link
Contributor Author

Thanks @thockin @BenTheElder .

also, perhaps it makes more sense to have --disable-iptables-nodeport-loopback (default false)?

I feel --boolean-flag=false is a bit awkward

@BenTheElder According to the discussion of #108250 (comment) with thockin and dan, we decide to make this flag to enable-nodeport-localhost something like. and which seems more "normal" for things like gates.

@thockin
Copy link
Member

thockin commented Oct 31, 2022

Just 1 nit!

@cyclinder
Copy link
Contributor Author

/test pull-kubernetes-unit

@cyclinder
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@cyclinder
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@cyclinder
Copy link
Contributor Author

/test pull-kubernetes-e2e-kind

@cyclinder
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Nov 2, 2022 •
edited

@cyclinder: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubernetes-e2e-capz-conformance f8033e0efb19f43188cf1d8688a4a61e18054ee8 link false /test pull-kubernetes-e2e-capz-conformance
pull-kubernetes-e2e-capz-azure-disk-vmss f8033e0efb19f43188cf1d8688a4a61e18054ee8 link false /test pull-kubernetes-e2e-capz-azure-disk-vmss
pull-kubernetes-e2e-capz-azure-disk f8033e0efb19f43188cf1d8688a4a61e18054ee8 link false /test pull-kubernetes-e2e-capz-azure-disk
pull-kubernetes-e2e-capz-azure-file f8033e0efb19f43188cf1d8688a4a61e18054ee8 link false /test pull-kubernetes-e2e-capz-azure-file
pull-kubernetes-e2e-capz-azure-file-vmss f8033e0efb19f43188cf1d8688a4a61e18054ee8 link false /test pull-kubernetes-e2e-capz-azure-file-vmss

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@cyclinder
Copy link
Contributor Author

current failing tests don't seem to be related to the changes

@aojea
Copy link
Member

aojea commented Nov 2, 2022

current failing tests don't seem to be related to the changes

yeah, it was #113548

/retest

thockin
thockin reviewed Nov 3, 2022
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

/approve

@thockin
Copy link
Member

thockin commented Nov 3, 2022

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cyclinder, thockin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Nov 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit d86c013 into kubernetes:master Nov 4, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022
@cyclinder cyclinder mentioned this pull request Nov 4, 2022
Merged
@ialidzhikov ialidzhikov mentioned this pull request Dec 16, 2022
Closed
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship requested changes

@BenTheElder BenTheElder BenTheElder left review comments

@thockin thockin thockin left review comments

@aojea aojea aojea left review comments

@freehan freehan Awaiting requested review from freehan

@johnbelamaric johnbelamaric Awaiting requested review from johnbelamaric

Assignees

@thockin thockin

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider area/code-generation area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node CI/Test Board
Archive-it
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.26
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@cyclinder @k8s-triage-robot @fedebongio @cici37 @aojea @thockin @BenTheElder @k8s-ci-robot @danwinship