feat: initial commit for DA #915
Conversation
solutions/simple/variables.tf
Outdated
| ############################################################################## | ||
|
|
||
| variable "network_acls" { | ||
| description = "The list of ACLs to create. Provide at least one rule for each ACL." |
There was a problem hiding this comment.
We should add a supporting markdown doc
There was a problem hiding this comment.
will add, once code is completed.
I created this draft PR with the intent of early review. Thanks for this. Yes, name will be changed once we decide. |
|
Currently,
|
There was a problem hiding this comment.
We really need to make use of cross variable validation in the variables.tf file. For example, if person sets kms_encryption_enabled_bucket or enable_vpc_flow_logs then other inputs become required.
Also question - should we support creating VPEs? Or are we planning to have a standalone VPE DA? (cc @vburckhardt )
|
@maheshwarishikha FYI these are the variation names we will go with:
|
There was a problem hiding this comment.
- As per our most recent discussions, we should no longer support creating a resource group, but instead we should require an existing resource group name (using input
existing_resource_group_name).- We can still use our terraform-ibm-resource-group module in the code to return the resource group ID.
- We should use the catalog resource group picker in the ibm_catalog.json for this input:
{ "key": "existing_resource_group_name", "custom_config": { "type": "resource_group", "grouping": "deployment", "original_grouping": "deployment", "config_constraints": { "identifier": "rg_name" } } }
- Add support for VPE (using https://github.com/terraform-ibm-modules/terraform-ibm-vpe-gateway)
- Add support for VPN (see current code at https://github.com/terraform-ibm-modules/terraform-ibm-landing-zone/blob/main/vpn.tf)
| } | ||
|
|
||
| variable "kms_encryption_enabled_bucket" { | ||
| description = "Set to true if Cloud Object Storage bucket needs to be KMS encryption enabled." |
There was a problem hiding this comment.
Set to true to encrypt the Cloud Object Storage Flow Logs bucket with a KMS key. If set to true, a value must be passed for existing_kms_key_crn (to use that key) or existing_kms_instance_crn (to create a new key). Value cannot be set to true if enable_vpc_flow_logs is set to false.
| # KMS | ||
| ############################################################################################################### | ||
|
|
||
| variable "existing_kms_key_crn" { |
There was a problem hiding this comment.
I actually suggest you name this existing_flow_logs_bucket_kms_key_crn so it is clear what the key is used for. Maybe some day there be something else in this DA that will need its own KMS key, so better to future proof it
| variable "provider_visibility" { | ||
| description = "Set the visibility value for the IBM terraform provider. Supported values are `public`, `private`, `public-and-private`. [Learn more](https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/guides/custom-service-endpoints)." | ||
| type = string | ||
| default = "public" |
| variable "prefix" { | ||
| type = string | ||
| nullable = true | ||
| description = "Prefix to add to all the resources created by this solution. To not use any prefix value, you can set this value to `null` or an empty string." |
There was a problem hiding this comment.
since this DA support creating a COS bucket, you should add extra validation on the value. See https://github.com/terraform-ibm-modules/terraform-ibm-scc-da/blob/49ba987b2679e8e8bae0af451922ec525252fe92/solutions/instances/variables.tf#L33-L40
| type = string | ||
| } | ||
|
|
||
| variable "resource_tags" { |
There was a problem hiding this comment.
different vpc resource support tags (e.g. a VPC can have different tags to subnets). I think this needs to be broken down. VPC tags variable should be explicitly named vpc_resource_tags
|
|
||
| variable "subnets" { | ||
| description = "List of subnets for the vpc. For each item in each array, a subnet will be created. Items can be either CIDR blocks or total ipv4 addressess. Public gateways will be enabled only in zones where a gateway has been created" | ||
| type = object({ |
There was a problem hiding this comment.
I suggest to add a subnet_resource_tags attribute so subnets can individually tagged
| variable "clean_default_sg_acl" { | ||
| description = "Remove all rules from the default VPC security group and VPC ACL (less permissive)" | ||
| type = bool | ||
| default = false |
There was a problem hiding this comment.
hmm, should this default to secure by design (aka true) ?
| } | ||
|
|
||
| variable "existing_cos_instance_crn" { | ||
| description = "CRN of the existing COS instance. It is required to create the bucket used for flow logs." |
There was a problem hiding this comment.
You should mention it is only required if enable_vpc_flow_logs is set to true in the description
| } | ||
| } | ||
|
|
||
| variable "cos_bucket_name" { |
There was a problem hiding this comment.
perhaps call it flow_logs_new_bucket_name so its clear that its a new bucket that will be created for flow logs
| variable "cos_bucket_name" { | ||
| description = "Name of the Cloud Object Storage bucket to be created collect VPC flow logs." | ||
| type = string | ||
| default = "cos-bucket" |
There was a problem hiding this comment.
maybe rename this to flow-logs-bucket
| variable "management_endpoint_type_for_bucket" { | ||
| description = "The type of endpoint for the IBM Terraform provider to use to manage Cloud Object Storage buckets (`public`, `private`, or `direct`). If you are using a private endpoint, make sure that you enable virtual routing and forwarding (VRF) in your account, and that the Terraform runtime can access the IBM Cloud Private network." | ||
| type = string | ||
| default = "public" |
| variable "kms_endpoint_type" { | ||
| type = string | ||
| description = "The type of endpoint to use for communicating with the Key Protect instance. Possible values: `public`, `private`. Applies only if `existing_cos_kms_key_crn` is not specified." | ||
| default = "public" |
| } | ||
| } | ||
|
|
||
| variable "kms_key_ring_name" { |
There was a problem hiding this comment.
should we be more explicit? flow_logs_bucket_kms_key_ring_name ?
|
|
||
| variable "kms_key_ring_name" { | ||
| type = string | ||
| default = "cos-key-ring" |
| description = "The name of the key ring to create for the Cloud Object Storage bucket key. If an existing key is used, this variable is not required. If the prefix input variable is passed, the name of the key ring is prefixed to the value in the `<prefix>-value` format." | ||
| } | ||
|
|
||
| variable "kms_key_name" { |
There was a problem hiding this comment.
should we be more explicit? flow_logs_bucket_kms_key_name ?
|
|
||
| variable "kms_key_name" { | ||
| type = string | ||
| default = "cos-key" |
| storage_class = var.cos_bucket_class | ||
| resource_instance_id = var.existing_cos_instance_crn | ||
| region_location = var.region | ||
| force_delete = true |
There was a problem hiding this comment.
Should we really default this to force delete? I suspect the safer option would be to not force delete by default? Especially since the bucket will contain flow logs, which would be needed for audit purposes. Perhaos it needs to be exposed, defaulted to false but set to true in all our tests (otherwise the destroy would fail)
|
Should we be supporting existing VPC @vburckhardt ? |
Description
Simple VPC DA
Issue: https://github.ibm.com/GoldenEye/issues/issues/12217
Release required?
x.x.X)x.X.x)X.x.x)Release notes content
Run the pipeline
If the CI pipeline doesn't run when you create the PR, the PR requires a user with GitHub collaborators access to run the pipeline.
Run the CI pipeline when the PR is ready for review and you expect tests to pass. Add a comment to the PR with the following text:
Checklist for reviewers
For mergers