ebay/go-ovn

A Go library for OVN Northbound/Southbound DB access using native OVSDB protocol

GO OVN

A Go library for OVN DB access using native OVSDB protocol. It is based on the OVSDB Library, but used own fork https://github.com/ebay/libovsdb.git with patches.

What is OVN?

OVN (Open Virtual Network) is a SDN solution built on top of OVS (Open vSwitch). The interface of OVN is its northbound DB which is an OVSDB database.

What is OVSDB?

OVSDB is a protocol for managing the configuration of OVS. It's defined in RFC 7047.

Why native OVSDB protocol?

There are projects accessing OVN DB based on the ovn-nbctl/sbctl CLI, which has some problems. Here are the majors ones and how native OVSDB protocol based approach solves them:

• Performance problem. Every CLI command would trigger a separate OVSDB connection setup/teardown, initial OVSDB client cache population, etc., which would impact performance significantly. This library uses OVSDB protocol directly so that the overhead happens only once for all OVSDB operations.

• Caching problem. When there is a change in desired state, which requires updates in OVN, we need to figure out first what's the current state in OVN, which requires either maintaining a client cache or executing a "list" command everytime. This library maintains an internal cache and ensures it is always up to date with the remote DB with the help of native OVSDB support.

• String parsing problem. CLI based implementation needs extra conversion from the string output to Go internal data types, while it is not necessary with this library since OVSDB JSON RPC takes care of it.

go-ovn's Issues

logical router questions

may be i miss something, but how can looks type LogicalRouter struct ?

type LogicalRouter struct {                   
  UUID       string                           
  Name       string                           
  Enabled    bool                             
  ExternalID map[interface{}]interface{}      
}                                             

or i'm wrong?

ACL: missing 'name' field support

nameless ACLs matching evens in logs look like this:
2021-03-02T19:29:05.415Z|06852|acl_log(ovn_pinctrl0)|INFO|name="<unnamed>", verdict=drop, severity=debug: ...
which is not good ...
trivial to add 'name' field support, but this will change the API signature for ACLAddEntity (extra parameter). Can keep the signature and pass the name in external_ids, but it's ugly... adding a new one (ACLAddEntityNamed) seems like an overkill.
Opinions?

change interface to not fatal or panic

What do you think about changing interface to return error and not doing fatal/os.Exit and panic?
I can provide pr to change interface definition and after that add ability to get/set dhcp options and other stuff

Add new loadbalancer fields

The OVN LoadBalancer added some new fields in the last releases

   Summary:
       name                          string
       vips                          map of string-string pairs
       protocol                      optional string, one of sctp, tcp, or udp
       health_check                  set of Load_Balancer_Health_Checks
       ip_port_mappings              map of string-string pairs
       selection_fields              set of strings, one of  eth_dst,  eth_src,  ip_dst,  ip_src,
                                     tp_dst, or tp_src
       Common Columns:
         external_ids                map of string-string pairs

but go-ovn only have

go-ovn/load_balancer.go

Lines 25 to 32 in da67e97

	// LoadBalancer ovnnb item
	type LoadBalancer struct {
	UUID string
	Name string
	vips map[interface{}]interface{}
	protocol string
	ExternalID map[interface{}]interface{}
	}

xref:
https://github.com/ovn-org/ovn/blob/master/ovn-nb.ovsschema#L188
http://manpages.ubuntu.com/manpages/hirsute/man5/ovn-nb.5.html#load_balancer%20table

List operations should get from cache instead of new transaction

A new transaction with opSelect is used for list operations for dhcp_options and logical_switches, which is not necessary, and maybe create inconsistency with local cache. All reads should from cache directly.

Missing support for logical router policies

From the submariner project we use go-ovn, and we noticed that policies on routers are missing, for
now we were using ovn-nbctl. But I will be sending a patch to support policies here so we can get rid of ovn-nbctl usage.

Proposed api interface changes

I want to propose interface changes, Firstly to follow go interface naming, api creates Client interface.
All methods have args with native types, so for adding port to logical switch we can provide
*LogicalSwitch struct to function, that have needed name. Also in this struct we can pass ExternalIDs, so LSPAdd can find needed ls by name, uuid, or external_ids. This is already handled with getRowUUIDs.

as in example of LSPAdd second arg is *LogicalSwitchPort with filled needed data. So function can receive only 2 args. We can easy handle optional params and default values.

Inside function we can use https://godoc.org/github.com/mitchellh/mapstructure to convert from struct to map[string]interface{}.

I think that this is more universal that current approach, and easy deduplicate many identical pieces of our code.

Support may-exist or --if-exists?

Support ovn-nbctl --if-exists lr-del default?

ovndbapi.LRSRList cannot get route policy

so I can't distinguish if the route is src-ip or dst-ip。

Re-work API to support full object modifications

Currently this library supports modifying a small aspect of all OVN objects, which makes it quite unusable in certain use cases. Ex:

Logical Routers in OVN currently have these fields:

_uuid               : 
enabled             : []
external_ids        : {}
load_balancer       : []
name                : 
nat                 : []
options             : {}
policies            : []
ports               : []
static_routes   : []

Now, these fields are internally modifiable and supported by the library as we can see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L26

However, the CRUD operations exposed by the API do not allow the modification of most of these fields, as we see here: https://github.com/eBay/go-ovn/blob/master/logical_router.go#L40

This means that a user of this library can effectively only set external_ids on any Logical Router, and moreover: also renders some functional operations completely impossible, such as setting options on the Logical Router.

I would expect to see the API allow the following operations

func (odbi *ovndb) lrAddImp(logicalRouter LogicalRouter) (*OvnCommand, error) 

and

func (odbi *ovndb) lrUpdateImp(logicalRouter LogicalRouter) (*OvnCommand, error) 

Depending on the time available to me, I might file a PR with this. But I would like to get some input on if these changes are valid. They directly modify the API, so it would be an important change.

Support SSL

Currently only TCP and Unix socket are supported, which doesn't satisfy security needs in some use cases.

When reconnecting, deleted rows are not handled

We have the following scenario in ovnk.
Some records (in this case LSPs) are deleted from nbdb, and a few seconds after we see a disconnection:

2021-04-19T11:40:26.193749278Z 2021/04/19 11:40:26 rpc2: client protocol error: read tcp 172.21.8.55:52212->172.21.8.53:9642: read: connection reset by peer
2021-04-19T11:40:26.193812317Z 2021/04/19 11:40:26 ssl:172.21.8.53:9642,ssl:172.21.8.55:9642,ssl:172.21.8.59:9642 disconnected. Reconnecting ... 
2021-04-19T11:40:26.701238456Z 2021/04/19 11:40:26 ssl:172.21.8.53:9642,ssl:172.21.8.55:9642,ssl:172.21.8.59:9642 reconnected.
2021-04-19T11:40:28.754648075Z 2021/04/19 11:40:28 rpc2: client protocol error: read tcp 172.21.8.55:45308->172.21.8.53:9641: read: connection reset by peer
2021-04-19T11:40:28.754903588Z 2021/04/19 11:40:28 ssl:172.21.8.53:9641,ssl:172.21.8.55:9641,ssl:172.21.8.59:9641 disconnected. Reconnecting ... 

What I am seeing is that, after the reconnection, the cache still contains one of those records which was supposed to be deleted.
What I believe is happening is that when we reconnect, and we try to re-align the cache contents in

go-ovn/client.go

Line 306 in 7376ba9

initial, err := c.MonitorTables("")

MonitorTables only returns existing items (as opposed to when populateCache is called from Update, where we get also the uuid / empty struct pairs.

I tried to validate with logs doing the following:

With master running, I put a "updating" log here

go-ovn/ovnimp.go

Line 246 in 7376ba9

, I created and deleted the pod and I see the empty struct coming:

Updating  0f6f40ea-b35a-4163-bd26-97af9d25a17f {map[_version:{5448a880-b9bf-4b74-b547-fb90e7e2e17c} addresses:{[]} dhcpv4_options:{[]} dhcpv6_options:{[]} dynamic_addresses:{[]} enabled:{[]} external_ids:{map[namespace:test pod:true]} ha_chassis_group:{[]} name:test_client options:{map[requested-chassis:ovn-worker]} parent_name:{[]} port_security:0a:58:0a:f4:02:05 10.244.2.5 tag:{[]} tag_request:{[]} type: up:false]}
Updating  0f6f40ea-b35a-4163-bd26-97af9d25a17f {map[]}

This is not happening when I start the master again, looking for the same uuid.

kubectl -n ovn-kubernetes logs ovnkube-master-6fbb9f798c-ggsdz -c ovnkube-master | grep 0f6f40ea-b35a-4163-bd26-97af9d25a17f

Returns empty.

@hzhou8 does it make sense? If so I can propose a fix.

Support multiple API calls in one transaction

acl add/delete code isn't handling external_ids correctly

external_ids should be ignored while checking for duplicates in ACLAdd and matching in ACLDel

The parameters that define a unique ACL for a given entity (logical switch or port group) are direction, match, and priority. The code currently also attempts to also use external_ids, but it should not.

Support connect to clustered DB leader only

Support option leader_only when connecting to a clustered DB so that only connection to leader is accepted.

Use mutation for adding/removing addresses to/from address set

Complete updating an address set can be inefficient, considering the size of an address set can be huge. New APIs should be provided to add/remove addresses to/from an address set.

crash on populateCache

Sometimes, when the cache is populated (e.g: on goovn.NewClient()) the library can crash the program (in several ways). An example crash traceback is here:

panic: interface conversion: interface {} is nil, not string
goroutine 94 [running]:
github.com/ebay/go-ovn.(*ovndb).rowToLogicalRouterPort(0xc000e89c80, 0xc0024bdcb0, 0x24, 0x5)
        /home/amorenoz/src/skynet/workdir/go/pkg/mod/github.com/amorenoz/[email protected]/logical_router_port.go:136 +0x14c4
github.com/ebay/go-ovn.(*ovndb).rowToLogicalRouter(0xc000e89c80, 0xc0024bdd40, 0x24, 0x24)
        /home/amorenoz/src/skynet/workdir/go/pkg/mod/github.com/amorenoz/[email protected]/logical_router.go:159 +0x1539
github.com/ebay/go-ovn.(*ovndb).populateCache(0xc000e89c80, 0xc0025daa50)
        /home/amorenoz/src/skynet/workdir/go/pkg/mod/github.com/amorenoz/[email protected]/ovnimp.go:235 +0x24d5
github.com/ebay/go-ovn.connect(0xc000e89c80, 0x0, 0x0)
        /home/amorenoz/src/skynet/workdir/go/pkg/mod/github.com/amorenoz/[email protected]/client.go:261 +0x1f7
github.com/ebay/go-ovn.NewClient(0xc0008ede70, 0xc002594600, 0x1f, 0x0, 0x0)
        /home/amorenoz/src/skynet/workdir/go/pkg/mod/github.com/amorenoz/[email protected]/client.go:290 +0x39f
github.com/skydive-project/skydive/topology/probes/ovn.(*Probe).Do(0xc000170000, 0x7c23700, 0xc0016e0600, 0xc00044a820, 0x0, 0xc000743ec0)
        /home/amorenoz/src/skynet/workdir/go/src/github.com/skydive-project/skydive/topology/probes/ovn/ovn.go:714 +0x228
github.com/skydive-project/skydive/topology/probes.(*serviceManager).Start.func1(0xc00044a800, 0x7c23700, 0xc0016e0600)
        /home/amorenoz/src/skynet/workdir/go/src/github.com/skydive-project/skydive/topology/probes/wrapper.go:51 +0xf5
created by github.com/skydive-project/skydive/topology/probes.(*serviceManager).Start
        /home/amorenoz/src/skynet/workdir/go/src/github.com/skydive-project/skydive/topology/probes/wrapper.go:49 +0xf1

The reason behind it is the lack of table order enforcement in PopulateCache due to golang not guaranteeing maps are ranged in the same order they are created. This can make a LogicalRouter object be created before its LogicalRouterPorts are, which causes the above crash.

I'll send a PR to fix this soon.

add some method to check connection alive

For persistent connection we need to have some method to understand connection status (alive or not), may be with optional callback to ability to reconnect.

logical_router_policy do not handle the situation nexthops contains only one item

for _, n := range cacheLogicalRouterPolicy.Fields["nexthops"].(libovsdb.OvsSet).GoSet {
lrpolicy.NextHops = append(lrpolicy.NextHops, n.(string))
}

when nexthops only contains one item, it panics.

In all of *ListImp() APIs allocate a slice with known capacity

At scale, we are allocating more memory than that is required. Since we already know the number of logical resources ahead of time, just allocate the slice with the correct capacity.

For example, lsListImp() is better implemented as below.

func (odbi *ovndb) lsListImp() ([]*LogicalSwitch, error) {
	odbi.cachemutex.RLock()
	defer odbi.cachemutex.RUnlock()

	cacheLogicalSwitch, ok := odbi.cache[TableLogicalSwitch]
	if !ok {
		return nil, ErrorSchema
	}

	listLS := make([]*LogicalSwitch, len(cacheLogicalSwitch))
	i := 0
	for uuid := range cacheLogicalSwitch {
		listLS[i] = odbi.rowToLogicalSwitch(uuid)
		i++
	}

	return listLS, nil
}

Update testing environment

The testing environment is out of date, It would be better to bring in a more updated version of openvswitch and ovn to test against.

As part of updating LRSRAdd() to support ecmp routes it I found that the version of ovs/ovn tested against does not support ecmp routes.

move sources from goovn to top level dir

I'm prepare patches to error handling, does it possible after that move goovn sources to top-level dir? I don't now understand why created goovn dir from the beginning.

client.Close() should handle cache release and remove the disconnect callback.

Maybe this API is not needed in most situation. However, current implementation has problems:

1. It doesn't cleanup resources and doesn't preventing the other APIs to continue reading the data. The expected behavior should be cleanup all the cache and all the later API calls for read/write data should fail.

2. Currently we have auto-reconnect supported. This API need to reset the disconnect callback so that it won't automatically reconnect by itself as a result of Close().

forking libovsdb

what do you think about forking libovsdb to Ebay namespace, add needed patches and use it on go-ovn without vendoring dir patching?

DHCP_Options_SET have no effect

after

	cmd, err = ovndbapi.DHCPOptionsAdd(
		"192.168.0.0/24",
		map[string]string{
			"server_id":  "192.168.1.1",
			"server_mac": "54:54:54:54:54:54",
			"lease_time": "6000",
		},
		nil)

I try to

cmd ,err = ovndbapi.DHCPOptionsSet("192.168.0.0/24",	map[string]string{
		"server_id":  "192.168.1.1",
		"server_mac": "54:54:54:54:54:54",
		"lease_time": "5000",
	},
		nil)

but it take no effect .

in ovn-nbctl

DHCP Options commands:
  dhcp-options-create CIDR [EXTERNAL_IDS]
                           create a DHCP options row with CIDR
  dhcp-options-del DHCP_OPTIONS_UUID
                           delete DHCP_OPTIONS_UUID
  dhcp-options-list
                           lists the DHCP_Options rows
  dhcp-options-set-options DHCP_OPTIONS_UUID  KEY=VALUE [KEY=VALUE]...
                           set DHCP options for DHCP_OPTIONS_UUID
  dhcp-options-get-options DHCO_OPTIONS_UUID
                           displays the DHCP options for DHCP_OPTIONS_UUID

and I find it cidr is not unique , may need to set it by use uuid ?

set ipv6 slaac ra and dhcp v4

Hi. I'm try to use this package to provide network for my libvirt vm. How can i add dhcp info to specific port?
Also - do you have roadmap or some plans to enhance this package?

introduce API force reconnect() for update SSL cert

for SSL-based goovn client, if the TLSconfig cert updated based on security, the client needs to disconnect the live connection and connect again use the updated TLSconfig.
@hzhou8 Any suggestions about how to support this?

the API can use internal function c.client.disconnect() and reconnect() I think.

ACL set APIs needed for match criteria and logging

ovn-kubernetes requires the ability to modify the match criteria and/or logging configuration for an existing ACL.

Cache syncing between write and read

While integrating go-ovn into ovn-kubernetes @girishmg noticed that we could hit a problem performing an immediate read after a write. For example, if we:

1. create an object in ovsdb via go-ovn
2. immediately attempt to read that object

The read can fail because a write transaction never guarantees that the cache is updated when it returns. Therefore we need to wait for an update to happen from ovsdb. One way to fix this is to block returning in the go-ovn transaction until the next update comes back and the cache is synced.

@hzhou8 thoughts?

panic on cacheACL type assertion in example

os:
Distributor ID: Ubuntu
Description: Ubuntu 18.04.4 LTS
Release: 18.04
Codename: bionic

ovn-nbctl (Open vSwitch) 2.9.5
DB Schema 5.10.0

ovn-sbctl (Open vSwitch) 2.9.5
DB Schema 1.15.0

how to reproduce:

go run examples/main.go

error message

{b7c93ac6-d263-47dc-bd9b-d1e7c10da7d5 test  map[] [12:34:56:78:90 10.10.10.1] []   map[]}
panic: interface conversion: interface {} is nil, not libovsdb.OvsSet

goroutine 1 [running]:
github.com/ebay/go-ovn.(*ovndb).rowToACL(0xc0000b2120, 0xc00001c4e0, 0x24, 0x0)
        /home/max/GolandProjects/go-ovn/acl.go:280 +0xa31
github.com/ebay/go-ovn.(*ovndb).aclListImp(0xc0000b2120, 0x74cc9c, 0x3, 0x0, 0x0, 0x0, 0x0, 0x0)
        /home/max/GolandProjects/go-ovn/acl.go:322 +0x36c
github.com/ebay/go-ovn.(*ovndb).ACLList(0xc0000b2120, 0x74cc9c, 0x3, 0x1, 0x0, 0x0, 0x51, 0x74cdf7)
        /home/max/GolandProjects/go-ovn/client.go:426 +0x3f
main.main()
        /home/max/GolandProjects/go-ovn/examples/main.go:70 +0x803

Support connecting to clustered DB

Support connecting to clustered DB using comma separated connection string, i.e. <method1>,<method2>,...,<methodN>

How to set logic bridge other-configs?

ovn-nbctl set Logical_Switch vpc1 other_config:subnet=172.66.1.10/24

We need dynamic IP address:

ovn-nbctl lsp-set-addresses vpc1-vm1 "02:ac:10:ff:01:30 dynamic"

So has a set logic switch other_config API？

broken update notifications

I don't understand when tests are broken, but i'm check commit d91fdd2 and it already broken

=== RUN   TestACLs
--- FAIL: TestACLs (0.00s)
    ovnnbimp_test.go:77:
                Error Trace:    ovnnbimp_test.go:77
                Error:          Not equal:
                                expected: true
                                actual  : false
                Test:           TestACLs
                Messages:       test[added port]: []
    ovnnbimp_test.go:78:
                Error Trace:    ovnnbimp_test.go:78
                Error:          Not equal:
                                expected: true
                                actual  : false
                Test:           TestACLs
                Messages:       test[setted port address]
    ovnnbimp_test.go:79:
                Error Trace:    ovnnbimp_test.go:79
                Error:          Not equal:  
                                expected: true
                                actual  : false
                Test:           TestACLs
                Messages:       test[setted port port security]
    ovnnbimp_test.go:85:
                Error Trace:    ovnnbimp_test.go:85
                Error:          Not equal:  
                                expected: true
                                actual  : false
                Test:           TestACLs
                Messages:       test[added 2 ports]: []
panic: runtime error: index out of range [recovered]
        panic: runtime error: index out of range

Exposing IPSec enabled/disabled API

Hey all,
I'd love to have an API in this lib to check if IPsec is enabled. Its available as a bool in nb_global table.
Any interest in exposing that?

Thank you,

Tag version and ACLList miss meter and severity filed.

Currently master branch may dependent by orther project , I think it should tag like v1.0.0 for better go mod support?

acl list can't show meter and severity .

go-ovn/acl.go

Lines 24 to 31 in ca0da4d

	type ACL struct {
	UUID string
	Action string
	Direction string
	Match string
	Priority int
	Log bool
	ExternalID map[interface{}]interface{}

it miss meter filed

_uuid               : f8a9eb74-2a0c-48ad-b858-fb922e41fcd3
action              : drop
direction           : to-lport
external_ids        : {}
log                 : true
match               : "outport == \"96d44061-1823-428b-a7ce-f473d10eb3d0\" && ip && ip.dst == 10.97.183.61"
meter               : "meterTest1"
name                : []
priority            : 1001
severity            : []

Meter and Meter bands method design problem.

I try to write meter and meter bands , now the problem is ovn-nbctl can only set one bands of each meter . and i see doc of ovn-nbctl said other ctl can set mutpli bands in one meter . So I wonder if we should follow ovn-nbctl or write own method to set meter and meter bands ?

you can check this patch , if anything ok , I will send a pr.
JackyCZJ@76852e9