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.

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

Proposal: Support specifying NodePort IP range #1547

Merged
merged 1 commit into from
Feb 23, 2018

Conversation

m1093782566
Copy link

@m1093782566 m1093782566 commented Dec 28, 2017

/sig network

@kubernetes/huawei

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 28, 2017

The `--nodeport-addresses` is defaulted to `0.0.0.0/0`, which means select all available interfaces and is compliance with current NodePort behaviour.

If people set the `--nodeport-addresses` flag to empty, kube-proxy will select the "who has the default route" + loopback interfaces. It's the same heuristic we use for `--advertise-address` in kube-apiserver and others. 
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this? If we default to 0.0.0.0/0, why not make "" an error? If anyone complains about it, we can consider something more heuristic like this (and maybe "default-route" is better than "", so 127.0.0.0/8,default-route becomes a valid value)

Copy link
Author

@m1093782566 m1093782566 Jan 3, 2018

Choose a reason for hiding this comment

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

I was seeking a way to express "default route". I agree an explicitly word, e.g. default-route you mentioned is better than "" to express "default route". THEN, we may need to modify the definition for --nodeport-addresses - it's a string list, valid values can either be IP block or "default-route".

So, the following values for --nodeport-addresses are all valid:

0.0.0.0/0
127.0.0.0/8
default-route
127.0.0.1/32,default-route
127.0.0.0/8,192.168.0.0/16

And an empty string for --nodeport-addresses is considered as invalid.


If people set the `--nodeport-addresses` flag to empty, kube-proxy will select the "who has the default route" + loopback interfaces. It's the same heuristic we use for `--advertise-address` in kube-apiserver and others. 

If people provide a non-zero IP block for `--nodeport-addresses`, kube-proxy will filter that down to just the IPs that applied to the node. 
Copy link
Member

Choose a reason for hiding this comment

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

Will kube-proxy periodically refresh the list of interfaces and recreate rules, in case of something like DHCP?

Copy link
Author

@m1093782566 m1093782566 Jan 3, 2018

Choose a reason for hiding this comment

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

Will kube-proxy periodically refresh the list of interfaces and recreate rules

Sorry, --advertise-address for kube-proxy is a list of IP blocks instead of interface names.

kube-proxy will periodically refresh the rules created based on --advertise-address. For example, if IP address of eth0 changes from 172.10.1.2 to 172.10.2.100 and user specifies 172.10.0.0/16 for --advertise-address. Kube-proxy WILL make sure proxy rules -d 172.10.0.0/16 exist.

However, if IP address of eth0 changes from 172.10.1.2 to 192.168.3.4 and user only specifies 172.10.0.0/16 for --advertise-address. Kube-proxy will NOT create proxy rules for 192.168.3.4 unless eth0 has the default route.

When refer to DHCP user case, network administrator usually reserves a RANGE of IP addresses for the DHCP server. So, IP address change will always fall in an IP range in DHCP scenario? That's to say an IP address of a interface will not change from 172.10.1.2 to 192.168.3.4 in our example?

iptables support specify multiple IPs in the destination parameter(`-d`). For example,

```shell
iptables -A PREROUTING -d 192.168.1.5,192.168.1.6 -p tcp --dport 22 -j ACCEPT
Copy link
Member

Choose a reason for hiding this comment

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

Why not use the CIDRs as specified by the user? Only the special default-route would need to be expanded, and that is IF we need it.

Copy link
Author

@m1093782566 m1093782566 Jan 3, 2018

Choose a reason for hiding this comment

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

Why not use the CIDRs as specified by the user?

It's a good implementation way as well 👍

We can make use of IP blocks directly in iptables command. However, we need to use individual IP in IPVS proxier.

Only the special default-route would need to be expanded, and that is IF we need it.

Yeah, that's true. -i eth0 works.

@m1093782566 m1093782566 force-pushed the nodeport-ip-range branch 3 times, most recently from 7ef4649 to f8fccb3 Compare January 3, 2018 08:38
@m1093782566
Copy link
Author

@thockin

All comments are addressed or clarified. Please to see if you are happy with the changes when you have a chance.

Copy link
Contributor

@dankenigsberg dankenigsberg left a comment

Choose a reason for hiding this comment

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

sorry for the very shallow review.


# Objective

This document propose creating a flag for kube-proxy to specify NodePort IP range.
Copy link
Contributor

Choose a reason for hiding this comment

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

English nit: propose -> proposes

Copy link
Author

Choose a reason for hiding this comment

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

Fixed now.


This proposal builds off of earlier requests to [[proxy] Listening on a specific IP for nodePort ](https://github.com/kubernetes/kubernetes/issues/21070), but proposes that we should find a way to tell kube-proxy what the NodePort IP blocks are instead of a single IP.

## Create new kube-proxy configuration flag
Copy link
Contributor

Choose a reason for hiding this comment

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

another English nit: I don't think that "flag" fits here. A "flag" is either raised or not. Here you suggest a more complex command-line option.

Copy link
Author

Choose a reason for hiding this comment

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

Sound fair. It's fixed now. Thanks!

@thockin
Copy link
Member

thockin commented Feb 23, 2018

I'm approving, though the impl may change just a bit (discussing the need for the "all")

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 23, 2018
@k8s-ci-robot
Copy link
Contributor

[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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 23, 2018
@k8s-ci-robot k8s-ci-robot merged commit e3d0fe2 into kubernetes:master Feb 23, 2018
@m1093782566
Copy link
Author

Okay, we can fix it up in follow-ups.

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021
Proposal: Support specifying NodePort IP range
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/design Categorizes issue or PR as related to design. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants