SDN-1569: Add support for multiple destinations in redirect mode #34

tssurya · 2021-02-05T16:40:50Z

Presently if more than one destination is specified through the
destinations array in the NAD, we only pick the first destination
and ignore the rest. This patch adds support for multiple
destinations.

Also in OCP 3.11 we used to support multiple destination specification
in 3 formats:

1) ["ip-address/mask"]
2) ["port protocol ip-address/mask"]
3) ["port protocol ip-address/mask remote-port"]

in multiple lines. We do the same for feature parity.

Note that the allowedDestinations field in CRD (same as the destinations field in the NAD) is just an array of strings, so right now we only support redirect mode. IN future if http/dns modes are supported, we can add further checks for dns aliases/hostnames that could get provided.

Note to reviewers: Unit tests can be added in future when the unit test module gets merged.

Sample Test done:
NAD:

apiVersion: "k8s.cni.cncf.io/v1" 
kind: NetworkAttachmentDefinition
metadata:
  name: egress-router
spec:
  config: '{
    "cniVersion": "0.4.0",
    "type": "egress-router",
    "name": "egress-router",
    "ip": { 
      "addresses": [ 
        "10.200.16.10/24" 
        ], 
      "destinations": [
        "80 udp 10.100.3.0/26",
        "8080 tcp 203.0.113.26/30 80", 
        "8443 tcp 203.0.113.26/30 443"
      ],
      "gateway": "10.200.16.1" 
      }
    }'

Router:

apiVersion: v1
kind: Pod 
metadata: 
  name: egress-router 
  annotations: 
    k8s.v1.cni.cncf.io/networks: egress-router 
spec: 
  nodeSelector: 
    multiple: destinations
  containers:
    - name: openshift-egress
      image: quay.io/openshifttest/hello-sdn@sha256:d5785550cf77b7932b090fcd1a2625472912fb3189d5973f177a5a2c347a1f95
      securityContext:
        privileged: true

IPTables output:

*nat
:PREROUTING ACCEPT [0:0]
:INPUT ACCEPT [0:0]
:POSTROUTING ACCEPT [0:0]
:OUTPUT ACCEPT [0:0]
-A PREROUTING -i eth0 -p udp -m udp --dport 80 -j DNAT --to-destination 10.100.3.0
-A PREROUTING -i eth0 -p tcp -m tcp --dport 8080 -j DNAT --to-destination 203.0.113.26:80
-A PREROUTING -i eth0 -p tcp -m tcp --dport 8443 -j DNAT --to-destination 203.0.113.26:443
-A POSTROUTING -o net1 -j SNAT --to-source 10.200.16.10
COMMIT
# Completed on Fri Feb  5 16:24:18 2021

Presently if more than one destination is specified through the "destinations" array in the NAD, we only pick the first destination and ignore the rest. This patch adds support for multiple destinations. Also in OCP 3.11 we used to support multiple destination specification in 3 formats: 1)["ip-address/mask"] 2)["port protocol ip-address/mask"] 3)["port protocol ip-address/mask remote-port"] in multiple lines. We do the same for feature parity.

tssurya · 2021-02-05T16:41:23Z

cc @danielmellado @danwinship

danielmellado

Overall LGTM, It'd be cool to have unit tests for this. As the PR for the unit testing support is also on WIP let's create a follow-up patch for that.

openshift-ci-robot · 2021-02-10T12:04:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielmellado, tssurya

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:

~~OWNERS~~ [danielmellado,tssurya]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fedepaol · 2021-02-10T12:39:31Z

pkg/macvlan/macvlan.go

@@ -270,6 +271,79 @@ func macvlanCmdDel(args *skel.CmdArgs) error {
 	return err
 }

+// validatePortRange validates the destination port value provided in the NAD.


As this (and the following) are "util" functions, it's good practice to have them after the function that consumes them (in this case macVlanCmdAdd), so the reader finds the "important" stuff first

Will move this in the next iteration when adding unit tests.

fedepaol · 2021-02-10T12:47:50Z

pkg/macvlan/macvlan.go

+			dest, _, err := net.ParseCIDR(destination[0])
+			if err != nil {
+				logging.Errorf("Unable to parse destination IP address %v: %v", dest, err)
+				return fmt.Errorf("Unable to parse destination IP address %v: %v", dest, err)


you can use errors.Wrapf here

fedepaol · 2021-02-10T12:49:54Z

pkg/macvlan/macvlan.go

+		} else if len(destination) == 3 || len(destination) == 4 {
+			// should be <localport protocol IPaddress/mask> format
+
+			if err := validatePortRange(destination[0]); err != nil {


I wonder if this plus the check on the protocol could be compacted in a single "validateDestionation" function, making this a bit more readable

ideally, I'd prefer to create a new file validation.go in the future where we can move the validation stuff.

fedepaol · 2021-02-10T12:52:13Z

pkg/macvlan/macvlan.go

+				continue
+			}
+
+			ipt.Append("nat", "PREROUTING", "-i", "eth0", "-p", destination[1], "--dport", destination[0], "-j", "DNAT", "--to-destination", dest.String())


if the remoteport is specified, do we still add this rule?

if the remoteport is specified, then we continue to the next item in the loop on L334 and don't add this rule.

fedepaol · 2021-02-10T12:54:09Z

pkg/macvlan/macvlan.go

+				return fmt.Errorf("Unable to parse destination IP address %v: %v", dest.String(), err)
+			}
+
+			if len(destination) == 4 && validatePortRange(destination[3]) == nil {


this nested if is a bit tricky to read, maybe splitting the one above (to the cost of duplicating some code) would make it more straightforward?

yeah, I didn't want to duplicate code...

openshift-bot · 2021-02-10T15:11:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from danielmellado and dcbw February 5, 2021 16:41

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2021

tssurya mentioned this pull request Feb 9, 2021

Add EgressRouter type and CRD openshift/cluster-network-operator#852

Closed

danielmellado approved these changes Feb 10, 2021

View reviewed changes

openshift-ci-robot assigned danielmellado Feb 10, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2021

fedepaol reviewed Feb 10, 2021

View reviewed changes

openshift-merge-robot merged commit 184e680 into openshift:master Feb 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDN-1569: Add support for multiple destinations in redirect mode #34

SDN-1569: Add support for multiple destinations in redirect mode #34

tssurya commented Feb 5, 2021

tssurya commented Feb 5, 2021

danielmellado left a comment

openshift-ci-robot commented Feb 10, 2021

fedepaol Feb 10, 2021

tssurya Feb 15, 2021

fedepaol Feb 10, 2021

fedepaol Feb 10, 2021

tssurya Feb 15, 2021

fedepaol Feb 10, 2021

tssurya Feb 15, 2021

fedepaol Feb 10, 2021

tssurya Feb 15, 2021

openshift-bot commented Feb 10, 2021

SDN-1569: Add support for multiple destinations in redirect mode #34

SDN-1569: Add support for multiple destinations in redirect mode #34

Conversation

tssurya commented Feb 5, 2021

tssurya commented Feb 5, 2021

danielmellado left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Feb 10, 2021