Multiple issues with the gcp terraform templates

[tactical-retreat]

Describe the bug

There are a number of problems with the GCP terraform templates. I fixed a few of them as I went along, making other changes, so I don't have a comprehensive list. But I'll note a few that I can quickly pull out of my diff:

• p2pport instead of p2p_port
• bucket_name is referenced in a few place that are irrelevant (e.g. declared in the node module, unused)
• ip_cidr_range is used in one place, hardcoded in another
• reference to "../../../modules/vpc" has an extra ../

[meaghanfitzgerald]

Hey Tactical, thanks for raising this issue.

I'm assuming these comments refer to these example terraform files referenced in this tutorial in our docs.

To the points you made above:

- p2pport instead of p2p_port

All the port variables in this repository are consistently named p2p_port. AFAICT, so long as this is defined consistently it shouldn't affect the deployment.

- bucket_name is referenced in a few place that are irrelevant (e.g. declared in the node module, unused)

This may have been included by the original writers of this tutorial as a way to provide the reader more clarity. Functionally, it shouldn't affect anything.

- ip_cidr_range is used in one place, hardcoded in another

Yes, I found what you were referring to in terraform-gcp/modules/vpc/main.tf. I will raise a PR to correct, unless you'd like to do so and get credit as a contributor.

- reference to "../../../modules/vpc" has an extra ../

Good catch, can also include in the above PR.

[meaghanfitzgerald]

Appreciate the callouts. If you have any more edits to suggest please let us know.

[tactical-retreat]

All the port variables in this repository are consistently named p2p_port. AFAICT, so long as this is defined consistently it shouldn't affect the deployment.

It's incorrectly used here:

https://github.com/ava-labs/avalanche-docs/blob/master/static/scripts/terraform-gcp/modules/node/main.tf#L124

The declaration with the correct name is here:

https://github.com/ava-labs/avalanche-docs/blob/master/static/scripts/terraform-gcp/modules/node/variables.tf#L36

This may have been included by the original writers of this tutorial as a way to provide the reader more clarity. Functionally, it shouldn't affect anything.

I disagree it adds clarity. Why would a GCE instance need a bucket? It's declared, and provided, but unused.

https://github.com/ava-labs/avalanche-docs/blob/master/static/scripts/terraform-gcp/modules/node/variables.tf#L46

I will raise a PR to correct, unless you'd like to do so and get credit as a contributor.

I don't need credit, you can PR the changes.

I have a few other notes, that are more of 'nice to haves' that you could consider if you plan to enhance this repo, although from what I can tell the focus is on the avalanche-cli support. Maybe it should be considered there?

• The current code has the option to spawn multiple nodes in the same region. This is a violation of cloud best practices, a better example would be to spawn them at a minimum in different zones, preferably in different regions.
• The code provisions a larger boot disk for the chain state to live on, it would be better to provision an additional disk and put it there.
• The size of the boot disk selected is excessive.
• The startup script could be enhanced to run the avalanchego installer script.
• Best practices are to run VMs with a locked down service account, the default compute service account is too broad.