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

Make nodeport ip configurable #58052

Merged
merged 9 commits into from Feb 27, 2018

Conversation

@m1093782566
Member

m1093782566 commented Jan 10, 2018

What this PR does / why we need it:

By default, kube-proxy accepts everything from NodePort without any filter. It can be a problem for nodes which has both public and private NICs, and people only want to provide a service in private network and avoid exposing any internal service on the public IPs.

This PR makes nodeport ip configurable.

Which issue(s) this PR fixes:
Closes: #21070

Special notes for your reviewer:

Design proposal see: kubernetes/community#1547

Issue in feature repo: kubernetes/enhancements#539

Release note:

Make NodePort IP addresses configurable

@m1093782566 m1093782566 changed the title from Nodeip config to [WIP]Nodeip config Jan 10, 2018

@m1093782566 m1093782566 changed the title from [WIP]Nodeip config to Make nodeport ip configurable Jan 16, 2018

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Jan 16, 2018

/assign @thockin @brendandburns

/unassign

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 16, 2018

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.


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.

@vfreex

This comment has been minimized.

Contributor

vfreex commented Jan 17, 2018

I love this feature.
It would be better if we can alternately specify the whitelisted NIC for incoming traffic if we don't case the IP address and want to share the config file among nodes.

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Jan 17, 2018

@vfreex

I understand, but NIC name is very tricky especially in systemd system, for example, eth0, encap9, ensap7 etc. I doubt NIC name is more portable than CIDR.

allErrs := field.ErrorList{}
LOOP:
for i := range nodePortAddresses {
switch nodePortAddresses[i] {

This comment has been minimized.

@brendandburns

brendandburns Jan 17, 2018

Contributor

Why is this a switch with a single case? shouldn't that be an if?

This comment has been minimized.

@m1093782566
} else {
addressList := addresses.UnsortedList()
if len(addressList) == 1 && utilproxy.IsZeroCIDR(addressList[0]) {
writeLine(proxier.natRules,

This comment has been minimized.

@brendandburns

brendandburns Jan 17, 2018

Contributor

Can we structure this like:

cmd := []string {
    ...
}
if len(addressList) && ... {
    cmd = append(cmd, ...)
} else {
    cmd = append(cmd, ...)
}
cmd = append(cmd, ...)
writeLine(...)

So that we don't have quite so much code duplication?

This comment has been minimized.

@m1093782566
if err != nil {
glog.Errorf("Failed to get node IP, err: %v", err)
glog.Errorf("Failed to get node ip address matching nodeport cidr")

This comment has been minimized.

@brendandburns

brendandburns Jan 17, 2018

Contributor

Shouldn't we bail out instead of having the else?

if ... {
   glog.Errorf(...)
   return
}
...

This comment has been minimized.

@m1093782566
@brendandburns

This comment has been minimized.

Contributor

brendandburns commented Jan 17, 2018

This generally looks good to me, but should probably be approved by

@kubernetes/sig-network-api-reviews
@thockin

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Jan 18, 2018

All comments are fixed now. PTAL, thanks!

allErrs = append(allErrs, field.Invalid(fldPath, nodePortAddresses, "must be a valid IP block"))
break
}
if nodePortAddresses[i] == string(kubeproxyconfig.IPv4ZeroCIDR) || nodePortAddresses[i] == string(kubeproxyconfig.IPv6ZeroCIDR) {

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

why bother with this? It's not going to make any difference, and it will break when we introduce dual-stack support.

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

Fixed now.

func validateKubeProxyNodePortAddress(nodePortAddresses []string, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
LOOP:

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

you don't need this label? continue does what you want, unless I am misunderstanding

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

Yes, you are right. Label is unnecessary. That's a legacy code...

"-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`,
"-m", "addrtype", "--dst-type", "LOCAL",
"-j", string(kubeNodePortsChain))
addresses, err := utilproxy.GetNodeAddresss(proxier.nodePortAddresses, proxier.networkInterface)

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

GetNodeAddresss has too many 's'es

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

Fixed :)

// create nodeport rules for each IP one by one
iptablesDestIPListString := strings.Join(addressList, ",")
args = append(args,
"-d", iptablesDestIPListString,

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

don't you still want the LOCAL check? Otherwise this will capture cross-node nodeport accesses, I think...

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

Sorry, I am not sure I fully understand...

Do you mean host A(1.2.3.4), host B(2.3.4.5)

From host A, access 2.3.4.5:port is not allowed? IIUC, it's allowed?

AFAIK, LOCAL check is intended to capture packets whose dest-address is "local". Am I missing something?

continue
}
addressList := addresses.UnsortedList()
if len(addressList) == 1 && utilproxy.IsZeroCIDR(addressList[0]) {

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

I think these len=1 checks are all broken as soon as dual-stack lands. Can we just not special-case this and let the 0 ranges behave like any other?

This comment has been minimized.

@m1093782566
@@ -166,6 +166,8 @@ func AddFlags(options *Options, fs *pflag.FlagSet) {
"NAT timeout for TCP connections in the CLOSE_WAIT state")
fs.BoolVar(&options.config.EnableProfiling, "profiling", options.config.EnableProfiling, "If true enables profiling via web interface on /debug/pprof handler.")
fs.StringVar(&options.config.IPVS.Scheduler, "ipvs-scheduler", options.config.IPVS.Scheduler, "The ipvs scheduler type when proxy mode is ipvs")
fs.StringSliceVar(&options.config.NodePortAddresses, "nodeport-addresses", options.config.NodePortAddresses,
"Values are as a parameter to select the interfaces where nodeport works. Values must be either IP blocks or `default-route`, default to `0.0.0.0/0`")

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

the default should be "all interfaces" not 0.0.0.0/0. we need to include v6

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

default value to be "all-interfaces"? all-interfaces == 0.0.0.0/0 + ::/0

const (
DefaultRoute InterfaceType = "default-route"
IPv4ZeroCIDR InterfaceType = "0.0.0.0/0"

This comment has been minimized.

@thockin

thockin Jan 19, 2018

Member

everywhere you use a default, it should be both IPV4 and IPV6.

This comment has been minimized.

@m1093782566

m1093782566 Jan 19, 2018

Member

Changed to "all-interfaces" now.

@thockin

This comment has been minimized.

Member

thockin commented Jan 19, 2018

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 26, 2018

/retest

@@ -169,6 +169,8 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
"NAT timeout for TCP connections in the CLOSE_WAIT state")
fs.BoolVar(&o.config.EnableProfiling, "profiling", o.config.EnableProfiling, "If true enables profiling via web interface on /debug/pprof handler.")
fs.StringVar(&o.config.IPVS.Scheduler, "ipvs-scheduler", o.config.IPVS.Scheduler, "The ipvs scheduler type when proxy mode is ipvs")
fs.StringSliceVar(&o.config.NodePortAddresses, "nodeport-addresses", o.config.NodePortAddresses,

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

This is a FLAG, not code. Someone using the command-line doesn't know what a slice is. You have to tell them how to use it. "A comma-delimited list of IP blocks (e.g. 10.0.0.0/8, 1.2.3.4/32) used to filter addresses local to this node. Defaults to use all local addresses".

This comment has been minimized.

@m1093782566

m1093782566 Feb 27, 2018

Member

ACK, THANKS!

allErrs = append(allErrs, field.Invalid(fldPath, nodePortAddresses, "must be non-empty"))
break
}
if _, _, err := net.ParseCIDR(nodePortAddresses[i]); err != nil {

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

OK for now. The UX is a little weird, but probably won't get that much use.

@thockin

Can be fixed in followup.

isIPv6 := proxier.iptables.IsIpv6()
for address := range addresses {
// TODO(thockin, m1093782566): If/when we have dual-stack support we will want to distinguish v4 from v6 zero-CIDRs.
if utilproxy.IsZeroCIDR(address) {

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

need to also test the v6-ness of it. If I specified "10.0.0.0/8,::/0" I would not expect 192.168.1.1 to match.

break
}
// Ignore IP addresses with incorrect version
if isIPv6 && !conntrack.IsIPv6String(address) || !isIPv6 && conntrack.IsIPv6String(address) {

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

if isIPv6 != conntrack.IsIPv6String(address)

?

NetworkInterfaces []net.Interface
// The key of map Addrs is the network interface name
Address map[string][]net.Addr
}

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

add

var _ proxyutil.NetworkInterfacer = &FakeNetwork{}

}
// First round of iteration to pick out `0.0.0.0/0` or `::/0` for the sake of excluding non-zero IPs.
for _, cidr := range cidrs {
if IsZeroCIDR(cidr) {

This comment has been minimized.

@thockin

thockin Feb 26, 2018

Member

This will allow the zero-cidr more than once, Not a big deal, but odd.

@thockin

This comment has been minimized.

Member

thockin commented Feb 26, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 26, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1093782566, 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

@fejta-bot

This comment has been minimized.

fejta-bot commented Feb 26, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 27, 2018

@ixdy

I0226 21:34:57.293] Verifying verify-bazel.sh
I0226 21:34:57.309] 
I0226 21:34:57.310] +++ Running case: verify.bazel 
I0226 21:34:57.312] +++ working dir: /go/src/k8s.io/kubernetes
I0226 21:34:57.313] +++ command: bash "hack/make-rules/../../hack/verify-bazel.sh"
W0226 21:35:10.863] # github.com/bazelbuild/bazel-gazelle/internal/merger
W0226 21:35:10.864] ../../internal/merger/fix.go:93:28: too few values in struct initializer
W0226 21:35:10.864] ../../internal/merger/merger.go:806:16: too few values in struct initializer
W0226 21:35:10.864] ../../internal/merger/merger.go:866:19: too few values in struct initializer
W0226 21:35:10.865] ../../internal/merger/merger.go:870:19: too few values in struct initializer
W0226 21:35:11.056] # github.com/bazelbuild/bazel-gazelle/internal/rules
W0226 21:35:11.056] ../../internal/rules/generator.go:183:16: too few values in struct initializer
W0226 21:35:11.057] ../../internal/rules/sort_labels.go:45:16: too few values in struct initializer
W0226 21:35:11.063] !!! [0226 21:35:11] Call tree:
W0226 21:35:11.065] !!! [0226 21:35:11]  1: ./hack/update-bazel.sh:34 kube::util::go_install_from_commit(...)
W0226 21:35:11.070] !!! [0226 21:35:11] Call tree:
W0226 21:35:11.072] !!! [0226 21:35:11]  1: ./hack/update-bazel.sh:34 kube::util::go_install_from_commit(...)
I0226 21:35:12.105] +++ exit code: 1
I0226 21:35:12.110] +++ error: 1
I0226 21:35:12.132] FAILED   verify-bazel.sh	15s

Seems verify-bazel is broken, PTAL.

@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 27, 2018

/retest

1 similar comment
@ixdy

This comment has been minimized.

Member

ixdy commented Feb 27, 2018

/retest

@kevin-wangzefeng

This comment has been minimized.

Member

kevin-wangzefeng commented Feb 27, 2018

adding milestone as it's for 1.10

@kevin-wangzefeng kevin-wangzefeng added this to the v1.10 milestone Feb 27, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

Automatic merge from submit-queue (batch tested with PRs 60430, 60115, 58052, 60355, 60116). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 42378ea into kubernetes:master Feb 27, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation m1093782566 authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@m1093782566

This comment has been minimized.

Member

m1093782566 commented Feb 28, 2018

All outstanding comments will be fixed after freeze.

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