Skip to content

Add dual-stack support for node-cache #657

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+125 −70
Open

Add dual-stack support for node-cache #657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 125 additions & 63 deletions cmd/node-cache/app/cache_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"k8s.io/dns/cmd/kube-dns/app/options"
"k8s.io/dns/pkg/dns/config"
"k8s.io/dns/pkg/netif"
"k8s.io/kubernetes/pkg/util/iptables"
utiliptables "k8s.io/kubernetes/pkg/util/iptables"
utilexec "k8s.io/utils/exec"
utilnet "k8s.io/utils/net"
Expand Down Expand Up @@ -62,13 +63,15 @@ type iptablesRule struct {

// CacheApp contains all the config required to run node-cache.
type CacheApp struct {
iptables utiliptables.Interface
iptablesRules []iptablesRule
params *ConfigParams
netifHandle *netif.NetifManager
kubednsConfig *options.KubeDNSConfig
exitChan chan struct{} // Channel to terminate background goroutines
clusterDNSIP net.IP
iptablesV4 utiliptables.Interface
iptablesV6 utiliptables.Interface
iptablesRulesV4 []iptablesRule
iptablesRulesV6 []iptablesRule
params *ConfigParams
netifHandle *netif.NetifManager
kubednsConfig *options.KubeDNSConfig
exitChan chan struct{} // Channel to terminate background goroutines
clusterDNSIP net.IP
}

func isLockedErr(err error) bool {
Expand Down Expand Up @@ -98,61 +101,86 @@ func (c *CacheApp) Init() {
c.params.SetupIptables = setupIptables
}

// isIPv6 return if the node-cache is working in IPv6 mode
// LocalIPs are guaranteed to have the same family
func (c *CacheApp) isIPv6() bool {
if len(c.params.LocalIPs) > 0 {
return utilnet.IsIPv6(c.params.LocalIPs[0])
}
return false
}

func (c *CacheApp) initIptables() {
// using the localIPStr param since we need ip strings here
for _, localIP := range strings.Split(c.params.LocalIPStr, ",") {
c.iptablesRules = append(c.iptablesRules, []iptablesRule{
// Match traffic destined for localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// There are rules in filter table to allow tracked connections to be accepted. Since we skipped connection tracking,
// need these additional filter table rules.
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Match traffic from localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// Additional filter table rules for traffic frpm localIp:localPort
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Skip connection tracking for requests to nodelocalDNS that are locally generated, example - by hostNetwork pods
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// skip connection tracking for healthcheck requests generated by liveness probe to health plugin
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
}...)
if utilnet.IsIPv6(net.ParseIP(localIP)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the loops for v4 and v6 should do the same, I'd suggest something like

const (
  ipv4 int = iota
  ipv6
)
func isIPv6ToIndex(isIPv6 bool) int {
  if isIPv6 {
    return ipv6
  }
  return ipv4
}
...
i := isIPv6ToIndex(utilnet.IsIPv6(net.ParseIP(localIP)))
c.iptablesRules[i] = = append(c.iptablesRules[i], ...)

c.iptablesRulesV6 = append(c.iptablesRulesV6, []iptablesRule{
// Match traffic destined for localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// There are rules in filter table to allow tracked connections to be accepted. Since we skipped connection tracking,
// need these additional filter table rules.
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Match traffic from localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// Additional filter table rules for traffic frpm localIp:localPort
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Skip connection tracking for requests to nodelocalDNS that are locally generated, example - by hostNetwork pods
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// skip connection tracking for healthcheck requests generated by liveness probe to health plugin
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
}...)
} else {
c.iptablesRulesV4 = append(c.iptablesRulesV4, []iptablesRule{
// Match traffic destined for localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainPrerouting, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// There are rules in filter table to allow tracked connections to be accepted. Since we skipped connection tracking,
// need these additional filter table rules.
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainInput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Match traffic from localIp:localPort and set the flows to be NOTRACKED, this skips connection tracking
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// Additional filter table rules for traffic frpm localIp:localPort
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
{utiliptables.TableFilter, utiliptables.ChainOutput, []string{"-p", "udp", "-s", localIP,
"--sport", c.params.LocalPort, "-j", "ACCEPT", "-m", "comment", "--comment", iptablesCommentAllowTraffic}},
// Skip connection tracking for requests to nodelocalDNS that are locally generated, example - by hostNetwork pods
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "udp", "-d", localIP,
"--dport", c.params.LocalPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
// skip connection tracking for healthcheck requests generated by liveness probe to health plugin
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-d", localIP,
"--dport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
{utiliptables.Table("raw"), utiliptables.ChainOutput, []string{"-p", "tcp", "-s", localIP,
"--sport", c.params.HealthPort, "-j", "NOTRACK", "-m", "comment", "--comment", iptablesCommentSkipConntrack}},
}...)
}

}
c.iptables = newIPTables(c.isIPv6())
c.iptablesV4 = newIPTables(iptables.ProtocolIPv4)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a system call? To save resources, it would probably be better to do something like

if len(c.iptablesRules[ipv4]) > 0 {
	c.iptables[ipv4] = newIPTables(iptables.ProtocolIPv4)
}
if len(c.iptablesRules[ipv6]) > 0 {
	c.iptables[ipv6] = newIPTables(iptables.ProtocolIPv6)
}

c.iptablesV6 = newIPTables(iptables.ProtocolIPv6)
}

func newIPTables(isIPv6 bool) utiliptables.Interface {
func newIPTables(protocol iptables.Protocol) utiliptables.Interface {
execer := utilexec.New()
protocol := utiliptables.ProtocolIPv4
if isIPv6 {
protocol = utiliptables.ProtocolIPv6
}
return utiliptables.New(execer, protocol)
}

Expand Down Expand Up @@ -180,32 +208,66 @@ func (c *CacheApp) TeardownNetworking() error {
err = c.netifHandle.RemoveDummyDevice(c.params.InterfaceName)
}
if c.params.SetupIptables {
for _, rule := range c.iptablesRules {
for _, rule := range c.iptablesRulesV4 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The treatment for ipv4 and ipv6 should be identical, right? If so, I'd suggest collapsing

	iptablesV4      utiliptables.Interface
	iptablesV6      utiliptables.Interface

to

	iptables      [2]utiliptables.Interface

and similarly

	iptablesRules      [2][]iptablesRule

This way, we can write something like

for i := range(2) {
	for _, rule := range c.iptablesRules[i] {
		do something with c.iptables[i] instead of c.iptablesV4 or c.iptablesV6
	}
}

Actually, instead of i := range 2 we could even write _, i := range []int{ipv4, ipv6}.

exists := true
for exists == true {
// check in a loop in case the same rule got added multiple times.
err = c.iptablesV4.DeleteRule(rule.table, rule.chain, rule.args...)
if err != nil {
clog.Errorf("Failed deleting iptablesV4 rule %v, error - %v", rule, err)
handleIPTablesError(err)
}
exists, err = c.iptablesV4.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
if err != nil {
clog.Errorf("Failed checking iptablesV4 rule after deletion, rule - %v, error - %v", rule, err)
handleIPTablesError(err)
}
}
// Delete the rule one last time since EnsureRule creates the rule if it doesn't exist
err = c.iptablesV4.DeleteRule(rule.table, rule.chain, rule.args...)
}
for _, rule := range c.iptablesRulesV6 {
exists := true
for exists == true {
// check in a loop in case the same rule got added multiple times.
err = c.iptables.DeleteRule(rule.table, rule.chain, rule.args...)
err = c.iptablesV6.DeleteRule(rule.table, rule.chain, rule.args...)
if err != nil {
clog.Errorf("Failed deleting iptables rule %v, error - %v", rule, err)
clog.Errorf("Failed deleting iptablesV6 rule %v, error - %v", rule, err)
handleIPTablesError(err)
}
exists, err = c.iptables.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
exists, err = c.iptablesV6.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
if err != nil {
clog.Errorf("Failed checking iptables rule after deletion, rule - %v, error - %v", rule, err)
clog.Errorf("Failed checking iptablesV6 rule after deletion, rule - %v, error - %v", rule, err)
handleIPTablesError(err)
}
}
// Delete the rule one last time since EnsureRule creates the rule if it doesn't exist
err = c.iptables.DeleteRule(rule.table, rule.chain, rule.args...)
err = c.iptablesV6.DeleteRule(rule.table, rule.chain, rule.args...)
}
}
return err
}

func (c *CacheApp) setupNetworking() {
if c.params.SetupIptables {
for _, rule := range c.iptablesRules {
exists, err := c.iptables.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
for _, rule := range c.iptablesRulesV4 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same remark about looping through {0,1} here.

exists, err := c.iptablesV4.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
switch {
case exists:
// debug messages can be printed by including "debug" plugin in coreFile.
clog.Debugf("iptables rule %v for nodelocaldns already exists", rule)
continue
case err == nil:
clog.Infof("Added back nodelocaldns rule - %v", rule)
continue
default:
// iptables check/rule add failed with error since control reached here.
clog.Errorf("Error checking/adding iptables rule %v, error - %v", rule, err)
handleIPTablesError(err)
}
}
for _, rule := range c.iptablesRulesV6 {
exists, err := c.iptablesV6.EnsureRule(utiliptables.Prepend, rule.table, rule.chain, rule.args...)
switch {
case exists:
// debug messages can be printed by including "debug" plugin in coreFile.
Expand Down
7 changes: 0 additions & 7 deletions cmd/node-cache/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (

corednsmain "github.com/coredns/coredns/coremain"
clog "github.com/coredns/coredns/plugin/pkg/log"
utilnet "k8s.io/utils/net"

"github.com/coredns/caddy"
// blank imports to make sure the plugin code is pulled in from vendor when building node-cache image
Expand Down Expand Up @@ -100,12 +99,6 @@ func parseAndValidateFlags() (*app.ConfigParams, error) {
params.LocalIPs = append(params.LocalIPs, newIP)
}

// validate all the IPs have the same IP family
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure the condition validated here is not assumed anywhere else? This seems to require testing. The deleted comment above func (c *CacheApp) isIPv6() bool says

// LocalIPs are guaranteed to have the same family

for _, ip := range params.LocalIPs {
if utilnet.IsIPv6(params.LocalIPs[0]) != utilnet.IsIPv6(ip) {
return params, fmt.Errorf("unexpected IP Family for localIP - %q, want IPv6=%v", ip, utilnet.IsIPv6(params.LocalIPs[0]))
}
}
// lookup specified dns port
f := flag.Lookup("dns.port")
if f == nil {
Expand Down