From 6d1d91d9f98a0a565d9d259640bb3278f8771f57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Oliveirinha?= Date: Fri, 15 Sep 2023 15:17:23 +0100 Subject: [PATCH] TUN-7787: Refactor cloudflared to use new route endpoints based on route IDs This commits makes sure that cloudflared starts using the new API endpoints for managing routes. Additionally, the delete route operation still allows deleting by CIDR and VNet but it is being marked as deprecated in favor of specifying the route ID. The goal of this change is to make it simpler for the user to delete routes without specifying Vnet. --- cfapi/client.go | 2 +- cfapi/ip_route.go | 19 ++++---- cfapi/ip_route_filter.go | 38 ++++++++-------- cfapi/ip_route_test.go | 5 ++- .../tunnel/subcommand_context_teamnet.go | 29 +++++++++++- cmd/cloudflared/tunnel/teamnet_subcommands.go | 45 ++++++++++--------- 6 files changed, 84 insertions(+), 54 deletions(-) diff --git a/cfapi/client.go b/cfapi/client.go index f8c2a734..0431ad53 100644 --- a/cfapi/client.go +++ b/cfapi/client.go @@ -22,7 +22,7 @@ type HostnameClient interface { type IPRouteClient interface { ListRoutes(filter *IpRouteFilter) ([]*DetailedRoute, error) AddRoute(newRoute NewRoute) (Route, error) - DeleteRoute(params DeleteRouteParams) error + DeleteRoute(id uuid.UUID) error GetByIP(params GetRouteByIpParams) (DetailedRoute, error) } diff --git a/cfapi/ip_route.go b/cfapi/ip_route.go index 6749aa89..a45c00bf 100644 --- a/cfapi/ip_route.go +++ b/cfapi/ip_route.go @@ -75,10 +75,12 @@ type NewRoute struct { // MarshalJSON handles fields with non-JSON types (e.g. net.IPNet). func (r NewRoute) MarshalJSON() ([]byte, error) { return json.Marshal(&struct { + Network string `json:"network"` TunnelID uuid.UUID `json:"tunnel_id"` Comment string `json:"comment"` VNetID *uuid.UUID `json:"virtual_network_id,omitempty"` }{ + Network: r.Network.String(), TunnelID: r.TunnelID, Comment: r.Comment, VNetID: r.VNetID, @@ -87,6 +89,7 @@ func (r NewRoute) MarshalJSON() ([]byte, error) { // DetailedRoute is just a Route with some extra fields, e.g. TunnelName. type DetailedRoute struct { + ID uuid.UUID `json:"id"` Network CIDR `json:"network"` TunnelID uuid.UUID `json:"tunnel_id"` // Optional field. When unset, it means the DetailedRoute belongs to the default virtual network. @@ -115,7 +118,8 @@ func (r DetailedRoute) TableString() string { } return fmt.Sprintf( - "%s\t%s\t%s\t%s\t%s\t%s\t%s\t", + "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t", + r.ID, r.Network.String(), vnetColumn, r.Comment, @@ -126,12 +130,6 @@ func (r DetailedRoute) TableString() string { ) } -type DeleteRouteParams struct { - Network net.IPNet - // Optional field. If unset, backend will assume the default vnet for the account. - VNetID *uuid.UUID -} - type GetRouteByIpParams struct { Ip net.IP // Optional field. If unset, backend will assume the default vnet for the account. @@ -158,7 +156,7 @@ func (r *RESTClient) ListRoutes(filter *IpRouteFilter) ([]*DetailedRoute, error) // AddRoute calls the Tunnelstore POST endpoint for a given route. func (r *RESTClient) AddRoute(newRoute NewRoute) (Route, error) { endpoint := r.baseEndpoints.accountRoutes - endpoint.Path = path.Join(endpoint.Path, "network", url.PathEscape(newRoute.Network.String())) + endpoint.Path = path.Join(endpoint.Path) resp, err := r.sendRequest("POST", endpoint, newRoute) if err != nil { return Route{}, errors.Wrap(err, "REST request failed") @@ -173,10 +171,9 @@ func (r *RESTClient) AddRoute(newRoute NewRoute) (Route, error) { } // DeleteRoute calls the Tunnelstore DELETE endpoint for a given route. -func (r *RESTClient) DeleteRoute(params DeleteRouteParams) error { +func (r *RESTClient) DeleteRoute(id uuid.UUID) error { endpoint := r.baseEndpoints.accountRoutes - endpoint.Path = path.Join(endpoint.Path, "network", url.PathEscape(params.Network.String())) - setVnetParam(&endpoint, params.VNetID) + endpoint.Path = path.Join(endpoint.Path, url.PathEscape(id.String())) resp, err := r.sendRequest("DELETE", endpoint, nil) if err != nil { diff --git a/cfapi/ip_route_filter.go b/cfapi/ip_route_filter.go index 398a9982..d68da323 100644 --- a/cfapi/ip_route_filter.go +++ b/cfapi/ip_route_filter.go @@ -58,31 +58,29 @@ type IpRouteFilter struct { // NewIpRouteFilterFromCLI parses CLI flags to discover which filters should get applied. func NewIpRouteFilterFromCLI(c *cli.Context) (*IpRouteFilter, error) { - f := &IpRouteFilter{ - queryParams: url.Values{}, - } + f := NewIPRouteFilter() // Set deletion filter if flag := filterIpRouteDeleted.Name; c.IsSet(flag) && c.Bool(flag) { - f.deleted() + f.Deleted() } else { - f.notDeleted() + f.NotDeleted() } if subset, err := cidrFromFlag(c, filterSubsetIpRoute); err != nil { return nil, err } else if subset != nil { - f.networkIsSupersetOf(*subset) + f.NetworkIsSupersetOf(*subset) } if superset, err := cidrFromFlag(c, filterSupersetIpRoute); err != nil { return nil, err } else if superset != nil { - f.networkIsSupersetOf(*superset) + f.NetworkIsSupersetOf(*superset) } if comment := c.String(filterIpRouteComment.Name); comment != "" { - f.commentIs(comment) + f.CommentIs(comment) } if tunnelID := c.String(filterIpRouteTunnelID.Name); tunnelID != "" { @@ -90,7 +88,7 @@ func NewIpRouteFilterFromCLI(c *cli.Context) (*IpRouteFilter, error) { if err != nil { return nil, errors.Wrapf(err, "Couldn't parse UUID from %s", filterIpRouteTunnelID.Name) } - f.tunnelID(u) + f.TunnelID(u) } if vnetId := c.String(filterIpRouteByVnet.Name); vnetId != "" { @@ -98,7 +96,7 @@ func NewIpRouteFilterFromCLI(c *cli.Context) (*IpRouteFilter, error) { if err != nil { return nil, errors.Wrapf(err, "Couldn't parse UUID from %s", filterIpRouteByVnet.Name) } - f.vnetID(u) + f.VNetID(u) } if maxFetch := c.Int("max-fetch-size"); maxFetch > 0 { @@ -124,35 +122,39 @@ func cidrFromFlag(c *cli.Context, flag cli.StringFlag) (*net.IPNet, error) { return subset, nil } -func (f *IpRouteFilter) commentIs(comment string) { +func NewIPRouteFilter() *IpRouteFilter { + return &IpRouteFilter{queryParams: url.Values{}} +} + +func (f *IpRouteFilter) CommentIs(comment string) { f.queryParams.Set("comment", comment) } -func (f *IpRouteFilter) notDeleted() { +func (f *IpRouteFilter) NotDeleted() { f.queryParams.Set("is_deleted", "false") } -func (f *IpRouteFilter) deleted() { +func (f *IpRouteFilter) Deleted() { f.queryParams.Set("is_deleted", "true") } -func (f *IpRouteFilter) networkIsSubsetOf(superset net.IPNet) { +func (f *IpRouteFilter) NetworkIsSubsetOf(superset net.IPNet) { f.queryParams.Set("network_subset", superset.String()) } -func (f *IpRouteFilter) networkIsSupersetOf(subset net.IPNet) { +func (f *IpRouteFilter) NetworkIsSupersetOf(subset net.IPNet) { f.queryParams.Set("network_superset", subset.String()) } -func (f *IpRouteFilter) existedAt(existedAt time.Time) { +func (f *IpRouteFilter) ExistedAt(existedAt time.Time) { f.queryParams.Set("existed_at", existedAt.Format(time.RFC3339)) } -func (f *IpRouteFilter) tunnelID(id uuid.UUID) { +func (f *IpRouteFilter) TunnelID(id uuid.UUID) { f.queryParams.Set("tunnel_id", id.String()) } -func (f *IpRouteFilter) vnetID(id uuid.UUID) { +func (f *IpRouteFilter) VNetID(id uuid.UUID) { f.queryParams.Set("virtual_network_id", id.String()) } diff --git a/cfapi/ip_route_test.go b/cfapi/ip_route_test.go index 93fa45ea..235b8673 100644 --- a/cfapi/ip_route_test.go +++ b/cfapi/ip_route_test.go @@ -69,6 +69,7 @@ func TestDetailedRouteJsonRoundtrip(t *testing.T) { }{ { `{ + "id":"91ebc578-cc99-4641-9937-0fb630505fa0", "network":"10.1.2.40/29", "tunnel_id":"fba6ffea-807f-4e7a-a740-4184ee1b82c8", "comment":"test", @@ -80,6 +81,7 @@ func TestDetailedRouteJsonRoundtrip(t *testing.T) { }, { `{ + "id":"91ebc578-cc99-4641-9937-0fb630505fa0", "network":"10.1.2.40/29", "tunnel_id":"fba6ffea-807f-4e7a-a740-4184ee1b82c8", "virtual_network_id":"38c95083-8191-4110-8339-3f438d44fdb9", @@ -167,9 +169,10 @@ func TestRouteTableString(t *testing.T) { require.NoError(t, err) require.NotNil(t, network) r := DetailedRoute{ + ID: uuid.Nil, Network: CIDR(*network), } row := r.TableString() fmt.Println(row) - require.True(t, strings.HasPrefix(row, "1.2.3.4/32")) + require.True(t, strings.HasPrefix(row, "00000000-0000-0000-0000-000000000000\t1.2.3.4/32")) } diff --git a/cmd/cloudflared/tunnel/subcommand_context_teamnet.go b/cmd/cloudflared/tunnel/subcommand_context_teamnet.go index 4605172c..55faa82a 100644 --- a/cmd/cloudflared/tunnel/subcommand_context_teamnet.go +++ b/cmd/cloudflared/tunnel/subcommand_context_teamnet.go @@ -1,6 +1,9 @@ package tunnel import ( + "net" + + "github.com/google/uuid" "github.com/pkg/errors" "github.com/cloudflare/cloudflared/cfapi" @@ -24,12 +27,12 @@ func (sc *subcommandContext) addRoute(newRoute cfapi.NewRoute) (cfapi.Route, err return client.AddRoute(newRoute) } -func (sc *subcommandContext) deleteRoute(params cfapi.DeleteRouteParams) error { +func (sc *subcommandContext) deleteRoute(id uuid.UUID) error { client, err := sc.client() if err != nil { return errors.Wrap(err, noClientMsg) } - return client.DeleteRoute(params) + return client.DeleteRoute(id) } func (sc *subcommandContext) getRouteByIP(params cfapi.GetRouteByIpParams) (cfapi.DetailedRoute, error) { @@ -39,3 +42,25 @@ func (sc *subcommandContext) getRouteByIP(params cfapi.GetRouteByIpParams) (cfap } return client.GetByIP(params) } + +func (sc *subcommandContext) getRouteId(network net.IPNet, vnetId *uuid.UUID) (uuid.UUID, error) { + filters := cfapi.NewIPRouteFilter() + filters.NotDeleted() + filters.NetworkIsSubsetOf(network) + filters.NetworkIsSupersetOf(network) + + if vnetId != nil { + filters.VNetID(*vnetId) + } + + result, err := sc.listRoutes(filters) + if err != nil { + return uuid.Nil, err + } + + if len(result) != 1 { + return uuid.Nil, errors.New("unable to find route for provided network and vnet") + } + + return result[0].ID, nil +} diff --git a/cmd/cloudflared/tunnel/teamnet_subcommands.go b/cmd/cloudflared/tunnel/teamnet_subcommands.go index 186efdb1..b46f52e1 100644 --- a/cmd/cloudflared/tunnel/teamnet_subcommands.go +++ b/cmd/cloudflared/tunnel/teamnet_subcommands.go @@ -21,6 +21,8 @@ var ( Aliases: []string{"vn"}, Usage: "The ID or name of the virtual network to which the route is associated to.", } + + routeAddError = errors.New("You must supply exactly one argument, the ID or CIDR of the route you want to delete") ) func buildRouteIPSubcommand() *cli.Command { @@ -68,11 +70,9 @@ which virtual network's routing table you want to add the route to with: Name: "delete", Action: cliutil.ConfiguredAction(deleteRouteCommand), Usage: "Delete a row from your organization's private routing table", - UsageText: "cloudflared tunnel [--config FILEPATH] route ip delete [flags] [CIDR]", - Description: `Deletes the row for a given CIDR from your routing table. That portion of your network -will no longer be reachable by the WARP clients. Note that if you use virtual -networks, then you have to tell which virtual network whose routing table you -have a row deleted from.`, + UsageText: "cloudflared tunnel [--config FILEPATH] route ip delete [flags] [Route ID or CIDR]", + Description: `Deletes the row for the given route ID from your routing table. That portion of your network +will no longer be reachable.`, Flags: []cli.Flag{vnetFlag}, }, { @@ -187,33 +187,36 @@ func deleteRouteCommand(c *cli.Context) error { } if c.NArg() != 1 { - return errors.New("You must supply exactly one argument, the network whose route you want to delete (in CIDR form e.g. 1.2.3.4/32)") + return routeAddError } - _, network, err := net.ParseCIDR(c.Args().First()) + var routeId uuid.UUID + routeId, err = uuid.Parse(c.Args().First()) if err != nil { - return errors.Wrap(err, "Invalid network CIDR") - } - if network == nil { - return errors.New("Invalid network CIDR") - } + _, network, err := net.ParseCIDR(c.Args().First()) + if err != nil || network == nil { + return routeAddError + } - params := cfapi.DeleteRouteParams{ - Network: *network, - } + var vnetId *uuid.UUID + if c.IsSet(vnetFlag.Name) { + id, err := getVnetId(sc, c.String(vnetFlag.Name)) + if err != nil { + return err + } + vnetId = &id + } - if c.IsSet(vnetFlag.Name) { - vnetId, err := getVnetId(sc, c.String(vnetFlag.Name)) + routeId, err = sc.getRouteId(*network, vnetId) if err != nil { return err } - params.VNetID = &vnetId } - if err := sc.deleteRoute(params); err != nil { + if err := sc.deleteRoute(routeId); err != nil { return errors.Wrap(err, "API error") } - fmt.Printf("Successfully deleted route for %s\n", network) + fmt.Printf("Successfully deleted route with ID %s\n", routeId) return nil } @@ -269,7 +272,7 @@ func formatAndPrintRouteList(routes []*cfapi.DetailedRoute) { defer writer.Flush() // Print column headers with tabbed columns - _, _ = fmt.Fprintln(writer, "NETWORK\tVIRTUAL NET ID\tCOMMENT\tTUNNEL ID\tTUNNEL NAME\tCREATED\tDELETED\t") + _, _ = fmt.Fprintln(writer, "ID\tNETWORK\tVIRTUAL NET ID\tCOMMENT\tTUNNEL ID\tTUNNEL NAME\tCREATED\tDELETED\t") // Loop through routes, create formatted string for each, and print using tabwriter for _, route := range routes {