Semi-automatic egress IP #16561

danwinship · 2017-09-26T16:43:21Z

This is the first half of the automatic egress IP work; it handles routing traffic through egress IPs, but not the assigning of IPs to nodes, which must be done manually by the administrator at this point (by editing the HostSubnet records; in the future the SDN master will do this automatically based on annotations on the Node records).

The set of Egress IPs active on a node was stored in HostSubnet rather than Node because (a) there are no upstream compatibility issues to deal with that way, and (b) nodes already have a HostSubnet watch, but not a Node watch. (And Node statuses change constantly, while HostSubnets are much more low-bandwidth.)

Oh, the other thing that's there for future expansion is that a NetNamespace can have an array of EgressIPs, but only the first one gets used. In the future we will likely support multiple IPs, either at the same time, or sequentially failing over.

@openshift/sig-networking PTAL

knobunc

Good stuff Dan. It's clear and well written. Thanks

knobunc · 2017-09-27T12:44:33Z

pkg/network/node/ovscontroller.go

-	otx.AddFlow("table=100, priority=%d,tcp,tcp_dst=53,nw_dst=%s,actions=output:2", networkapi.EgressNetworkPolicyMaxRules+1, nodeIP)
-	otx.AddFlow("table=100, priority=%d,udp,udp_dst=53,nw_dst=%s,actions=output:2", networkapi.EgressNetworkPolicyMaxRules+1, nodeIP)
+	// Table 100: egress routing; edited by UpdateNamespaceEgressRules()
+	// eg, FIXME


Minor omission

knobunc · 2017-09-27T12:48:17Z

/test all

knobunc · 2017-09-27T18:42:03Z

@liggitt, @smarterclayton: Can I get an API review on this one too please?

knobunc · 2017-09-27T19:10:27Z

/lgtm

smarterclayton · 2017-09-27T20:28:00Z

pkg/network/apis/network/validation/validation.go

+
+	for i, ip := range netnamespace.EgressIPs {
+		if net.ParseIP(ip) == nil {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), ip, "invalid IP address"))


Is this allowed to be localhost? Multicast? Any implications of those? (extra validation is needed only if it there is no clear use case for ever allowing those)

There's no use case for setting it to either of those, but there's no danger either. (It would just fail, just like if they tried to claim a random far-away IP address; EgressIPs have to be on the same subnet as some node's primary IP.) We don't check for semantically bad IPs in validation anywhere else, so I think just letting it through here and then failing later makes sense.

knobunc · 2017-09-27T22:43:06Z

@smarterclayton since you've looked over it, can you approve it if it meets with your approval? Thanks

rajatchopra · 2017-09-27T23:33:43Z

pkg/network/node/egressip.go

+
+		if nodeIP == eip.localIP {
+			if err := eip.claimEgressIP(ip, hex); err != nil {
+				glog.Errorf("Error claiming Egress IP %q: %v", ip, err)


Should we continue at this point? Or update the rules anyway?

We should update the rules, but we should update them to drop the packets (so the code is currently wrong)

soltysh · 2017-09-28T11:05:08Z

/unassign

eparis · 2017-09-28T13:29:04Z

/retest

danwinship · 2017-09-28T14:50:08Z

@smarterclayton the bot added needs-api-review back after you removed it...

eparis · 2017-09-28T14:57:29Z

/lgtm
the 'needs review' doesn't matter. You have the approval.

eparis · 2017-09-28T19:34:05Z

/lgtm
after the rebases...

openshift-merge-robot · 2017-09-28T19:34:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, knobunc

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [eparis]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

danwinship · 2017-09-28T21:16:44Z

/retest

eparis · 2017-09-29T04:01:59Z

/retest

openshift-bot · 2017-09-29T11:41:24Z

/retest

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

danwinship · 2017-09-29T17:23:50Z

/retest

danwinship · 2017-09-29T20:22:21Z

/retest

pravisankar · 2017-09-28T22:07:34Z

pkg/network/apis/network/v1/types.go

@@ -68,6 +68,9 @@ type HostSubnet struct {
 	HostIP string `json:"hostIP" protobuf:"bytes,3,opt,name=hostIP"`
 	// Subnet is the CIDR range of the overlay network assigned to the node for its pods
 	Subnet string `json:"subnet" protobuf:"bytes,4,opt,name=subnet"`
+
+	// EgressIPs is the list of automatic egress IP addresses currently hosted by this node
+	EgressIPs []string `json:"egressIPs" protobuf:"bytes,5,rep,name=egressIPs"`


pravisankar · 2017-09-28T22:07:42Z

pkg/network/apis/network/v1/types.go

+
+	// EgressIPs is a list of reserved IPs that will be used as the source for external traffic coming from pods in this namespace. (If empty, external traffic will be masqueraded to Node IPs.)
+	// +optional
+	EgressIPs []string `json:"egressIPs" protobuf:"bytes,4,rep,name=egressIPs"`


same omitempty?

pravisankar · 2017-09-28T22:12:32Z

pkg/network/apis/network/validation/validation.go

+	for i, egressIP := range hs.EgressIPs {
+		if net.ParseIP(egressIP) == nil {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("egressIPs").Index(i), egressIP, "invalid IP address"))
+		}


Do we expect egress IP to work for IPV6 as well? If not, may be we should add ipv4 check here and in netnamespace.EgressIPs validation.

pravisankar · 2017-09-28T22:25:55Z

pkg/network/node/egressip.go

+	}
+}
+
+func (eip *egressIPWatcher) Start(networkClient networkclient.Interface, iptables *NodeIPTables) error {


Since this method will not return an error may be this method can have no return value.

pravisankar · 2017-09-28T23:51:32Z

pkg/network/node/egressip.go

+		for _, link := range links {
+			addrs, err := netlink.AddrList(link, syscall.AF_INET)
+			if err != nil {
+				return fmt.Errorf("could not get addresses of interface %q while adding egress IP: %v", link.Attrs().Name, err)


Failed link may not have desired IP address, we could log an error and continue

pravisankar · 2017-09-29T07:52:03Z

pkg/network/node/egressip.go

+	iptables      *NodeIPTables
+
+	// from HostSubnets
+	nodesByNodeIP   map[string]*nodeEgress


We already know the nodeIP (map key), don't need to duplicate in the map value
nodesByNodeIP map[string]*sets.String ?

pravisankar · 2017-09-29T19:59:35Z

pkg/network/node/egressip.go

+
+	// from HostSubnets
+	nodesByNodeIP   map[string]*nodeEgress
+	nodesByEgressIP map[string]*nodeEgress


Similarly this could be nodesByEgressIP map[string]string

pravisankar · 2017-09-29T20:05:09Z

pkg/network/node/egressip.go

+	nodesByEgressIP map[string]*nodeEgress
+
+	// From NetNamespaces
+	namespacesByVNID     map[uint32]*namespaceEgress


vnid may be duplicate here but considering most of the fields are also used by namespacesByEgressIP, I think this is a good compromise.

pravisankar · 2017-09-29T20:49:44Z

pkg/network/node/egressip.go

+		}
+
+		if ns, exists := eip.namespacesByEgressIP[ip]; exists {
+			if ns.assignedIP == "" {


If assignedIP is already set, then drop the traffic for this namespace?

pravisankar · 2017-09-29T20:58:00Z

pkg/network/node/egressip.go

+		}
+		ns = &namespaceEgress{vnid: vnid}
+		eip.namespacesByVNID[vnid] = ns
+	}


When namespace is deleted, where are we deleting vnid from namespacesByVNID map?

danwinship · 2017-09-30T01:19:06Z

/retest

openshift-merge-robot · 2017-09-30T04:18:47Z

Automatic merge from submit-queue.

pravisankar · 2017-10-02T21:47:19Z

pkg/network/node/node.go

@@ -332,7 +335,9 @@ func (node *OsdnNode) Start() error {
 	if err != nil {
 		return err
 	}
-
+	if err = node.egressIP.Start(node.networkClient, nodeIPTables); err != nil {


We should not call this for single-tenant/subnet plugin as it depends on NetNamespaces.
I think, we should move this to node.policy.StartEgressIP?

openshift-merge-robot assigned knobunc and soltysh Sep 26, 2017

openshift-merge-robot added the needs-api-review label Sep 26, 2017

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 26, 2017

danwinship force-pushed the auto-egress-ip branch from e6046ee to 89cca5f Compare September 26, 2017 17:23

knobunc approved these changes Sep 27, 2017

View reviewed changes

danwinship force-pushed the auto-egress-ip branch from 89cca5f to a8df5db Compare September 27, 2017 13:32

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2017

smarterclayton reviewed Sep 27, 2017

View reviewed changes

rajatchopra reviewed Sep 27, 2017

View reviewed changes

smarterclayton added api-approved and removed needs-api-review labels Sep 28, 2017

openshift-merge-robot added the needs-api-review label Sep 28, 2017

openshift-ci-robot unassigned soltysh Sep 28, 2017

danwinship force-pushed the auto-egress-ip branch from a8df5db to b398614 Compare September 28, 2017 14:48

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2017

openshift-ci-robot assigned eparis Sep 28, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017

eparis added the kind/bug Categorizes issue or PR as related to a bug. label Sep 28, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2017

add NetNamespace.EgressIPs and HostSubnet.EgressIPs

eeca979

danwinship force-pushed the auto-egress-ip branch from b398614 to e6919ba Compare September 28, 2017 18:04

openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 28, 2017

danwinship added 2 commits September 28, 2017 14:56

Regenerate files

02ca247

Implement the node side of automatic egress IP support

7d92222

danwinship force-pushed the auto-egress-ip branch from e6919ba to 7d92222 Compare September 28, 2017 18:56

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2017

eparis added the queue/multiple-rebases label Sep 28, 2017

pravisankar reviewed Sep 29, 2017

View reviewed changes

openshift-merge-robot merged commit 060b43b into openshift:master Sep 30, 2017

danwinship deleted the auto-egress-ip branch October 2, 2017 20:43

pravisankar reviewed Oct 2, 2017

View reviewed changes

danwinship mentioned this pull request Oct 3, 2017

auto egress IP fixes #16659

Merged

Semi-automatic egress IP #16561

Semi-automatic egress IP #16561

Conversation

danwinship commented Sep 26, 2017

knobunc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Sep 27, 2017

knobunc commented Sep 27, 2017

knobunc commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knobunc commented Sep 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Sep 28, 2017

eparis commented Sep 28, 2017

danwinship commented Sep 28, 2017

eparis commented Sep 28, 2017

eparis commented Sep 28, 2017

openshift-merge-robot commented Sep 28, 2017

danwinship commented Sep 28, 2017

eparis commented Sep 29, 2017

openshift-bot commented Sep 29, 2017

danwinship commented Sep 29, 2017

danwinship commented Sep 29, 2017

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

Choose a reason for hiding this comment

danwinship commented Sep 30, 2017

openshift-merge-robot commented Sep 30, 2017

Choose a reason for hiding this comment