Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add vnet param to routes #912

Merged
merged 3 commits into from Jun 2, 2022
Merged

Conversation

vavsab
Copy link
Contributor

@vavsab vavsab commented May 27, 2022

Description

I'm trying to build VPN for my company based on Cloudflare, but cloudflare terraform package is missing vnet param. I need to add it here first to update cloudflare terraform package.

Has your change been tested?

Covered with unit tests. Have not tried to run it yet. But will run as soon as we merge it. Cloudlare has no docs for this yet. I tested in Insomnia that all calls work correctly.

Types of changes

What sort of change does your code introduce/modify?

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly. How to do it??? Could you please give me the link when the docs are stored?
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • This change is using publicly documented (api.cloudflare.com or developers.cloudflare.com) and stable APIs. It's not documented. But cloudflared CLI is already using it.

Resolves #911
Unblocks cloudflare/terraform-provider-cloudflare#1605

@vavsab
Copy link
Contributor Author

vavsab commented May 28, 2022

Hi @jacobbednarz. Is there a way I can speed up changes in api docs?
My company needs this change asap to unblock terraform provider changes.

I could make the changes but it looks like api docs have no public repo for this.

If we could buy paid support to speed it up I would really appreciate it.

@jacobbednarz
Copy link
Member

I can see if the service team have a ticket tracking this. If you have an account team, you can always raise it with them as well.

If you need it immediately, there is nothing stopping you from running whatever combination of dependencies and code by building it yourself. See https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/docs/using-non-released-versions.md

@vavsab
Copy link
Contributor Author

vavsab commented May 30, 2022

I can see if the service team have a ticket tracking this. If you have an account team, you can always raise it with them as well.

If you need it immediately, there is nothing stopping you from running whatever combination of dependencies and code by building it yourself. See https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/docs/using-non-released-versions.md

@jacobbednarz Yeah. This is an option. However it's a bit hard. I'm using pulumi_cloudflare_package -> terraform_cloudflare_package -> cloudflare-go. The dependency is so deep that I would better just make direct API calls than fork all these repos and setup CI/CD for them.

If adding 1 existing param to API docs takes a week, I'm ok to wait. If longer then I won't use terraform package at all.
If I can help with adding the docs, please let me know. I will be glad write all the code and make a PR. I just have not found the right repo. Looks like it's private.

tunnel_routes.go Outdated
@@ -114,7 +120,7 @@ func (api *API) GetTunnelRouteForIP(ctx context.Context, params TunnelRoutesForI

uri := fmt.Sprintf("/%s/%s/teamnet/routes/ip/%s", AccountRouteRoot, params.AccountID, params.Network)

responseBody, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)
responseBody, err := api.makeRequestContext(ctx, http.MethodGet, uri, params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be nil - we don't send a request body with GET requests.

Suggested change
responseBody, err := api.makeRequestContext(ctx, http.MethodGet, uri, params)
responseBody, err := api.makeRequestContext(ctx, http.MethodGet, uri, nil)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed get param. But I still need virtual_network_id there.
Here is an example:
I have created 2 routes with different comment & virtual_network_id
image

You may see here that by default it uses vnet with id 9b818e6e-4998-4428-930c-c2e319518a6f because it's a default one for my account.
image

Without virtual_network_id this request would not be possible:
image

cloudflared is already using it here

}
}`)
}

mux.HandleFunc("/accounts/"+testAccountID+"/teamnet/routes/network/10.0.0.0/16", handler)
err := client.DeleteTunnelRoute(context.Background(), TunnelRoutesDeleteParams{AccountID: testAccountID, Network: "10.0.0.0/16"})
err := client.DeleteTunnelRoute(context.Background(), TunnelRoutesDeleteParams{AccountID: testAccountID, Network: "10.0.0.0/16", VirtualNetworkID: "9f322de4-5988-4945-b770-f1d6ac200f86"})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to send VirtualNetworkID for the Delete, it is only addressable by the Network parameter.

Copy link
Contributor Author

@vavsab vavsab Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question here. For example,
I have vnetDev and vnetProd. Both of them have "10.0.0.0/16" routes that lead to the corresponding dev & prod tunnels.

How do I remove only vnetProd route without specifying VirtualNetworkID?

It's not some specific non-real case. I will have exactly this situation on my environment after we merge this PR.

Delete without VirtualNetworkID only removes it for default network.

Copy link
Contributor Author

@vavsab vavsab Jun 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah.. I see that it's not reflected in docs. You may test it in Postman/insomnia as I did. I couldn't remove the route from non-default vnet until I explicitly specified vnetid as Get param.

I guess we need to update api docs s bit. It's super important for those folks who use vnets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cloudflared is already using it here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha, one to raise with a support ticket if you can in order to get the docs reflecting the correct state of affairs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Purchased paid cloudflare plan and created a support ticket to change the docs.

Network string `json:"-"`
AccountID string `json:"-"`
Network string `json:"-"`
VirtualNetworkID string `url:"virtual_network_id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
VirtualNetworkID string `url:"virtual_network_id,omitempty"`

Network string `json:"-"`
AccountID string `json:"-"`
Network string `json:"-"`
VirtualNetworkID string `url:"virtual_network_id,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
VirtualNetworkID string `url:"virtual_network_id,omitempty"`

@jacobbednarz
Copy link
Member

the team has shipped the API documentation for this to api.cloudflare.com so that's unblocked us there. we just need to fix up these last few things (overuse of VirtualNetworkID in params mostly) and we're good to ship.

@vavsab vavsab requested a review from jacobbednarz June 1, 2022 11:40
@jacobbednarz jacobbednarz merged commit 6687785 into cloudflare:master Jun 2, 2022
@jacobbednarz
Copy link
Member

thanks for getting this over the line! appreciate it!

@vavsab
Copy link
Contributor Author

vavsab commented Jun 2, 2022

@jacobbednarz How long does it take to wait for release? I want to bump this package in https://github.com/cloudflare/terraform-provider-cloudflare.

@jacobbednarz
Copy link
Member

jacobbednarz commented Jun 2, 2022

releases are fortnightly. you can check out the latest release date to estimate when the next one is due.

cloudflare-go and the terraform provider alternate weeks for releases.

dependency upgrades are automatic in the terraform provider, you don't need to do anything for it to be picked up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add vnet (virtual network) param for routes
3 participants