f5 vxlan integration with sdn by rajatchopra · Pull Request #11181 · openshift/origin

rajatchopra · 2016-10-01T00:36:07Z

This PR implements the VxLAN integration of F5 with a vxlan based overlay. Three pieces of work primarily:

Router plugin now watches for Nodes in the cluster (to program the F5's vxlan FDB; in the case of template router its a noop)
F5 router programs the VxLAN devices/routing on the F5 BIG IP on initialization
Command line changes to accept the special parameters needed (including a boolean that disables the setup)

rajatchopra · 2016-10-03T18:00:38Z

[test]

knobunc · 2016-10-03T19:56:21Z

[test]

chen23 · 2016-10-06T02:39:21Z

FYI in the policy section of the code the use of the Legacy flag will only work in TMOS version 12.1 or newer. Ideally have a version check to only add the flag if TMOS >= 12.1

policyPayload := f5Policy{
        Name:     policyName,
        Controls: []string{"forwarding"},
        Requires: []string{"http"},
        Strategy: "best-match",
        Legacy:   true,
}

Python example of version check here: https://github.com/f5devcentral/f5-icontrol-codeshare-python/blob/master/kubernetes-example/k8s2f5.py#L328

chen23 · 2016-10-06T02:44:46Z

The auth code looks to only support basicauth. This will limit authentication to local accounts and not any remote (radius/tacacs), etc... https://github.com/rajatchopra/origin/blob/3ad39a5c60f344d8fa6cb673f021effe9e139c4b/pkg/router/f5/f5.go#L413

    req.Header.Set("Content-Type", "application/json")
    req.Header.Set("Accept", "application/json")
    req.SetBasicAuth(f5.username, f5.password)

client := &http.Client{Transport: tr}

Python example: https://github.com/F5Networks/f5-icontrol-rest-python/blob/5c43b0f6424a5f495c50fd2a318e172d5c3d7e37/icontrol/authtoken.py
DevCentral article: https://devcentral.f5.com/articles/demystifying-icontrol-rest-part-6-token-based-authentication

chen23 · 2016-10-06T03:15:02Z

Doing a quick scan of the code I'm not seeing an explicit save of the running config after it is generated. F5Networks/f5-common-python#342

iControl REST sits in front of tmsh

https://support.f5.com/kb/en-us/products/big-ip_ltm/manuals/product/bigip-system-essentials-11-6-0/2.html

"When you use the Traffic Management Shell (tmsh) to configure the system, you must explicitly issue a save command to store the configuration data that you have generated. Otherwise, the newly-generated configuration data is not actually stored on the system. For more information about tmsh, see the Traffic Management Shell (tmsh) Reference Guide."

kmunson1973 · 2016-10-06T04:25:54Z

You can save the running config by issuing a POST /mgmt/tm/sys/config with the following json payload.
{
"command":"save"
}

knobunc · 2016-10-18T18:48:58Z

pkg/router/f5/f5.go

+func checkIPAndGetMac(ipStr string) (string, error) {
+	ip := net.ParseIP(ipStr)
+	if ip == nil {
+		errStr := fmt.Sprintf("'%s' is not a valid IP address for a vtep", ipStr)


This implies that the vtep has other restrictions. Perhaps:
'vtep IP '%s' is not a valid IP address'

knobunc

The code structure LGTM... I have no idea if what it is doing with the F5 is right though :-)

Are there any tests we can add? Perhaps at least to exercise the vtep MAC generation function?

knobunc · 2016-10-18T18:50:08Z

pkg/router/f5/f5.go

 	return nil
 }

+func checkIPAndGetMac(ipStr string) (string, error) {


Should this have vtep in it? Or is the package scope enough to indicate that.

Its just a util function. The package scope is enough I guess.

Consider changing this function name to be more descriptive (e.g. convertIpToVtepMac).

Also, consider adding a unit test.

knobunc · 2016-10-19T18:21:18Z

Repo problems re-[test]

knobunc · 2016-10-19T18:22:39Z

@smarterclayton PTAL. The tests are still being worked on, but can you please look over the code to see if it is good?

marun

I don't see much in the way of test changes that would target the new functionality. Is that an oversight or is qa intended to validate this feature post-merge?

marun · 2016-10-20T17:14:14Z

docs/man/man1/openshift-infra-f5-router.1

    The path to the F5 BIG\-IP SSH private key file

+.PP
+\fB\-\-f5\-setup\-osdn\-vxlan\fP=false


openshif -> openshift

marun · 2016-10-20T18:14:21Z

pkg/router/controller/factory/factory.go

+	nodeEventQueue := oscache.NewEventQueue(cache.MetaNamespaceKeyFunc)
+	cache.NewReflector(&nodeLW{
+		client: factory.NClient,
+		field:  factory.Fields,


Does it makes sense to apply the same filtering to nodes as is being applied to routes?

Dunno - would it work if you only have a subset of nodes added to the F5 config and the endpoints for a service are on nodes that are not part of the set?

I see no harm in keeping the fields/filtering. Possible use case with FDB being populated for only a portion of the cluster (say a datacenter).

I have no issue with supporting filtering on the nodes. I was asking whether it made sense for the same filtering parameters to be applied to both nodes and routes. The current approach of reusing the same fields and labels to filter both would preclude filtering one but not the other.

It does not make sense to just apply field labeling to nodes - we should never apply the same field selector to two types.

We should not be watching nodes unless the underlying router is using it. This loop should be optional and the factory must explicitly choose to use it.

marun · 2016-10-20T18:18:45Z

pkg/router/controller/factory/factory.go

 	return lw.client.Endpoints(lw.namespace).Watch(opts)
 }
+
+// nodeLW is a list watcher for routes.


routes -> nodes

marun · 2016-10-20T19:09:06Z

pkg/router/f5/f5.go

+		UsePmtu:      "enabled",
+	}
+	err = f5.post(url, tunnelPayload, nil)
+	if err != nil && err.(F5Error).httpStatusCode != 409 {


Consider defining 409 as a constant whose name is self-documenting (e.g. STATUS_ALREADY_EXISTS) and avoid having to comment as to why 409 is special.

marun · 2016-10-20T20:05:57Z

pkg/router/f5/f5.go

+	}
+	// create the net self IP
+	err = f5.post(selfUrl, netSelfPayload, nil)
+	if err != nil && err.(F5Error).httpStatusCode != 409 {


ditto re: named constant

marun · 2016-10-20T20:10:57Z

pkg/router/f5/f5.go

 	return nil
 }

+func checkIPAndGetMac(ipStr string) (string, error) {


Consider changing this function name to be more descriptive (e.g. convertIpToVtepMac).

marun · 2016-10-20T20:15:09Z

pkg/router/f5/plugin.go

+			glog.Warningf("Error in obtaining IP address of newly added node %s - %v", node.Name, err)
+			return nil
+		}
+		p.F5Client.AddVtep(ip)


Should this call be returning the error that is potentially returned by AddVtep()?

marun · 2016-10-20T20:15:20Z

pkg/router/f5/plugin.go

+			glog.Warningf("Error in obtaining IP address of deleted node %s - %v", node.Name, err)
+			return nil
+		}
+		p.F5Client.RemoveVtep(ip)


Should this call be returning the error that is potentially returned by RemoveVtep?

marun · 2016-10-20T20:16:32Z

pkg/router/f5/plugin.go

+		}
+		p.F5Client.RemoveVtep(ip)
+	case watch.Modified:
+		// ignore the modified event. Change in IP address of the node is not supported.


What's the implication of not supporting node address changes? Is it just going to be documented? Should there be logging to diagnose issues if/when address changes occur?

I had the same question re: changes and asked Rajat on this. He said on the openshift-sdn side we don't support IP address changes for a node.

marun · 2016-10-20T20:19:10Z

pkg/router/f5/f5.go

 	return nil
 }

+func checkIPAndGetMac(ipStr string) (string, error) {


Also, consider adding a unit test.

ramr

a question on the SetupOSDNVxLAN parameter if we can define it implicitly based on the internal ip and cidr, some comments re: constants and todos.
And a note on HandleNode for the host_admitter code in PR #11201

ramr · 2016-10-20T22:25:38Z

pkg/cmd/infra/router/f5.go

 	flag.StringVar(&o.PartitionPath, "f5-partition-path", util.Env("ROUTER_EXTERNAL_HOST_PARTITION_PATH", f5plugin.F5DefaultPartitionPath), "The F5 BIG-IP partition path to use")
+	flag.StringVar(&o.InternalAddress, "f5-internal-address", util.Env("ROUTER_EXTERNAL_HOST_INTERNAL_ADDRESS", ""), "The F5 BIG-IP internal interface's IP address")
+	flag.StringVar(&o.VxlanGateway, "f5-vxlan-gateway-cidr", util.Env("ROUTER_EXTERNAL_HOST_VXLAN_GW_CIDR", ""), "The F5 BIG-IP gateway-ip-address/cidr-mask for setting up the VxLAN")
+	flag.BoolVar(&o.SetupOSDNVxLAN, "f5-setup-osdn-vxlan", false, "Flag to enable VxLAN setup for integration openshif-sdn with the F5 BIG-IP")


so this needs to be set with a command line arg option? It maybe better to implicitly set it if f5-internal-address and f5-vxlan-gateway-cidr is set - basically one without the other(s) doesn't make sense does it?

ramr · 2016-10-20T22:26:08Z

pkg/cmd/infra/router/f5.go

 		return errors.New("F5 HTTP and HTTPS vservers cannot both be blank")
 	}

+	if o.SetupOSDNVxLAN && (len(o.VxlanGateway) == 0 || len(o.InternalAddress) == 0) {


See above comment re: o.SetupOSDNVxLAN.

ramr · 2016-10-20T22:26:43Z

pkg/router/controller/controller.go

 	}
 	go utilwait.Forever(c.HandleRoute, 0)
 	go utilwait.Forever(c.HandleEndpoints, 0)
+	c.HandleNode()


Is this needed? The one from the goroutine ahead should be called soon anyway.

ramr · 2016-10-20T22:28:13Z

pkg/router/controller/factory/factory.go

 	return &RouterControllerFactory{
 		KClient:        kc,
 		OSClient:       oc,
+		NClient:        kc,


Yeah, if its same as kc - we should cleanup to just use 1 ?Client. Maybe cleanup in follow on work. I know changing it would touch a lot more code in this PR.

A comment/todo here or in the type definition would be useful.

ramr · 2016-10-20T22:29:12Z

pkg/router/controller/unique_host.go

+func (p *UniqueHost) HandleNode(eventType watch.EventType, node *kapi.Node) error {
+	return p.plugin.HandleNode(eventType, node)
+}
+


FYI, depending on which PR merges first - #11201 - will need to add HandleNode to the host_admitter plugin.

ramr · 2016-10-20T22:34:17Z

pkg/router/f5/plugin.go

+		}
+		p.F5Client.RemoveVtep(ip)
+	case watch.Modified:
+		// ignore the modified event. Change in IP address of the node is not supported.


I had the same question re: changes and asked Rajat on this. He said on the openshift-sdn side we don't support IP address changes for a node.

ramr · 2016-10-20T22:35:19Z

pkg/router/f5/types.go

 	// other possible values are all-match and first-match.
 	Strategy string `json:"strategy"`
+
+	Legacy bool `json:"legacy"`


I was curious on this one - thanks for explaining it. So if we ever decide to lock down to a specific version of the F5 api, then we would have to remove this - maybe adding a comment here would be helpful. Thx

ramr · 2016-10-20T22:36:29Z

pkg/router/f5/types.go

 	Name string `json:"name"`

+	// Partition name for the policy
+	TmPartition string `json:"tmPartition"`


Felt a little weird that this one is called TmPartition but every other F5 api call payload uses Partition. Thanks for explaining that one.

Update the godoc explaining what TmPartition means or be more explicit

ramr · 2016-10-20T22:38:59Z

pkg/router/f5/f5.go

+		LocalAddress: f5.internalAddress,
+		Mode:         "bidirectional",
+		Mtu:          "0",
+		Profile:      path.Join(f5.partitionPath, "vxlan-ose"),


constant for vxlan-ose.

ramr · 2016-10-20T22:43:06Z

pkg/router/controller/factory/factory.go

+	nodeEventQueue := oscache.NewEventQueue(cache.MetaNamespaceKeyFunc)
+	cache.NewReflector(&nodeLW{
+		client: factory.NClient,
+		field:  factory.Fields,


Dunno - would it work if you only have a subset of nodes added to the F5 config and the endpoints for a service are on nodes that are not part of the set?

ramr · 2016-10-21T00:56:39Z

LGTM

marun · 2016-10-21T02:18:11Z

networking job flaked with #11452 re-[test]

smarterclayton · 2016-10-22T00:06:14Z

pkg/router/controller/controller.go

 	}
 	go utilwait.Forever(c.HandleRoute, 0)
 	go utilwait.Forever(c.HandleEndpoints, 0)
+	go utilwait.Forever(c.HandleNode, 0)


Why is this always on if it's only used in F5?

smarterclayton · 2016-10-22T00:06:52Z

pkg/router/controller/factory/factory.go

 type RouterControllerFactory struct {
 	KClient        kclient.EndpointsNamespacer
 	OSClient       osclient.RoutesNamespacer
+	NClient        kclient.NodesInterface


NamespaceClient, abbreviations were a special case but shouldn't be propogared.

smarterclayton · 2016-10-22T00:10:07Z

test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml

    verbs:
    - list
    - watch
+  - apiGroups:


This is a pretty big security expansion - @deads2k

@smarterclayton didn't you want this role bindable by project-admins? This will prevent that from being possible.

As for our system router, it doesn't worry overly much, but for per-project routers, this makes it impossible to create those.

Its also required. If we expect the F5 to participate in the SDN it needs to know about the nodes its talking. We don't have OpenShift-sdn doing that. If this is a big problem it would mean splitting the f5 into its own role, I guess?

Its also required. If we expect the F5 to participate in the SDN it needs to know about the nodes its talking. We don't have OpenShift-sdn doing that. If this is a big problem it would mean splitting the f5 into its own role, I guess?

Or adding it as the delta required.

smarterclayton · 2016-10-22T00:12:01Z

Removing from merge queue until my comments are addressed.

rajatchopra · 2016-10-23T17:36:01Z

@smarterclayton I have addressed all your feedback. Primarily two:

Node watch loop does not run unless its F5 && a vxlan setup is needed
All fields/labels are put in for node watch (we have no use case yet where only a portion of the sdn needs to be addressed by F5)

Marking it [merge]. Please feel free to revert the commit if anything is loose.

TODO:
Document on how to use the native VxLAN feature

eparis · 2016-10-23T21:40:59Z

[merge] you stoopid flake. Merge!

openshift-bot · 2016-10-23T23:05:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10513/) (Image: devenv-rhel7_5234)

eparis · 2016-10-23T23:07:13Z

I'm an idiot, it wasn't a flake!

test/integration/router_stress_test.go:285: not enough arguments in call to factory.Create

openshift-bot · 2016-10-23T23:37:20Z

Evaluated for origin merge up to 96354f7

openshift-bot · 2016-10-23T23:37:22Z

Evaluated for origin test up to 96354f7

eparis · 2016-10-24T00:18:45Z

back in at #5. wait and see!

openshift-bot · 2016-10-24T01:01:53Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10513/) (Base Commit: 35b539d)

smarterclayton · 2016-10-24T14:14:11Z

As David noted this causes a security vulnerability because it's escalating the privileges granted to system:router. I'm going to push a change that removes the node permission granted here and to fix the issue you can open another one (instead of reverting here).

smarterclayton · 2016-10-24T14:38:13Z

Opened a PR that removes the permission, we can iterate on the next solution at a lower pace.

smarterclayton · 2016-10-25T18:12:35Z

It's not a flake. The tests don't compile.

smarterclayton · 2016-10-25T18:23:38Z

We could also create system:local-router or system:personal-router.

On Mon, Oct 24, 2016 at 9:14 AM, David Eads notifications@github.com
wrote:

@deads2k commented on this pull request.

In test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml
#11181:

@@ -1776,6 +1776,14 @@ items:
- ""
attributeRestrictions: null
resources:

nodes

Its also required. If we expect the F5 to participate in the SDN it needs
to know about the nodes its talking. We don't have OpenShift-sdn doing
that. If this is a big problem it would mean splitting the f5 into its own
role, I guess?

Or adding it as the delta required.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11181, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p5OJReoyNdhUMz449EXSxXww4BvEks5q3K84gaJpZM4KLn0M
.

eparis · 2016-10-25T18:23:58Z

You mean why I said I'm an idiot, it wasn't a flake!

smarterclayton · 2016-10-25T18:31:31Z

Integration tests aren't compiling

On Oct 23, 2016, at 4:45 PM, OpenShift Bot notifications@github.com wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10508/) (Base
Commit: 35b539d
35b539d
)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11181 (comment),
or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABG_p2r1fYHWcAjiUQ-TekgW_5ppbsHqks5q28dfgaJpZM4KLn0M
.

smarterclayton · 2016-10-25T18:36:31Z

The least disruptive thing is to create a new role with the additional
permission and require admins to set it up. We can't retroactively add it
to old clusters (reconcile) if anyone has used it, and it's a very big and
surprising role.

So create system:router-.... (david i need a name suggestion) that contains
the node role.

On Mon, Oct 24, 2016 at 10:11 AM, Clayton Coleman ccoleman@redhat.com
wrote:

We could also create system:local-router or system:personal-router.

On Mon, Oct 24, 2016 at 9:14 AM, David Eads notifications@github.com
wrote:

@deads2k commented on this pull request.

In test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml
#11181:

@@ -1776,6 +1776,14 @@ items:
- ""
attributeRestrictions: null
resources:

nodes

Its also required. If we expect the F5 to participate in the SDN it needs
to know about the nodes its talking. We don't have OpenShift-sdn doing
that. If this is a big problem it would mean splitting the f5 into its own
role, I guess?

Or adding it as the delta required.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#11181, or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p5OJReoyNdhUMz449EXSxXww4BvEks5q3K84gaJpZM4KLn0M
.

smarterclayton · 2016-10-25T20:09:02Z

GitHub is delivering emails I sent on Friday. Thanks github.

deads2k · 2016-10-25T20:10:27Z

system:rouer-privileged, system:rouer-extensions?

rajatchopra force-pushed the f5vxlan branch from a5b9761 to 609817b Compare October 3, 2016 17:59

rajatchopra force-pushed the f5vxlan branch 2 times, most recently from f93a55e to 3ad39a5 Compare October 5, 2016 18:45

dcbw added the component/networking label Oct 14, 2016

knobunc reviewed Oct 18, 2016

View reviewed changes

knobunc approved these changes Oct 18, 2016

View reviewed changes

rajatchopra force-pushed the f5vxlan branch 2 times, most recently from 52b068c to 0ac78a7 Compare October 19, 2016 16:13

rajatchopra changed the title ~~WIP[do not merge]: f5 vxlan integration with sdn~~ f5 vxlan integration with sdn Oct 19, 2016

rajatchopra force-pushed the f5vxlan branch from 0ac78a7 to aae7a2f Compare October 20, 2016 01:11

marun suggested changes Oct 20, 2016

View reviewed changes

ramr reviewed Oct 20, 2016

View reviewed changes

rajatchopra force-pushed the f5vxlan branch from aae7a2f to 4b09057 Compare October 20, 2016 23:31

ramr approved these changes Oct 21, 2016

View reviewed changes

rajatchopra force-pushed the f5vxlan branch from 4b09057 to 6c29ea3 Compare October 21, 2016 03:39

smarterclayton reviewed Oct 22, 2016

View reviewed changes

rajatchopra force-pushed the f5vxlan branch from 6c29ea3 to eb0a9c4 Compare October 23, 2016 17:00

rajatchopra force-pushed the f5vxlan branch 2 times, most recently from 41d5983 to 1d695d3 Compare October 23, 2016 17:32

f5 vxlan integration with sdn

96354f7

rajatchopra force-pushed the f5vxlan branch from 1d695d3 to 96354f7 Compare October 23, 2016 23:33

openshift-bot merged commit 0787d9f into openshift:master Oct 24, 2016

smarterclayton mentioned this pull request Oct 24, 2016

Remove node access from the system:router roles #11528

Merged

rajatchopra deleted the f5vxlan branch November 4, 2016 17:46

Conversation

rajatchopra commented Oct 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rajatchopra commented Oct 3, 2016

Uh oh!

knobunc commented Oct 3, 2016

Uh oh!

chen23 commented Oct 6, 2016

Uh oh!

chen23 commented Oct 6, 2016

Uh oh!

chen23 commented Oct 6, 2016

Uh oh!

kmunson1973 commented Oct 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knobunc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knobunc commented Oct 19, 2016

Uh oh!

knobunc commented Oct 19, 2016

Uh oh!

marun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rajatchopra commented Oct 1, 2016 •

edited

Loading