Skip to content

Commit

Permalink
Permit ExternalIP on input (#970)
Browse files Browse the repository at this point in the history 
* fact(network_policy): validate ClusterIP CIDR

Ensure that --service-cluster-ip-range is a valid CIDR while controller
is starting up.

* fix(network_policy): parse/validate NodePort

Validate the NodePort range that is passed and allow for it to be
specified with hyphens which is what the previous example used to show
and is more cohesive with the way NodePort ranges are specified when
passed to the kube-apiserver.

* test(network_policy): add tests for input validation

* feat(network_policy): permit ExternalIP on input

fixes #934

* fix(network_policy): ensure pos with index offset

Because iptables list function now appears to be returning -N and -P
items in the chain results, we need to account for them when taking into
consideration the rule position.

* fix(network_policy): add uuid to comments on ensure

iptables list is now no longer keeping the position of parameters which
means that we can't compare string to string. In absence of a better way
to handle this, this adds a UUID to the comment string which can then be
looked for when determining what position a rule occupies.
  • Loading branch information
@aauren
aauren authored Aug 25, 2020
1 parent c6ef3b8 commit 827ce55
Show file tree
Hide file tree
Showing 4 changed files with 280 additions and 26 deletions.
3 changes: 2 additions & 1 deletion docs/user-guide.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ Usage of kube-router:
--run-router Enables Pod Networking -- Advertises and learns the routes to Pods via iBGP. (default true)
--run-service-proxy Enables Service Proxy -- sets up IPVS for Kubernetes Services. (default true)
--service-cluster-ip-range string CIDR value from which service cluster IPs are assigned. Default: 10.96.0.0/12 (default "10.96.0.0/12")
--service-node-port-range string NodePort range. Default: 30000-32767 (default "30000:32767")
--service-external-ip-range strings Specify external IP CIDRs that are used for inter-cluster communication (can be specified multiple times)
--service-node-port-range string NodePort range specified with either a hyphen or colon (default "30000-32767")
-v, --v string log level for V logs (default "0")
-V, --version Print version information.
```
Expand Down
126 changes: 103 additions & 23 deletions pkg/controllers/netpol/network_policy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,17 @@ const (

// NetworkPolicyController strcut to hold information required by NetworkPolicyController
type NetworkPolicyController struct {
nodeIP net.IP
nodeHostName string
serviceClusterIPRange string
serviceNodePortRange string
mu sync.Mutex
syncPeriod time.Duration
MetricsEnabled bool
v1NetworkPolicy bool
healthChan chan<- *healthcheck.ControllerHeartbeat
fullSyncRequestChan chan struct{}
nodeIP net.IP
nodeHostName string
serviceClusterIPRange net.IPNet
serviceExternalIPRanges []net.IPNet
serviceNodePortRange string
mu sync.Mutex
syncPeriod time.Duration
MetricsEnabled bool
v1NetworkPolicy bool
healthChan chan<- *healthcheck.ControllerHeartbeat
fullSyncRequestChan chan struct{}

ipSetHandler *utils.IPSet

Expand Down Expand Up @@ -200,7 +201,19 @@ func (npc *NetworkPolicyController) ensureTopLevelChains() {
glog.Fatalf("Failed to initialize iptables executor due to %s", err.Error())
}

ensureRuleAtposition := func(chain string, ruleSpec []string, position int) {
addUuidForRuleSpec := func(chain string, ruleSpec *[]string) (string, error) {
hash := sha256.Sum256([]byte(chain + strings.Join(*ruleSpec, "")))
encoded := base32.StdEncoding.EncodeToString(hash[:])[:16]
for idx, part := range *ruleSpec {
if "--comment" == part {
(*ruleSpec)[idx+1] = (*ruleSpec)[idx+1] + " - " + encoded
return encoded, nil
}
}
return "", fmt.Errorf("could not find a comment in the ruleSpec string given: %s", strings.Join(*ruleSpec, " "))
}

ensureRuleAtPosition := func(chain string, ruleSpec []string, uuid string, position int) {
exists, err := iptablesCmdHandler.Exists("filter", chain, ruleSpec...)
if err != nil {
glog.Fatalf("Failed to verify rule exists in %s chain due to %s", chain, err.Error())
Expand All @@ -217,11 +230,18 @@ func (npc *NetworkPolicyController) ensureTopLevelChains() {
glog.Fatalf("failed to list rules in filter table %s chain due to %s", chain, err.Error())
}

var ruleNo int
var ruleNo, ruleIndexOffset int
for i, rule := range rules {
rule = strings.Replace(rule, "\"", "", 2) //removes quote from comment string
if strings.Contains(rule, strings.Join(ruleSpec, " ")) {
ruleNo = i
if strings.HasPrefix(rule, "-P") || strings.HasPrefix(rule, "-N") {
// if this chain has a default policy, then it will show as rule #1 from iptablesCmdHandler.List so we
// need to account for this offset
ruleIndexOffset += 1
continue
}
if strings.Contains(rule, uuid) {
// range uses a 0 index, but iptables uses a 1 index so we need to increase ruleNo by 1
ruleNo = i+1-ruleIndexOffset
break
}
}
Expand All @@ -245,19 +265,44 @@ func (npc *NetworkPolicyController) ensureTopLevelChains() {
glog.Fatalf("Failed to run iptables command to create %s chain due to %s", customChain, err.Error())
}
args := []string{"-m", "comment", "--comment", "kube-router netpol", "-j", customChain}
ensureRuleAtposition(builtinChain, args, 1)
uuid, err := addUuidForRuleSpec(builtinChain, &args)
if err != nil {
glog.Fatalf("Failed to get uuid for rule: %s", err.Error())
}
ensureRuleAtPosition(builtinChain, args, uuid,1)
}

whitelistServiceVips := []string{"-m", "comment", "--comment", "allow traffic to cluster IP", "-d", npc.serviceClusterIPRange, "-j", "RETURN"}
ensureRuleAtposition(kubeInputChainName, whitelistServiceVips, 1)
whitelistServiceVips := []string{"-m", "comment", "--comment", "allow traffic to cluster IP", "-d", npc.serviceClusterIPRange.String(), "-j", "RETURN"}
uuid, err := addUuidForRuleSpec(kubeInputChainName, &whitelistServiceVips)
if err != nil {
glog.Fatalf("Failed to get uuid for rule: %s", err.Error())
}
ensureRuleAtPosition(kubeInputChainName, whitelistServiceVips, uuid, 1)

whitelistTCPNodeports := []string{"-p", "tcp", "-m", "comment", "--comment", "allow LOCAL traffic to node ports", "-m", "addrtype", "--dst-type", "LOCAL",
whitelistTCPNodeports := []string{"-p", "tcp", "-m", "comment", "--comment", "allow LOCAL TCP traffic to node ports", "-m", "addrtype", "--dst-type", "LOCAL",
"-m", "multiport", "--dports", npc.serviceNodePortRange, "-j", "RETURN"}
ensureRuleAtposition(kubeInputChainName, whitelistTCPNodeports, 2)
uuid, err = addUuidForRuleSpec(kubeInputChainName, &whitelistTCPNodeports)
if err != nil {
glog.Fatalf("Failed to get uuid for rule: %s", err.Error())
}
ensureRuleAtPosition(kubeInputChainName, whitelistTCPNodeports, uuid, 2)

whitelistUDPNodeports := []string{"-p", "udp", "-m", "comment", "--comment", "allow LOCAL traffic to node ports", "-m", "addrtype", "--dst-type", "LOCAL",
whitelistUDPNodeports := []string{"-p", "udp", "-m", "comment", "--comment", "allow LOCAL UDP traffic to node ports", "-m", "addrtype", "--dst-type", "LOCAL",
"-m", "multiport", "--dports", npc.serviceNodePortRange, "-j", "RETURN"}
ensureRuleAtposition(kubeInputChainName, whitelistUDPNodeports, 3)
uuid, err = addUuidForRuleSpec(kubeInputChainName, &whitelistUDPNodeports)
if err != nil {
glog.Fatalf("Failed to get uuid for rule: %s", err.Error())
}
ensureRuleAtPosition(kubeInputChainName, whitelistUDPNodeports, uuid, 3)

for externalIPIndex, externalIPRange := range npc.serviceExternalIPRanges {
whitelistServiceVips := []string{"-m", "comment", "--comment", "allow traffic to external IP range: " + externalIPRange.String(), "-d", externalIPRange.String(), "-j", "RETURN"}
uuid, err = addUuidForRuleSpec(kubeInputChainName, &whitelistServiceVips)
if err != nil {
glog.Fatalf("Failed to get uuid for rule: %s", err.Error())
}
ensureRuleAtPosition(kubeInputChainName, whitelistServiceVips, uuid, externalIPIndex+4)
}

}

Expand Down Expand Up @@ -1815,8 +1860,43 @@ func NewNetworkPolicyController(clientset kubernetes.Interface,
// be up to date with all of the policy changes from any enqueued request after that
npc.fullSyncRequestChan = make(chan struct{}, 1)

npc.serviceClusterIPRange = config.ClusterIPCIDR
npc.serviceNodePortRange = config.NodePortRange
// Validate and parse ClusterIP service range
_, ipnet, err := net.ParseCIDR(config.ClusterIPCIDR)
if err != nil {
return nil, fmt.Errorf("failed to get parse --service-cluster-ip-range parameter: %s", err.Error())
}
npc.serviceClusterIPRange = *ipnet

// Validate and parse NodePort range
nodePortValidator := regexp.MustCompile(`^([0-9]+)[:-]{1}([0-9]+)$`)
if matched := nodePortValidator.MatchString(config.NodePortRange); !matched {
return nil, fmt.Errorf("failed to parse node port range given: '%s' please see specification in help text", config.NodePortRange)
}
matches := nodePortValidator.FindStringSubmatch(config.NodePortRange)
if len(matches) != 3 {
return nil, fmt.Errorf("could not parse port number from range given: '%s'", config.NodePortRange)
}
port1, err := strconv.ParseInt(matches[1], 10, 16)
if err != nil {
return nil, fmt.Errorf("could not parse first port number from range given: '%s'", config.NodePortRange)
}
port2, err := strconv.ParseInt(matches[2], 10, 16)
if err != nil {
return nil, fmt.Errorf("could not parse second port number from range given: '%s'", config.NodePortRange)
}
if port1 >= port2 {
return nil, fmt.Errorf("port 1 is greater than or equal to port 2 in range given: '%s'", config.NodePortRange)
}
npc.serviceNodePortRange = fmt.Sprintf("%d:%d", port1, port2)

// Validate and parse ExternalIP service range
for _, externalIPRange := range config.ExternalIPCIDRs {
_, ipnet, err := net.ParseCIDR(externalIPRange)
if err != nil {
return nil, fmt.Errorf("failed to get parse --service-external-ip-range parameter: '%s'. Error: %s", externalIPRange, err.Error())
}
npc.serviceExternalIPRanges = append(npc.serviceExternalIPRanges, *ipnet)
}

if config.MetricsEnabled {
//Register the metrics for this controller
Expand Down
170 changes: 170 additions & 0 deletions pkg/controllers/netpol/network_policy_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package netpol

import (
"context"
"github.com/cloudnativelabs/kube-router/pkg/options"
"net"
"strings"
"testing"
Expand Down Expand Up @@ -246,6 +247,30 @@ func testForMissingOrUnwanted(t *testing.T, targetMsg string, got []podInfo, wan
}
}

func newMinimalKubeRouterConfig(clusterIPCIDR string, nodePortRange string, hostNameOverride string, externalIPs []string) *options.KubeRouterConfig {
kubeConfig := options.NewKubeRouterConfig()
if clusterIPCIDR != "" {
kubeConfig.ClusterIPCIDR = clusterIPCIDR
}
if nodePortRange != "" {
kubeConfig.NodePortRange = nodePortRange
}
if hostNameOverride != "" {
kubeConfig.HostnameOverride = hostNameOverride
}
if externalIPs != nil {
kubeConfig.ExternalIPCIDRs = externalIPs
}
return kubeConfig
}

type tNetPolConfigTestCase struct {
name string
config *options.KubeRouterConfig
expectError bool
errorText string
}

func TestNewNetworkPolicySelectors(t *testing.T) {
testCases := []tNetpolTestCase{
{
Expand Down Expand Up @@ -370,6 +395,151 @@ func TestNewNetworkPolicySelectors(t *testing.T) {
}
}

func TestNetworkPolicyController(t *testing.T) {
testCases := []tNetPolConfigTestCase{
{
"Default options are successful",
newMinimalKubeRouterConfig("", "", "node", nil),
false,
"",
},
{
"Missing nodename fails appropriately",
newMinimalKubeRouterConfig("", "", "", nil),
true,
"Failed to identify the node by NODE_NAME, hostname or --hostname-override",
},
{
"Test bad cluster CIDR (not properly formatting ip address)",
newMinimalKubeRouterConfig("10.10.10", "", "node", nil),
true,
"failed to get parse --service-cluster-ip-range parameter: invalid CIDR address: 10.10.10",
},
{
"Test bad cluster CIDR (not using an ip address)",
newMinimalKubeRouterConfig("foo", "", "node", nil),
true,
"failed to get parse --service-cluster-ip-range parameter: invalid CIDR address: foo",
},
{
"Test bad cluster CIDR (using an ip address that is not a CIDR)",
newMinimalKubeRouterConfig("10.10.10.10", "", "node", nil),
true,
"failed to get parse --service-cluster-ip-range parameter: invalid CIDR address: 10.10.10.10",
},
{
"Test good cluster CIDR (using single IP with a /32)",
newMinimalKubeRouterConfig("10.10.10.10/32", "", "node", nil),
false,
"",
},
{
"Test good cluster CIDR (using normal range with /24)",
newMinimalKubeRouterConfig("10.10.10.0/24", "", "node", nil),
false,
"",
},
{
"Test bad node port specification (using commas)",
newMinimalKubeRouterConfig("", "8080,8081", "node", nil),
true,
"failed to parse node port range given: '8080,8081' please see specification in help text",
},
{
"Test bad node port specification (not using numbers)",
newMinimalKubeRouterConfig("", "foo:bar", "node", nil),
true,
"failed to parse node port range given: 'foo:bar' please see specification in help text",
},
{
"Test bad node port specification (using anything in addition to range)",
newMinimalKubeRouterConfig("", "8080,8081-8090", "node", nil),
true,
"failed to parse node port range given: '8080,8081-8090' please see specification in help text",
},
{
"Test bad node port specification (using reversed range)",
newMinimalKubeRouterConfig("", "8090-8080", "node", nil),
true,
"port 1 is greater than or equal to port 2 in range given: '8090-8080'",
},
{
"Test bad node port specification (port out of available range)",
newMinimalKubeRouterConfig("", "132000-132001", "node", nil),
true,
"could not parse first port number from range given: '132000-132001'",
},
{
"Test good node port specification (using colon separator)",
newMinimalKubeRouterConfig("", "8080:8090", "node", nil),
false,
"",
},
{
"Test good node port specification (using hyphen separator)",
newMinimalKubeRouterConfig("", "8080-8090", "node", nil),
false,
"",
},
{
"Test bad external IP CIDR (not properly formatting ip address)",
newMinimalKubeRouterConfig("", "", "node", []string{"199.10.10"}),
true,
"failed to get parse --service-external-ip-range parameter: '199.10.10'. Error: invalid CIDR address: 199.10.10",
},
{
"Test bad external IP CIDR (not using an ip address)",
newMinimalKubeRouterConfig("", "", "node", []string{"foo"}),
true,
"failed to get parse --service-external-ip-range parameter: 'foo'. Error: invalid CIDR address: foo",
},
{
"Test bad external IP CIDR (using an ip address that is not a CIDR)",
newMinimalKubeRouterConfig("", "", "node", []string{"199.10.10.10"}),
true,
"failed to get parse --service-external-ip-range parameter: '199.10.10.10'. Error: invalid CIDR address: 199.10.10.10",
},
{
"Test bad external IP CIDR (making sure that it processes all items in the list)",
newMinimalKubeRouterConfig("", "", "node", []string{"199.10.10.10/32", "199.10.10.11"}),
true,
"failed to get parse --service-external-ip-range parameter: '199.10.10.11'. Error: invalid CIDR address: 199.10.10.11",
},
{
"Test good external IP CIDR (using single IP with a /32)",
newMinimalKubeRouterConfig("", "", "node", []string{"199.10.10.10/32"}),
false,
"",
},
{
"Test good external IP CIDR (using normal range with /24)",
newMinimalKubeRouterConfig("", "", "node", []string{"199.10.10.10/24"}),
false,
"",
},
}
client := fake.NewSimpleClientset(&v1.NodeList{Items: []v1.Node{*newFakeNode("node", "10.10.10.10")}})
_, podInformer, nsInformer, netpolInformer := newFakeInformersFromClient(client)
for _, test := range testCases {
t.Run(test.name, func(t *testing.T) {
_, err := NewNetworkPolicyController(client, test.config, podInformer, netpolInformer, nsInformer)
if err == nil && test.expectError {
t.Error("This config should have failed, but it was successful instead")
} else if err != nil {
// Unfortunately without doing a ton of extra refactoring work, we can't remove this reference easily
// from the controllers start up. Luckily it's one of the last items to be processed in the controller
// so for now we'll consider that if we hit this error that we essentially didn't hit an error at all
// TODO: refactor NPC to use an injectable interface for ipset operations
if !test.expectError && err.Error() != "Ipset utility not found" {
t.Errorf("This config should have been successful, but it failed instead. Error: %s", err)
} else if test.expectError && err.Error() != test.errorText {
t.Errorf("Expected error: '%s' but instead got: '%s'", test.errorText, err)
}
}
})
}
}

// Ref:
// https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/podgc/gc_controller_test.go
// https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/testutil/test_utils.go
7 changes: 5 additions & 2 deletions pkg/options/options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type KubeRouterConfig struct {
EnablePodEgress bool
EnablePprof bool
ExcludedCidrs []string
ExternalIPCIDRs []string
FullMeshMode bool
GlobalHairpinMode bool
HealthPort uint16
Expand Down Expand Up @@ -78,7 +79,7 @@ func NewKubeRouterConfig() *KubeRouterConfig {
IPTablesSyncPeriod: 5 * time.Minute,
IpvsGracefulPeriod: 30 * time.Second,
IpvsSyncPeriod: 5 * time.Minute,
NodePortRange: "30000:32767",
NodePortRange: "30000-32767",
OverlayType: "subnet",
RoutesSyncPeriod: 5 * time.Minute,
}
Expand Down Expand Up @@ -179,8 +180,10 @@ func (s *KubeRouterConfig) AddFlags(fs *pflag.FlagSet) {
"Enables Service Proxy -- sets up IPVS for Kubernetes Services.")
fs.StringVar(&s.ClusterIPCIDR, "service-cluster-ip-range", s.ClusterIPCIDR,
"CIDR value from which service cluster IPs are assigned. Default: 10.96.0.0/12")
fs.StringSliceVar(&s.ExternalIPCIDRs, "service-external-ip-range", s.ExternalIPCIDRs,
"Specify external IP CIDRs that are used for inter-cluster communication (can be specified multiple times)")
fs.StringVar(&s.NodePortRange, "service-node-port-range", s.NodePortRange,
"NodePort range. Default: 30000-32767")
"NodePort range specified with either a hyphen or colon")
fs.StringVarP(&s.VLevel, "v", "v", "0", "log level for V logs")
fs.BoolVarP(&s.Version, "version", "V", false,
"Print version information.")
Expand Down

0 comments on commit 827ce55

Please sign in to comment.