[networking]: add BGP VPNs support #3078
Conversation
|
I don't know what can we do with the |
| type ListOpts struct { | ||
| Fields []string `q:"fields"` | ||
| ProjectID string `q:"project_id"` | ||
| Networks []string `q:"networks"` |
There was a problem hiding this comment.
According to the doc, the networks, routers, and ports are arrays. Do you know how they should be represented in the query? Are they repeated like fields is, or should we use a different format, such as comma-separated?
There was a problem hiding this comment.
It should be this way: ?fields=name&fields=id&networks=UUID1&networks=UUID2
The response should return name and id of the resources, which correspond to networks=UUID1 or networks=UUID2 filter.
| return | ||
| } | ||
| resp, err := c.Put(ctx, updateURL(c, id), b, &r.Body, &gophercloud.RequestOpts{ | ||
| OkCodes: []int{200}, |
There was a problem hiding this comment.
Doc says we're expecting 201 on success, can we double check this is correct?
There was a problem hiding this comment.
201 is not returned. You can confirm this in functional tests.
| // router association. | ||
| type CreateRouterAssociationOpts struct { | ||
| RouterID string `json:"router_id" required:"true"` | ||
| AdvertiseExtraRoutes *bool `json:"advertise_extra_routes,omitempty"` |
There was a problem hiding this comment.
Funny, this field is undocumented.
There was a problem hiding this comment.
It's documented in PUT method. I've just checked whether project_id can be set as well for cases, when admins need to create a resource. I'll add this option as well. Otherwise cloud admins will get Request forbidden: [POST https://network-3.qa-de-1.cloud.sap/v2.0/bgpvpn/bgpvpns/920b4f41-6553-4ec5-ad87-f3bf156a7650/router_associations], error message: {"NeutronError": {"type": "NotAuthorized", "message": "Not authorized.", "detail": ""}}
| return | ||
| } | ||
| resp, err := client.Put(ctx, updatePortAssociationURL(client, bgpVpnID, id), b, &r.Body, &gophercloud.RequestOpts{ | ||
| OkCodes: []int{200}, |
There was a problem hiding this comment.
Here too, doc says the successful http code is 201. Which is correct?
There was a problem hiding this comment.
The correct is the one, which is returned by API and passes functional tests :)
| Type string `json:"type"` | ||
|
|
||
| // Indicates whether this BGP VPN is shared across tenants. | ||
| Shared bool `json:"shared"` |
There was a problem hiding this comment.
Again, this field is undocumented.
There was a problem hiding this comment.
It's a read-only field. It's set to true, when rbac rules are used.
|
|
||
| // The default BGP LOCAL_PREF of routes that will be advertised to the | ||
| // BGPVPN (unless overridden per-route). | ||
| LocalPref *int `json:"local_pref"` |
There was a problem hiding this comment.
Because we need to have an ability to set it to 0 on update.
| type PortAssociation struct { | ||
| ID string `json:"id"` | ||
| PortID string `json:"port_id"` | ||
| TenantID string `json:"tenant_id"` |
There was a problem hiding this comment.
API returns these attributes.
| } | ||
| bgpVpnUpdated, err := bgpvpns.Update(context.TODO(), client, bgpVpnGot.ID, updateBGPOpts).Extract() | ||
| th.AssertNoErr(t, err) | ||
| th.AssertEquals(t, bgpVpnUpdated.Name, newBGPVPNName) |
There was a problem hiding this comment.
The arguments should be reversed. It's expected, actual.
There was a problem hiding this comment.
You have the same issue in a few places.
There was a problem hiding this comment.
true, I always mix them up. Will fix soon.
|
@mandre addressed all your comments. |
|
@mandre thanks, merging |
Fixes #2209
Links to the line numbers/files in the OpenStack source code that support the
code in this PR: