Multiple issues with the gcp terraform templates
tactical-retreat opened this issue · 3 comments
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:
p2pportinstead ofp2p_portbucket_nameis referenced in a few place that are irrelevant (e.g. declared in the node module, unused)ip_cidr_rangeis used in one place, hardcoded in another- reference to
"../../../modules/vpc"has an extra../
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.
Appreciate the callouts. If you have any more edits to suggest please let us know.
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:
The declaration with the correct name is here:
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.
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.