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

Fix numeric indexes for tunnels and subnets #43

Merged

Conversation

gtrog
Copy link
Contributor

@gtrog gtrog commented Aug 28, 2020

This PR fixes compute route resource naming when tunnel_count is greater than 1.

Related issue:

Fixes #38.

To give more context what this PR is doing:

Originally, the module is trying to name and number the routes by tunnel number and subnet number, something like gateway-route-1-2, where 1 here is the tunnel number and 2 is the subnet number.

To do so, it first computes the total number of routes (#tunnels * #subnets), and then tries to divvy that number up and number them, but it does this part incorrectly. In the old way, it was setting tunnel index to the index % # tunnels + 1, and subnet index to index % # subnets + 1, which if you have 2 tunnels and 2 subets, would lead to this result:

count = 2 tunnels * 2 subnets = 4

index | index % 2 (tunnels) + 1 | index % 2 (subnets) + 1
0     |                       1 |                       1
1     |                       2 |                       2
2     |                       1 |                       1
3     |                       2 |                       2

As you can see, you end up with duplicates. The correct way to go about this (which is what Jonathan is doing here), is to do effectively long division by some divisor (either # tunnels or # subnets), where the whole number is one part, and the remainder is the other part. So, in this code, he's choosing # subnets as the divisor, so, dividing by it for the tunnel index, and then modulo it for the subnet index. Doing this gives the intended behavior:

count = 2 tunnels * 2 subnets = 4

index | floor(index / 2 (subnets)) + 1 | index % 2 (subnets) + 1
0     |                              1 |                       1
1     |                              1 |                       2
2     |                              2 |                       1
3     |                              2 |                       2

Another example:

count = 2 tunnels * 1 subnets = 2

index | floor(index / 1 (subnets)) + 1 | index % 1 (subnets) + 1
0     |                              1 |                       1
1     |                              2 |                       1

We're always dividing by number of subnets, so if it's just 1, then 0 / 1 = 0 + 1 = 1, and for the 2nd one, 1 / 1 = 1 + 1 = 2, while anything modulo 1 will always be 0, +1 = 1

@gtrog
Copy link
Contributor Author

gtrog commented Aug 28, 2020

I managed to get the integration tests running locally via make docker_test_integration:

Profile Summary: 1 successful control, 0 control failures, 0 controls skipped
Test Summary: 18 successful, 0 failures, 0 skipped
...
-----> Kitchen is finished. (3m31.18s)

This is on Windows 10. One issue that I did run into (which I wasn't sure if it was how my base GCP enviroment was set up) was it setting the billing account:

Error: Error setting billing account "billingAccounts/012345-DEDBEF-012345" for project "projects/ci-vpn-51c0": googleapi: Error 403: Cloud Billing API has not been used in project 123456789012 before or it is disabled. Enable it by visiting https://console.developers.google.com/apis/api/cloudbilling.googleapis.com/overview?project=123456789012 then retry. If you enabled this API recently, wait a few minutes for the action to propagate to our systems and retry., accessNotConfigured

I had to get around this issue by setting TF_CLI_ARGS_apply="-parallelism=1" in the Makefile for the docker_test_prepare target, then, it seemed to resolve the issue, I'm guessing it's a race condition. I'm not sure why the integration tests for the pull request are failing, since I can't see the cloud build output.

@morgante morgante merged commit a78f08d into terraform-google-modules:master Aug 28, 2020
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.

Two or more tunnels lead to double route entries leading to a Terraform failed job
2 participants