-
Notifications
You must be signed in to change notification settings - Fork 3
fix: update Terraform examples #94
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
Conversation
kirklandnuts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for updating these @wilderj
| region = <region1> | ||
| # Set the project name for where the scanning resources are hosted. | ||
| # This must be assigned to the `global` region. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, does this mean that region in this block must be set to "global"? If so, why not just set region = "global" instead of region = <region1>?
Otherwise, does it mean that the GCP project where the customer wants to deploy scanning resources must be assigned to the global region in GCP?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It refers to the per-region blocks below that actually define our module, like lacework_gcp_agentless_scanning_project_multi_region_<alias1>.
Exactly one of these blocks must have global = true, since we create "global" resources that still have to be created in a particular region.
Since those resources must also live in the scanning account, this comment is saying you have to use this provider in whichever module uses global = true. Does that make sense? Not sure if I am explaining this well. I also lifted this comment directly from the example in the FortiCNAPP docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this makes total sense - thanks for explaining!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following changes would help reduce confusion:
- Instead of having this comment right above the project assignment statement, move it to apply to the entire provider block.
- Clarify the comment to be more aligned with your explanatory comment above.
/*
This provider will be used to deploy AWLS's scanning resources. As such, it must be assigned as
the provider in the per-region AWLS module block where `global == true`.
For reference, see module "lacework_gcp_agentless_scanning_project_multi_region_<alias1>", which
has `global = true` and therefore is where we set this provider as the google provider.
*/
provider "google" {
alias = <alias1>
# Set the region in which the scanning resources will be hosted.
region = <region1>
# Set the project in which the scanning resources will be hosted.
project = <your-project-id>
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beyond that, perhaps we can even be opinionated about the naming for this specific global provider/AWLS module - for example:
provider "google" {
alias = "scanning_resources"
...
}
...
module "lacework_gcp_agentless_scanning_project_multi_region_scanning_resources" {
...
providers = {
google = google.scanning_resources
}
...
global = true
...
}Though I'm not sure if scanning_resources is the right choice here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the inline comment explaining in more detail is a good idea. Just applied that to all of the relevant examples.
I held off on aliasing the module to something like scanning_resources or similar because I think this can actually cause additional confusion. Technically every module is for scanning resources, but the one with global = true will just also contain all of the once-per-integration resources as well. But I'd be hesitant to rename it to global_scanning_resources since it will also contain the per-region resources.
a69fc0f to
426f03d
Compare
426f03d to
7fbe82a
Compare
Summary
The examples in this repo were a bit out of date, and they no longer match the examples given in the Fortinet documentation. This PR cleans things up. In particular, this changeset:
versions.tfprojectfield, since this is now requiredSpecial focus was given to getting the README files up-to-date since in the near future, the Fortinet documentation won't contain Terraform examples at all; the docs will simply point to these READMEs on https://registry.terraform.io/modules/lacework/agentless-scanning/gcp/latest.
How did you test this change?
I test deployed both a project level and and organization level integration using the examples with these changes in place. In both cases,
terraform applywas successful and the integration appears properly configured.Issue
https://lacework.atlassian.net/browse/AWLS2-550