From 750a7d73eee9034e1bcdb5365ed5210ce99f6302 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Fri, 12 Apr 2019 10:38:15 +0200 Subject: [PATCH 01/13] missing comment --- pkg/controller/mapping/mapping_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/controller/mapping/mapping_controller.go b/pkg/controller/mapping/mapping_controller.go index 5da9878..9829b26 100644 --- a/pkg/controller/mapping/mapping_controller.go +++ b/pkg/controller/mapping/mapping_controller.go @@ -622,6 +622,7 @@ func (r *ReconcileMapping) checkSetupFinalizer(mapping *smartnatv1alpha1.Mapping return false, false, nil } +// GetHeartbeatString returns string printed by heartbeat http request func GetHeartbeatString() string { return fmt.Sprintf("OK %v", time.Now().UTC().Format(time.RFC3339)) } From bba9a61d10d30ba4662a8e2fcad49bf76af35e9b Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Fri, 12 Apr 2019 15:37:34 +0200 Subject: [PATCH 02/13] update dependencies, WIP --- Gopkg.lock | 6 +- Gopkg.toml | 2 +- pkg/controller/mapping/dnat_providers.go | 80 +++++++++++++------- pkg/controller/mapping/mapping_controller.go | 4 +- 4 files changed, 59 insertions(+), 33 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 3e657aa..ae7a23c 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -10,7 +10,7 @@ version = "v0.34.0" [[projects]] - digest = "1:8645fbb3be6573f110455b93bb7c2dd29297a438229e4ae83669f7579f1921e0" + digest = "1:eff11906138aea0f1d482dc1233fc3e633a453b6d649f2e09a6a39fbdb7e57b6" name = "github.com/DevFactory/go-tools" packages = [ "pkg/extensions/collections", @@ -25,8 +25,8 @@ "pkg/nettools/testhelpers", ] pruneopts = "T" - revision = "507ef6c9f6a3056dae91a307d2a7db8fabfb44c1" - version = "v0.1.1" + revision = "cc9422a3ec153d0f3d533fc9c9c8ce8b85c10a15" + version = "v0.2.0" [[projects]] branch = "master" diff --git a/Gopkg.toml b/Gopkg.toml index 1f7df9c..a0f007c 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -25,7 +25,7 @@ required = [ [[constraint]] name = "github.com/DevFactory/go-tools" - version = "0.1.1" + version = "0.2.0" [[constraint]] name = "github.com/stretchr/testify" diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index baa8b94..f75fb9e 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -27,6 +27,7 @@ import ( const ( chainNamePrefix = "MAP" + ipSetNamePrefix = "DNAT" markMasqComment = "mark for masquerade in " + masqPostroutingChain ) @@ -50,13 +51,16 @@ type DNATProvider interface { // supported configuration. type ThroughServiceDNAT struct { iptables nettools.IPTablesHelper + ipset nettools.IPSetHelper namer Namer } // NewThroughServiceDNATProvider returns new instance of the ThroughServiceDNAT -func NewThroughServiceDNATProvider(iptables nettools.IPTablesHelper, namer Namer) DNATProvider { +func NewThroughServiceDNATProvider(iptables nettools.IPTablesHelper, ipset nettools.IPSetHelper, + namer Namer) DNATProvider { return &ThroughServiceDNAT{ iptables: iptables, + ipset: ipset, namer: namer, } } @@ -79,6 +83,7 @@ func (p *ThroughServiceDNAT) SetupDNAT(externalIP net.IP, mapping *v1alpha1.Mapp return err } + //////////////////// // sort all external port numbers by protocol tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) // jumps to chain @@ -88,6 +93,7 @@ func (p *ThroughServiceDNAT) SetupDNAT(externalIP net.IP, mapping *v1alpha1.Mapp if err := p.synchronizeJumpPerProtocol(mapping, udpPorts, externalIP, "udp", preroutingChain, chainName); err != nil { return err } + //////////////////// // setup Masquerade marks if enabled if setupMasquerade { @@ -143,6 +149,44 @@ func (p *ThroughServiceDNAT) DeleteDNAT(externalIP net.IP, mapping *v1alpha1.Map return nil } +func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, externalIP net.IP, fromChain, + toChain string) error { + // ensure ipset for this services exists + setName := p.getIPSetName(mapping) + logrusWithMapping(mapping).Debugf("Ensuring ipset %s exists", setName) + if err := p.ipset.EnsureSetExists(setName, "hash:net,port"); err != nil { + logrusWithMapping(mapping).Errorf("Error creating ipset: %v", err) + return err + } + + // synchronize ipset content + tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) + // ensure jump to toChain exists with match against the ipset +} + +func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, + ports []v1alpha1.MappingPort, chainName string, mapping *v1alpha1.Mapping) error { + list := make([]string, 0, len(ports)) + for _, port := range ports { + list = append(list, fmt.Sprintf("%d", port.Port)) + } + stringList := strings.Join(list, ",") + logrusWithMapping(mapping).Debugf("Setting jump from %s to %s for protocol %s", + preroutingChain, chainName, protocol) + src := make([]string, len(mapping.Spec.AllowedSources)) + copy(src, mapping.Spec.AllowedSources) + sort.Strings(src) + allowedSources := strings.Join(src, ",") + return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ + Table: natTableName, + ChainName: preroutingChain, + Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-p", protocol, + "-m", "multiport", "--dports", stringList, "-s", allowedSources}, + Action: []string{chainName}, + Comment: p.getJumpComment(mapping, protocol), + }) +} + func (p *ThroughServiceDNAT) synchronizeJumpPerProtocol(mapping *v1alpha1.Mapping, ports []v1alpha1.MappingPort, externalIP net.IP, protocol, fromChain, toChain string) error { if len(ports) > 0 { @@ -169,14 +213,15 @@ func (p *ThroughServiceDNAT) synchronizeJumpPerProtocol(mapping *v1alpha1.Mappin func (p *ThroughServiceDNAT) synchronizePerPortRules(mapping *v1alpha1.Mapping, svc *v1.Service, chainName string) error { logrusWithMapping(mapping).Debugf("Starting to synchronize per port rules in chain %s", chainName) - // load current rules from the chain - tentatively we will remove all rules from this list + // load current rules from the chain current, err := p.iptables.LoadRules(natTableName, chainName) if err != nil { logrusWithMapping(mapping).Errorf("Error getting iptables rules from the operating system for"+ " chain %s. Error: %v", chainName, err) return err } - current = p.skipMarkMasq(current) + // exclude the mark for masquerade rule, so it is always kept + current = p.excludeMarkMasqRule(current) new := p.rulesFromMappedPorts(mapping, svc.Spec.ClusterIP, chainName) // now find set difference "current\new" and "new\current" @@ -220,7 +265,7 @@ func (p *ThroughServiceDNAT) synchronizePerPortRules(mapping *v1alpha1.Mapping, return nil } -func (*ThroughServiceDNAT) skipMarkMasq(rules []*nettools.IPTablesRuleArgs) []*nettools.IPTablesRuleArgs { +func (*ThroughServiceDNAT) excludeMarkMasqRule(rules []*nettools.IPTablesRuleArgs) []*nettools.IPTablesRuleArgs { res := make([]*nettools.IPTablesRuleArgs, 0, len(rules)) for _, rule := range rules { if rule.Comment == markMasqComment { @@ -248,29 +293,6 @@ func (p *ThroughServiceDNAT) rulesFromMappedPorts(mapping *v1alpha1.Mapping, svc return new } -func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, - ports []v1alpha1.MappingPort, chainName string, mapping *v1alpha1.Mapping) error { - list := make([]string, 0, len(ports)) - for _, port := range ports { - list = append(list, fmt.Sprintf("%d", port.Port)) - } - stringList := strings.Join(list, ",") - logrusWithMapping(mapping).Debugf("Setting jump from %s to %s for protocol %s", - preroutingChain, chainName, protocol) - src := make([]string, len(mapping.Spec.AllowedSources)) - copy(src, mapping.Spec.AllowedSources) - sort.Strings(src) - allowedSources := strings.Join(src, ",") - return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ - Table: natTableName, - ChainName: preroutingChain, - Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-p", protocol, - "-m", "multiport", "--dports", stringList, "-s", allowedSources}, - Action: []string{chainName}, - Comment: p.getJumpComment(mapping, protocol), - }) -} - func (p *ThroughServiceDNAT) deleteJumpPerProtocol(protocol string, chainName string, mapping *v1alpha1.Mapping) error { comment := p.getJumpComment(mapping, protocol) @@ -301,3 +323,7 @@ func (p *ThroughServiceDNAT) getPerProtocolPorts(mapping *v1alpha1.Mapping) ( func (p *ThroughServiceDNAT) getChainName(mapping *v1alpha1.Mapping) string { return fmt.Sprintf("%s-%s", chainNamePrefix, p.namer.Name(mapping.ObjectMeta)) } + +func (p *ThroughServiceDNAT) getIPSetName(mapping *v1alpha1.Mapping) string { + return fmt.Sprintf("%s-%s", ipSetNamePrefix, p.namer.Name(mapping.ObjectMeta)) +} diff --git a/pkg/controller/mapping/mapping_controller.go b/pkg/controller/mapping/mapping_controller.go index 9829b26..b1bbc80 100644 --- a/pkg/controller/mapping/mapping_controller.go +++ b/pkg/controller/mapping/mapping_controller.go @@ -67,7 +67,8 @@ func Add(mgr manager.Manager, heartbeatChan chan<- string) error { } iptablesHelper := nettools.NewExecIPTablesHelper(executor, time.Duration(cfg.IPTablesTimeoutSec)*time.Second) namer := NewNamer() - dnatProvider := NewThroughServiceDNATProvider(iptablesHelper, namer) + ipsetHelper := nettools.NewExecIPSetHelper(executor) + dnatProvider := NewThroughServiceDNATProvider(iptablesHelper, ipsetHelper, namer) ipTablesHelper, err := NewIPTablesHelper(dnatProvider, iptablesHelper, namer, interfaceProvider, cfg.SetupMasquerade, cfg.SetupSNAT) if err != nil { @@ -77,7 +78,6 @@ func Add(mgr manager.Manager, heartbeatChan chan<- string) error { ipRouteHelper := NewIPRouteSmartNatHelper(executor, routeIoOp, interfaceProvider, refreshInterval, cfg.GwAddressOffset) scrubber := NewScrubber(interfaceProvider) conntrackHelper := nettools.NewExecConntrackHelper(executor) - ipsetHelper := nettools.NewExecIPSetHelper(executor) syncer := NewSyncer(namer, interfaceProvider, ipRouteHelper, conntrackHelper, ipTablesHelper, ipsetHelper, cfg.SetupSNAT, cfg.SetupMasquerade) return add(mgr, newReconciler(mgr, cfg, interfaceProvider, ipRouteHelper, syncer, scrubber, heartbeatChan)) From 02a3eabaef209e9887d6745f7547742b4c6f3244 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 12:21:22 +0200 Subject: [PATCH 03/13] syncing ipset netports --- pkg/controller/mapping/dnat_providers.go | 32 ++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index f75fb9e..283654c 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -161,7 +161,39 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, // synchronize ipset content tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) + netPorts := []nettools.NetPort{} + for _, allowedSrc := range mapping.Spec.AllowedSources { + _, srcNet, err := net.ParseCIDR(allowedSrc) + if err != nil { + return fmt.Errorf("Error parsing allowed source %s: %v", allowedSrc, err) + } + endpoints := []struct { + ports []v1alpha1.MappingPort + proto nettools.Protocol + }{ + {tcpPorts, nettools.TCP}, + {udpPorts, nettools.UDP}, + } + for _, ep := range endpoints { + for _, port := range ep.ports { + netPort := nettools.NetPort{ + Net: *srcNet, + Port: uint16(port.Port), + Protocol: ep.proto, + } + netPorts = append(netPorts, netPort) + } + } + } + p.ipset.EnsureSetHasOnlyNetPort(setName, netPorts) // ensure jump to toChain exists with match against the ipset + return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ + Table: natTableName, + ChainName: fromChain, + Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-m", "set", "--match-set", setName, "src,dst"}, + Action: []string{toChain}, + Comment: p.getJumpComment(mapping, "all"), + }) } func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, From f132061305b31bcbdaf991b5bd4c4b4b33e468b4 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 12:32:22 +0200 Subject: [PATCH 04/13] first try --- pkg/controller/mapping/dnat_providers.go | 109 ++++++++++++----------- 1 file changed, 55 insertions(+), 54 deletions(-) diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index 283654c..9292ece 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -16,7 +16,6 @@ package mapping import ( "fmt" "net" - "sort" "strings" "github.com/DevFactory/go-tools/pkg/extensions/collections" @@ -83,16 +82,18 @@ func (p *ThroughServiceDNAT) SetupDNAT(externalIP net.IP, mapping *v1alpha1.Mapp return err } + //////////////////// + p.synchronizeJumpAndIPSet(mapping, externalIP, preroutingChain, chainName) //////////////////// // sort all external port numbers by protocol - tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) - // jumps to chain - if err := p.synchronizeJumpPerProtocol(mapping, tcpPorts, externalIP, "tcp", preroutingChain, chainName); err != nil { - return err - } - if err := p.synchronizeJumpPerProtocol(mapping, udpPorts, externalIP, "udp", preroutingChain, chainName); err != nil { - return err - } + // tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) + // // jumps to chain + // if err := p.synchronizeJumpPerProtocol(mapping, tcpPorts, externalIP, "tcp", preroutingChain, chainName); err != nil { + // return err + // } + // if err := p.synchronizeJumpPerProtocol(mapping, udpPorts, externalIP, "udp", preroutingChain, chainName); err != nil { + // return err + // } //////////////////// // setup Masquerade marks if enabled @@ -190,54 +191,54 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ Table: natTableName, ChainName: fromChain, - Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-m", "set", "--match-set", setName, "src,dst"}, - Action: []string{toChain}, - Comment: p.getJumpComment(mapping, "all"), + Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-m", "set", "--match-set", setName, "src,dst"}, + Action: []string{toChain}, + Comment: p.getJumpComment(mapping, "all"), }) } -func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, - ports []v1alpha1.MappingPort, chainName string, mapping *v1alpha1.Mapping) error { - list := make([]string, 0, len(ports)) - for _, port := range ports { - list = append(list, fmt.Sprintf("%d", port.Port)) - } - stringList := strings.Join(list, ",") - logrusWithMapping(mapping).Debugf("Setting jump from %s to %s for protocol %s", - preroutingChain, chainName, protocol) - src := make([]string, len(mapping.Spec.AllowedSources)) - copy(src, mapping.Spec.AllowedSources) - sort.Strings(src) - allowedSources := strings.Join(src, ",") - return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ - Table: natTableName, - ChainName: preroutingChain, - Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-p", protocol, - "-m", "multiport", "--dports", stringList, "-s", allowedSources}, - Action: []string{chainName}, - Comment: p.getJumpComment(mapping, protocol), - }) -} - -func (p *ThroughServiceDNAT) synchronizeJumpPerProtocol(mapping *v1alpha1.Mapping, ports []v1alpha1.MappingPort, - externalIP net.IP, protocol, fromChain, toChain string) error { - if len(ports) > 0 { - logrusWithMapping(mapping).Debugf("Setting up jump to chain %s from %s for protocol %s", toChain, - fromChain, protocol) - if err := p.setupJumpPerProtocol(externalIP, protocol, ports, toChain, mapping); err != nil { - logrusWithMapping(mapping).Errorf("Error when setting up jump to chain %s from %s "+ - "and protocol protocol: %v; %v", toChain, fromChain, protocol, err) - return err - } - } else { - if err := p.deleteJumpPerProtocol(protocol, fromChain, mapping); err != nil { - logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ - "protocol %s: %v", toChain, fromChain, protocol, err) - return err - } - } - return nil -} +// func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, +// ports []v1alpha1.MappingPort, chainName string, mapping *v1alpha1.Mapping) error { +// list := make([]string, 0, len(ports)) +// for _, port := range ports { +// list = append(list, fmt.Sprintf("%d", port.Port)) +// } +// stringList := strings.Join(list, ",") +// logrusWithMapping(mapping).Debugf("Setting jump from %s to %s for protocol %s", +// preroutingChain, chainName, protocol) +// src := make([]string, len(mapping.Spec.AllowedSources)) +// copy(src, mapping.Spec.AllowedSources) +// sort.Strings(src) +// allowedSources := strings.Join(src, ",") +// return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ +// Table: natTableName, +// ChainName: preroutingChain, +// Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-p", protocol, +// "-m", "multiport", "--dports", stringList, "-s", allowedSources}, +// Action: []string{chainName}, +// Comment: p.getJumpComment(mapping, protocol), +// }) +// } + +// func (p *ThroughServiceDNAT) synchronizeJumpPerProtocol(mapping *v1alpha1.Mapping, ports []v1alpha1.MappingPort, +// externalIP net.IP, protocol, fromChain, toChain string) error { +// if len(ports) > 0 { +// logrusWithMapping(mapping).Debugf("Setting up jump to chain %s from %s for protocol %s", toChain, +// fromChain, protocol) +// if err := p.setupJumpPerProtocol(externalIP, protocol, ports, toChain, mapping); err != nil { +// logrusWithMapping(mapping).Errorf("Error when setting up jump to chain %s from %s "+ +// "and protocol protocol: %v; %v", toChain, fromChain, protocol, err) +// return err +// } +// } else { +// if err := p.deleteJumpPerProtocol(protocol, fromChain, mapping); err != nil { +// logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ +// "protocol %s: %v", toChain, fromChain, protocol, err) +// return err +// } +// } +// return nil +// } // synchronizePerPortRules needs to get current rules in the specified chain and the new set // of MappedPorts to configure. Than, it has to leave without changes what is unchanged and From cfd6cb04c3a17916e73ea669844383bf0bc5e3e2 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 17:30:18 +0200 Subject: [PATCH 05/13] deletion implementation --- pkg/controller/mapping/dnat_providers.go | 60 +++++++++++++++++------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index 9292ece..bfcbbb3 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -131,17 +131,35 @@ func (p *ThroughServiceDNAT) DeleteDNAT(externalIP net.IP, mapping *v1alpha1.Map logrusWithMapping(mapping).Errorf("Error when flushing chain %s: %v", chainName, err) return err } - logrusWithMapping(mapping).Debugf("Removing jump(s) from chain %s to %s", preroutingChain, chainName) - if err := p.deleteJumpPerProtocol("tcp", preroutingChain, mapping); err != nil { - logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ - "protocol TCP: %v", chainName, preroutingChain, err) + ////////////// + // remove ipset + setName := p.getIPSetName(mapping) + if err := p.ipset.DeleteSet(setName); err != nil { + logrusWithMapping(mapping).Errorf("Error removing ipset %s: %v", setName, err) return err } - if err := p.deleteJumpPerProtocol("udp", preroutingChain, mapping); err != nil { - logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ - "protocol UDP: %v", chainName, preroutingChain, err) + // remove jump rule + comment := p.getJumpComment(mapping, "all") + logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", + chainName, comment) + if err := p.iptables.DeleteByComment(natTableName, preroutingChain, comment); err != nil { + logrusWithMapping(mapping).Errorf("Error deleting iptables jump from chain %s to %s: %v", + preroutingChain, chainName, err) return err } + ///////////////// + // logrusWithMapping(mapping).Debugf("Removing jump(s) from chain %s to %s", preroutingChain, chainName) + // if err := p.deleteJumpPerProtocol("tcp", preroutingChain, mapping); err != nil { + // logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ + // "protocol TCP: %v", chainName, preroutingChain, err) + // return err + // } + // if err := p.deleteJumpPerProtocol("udp", preroutingChain, mapping); err != nil { + // logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ + // "protocol UDP: %v", chainName, preroutingChain, err) + // return err + // } + ///////////////////// logrusWithMapping(mapping).Debugf("Deleting chain %s", chainName) if err := p.iptables.DeleteChain(natTableName, chainName); err != nil { logrusWithMapping(mapping).Errorf("Error when deleting chain %s: %v", chainName, err) @@ -161,12 +179,14 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, } // synchronize ipset content + logrusWithMapping(mapping).Debugf("Synchronizing entries in ipset %s", setName) tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) netPorts := []nettools.NetPort{} for _, allowedSrc := range mapping.Spec.AllowedSources { _, srcNet, err := net.ParseCIDR(allowedSrc) if err != nil { - return fmt.Errorf("Error parsing allowed source %s: %v", allowedSrc, err) + logrusWithMapping(mapping).Errorf("Error parsing allowed source %s: %v", allowedSrc, err) + return err } endpoints := []struct { ports []v1alpha1.MappingPort @@ -186,8 +206,14 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, } } } - p.ipset.EnsureSetHasOnlyNetPort(setName, netPorts) + if err := p.ipset.EnsureSetHasOnlyNetPort(setName, netPorts); err != nil { + logrusWithMapping(mapping).Errorf("Error synchronizing entries in ipset %s: %v", setName, err) + return err + } + // ensure jump to toChain exists with match against the ipset + logrusWithMapping(mapping).Debugf("Adding jump from chain %s to chain %s using ipset %s", + fromChain, toChain, setName) return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ Table: natTableName, ChainName: fromChain, @@ -326,14 +352,14 @@ func (p *ThroughServiceDNAT) rulesFromMappedPorts(mapping *v1alpha1.Mapping, svc return new } -func (p *ThroughServiceDNAT) deleteJumpPerProtocol(protocol string, chainName string, - mapping *v1alpha1.Mapping) error { - comment := p.getJumpComment(mapping, protocol) - logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", - chainName, comment) - return p.iptables.DeleteByComment(natTableName, chainName, comment) -} - +// func (p *ThroughServiceDNAT) deleteJumpPerProtocol(protocol string, chainName string, +// mapping *v1alpha1.Mapping) error { +// comment := p.getJumpComment(mapping, protocol) +// logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", +// chainName, comment) +// return p.iptables.DeleteByComment(natTableName, chainName, comment) +// } +// func (p *ThroughServiceDNAT) getJumpComment(mapping *v1alpha1.Mapping, protocol string) string { return fmt.Sprintf("for mapping %s/%s [%s]", mapping.Namespace, mapping.Name, protocol) } From 95589da8d653cff39357cfb6007c2c5ea6295758 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 17:59:41 +0200 Subject: [PATCH 06/13] update go-tools to v0.2.1 --- Gopkg.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index ae7a23c..79626e4 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -10,7 +10,7 @@ version = "v0.34.0" [[projects]] - digest = "1:eff11906138aea0f1d482dc1233fc3e633a453b6d649f2e09a6a39fbdb7e57b6" + digest = "1:7debe02d52b10a23c10f1814e1b2a854270ff3c0c54740365a1093a113fde14e" name = "github.com/DevFactory/go-tools" packages = [ "pkg/extensions/collections", @@ -25,8 +25,8 @@ "pkg/nettools/testhelpers", ] pruneopts = "T" - revision = "cc9422a3ec153d0f3d533fc9c9c8ce8b85c10a15" - version = "v0.2.0" + revision = "d423bb44e94a7aa02739294dcfb2866f4d2c66f6" + version = "v0.2.1" [[projects]] branch = "master" From 69656c44a9d3c9b22a2a0bee4a8d03c494c8dde4 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 19:00:04 +0200 Subject: [PATCH 07/13] unit tests adjusted --- pkg/controller/mapping/dnat_providers_test.go | 67 +++++++++++++------ 1 file changed, 45 insertions(+), 22 deletions(-) diff --git a/pkg/controller/mapping/dnat_providers_test.go b/pkg/controller/mapping/dnat_providers_test.go index 77b9731..3fc67c1 100644 --- a/pkg/controller/mapping/dnat_providers_test.go +++ b/pkg/controller/mapping/dnat_providers_test.go @@ -47,11 +47,12 @@ type testCase struct { externalIP net.IP dport, servicePort int32 allowed string + allowedNet *net.IPNet serviceIP string meta v1meta.ObjectMeta mapping *v1alpha1.Mapping svc *v1.Service - customChain string + customChain, ipset string } func getTestCase() testCase { @@ -61,6 +62,7 @@ func getTestCase() testCase { Namespace: "test", } allowed := "0.0.0.0/0" + _, allowedNet, _ := net.ParseCIDR(allowed) serviceIP := "172.16.1.1" return testCase{ extIP: extIP, @@ -68,6 +70,7 @@ func getTestCase() testCase { dport: 80, servicePort: 8080, allowed: allowed, + allowedNet: allowedNet, serviceIP: serviceIP, meta: meta, mapping: &v1alpha1.Mapping{ @@ -85,6 +88,7 @@ func getTestCase() testCase { }, }, customChain: "MAP-MDICIJE6TVWKE333NKHBEORO", + ipset: "DNAT-MDICIJE6TVWKE333NKHBEORO", } } @@ -102,7 +106,7 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { tests := []struct { name string ports []v1alpha1.MappingPort - init func(ipt *ntmocks.IPTablesHelper) + init func(ipt *ntmocks.IPTablesHelper, ips *ntmocks.IPSetHelper) }{ { name: "tcp only, no existing rules", @@ -113,16 +117,23 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { ServicePort: c.servicePort, }, }, - init: func(ipt *ntmocks.IPTablesHelper) { + init: func(ipt *ntmocks.IPTablesHelper, ips *ntmocks.IPSetHelper) { ipt. + On("EnsureChainExists", "nat", c.customChain).Return(nil). On("EnsureExistsOnlyAppend", - getIptablesProtoJumpRule(c.extIP, protoTcp, c.allowed, c.customChain, c.dport, c.meta)).Return(nil). - On("DeleteByComment", natTableName, preroutingChain, fmt.Sprintf("for mapping %s/%s [%s]", - c.mapping.Namespace, c.mapping.Name, protoUdp)).Return(nil). + getIptablesProtoJumpRule(c.extIP, "all", c.customChain, c.ipset, c.meta)).Return(nil). On("EnsureExistsInsert", iptablesMarkRule).Return(nil). On("LoadRules", natTableName, c.customChain).Return([]*nt.IPTablesRuleArgs{}, nil). On("EnsureExistsAppend", getIptablesDNATRule(c.serviceIP, protoTcp, c.customChain, c.dport, c.servicePort, c.meta)).Return(nil) + ips.On("EnsureSetExists", c.ipset, "hash:net,port").Return(nil). + On("EnsureSetHasOnlyNetPort", c.ipset, []nettools.NetPort{ + nettools.NetPort{ + Net: *c.allowedNet, + Port: uint16(c.dport), + Protocol: nettools.TCP, + }, + }).Return(nil) }, }, { @@ -139,12 +150,11 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { ServicePort: c.servicePort, }, }, - init: func(ipt *ntmocks.IPTablesHelper) { + init: func(ipt *ntmocks.IPTablesHelper, ips *ntmocks.IPSetHelper) { ipt. + On("EnsureChainExists", "nat", c.customChain).Return(nil). On("EnsureExistsOnlyAppend", - getIptablesProtoJumpRule(c.extIP, protoTcp, c.allowed, c.customChain, c.dport, c.meta)).Return(nil). - On("EnsureExistsOnlyAppend", - getIptablesProtoJumpRule(c.extIP, protoUdp, c.allowed, c.customChain, c.dport, c.meta)).Return(nil). + getIptablesProtoJumpRule(c.extIP, "all", c.customChain, c.ipset, c.meta)).Return(nil). On("EnsureExistsInsert", iptablesMarkRule).Return(nil). On("LoadRules", natTableName, c.customChain).Return( []*nt.IPTablesRuleArgs{&iptablesBogusExistingRule}, nil). @@ -153,6 +163,19 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { getIptablesDNATRule(c.serviceIP, protoTcp, c.customChain, c.dport, c.servicePort, c.meta)).Return(nil). On("EnsureExistsAppend", getIptablesDNATRule(c.serviceIP, protoUdp, c.customChain, c.dport, c.servicePort, c.meta)).Return(nil) + ips.On("EnsureSetExists", c.ipset, "hash:net,port").Return(nil). + On("EnsureSetHasOnlyNetPort", c.ipset, []nettools.NetPort{ + nettools.NetPort{ + Net: *c.allowedNet, + Port: uint16(c.dport), + Protocol: nettools.TCP, + }, + nettools.NetPort{ + Net: *c.allowedNet, + Port: uint16(c.dport), + Protocol: nettools.UDP, + }, + }).Return(nil) }, }, } @@ -160,13 +183,14 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { t.Run(tt.name, func(t *testing.T) { c.mapping.Spec.Ports = tt.ports iptables := &ntmocks.IPTablesHelper{} - iptables.On("EnsureChainExists", "nat", c.customChain).Return(nil) - tt.init(iptables) - dnat := mapping.NewThroughServiceDNATProvider(iptables, mapping.NewNamer()) + ipset := &ntmocks.IPSetHelper{} + tt.init(iptables, ipset) + dnat := mapping.NewThroughServiceDNATProvider(iptables, ipset, mapping.NewNamer()) // call SetupDNAT err := dnat.SetupDNAT(c.externalIP, c.mapping, c.svc, nil, true) assert.Nil(t, err) iptables.AssertExpectations(t) + ipset.AssertExpectations(t) }) } } @@ -211,11 +235,11 @@ func Test_throughServiceDNATProvider_DeleteDNAT(t *testing.T) { iptables. On("FlushChain", "nat", c.customChain).Return(nil). On("DeleteByComment", natTableName, preroutingChain, fmt.Sprintf("for mapping %s/%s [%s]", - c.mapping.Namespace, c.mapping.Name, protoTcp)).Return(nil). - On("DeleteByComment", natTableName, preroutingChain, fmt.Sprintf("for mapping %s/%s [%s]", - c.mapping.Namespace, c.mapping.Name, protoUdp)).Return(nil). + c.mapping.Namespace, c.mapping.Name, "all")).Return(nil). On("DeleteChain", natTableName, c.customChain).Return(nil) - dnat := mapping.NewThroughServiceDNATProvider(iptables, mapping.NewNamer()) + ipset := &ntmocks.IPSetHelper{} + ipset.On("DeleteSet", c.ipset).Return(nil) + dnat := mapping.NewThroughServiceDNATProvider(iptables, ipset, mapping.NewNamer()) // call DeleteDNAT err := dnat.DeleteDNAT(c.externalIP, c.mapping) assert.Nil(t, err) @@ -224,15 +248,14 @@ func Test_throughServiceDNATProvider_DeleteDNAT(t *testing.T) { } } -func getIptablesProtoJumpRule(extIP, proto, allowed, customChain string, dport int32, +func getIptablesProtoJumpRule(extIP, proto, customChain, ipset string, meta v1meta.ObjectMeta) nettools.IPTablesRuleArgs { return nettools.IPTablesRuleArgs{ Table: natTableName, ChainName: preroutingChain, - Selector: []string{"-d", fmt.Sprintf("%s/32", extIP), "-p", proto, - "-m", "multiport", "--dports", fmt.Sprintf("%d", dport), "-s", allowed}, - Action: []string{customChain}, - Comment: fmt.Sprintf("for mapping %s/%s [%s]", meta.Namespace, meta.Name, proto), + Selector: []string{"-d", fmt.Sprintf("%s/32", extIP), "-m", "set", "--match-set", ipset, "src,dst"}, + Action: []string{customChain}, + Comment: fmt.Sprintf("for mapping %s/%s [%s]", meta.Namespace, meta.Name, proto), } } From 6cf1580768631a3ca5c613b1cd3edb275729acfb Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 20:12:56 +0200 Subject: [PATCH 08/13] update go-tools to v0.2.2 --- Gopkg.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gopkg.lock b/Gopkg.lock index 79626e4..28b405a 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -10,7 +10,7 @@ version = "v0.34.0" [[projects]] - digest = "1:7debe02d52b10a23c10f1814e1b2a854270ff3c0c54740365a1093a113fde14e" + digest = "1:5fab90311f251dd2813cb016ba64841c953fb7070bc028eedc1e7852d1fa7347" name = "github.com/DevFactory/go-tools" packages = [ "pkg/extensions/collections", @@ -25,8 +25,8 @@ "pkg/nettools/testhelpers", ] pruneopts = "T" - revision = "d423bb44e94a7aa02739294dcfb2866f4d2c66f6" - version = "v0.2.1" + revision = "5438de9ccc074183d42dd5cf7cc9e9710aca3b1e" + version = "v0.2.2" [[projects]] branch = "master" From 79251362d6536eafe9d36aa5a9facb87da7f61e4 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 20:16:01 +0200 Subject: [PATCH 09/13] remove unused code --- pkg/controller/mapping/dnat_providers.go | 84 ++---------------------- 1 file changed, 7 insertions(+), 77 deletions(-) diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index bfcbbb3..0ac20ad 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -82,19 +82,10 @@ func (p *ThroughServiceDNAT) SetupDNAT(externalIP net.IP, mapping *v1alpha1.Mapp return err } - //////////////////// + // create ipset that contains all "allowed_source_ip:proto,port" pairs + // and add iptables rules that checks against that set and jumps to + // per-service chain with per-port translation rules p.synchronizeJumpAndIPSet(mapping, externalIP, preroutingChain, chainName) - //////////////////// - // sort all external port numbers by protocol - // tcpPorts, udpPorts := p.getPerProtocolPorts(mapping) - // // jumps to chain - // if err := p.synchronizeJumpPerProtocol(mapping, tcpPorts, externalIP, "tcp", preroutingChain, chainName); err != nil { - // return err - // } - // if err := p.synchronizeJumpPerProtocol(mapping, udpPorts, externalIP, "udp", preroutingChain, chainName); err != nil { - // return err - // } - //////////////////// // setup Masquerade marks if enabled if setupMasquerade { @@ -131,13 +122,14 @@ func (p *ThroughServiceDNAT) DeleteDNAT(externalIP net.IP, mapping *v1alpha1.Map logrusWithMapping(mapping).Errorf("Error when flushing chain %s: %v", chainName, err) return err } - ////////////// + // remove ipset setName := p.getIPSetName(mapping) if err := p.ipset.DeleteSet(setName); err != nil { logrusWithMapping(mapping).Errorf("Error removing ipset %s: %v", setName, err) return err } + // remove jump rule comment := p.getJumpComment(mapping, "all") logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", @@ -147,19 +139,8 @@ func (p *ThroughServiceDNAT) DeleteDNAT(externalIP net.IP, mapping *v1alpha1.Map preroutingChain, chainName, err) return err } - ///////////////// - // logrusWithMapping(mapping).Debugf("Removing jump(s) from chain %s to %s", preroutingChain, chainName) - // if err := p.deleteJumpPerProtocol("tcp", preroutingChain, mapping); err != nil { - // logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ - // "protocol TCP: %v", chainName, preroutingChain, err) - // return err - // } - // if err := p.deleteJumpPerProtocol("udp", preroutingChain, mapping); err != nil { - // logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ - // "protocol UDP: %v", chainName, preroutingChain, err) - // return err - // } - ///////////////////// + + // delete custom chain logrusWithMapping(mapping).Debugf("Deleting chain %s", chainName) if err := p.iptables.DeleteChain(natTableName, chainName); err != nil { logrusWithMapping(mapping).Errorf("Error when deleting chain %s: %v", chainName, err) @@ -223,49 +204,6 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, }) } -// func (p *ThroughServiceDNAT) setupJumpPerProtocol(externalIP net.IP, protocol string, -// ports []v1alpha1.MappingPort, chainName string, mapping *v1alpha1.Mapping) error { -// list := make([]string, 0, len(ports)) -// for _, port := range ports { -// list = append(list, fmt.Sprintf("%d", port.Port)) -// } -// stringList := strings.Join(list, ",") -// logrusWithMapping(mapping).Debugf("Setting jump from %s to %s for protocol %s", -// preroutingChain, chainName, protocol) -// src := make([]string, len(mapping.Spec.AllowedSources)) -// copy(src, mapping.Spec.AllowedSources) -// sort.Strings(src) -// allowedSources := strings.Join(src, ",") -// return p.iptables.EnsureExistsOnlyAppend(nettools.IPTablesRuleArgs{ -// Table: natTableName, -// ChainName: preroutingChain, -// Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-p", protocol, -// "-m", "multiport", "--dports", stringList, "-s", allowedSources}, -// Action: []string{chainName}, -// Comment: p.getJumpComment(mapping, protocol), -// }) -// } - -// func (p *ThroughServiceDNAT) synchronizeJumpPerProtocol(mapping *v1alpha1.Mapping, ports []v1alpha1.MappingPort, -// externalIP net.IP, protocol, fromChain, toChain string) error { -// if len(ports) > 0 { -// logrusWithMapping(mapping).Debugf("Setting up jump to chain %s from %s for protocol %s", toChain, -// fromChain, protocol) -// if err := p.setupJumpPerProtocol(externalIP, protocol, ports, toChain, mapping); err != nil { -// logrusWithMapping(mapping).Errorf("Error when setting up jump to chain %s from %s "+ -// "and protocol protocol: %v; %v", toChain, fromChain, protocol, err) -// return err -// } -// } else { -// if err := p.deleteJumpPerProtocol(protocol, fromChain, mapping); err != nil { -// logrusWithMapping(mapping).Errorf("Error when deleting jump to chain %s from %s for "+ -// "protocol %s: %v", toChain, fromChain, protocol, err) -// return err -// } -// } -// return nil -// } - // synchronizePerPortRules needs to get current rules in the specified chain and the new set // of MappedPorts to configure. Than, it has to leave without changes what is unchanged and // remove and add objects as needed @@ -352,14 +290,6 @@ func (p *ThroughServiceDNAT) rulesFromMappedPorts(mapping *v1alpha1.Mapping, svc return new } -// func (p *ThroughServiceDNAT) deleteJumpPerProtocol(protocol string, chainName string, -// mapping *v1alpha1.Mapping) error { -// comment := p.getJumpComment(mapping, protocol) -// logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", -// chainName, comment) -// return p.iptables.DeleteByComment(natTableName, chainName, comment) -// } -// func (p *ThroughServiceDNAT) getJumpComment(mapping *v1alpha1.Mapping, protocol string) string { return fmt.Sprintf("for mapping %s/%s [%s]", mapping.Namespace, mapping.Name, protocol) } From b08825e0182000b4aa213c4de6d0383e18af26e0 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 20:21:45 +0200 Subject: [PATCH 10/13] remove unnecessary parameter --- pkg/controller/mapping/dnat_providers.go | 8 ++++---- pkg/controller/mapping/dnat_providers_test.go | 12 ++++++------ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/controller/mapping/dnat_providers.go b/pkg/controller/mapping/dnat_providers.go index 0ac20ad..569ba0c 100644 --- a/pkg/controller/mapping/dnat_providers.go +++ b/pkg/controller/mapping/dnat_providers.go @@ -131,7 +131,7 @@ func (p *ThroughServiceDNAT) DeleteDNAT(externalIP net.IP, mapping *v1alpha1.Map } // remove jump rule - comment := p.getJumpComment(mapping, "all") + comment := p.getJumpComment(mapping) logrusWithMapping(mapping).Debugf("Deleting jump in chain %s with comment: %s", chainName, comment) if err := p.iptables.DeleteByComment(natTableName, preroutingChain, comment); err != nil { @@ -200,7 +200,7 @@ func (p *ThroughServiceDNAT) synchronizeJumpAndIPSet(mapping *v1alpha1.Mapping, ChainName: fromChain, Selector: []string{"-d", fmt.Sprintf("%s/32", externalIP.String()), "-m", "set", "--match-set", setName, "src,dst"}, Action: []string{toChain}, - Comment: p.getJumpComment(mapping, "all"), + Comment: p.getJumpComment(mapping), }) } @@ -290,8 +290,8 @@ func (p *ThroughServiceDNAT) rulesFromMappedPorts(mapping *v1alpha1.Mapping, svc return new } -func (p *ThroughServiceDNAT) getJumpComment(mapping *v1alpha1.Mapping, protocol string) string { - return fmt.Sprintf("for mapping %s/%s [%s]", mapping.Namespace, mapping.Name, protocol) +func (p *ThroughServiceDNAT) getJumpComment(mapping *v1alpha1.Mapping) string { + return fmt.Sprintf("for mapping %s/%s", mapping.Namespace, mapping.Name) } func (p *ThroughServiceDNAT) getPerProtocolPorts(mapping *v1alpha1.Mapping) ( diff --git a/pkg/controller/mapping/dnat_providers_test.go b/pkg/controller/mapping/dnat_providers_test.go index 3fc67c1..41b439f 100644 --- a/pkg/controller/mapping/dnat_providers_test.go +++ b/pkg/controller/mapping/dnat_providers_test.go @@ -121,7 +121,7 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { ipt. On("EnsureChainExists", "nat", c.customChain).Return(nil). On("EnsureExistsOnlyAppend", - getIptablesProtoJumpRule(c.extIP, "all", c.customChain, c.ipset, c.meta)).Return(nil). + getIptablesProtoJumpRule(c.extIP, c.customChain, c.ipset, c.meta)).Return(nil). On("EnsureExistsInsert", iptablesMarkRule).Return(nil). On("LoadRules", natTableName, c.customChain).Return([]*nt.IPTablesRuleArgs{}, nil). On("EnsureExistsAppend", @@ -154,7 +154,7 @@ func Test_throughServiceDNATProvider_SetupDNAT(t *testing.T) { ipt. On("EnsureChainExists", "nat", c.customChain).Return(nil). On("EnsureExistsOnlyAppend", - getIptablesProtoJumpRule(c.extIP, "all", c.customChain, c.ipset, c.meta)).Return(nil). + getIptablesProtoJumpRule(c.extIP, c.customChain, c.ipset, c.meta)).Return(nil). On("EnsureExistsInsert", iptablesMarkRule).Return(nil). On("LoadRules", natTableName, c.customChain).Return( []*nt.IPTablesRuleArgs{&iptablesBogusExistingRule}, nil). @@ -234,8 +234,8 @@ func Test_throughServiceDNATProvider_DeleteDNAT(t *testing.T) { iptables := &ntmocks.IPTablesHelper{} iptables. On("FlushChain", "nat", c.customChain).Return(nil). - On("DeleteByComment", natTableName, preroutingChain, fmt.Sprintf("for mapping %s/%s [%s]", - c.mapping.Namespace, c.mapping.Name, "all")).Return(nil). + On("DeleteByComment", natTableName, preroutingChain, fmt.Sprintf("for mapping %s/%s", + c.mapping.Namespace, c.mapping.Name)).Return(nil). On("DeleteChain", natTableName, c.customChain).Return(nil) ipset := &ntmocks.IPSetHelper{} ipset.On("DeleteSet", c.ipset).Return(nil) @@ -248,14 +248,14 @@ func Test_throughServiceDNATProvider_DeleteDNAT(t *testing.T) { } } -func getIptablesProtoJumpRule(extIP, proto, customChain, ipset string, +func getIptablesProtoJumpRule(extIP, customChain, ipset string, meta v1meta.ObjectMeta) nettools.IPTablesRuleArgs { return nettools.IPTablesRuleArgs{ Table: natTableName, ChainName: preroutingChain, Selector: []string{"-d", fmt.Sprintf("%s/32", extIP), "-m", "set", "--match-set", ipset, "src,dst"}, Action: []string{customChain}, - Comment: fmt.Sprintf("for mapping %s/%s [%s]", meta.Namespace, meta.Name, proto), + Comment: fmt.Sprintf("for mapping %s/%s", meta.Namespace, meta.Name), } } From 0d42c12ab67f59e525fe094e45b47e37a5653202 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Mon, 15 Apr 2019 20:49:13 +0200 Subject: [PATCH 11/13] add max ports as config option - WIP --- pkg/config/config.go | 10 +++++++++- pkg/controller/mapping/mapping_controller.go | 2 +- pkg/controller/mapping/scrubber.go | 20 +++++++------------ .../mapping/testhelper_reconciler_deps.go | 2 +- pkg/controller/mapping/testhelpers/config.go | 2 +- 5 files changed, 19 insertions(+), 17 deletions(-) diff --git a/pkg/config/config.go b/pkg/config/config.go index a1a3bdf..442b62b 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -33,6 +33,7 @@ type Config struct { MaxFinalizeWaitMinutes int GwAddressOffset int32 MonPort int + MaxPortsPerMapping int } const ( @@ -79,6 +80,8 @@ const ( snctrlrGwAddressOffset = "SNCTRLR_GW_ADDRESS_OFFSET" // snctrlrMonitorPort configures TCP port for exposing HTTP healthcheck and performance metrics snctrlrMonitorPort = "SNCTRLR_MONITOR_PORT" + // snctrlrMaxPortsPerMapping configures a limit of ports used by a single mapping + snctrlrMaxPortsPerMapping = "SNCTRLR_MAX_PORTS_PER_MAPPING" ) // NewConfig returns new configuration settings for smart-nat-controller @@ -91,7 +94,7 @@ func NewConfig() (*Config, error) { // NewConfigFromArgs creates new config struct from arguments, after running validation func NewConfigFromArgs(interfaceNamePattern string, setupMasquerade, setupSNAT bool, defaultGWInterface string, ipTablesTimeoutSec int, enableDebug bool, interfaceAutorefreshPeriodSec, maxFinalizeWaitMinutes int, - gwAddressOffset int32, monitorPort int) (*Config, error) { + gwAddressOffset int32, monitorPort int, maxPortsPerMapping int) (*Config, error) { cfg := &Config{ DefaultGWInterface: defaultGWInterface, EnableDebug: enableDebug, @@ -103,6 +106,7 @@ func NewConfigFromArgs(interfaceNamePattern string, setupMasquerade, setupSNAT b MaxFinalizeWaitMinutes: maxFinalizeWaitMinutes, GwAddressOffset: gwAddressOffset, MonPort: monitorPort, + MaxPortsPerMapping: maxPortsPerMapping, } err := cfg.validate() return cfg, err @@ -121,6 +125,7 @@ func (cfg *Config) load() error { offset, _ := eos.GetIntFromEnvVarWithDefault(snctrlrGwAddressOffset, 1) cfg.GwAddressOffset = int32(offset) cfg.MonPort, _ = eos.GetIntFromEnvVarWithDefault(snctrlrMonitorPort, 8080) + cfg.MaxPortsPerMapping, _ = eos.GetIntFromEnvVarWithDefault(snctrlrMaxPortsPerMapping, 50) return cfg.validate() } @@ -143,6 +148,9 @@ func (cfg *Config) validate() error { if cfg.MaxFinalizeWaitMinutes <= 0 { return fmt.Errorf("Configuration env var %s can't be <= 0", snctrlrMaxFinalizeWaitMinutes) } + if cfg.MaxPortsPerMapping <= 0 { + return fmt.Errorf("Configuration env var %s can't be <= 0", snctrlrMaxPortsPerMapping) + } if cfg.MonPort <= 0 || cfg.MonPort > 65535 { return fmt.Errorf("Configuration env var %s must be a valid TCP port num (1-65535)", snctrlrMonitorPort) } diff --git a/pkg/controller/mapping/mapping_controller.go b/pkg/controller/mapping/mapping_controller.go index b1bbc80..2730645 100644 --- a/pkg/controller/mapping/mapping_controller.go +++ b/pkg/controller/mapping/mapping_controller.go @@ -76,7 +76,7 @@ func Add(mgr manager.Manager, heartbeatChan chan<- string) error { } routeIoOp := nettools.NewIOSimpleFileOperator() ipRouteHelper := NewIPRouteSmartNatHelper(executor, routeIoOp, interfaceProvider, refreshInterval, cfg.GwAddressOffset) - scrubber := NewScrubber(interfaceProvider) + scrubber := NewScrubber(interfaceProvider, cfg) conntrackHelper := nettools.NewExecConntrackHelper(executor) syncer := NewSyncer(namer, interfaceProvider, ipRouteHelper, conntrackHelper, ipTablesHelper, ipsetHelper, cfg.SetupSNAT, cfg.SetupMasquerade) diff --git a/pkg/controller/mapping/scrubber.go b/pkg/controller/mapping/scrubber.go index 88f0ca2..46cf05c 100644 --- a/pkg/controller/mapping/scrubber.go +++ b/pkg/controller/mapping/scrubber.go @@ -22,14 +22,13 @@ import ( "github.com/DevFactory/go-tools/pkg/nettools" "github.com/DevFactory/smartnat/pkg/apis/smartnat/v1alpha1" smartnatv1alpha1 "github.com/DevFactory/smartnat/pkg/apis/smartnat/v1alpha1" + "github.com/DevFactory/smartnat/pkg/config" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/validation" ) const ( - pfx = "SmartNat validation failed: " - // MaxPortsPerProtocol - iptables multiport extension supports max 15 ports as arguments - MaxPortsPerProtocol = 15 + pfx = "SmartNat validation failed: " defaultInvalidOKStatus = "no" ) @@ -49,10 +48,11 @@ type Scrubber interface { type scrubber struct { provider nettools.InterfaceProvider + config *config.Config } // NewScrubber returns a scrubber for SmartNat objects -func NewScrubber(interfaceProvider nettools.InterfaceProvider) Scrubber { +func NewScrubber(interfaceProvider nettools.InterfaceProvider, cfg *config.Config) Scrubber { return &scrubber{ provider: interfaceProvider, } @@ -219,15 +219,9 @@ func (s *scrubber) validatePorts(mapping *v1alpha1.Mapping) (bool, string) { } usedPorts[key] = true } - if tcpCount > MaxPortsPerProtocol { - msg := fmt.Sprintf("Too many TCP ports configured. Max is %d, found %d", - MaxPortsPerProtocol, tcpCount) - logrusWithMapping(mapping).Info(pfx + msg) - return false, msg - } - if udpCount > MaxPortsPerProtocol { - msg := fmt.Sprintf("Too many UDP ports configured. Max is %d, found %d", - MaxPortsPerProtocol, udpCount) + if tcpCount+udpCount > s.config.MaxPortsPerMapping { + msg := fmt.Sprintf("Too many ports configured. Max is %d, found %d", s.config.MaxPortsPerMapping, + tcpCount+udpCount) logrusWithMapping(mapping).Info(pfx + msg) return false, msg } diff --git a/pkg/controller/mapping/testhelper_reconciler_deps.go b/pkg/controller/mapping/testhelper_reconciler_deps.go index c3421f8..63deb59 100644 --- a/pkg/controller/mapping/testhelper_reconciler_deps.go +++ b/pkg/controller/mapping/testhelper_reconciler_deps.go @@ -67,7 +67,7 @@ func getReconcilerDependencies(t *testing.T) *reconcilerDependencies { ipRouteSmartNatHelperExecutor: execMock, ipRouteSmartNatHelperFileOperator: ioOp, ipRouteSmartNatHelperUpdateChan: routeUpdateChan, - scrubber: NewScrubber(ifaceProvider), + scrubber: NewScrubber(ifaceProvider, cfg), mappingSyncer: &mocks.Syncer{}, heartbeatChan: heartbeatChan, } diff --git a/pkg/controller/mapping/testhelpers/config.go b/pkg/controller/mapping/testhelpers/config.go index bcec7f1..c902ecf 100644 --- a/pkg/controller/mapping/testhelpers/config.go +++ b/pkg/controller/mapping/testhelpers/config.go @@ -20,7 +20,7 @@ import ( ) func GetTestConfig(t *testing.T) *config.Config { - cfg, err := config.NewConfigFromArgs("eth[0-9]+", true, false, "eth0", 10, true, 10, 60, 1, 8080) + cfg, err := config.NewConfigFromArgs("eth[0-9]+", true, false, "eth0", 10, true, 10, 60, 1, 8080, 50) if err != nil { t.Logf("Cannot create configuration object: %v", err) t.Fail() From 2b67b9e737f41be185724e27b446503227a5791f Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Tue, 16 Apr 2019 10:50:49 +0200 Subject: [PATCH 12/13] bugfixes and test fixes --- pkg/controller/mapping/scrubber.go | 1 + pkg/controller/mapping/scrubber_test.go | 40 +++++++++++-------------- 2 files changed, 19 insertions(+), 22 deletions(-) diff --git a/pkg/controller/mapping/scrubber.go b/pkg/controller/mapping/scrubber.go index 46cf05c..e4c517c 100644 --- a/pkg/controller/mapping/scrubber.go +++ b/pkg/controller/mapping/scrubber.go @@ -55,6 +55,7 @@ type scrubber struct { func NewScrubber(interfaceProvider nettools.InterfaceProvider, cfg *config.Config) Scrubber { return &scrubber{ provider: interfaceProvider, + config: cfg, } } diff --git a/pkg/controller/mapping/scrubber_test.go b/pkg/controller/mapping/scrubber_test.go index b6511fb..5b57566 100644 --- a/pkg/controller/mapping/scrubber_test.go +++ b/pkg/controller/mapping/scrubber_test.go @@ -19,12 +19,14 @@ import ( ntmocks "github.com/DevFactory/go-tools/pkg/nettools/mocks" "github.com/DevFactory/smartnat/pkg/apis/smartnat/v1alpha1" "github.com/DevFactory/smartnat/pkg/controller/mapping" + "github.com/DevFactory/smartnat/pkg/controller/mapping/testhelpers" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" v1meta "k8s.io/apimachinery/pkg/apis/meta/v1" ) func Test_scrubber_ScrubMapping(t *testing.T) { + cfg := testhelpers.GetTestConfig(t) // OK uses all features valid1 := &v1alpha1.Mapping{ ObjectMeta: v1meta.ObjectMeta{ @@ -118,19 +120,17 @@ func Test_scrubber_ScrubMapping(t *testing.T) { }, } // wrong - uses to many ports per protocol - wrongTooManyTcpPorts := valid1.DeepCopy() - wrongTooManyUdpPorts := valid1.DeepCopy() - var i int32 - for i = 0; i < mapping.MaxPortsPerProtocol+1; i++ { - wrongTooManyTcpPorts.Spec.Ports = append(wrongTooManyTcpPorts.Spec.Ports, v1alpha1.MappingPort{ - Port: i + 2000, - Protocol: "tcp", - ServicePort: i + 2000, - }) - wrongTooManyUdpPorts.Spec.Ports = append(wrongTooManyUdpPorts.Spec.Ports, v1alpha1.MappingPort{ - Port: i + 2000, - Protocol: "udp", - ServicePort: i + 2000, + wrongTooManyPorts := valid1.DeepCopy() + for i := 0; i < cfg.MaxPortsPerMapping+1; i++ { + num := int32(i + 2000) + proto := "tcp" + if num%2 == 0 { + proto = "udp" + } + wrongTooManyPorts.Spec.Ports = append(wrongTooManyPorts.Spec.Ports, v1alpha1.MappingPort{ + Port: num, + Protocol: proto, + ServicePort: num, }) } @@ -227,13 +227,8 @@ func Test_scrubber_ScrubMapping(t *testing.T) { want: false, }, { - name: "Fails because uses too many TCP ports", - sn: wrongTooManyTcpPorts, - want: false, - }, - { - name: "Fails because uses too many UDP ports", - sn: wrongTooManyUdpPorts, + name: "Fails because uses too many ports", + sn: wrongTooManyPorts, want: false, }, } @@ -244,7 +239,7 @@ func Test_scrubber_ScrubMapping(t *testing.T) { t.Logf("Cannot create ifaceProvider object: %v", err) t.Fail() } - s := mapping.NewScrubber(ifaceProvider) + s := mapping.NewScrubber(ifaceProvider, cfg) valid, _, _, _ := s.ScrubMapping(tt.sn, tt.others) if valid != tt.want { t.Errorf("scrubber.ScrubMapping() got = %v, want %v", valid, tt.want) @@ -260,6 +255,7 @@ func Test_scrubber_ScrubMapping(t *testing.T) { } func Test_scrubber_ValidateEndpoints(t *testing.T) { + cfg := testhelpers.GetTestConfig(t) tests := []struct { name string endpoints *v1.Endpoints @@ -316,7 +312,7 @@ func Test_scrubber_ValidateEndpoints(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { provider, _, _ := ntmocks.NewMockInterfaceProvider(".*", false) - s := mapping.NewScrubber(provider) + s := mapping.NewScrubber(provider, cfg) if err := s.ValidateEndpoints(nil, tt.endpoints); (err != nil) != tt.wantErr { t.Errorf("scrubber.ValidateEndpoints() error = %v, wantErr %v", err, tt.wantErr) } From 440a6d06aa4b7286ab82380d19370bc1e576f2d8 Mon Sep 17 00:00:00 2001 From: Lukasz Piatkowski Date: Tue, 16 Apr 2019 10:53:38 +0200 Subject: [PATCH 13/13] update docs --- doc/ops_guide.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/ops_guide.md b/doc/ops_guide.md index 12b71d5..101ea0d 100644 --- a/doc/ops_guide.md +++ b/doc/ops_guide.md @@ -130,6 +130,7 @@ Advanced options: * "SNCTRLR_IPTABLES_TIMEOUT_SEC" - timeout in seconds for the `iptables` system command that is frequently called by smartnat. Default should be OK, might need to be increased on heavily loaded smartnat instances. * "SNCTRLR_AUTOREFRESH_PERIOD_SEC" - smartnat automatically reloads information about available network interface and assigned IP addresses. This setting controls how frequently (in seconds) should this information be loaded from the operating system. The default is 60 s. * "SNCTRLR_MAX_FINALIZE_WAIT_MINUTES" - sets the absolute finalizer timeout in minutes. The finalizer timeout is a safety mechanism that needs to be triggered if a smartnat instance has configured a Mapping and then the instance had failed and never came back. In that case, when the Mapping is removed, kubernetes doesn't really delete the Mapping until all smartnat nodes that had been configured for it confirm that the configuration was removed. A failed instance will never do that, so we use this timeout to forcibly remove the Mapping after SNCTRLR_AUTOREFRESH_PERIOD_SEC, even if it is indicated as being still configured on some smartnat nodes. +* "SNCTRLR_MAX_PORTS_PER_MAPPING" - limits the number of Ports a single Mapping can configure. The number of exposed ports has influence on traffic forwarding rate and thus might be needed to tune by the administrator. The default is 50. ### 3. Cluster configuration