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

add apigee non-vpc peering mode support #1648

Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions modules/apigee/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,24 @@ module "apigee" {
module "apigee" {
source = "./fabric/modules/apigee"
project_id = "my-project"
organization = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add more examples and keep this one as it was?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

display_name = "My Organization"
description = "My Organization"
authorized_network = "my-vpc"
runtime_type = "CLOUD"
billing_type = "Pay-as-you-go"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = false
}
instances = {
europe-west1 = {
runtime_ip_cidr_range = "10.0.4.0/22"
troubleshooting_ip_cidr_range = "10.1.1.0/28"
}
}
}
# tftest modules=1 resources=1
# tftest modules=1 resources=2
```

### New endpoint attachment
Expand Down Expand Up @@ -180,13 +190,13 @@ module "apigee" {

| name | description | type | required | default |
|---|---|:---:|:---:|:---:|
| [project_id](variables.tf#L95) | Project ID. | <code>string</code> | ✓ | |
| [project_id](variables.tf#L96) | Project ID. | <code>string</code> | ✓ | |
| [addons_config](variables.tf#L17) | Addons configuration. | <code title="object&#40;&#123;&#10; advanced_api_ops &#61; optional&#40;bool, false&#41;&#10; api_security &#61; optional&#40;bool, false&#41;&#10; connectors_platform &#61; optional&#40;bool, false&#41;&#10; integration &#61; optional&#40;bool, false&#41;&#10; monetization &#61; optional&#40;bool, false&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> | | <code>null</code> |
| [endpoint_attachments](variables.tf#L29) | Endpoint attachments. | <code title="map&#40;object&#40;&#123;&#10; region &#61; string&#10; service_attachment &#61; string&#10;&#125;&#41;&#41;">map&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> | | <code>&#123;&#125;</code> |
| [envgroups](variables.tf#L39) | Environment groups (NAME => [HOSTNAMES]). | <code>map&#40;list&#40;string&#41;&#41;</code> | | <code>&#123;&#125;</code> |
| [environments](variables.tf#L46) | Environments. | <code title="map&#40;object&#40;&#123;&#10; display_name &#61; optional&#40;string&#41;&#10; description &#61; optional&#40;string, &#34;Terraform-managed&#34;&#41;&#10; deployment_type &#61; optional&#40;string&#41;&#10; api_proxy_type &#61; optional&#40;string&#41;&#10; node_config &#61; optional&#40;object&#40;&#123;&#10; min_node_count &#61; optional&#40;number&#41;&#10; max_node_count &#61; optional&#40;number&#41;&#10; &#125;&#41;&#41;&#10; iam &#61; optional&#40;map&#40;list&#40;string&#41;&#41;&#41;&#10; envgroups &#61; optional&#40;list&#40;string&#41;&#41;&#10; regions &#61; optional&#40;list&#40;string&#41;&#41;&#10;&#125;&#41;&#41;">map&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> | | <code>&#123;&#125;</code> |
| [instances](variables.tf#L65) | Instances ([REGION] => [INSTANCE]). | <code title="map&#40;object&#40;&#123;&#10; display_name &#61; optional&#40;string&#41;&#10; description &#61; optional&#40;string, &#34;Terraform-managed&#34;&#41;&#10; runtime_ip_cidr_range &#61; string&#10; troubleshooting_ip_cidr_range &#61; string&#10; disk_encryption_key &#61; optional&#40;string&#41;&#10; consumer_accept_list &#61; optional&#40;list&#40;string&#41;&#41;&#10; enable_nat &#61; optional&#40;bool, false&#41;&#10;&#125;&#41;&#41;">map&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> | | <code>&#123;&#125;</code> |
| [organization](variables.tf#L80) | Apigee organization. If set to null the organization must already exist. | <code title="object&#40;&#123;&#10; display_name &#61; optional&#40;string&#41;&#10; description &#61; optional&#40;string, &#34;Terraform-managed&#34;&#41;&#10; authorized_network &#61; optional&#40;string&#41;&#10; runtime_type &#61; optional&#40;string, &#34;CLOUD&#34;&#41;&#10; billing_type &#61; optional&#40;string&#41;&#10; database_encryption_key &#61; optional&#40;string&#41;&#10; analytics_region &#61; optional&#40;string, &#34;europe-west1&#34;&#41;&#10; retention &#61; optional&#40;string&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> | | <code>null</code> |
| [instances](variables.tf#L65) | Instances ([REGION] => [INSTANCE]). | <code title="map&#40;object&#40;&#123;&#10; display_name &#61; optional&#40;string&#41;&#10; description &#61; optional&#40;string, &#34;Terraform-managed&#34;&#41;&#10; runtime_ip_cidr_range &#61; optional&#40;string&#41;&#10; troubleshooting_ip_cidr_range &#61; optional&#40;string&#41;&#10; disk_encryption_key &#61; optional&#40;string&#41;&#10; consumer_accept_list &#61; optional&#40;list&#40;string&#41;&#41;&#10; enable_nat &#61; optional&#40;bool, false&#41;&#10;&#125;&#41;&#41;">map&#40;object&#40;&#123;&#8230;&#125;&#41;&#41;</code> | | <code>&#123;&#125;</code> |
| [organization](variables.tf#L80) | Apigee organization. If set to null the organization must already exist. | <code title="object&#40;&#123;&#10; display_name &#61; optional&#40;string&#41;&#10; description &#61; optional&#40;string, &#34;Terraform-managed&#34;&#41;&#10; authorized_network &#61; optional&#40;string&#41;&#10; runtime_type &#61; optional&#40;string, &#34;CLOUD&#34;&#41;&#10; billing_type &#61; optional&#40;string&#41;&#10; database_encryption_key &#61; optional&#40;string&#41;&#10; analytics_region &#61; optional&#40;string, &#34;europe-west1&#34;&#41;&#10; retention &#61; optional&#40;string&#41;&#10; disable_vpc_peering &#61; optional&#40;bool, false&#41;&#10;&#125;&#41;">object&#40;&#123;&#8230;&#125;&#41;</code> | | <code>null</code> |

## Outputs

Expand Down
8 changes: 6 additions & 2 deletions modules/apigee/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@ resource "google_apigee_organization" "organization" {
count = var.organization == null ? 0 : 1
analytics_region = var.organization.analytics_region
project_id = var.project_id
authorized_network = var.organization.authorized_network
authorized_network = var.organization.disable_vpc_peering ? null : var.organization.authorized_network
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of changing the value here I'd add validation in the variable ensuring var.organization.authorized_network = null when var.organization.disable_vpc_peering is truee

billing_type = var.organization.billing_type
runtime_type = var.organization.runtime_type
runtime_database_encryption_key_name = var.organization.database_encryption_key
retention = var.organization.retention
disable_vpc_peering = var.organization.disable_vpc_peering
}

resource "google_apigee_envgroup" "envgroups" {
Expand Down Expand Up @@ -91,7 +92,7 @@ resource "google_apigee_instance" "instances" {
description = each.value.description
location = each.key
org_id = local.org_id
ip_range = "${each.value.runtime_ip_cidr_range},${each.value.troubleshooting_ip_cidr_range}"
ip_range = var.organization.disable_vpc_peering ? null : "${each.value.runtime_ip_cidr_range},${each.value.troubleshooting_ip_cidr_range}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before, I'd add a validation to variable (or maybe here you need a precondition check since it's s different variable)

disk_encryption_key_name = each.value.disk_encryption_key
consumer_accept_list = each.value.consumer_accept_list
}
Expand All @@ -117,6 +118,9 @@ resource "google_apigee_instance_attachment" "instance_attachments" {
instance_id = google_apigee_instance.instances[each.value.region].id
environment = try(google_apigee_environment.environments[each.value.environment].name,
"${local.org_id}/environments/${each.value.environment}")
lifecycle {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is ignore_changes needed here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an issue which is occurring because try function is being used for setting the name of the Apigee Environment. Because of this, the actual value for the environment argument cannot be determined until runtime and terraform plan shows force replaces comment for corresponding Apigee Environment. terraform apply force replaces the resource as well. If the same is not fixed, it will end up causing outage on each terraform apply.

lifecycle ignore_changes configuration has been added to overcome the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had some doubts about this too:

  • why do we need try here (to make destroy work? because otherwise it should always evaluate properly)
  • is indeed try the issue here, and not the for_each that does the transformations

But in the end it will work as expected, because if you change the environment name, you change the key in the resource, thus forcing replacement.

I'd love to have a comment here in the code about one and the other (so save my future-self from thinking this over again)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The try function is actually not even required here as if var.environments is {} then the instance attachment resource won't even be getting created.

And on another note, environment attribute requires the name of the Apigee Environment and not the id.
So usage of "${local.org_id}/environments/${each.value.environment}" is not right and instead it should just be each.value.environment. However, since we don't require the try function here, environment = google_apigee_environment.environments[each.value.environment].name should be sufficient.

ignore_changes = [environment]
}
}

resource "google_apigee_endpoint_attachment" "endpoint_attachments" {
Expand Down
5 changes: 3 additions & 2 deletions modules/apigee/variables.tf
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ variable "instances" {
type = map(object({
display_name = optional(string)
description = optional(string, "Terraform-managed")
runtime_ip_cidr_range = string
troubleshooting_ip_cidr_range = string
runtime_ip_cidr_range = optional(string)
troubleshooting_ip_cidr_range = optional(string)
disk_encryption_key = optional(string)
consumer_accept_list = optional(list(string))
enable_nat = optional(bool, false)
Expand All @@ -88,6 +88,7 @@ variable "organization" {
database_encryption_key = optional(string)
analytics_region = optional(string, "europe-west1")
retention = optional(string)
disable_vpc_peering = optional(bool, false)
})
default = null
}
Expand Down
1 change: 1 addition & 0 deletions tests/modules/apigee/all.tfvars
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ organization = {
billing_type = "Pay-as-you-go"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = false
}
envgroups = {
test = ["test.example.com"]
Expand Down
1 change: 1 addition & 0 deletions tests/modules/apigee/all.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ values:
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: false

counts:
google_apigee_endpoint_attachment: 2
Expand Down
7 changes: 0 additions & 7 deletions tests/modules/apigee/instance_only.tfvars

This file was deleted.

18 changes: 18 additions & 0 deletions tests/modules/apigee/instance_only_psc_mode.tfvars
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
project_id = "my-project"
organization = {
display_name = "My Organization"
description = "My Organization"
# authorized_network = "my-vpc"
runtime_type = "CLOUD"
billing_type = "Pay-as-you-go"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = true
}
instances = {
europe-west1 = {
# runtime_ip_cidr_range = "10.0.4.0/22"
# troubleshooting_ip_cidr_range = "10.1.1.0/28"
}
}

37 changes: 37 additions & 0 deletions tests/modules/apigee/instance_only_psc_mode.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Copyright 2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

values:
google_apigee_instance.instances["europe-west1"]:
description: Terraform-managed
disk_encryption_key_name: null
display_name: null
# ip_range: 10.0.4.0/22,10.1.1.0/28
location: europe-west1
name: instance-europe-west1
google_apigee_organization.organization[0]:
analytics_region: europe-west1
# authorized_network: my-vpc
billing_type: Pay-as-you-go
description: null
display_name: null
project_id: my-project
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: true

counts:
google_apigee_instance: 1
google_apigee_organization: 1
18 changes: 18 additions & 0 deletions tests/modules/apigee/instance_only_vpc_mode.tfvars
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
project_id = "my-project"
organization = {
display_name = "My Organization"
description = "My Organization"
authorized_network = "my-vpc"
runtime_type = "CLOUD"
billing_type = "Pay-as-you-go"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = false
}
instances = {
europe-west1 = {
runtime_ip_cidr_range = "10.0.4.0/22"
troubleshooting_ip_cidr_range = "10.1.1.0/28"
}
}

37 changes: 37 additions & 0 deletions tests/modules/apigee/instance_only_vpc_mode.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Copyright 2023 Google LLC
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

values:
google_apigee_instance.instances["europe-west1"]:
description: Terraform-managed
disk_encryption_key_name: null
display_name: null
ip_range: 10.0.4.0/22,10.1.1.0/28
location: europe-west1
name: instance-europe-west1
google_apigee_organization.organization[0]:
analytics_region: europe-west1
authorized_network: my-vpc
billing_type: Pay-as-you-go
description: null
display_name: null
project_id: my-project
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: false

counts:
google_apigee_instance: 1
google_apigee_organization: 1
1 change: 1 addition & 0 deletions tests/modules/apigee/no_instances.tfvars
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ organization = {
billing_type = "PAYG"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = false
}
envgroups = {
test = ["test.example.com"]
Expand Down
1 change: 1 addition & 0 deletions tests/modules/apigee/no_instances.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ values:
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: false

counts:
google_apigee_envgroup: 2
Expand Down
11 changes: 11 additions & 0 deletions tests/modules/apigee/organization_only_psc_mode.tfvars
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
project_id = "my-project"
organization = {
display_name = "My Organization"
description = "My Organization"
authorized_network = null
runtime_type = "CLOUD"
billing_type = "PAYG"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
disable_vpc_peering = true
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,17 @@
# limitations under the License.

values:
google_apigee_instance.instances["europe-west1"]:
ip_range: 10.0.4.0/22,10.1.1.0.0/28
location: europe-west1
name: "instance-europe-west1"
org_id: organizations/my-project
google_apigee_organization.organization[0]:
analytics_region: europe-west1
authorized_network: null
billing_type: PAYG
description: null
display_name: null
project_id: my-project
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: true

counts:
google_apigee_instance: 1
google_apigee_organization: 1
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ organization = {
billing_type = "PAYG"
database_encryption_key = "123456789"
analytics_region = "europe-west1"
}
disable_vpc_peering = false
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ values:
retention: DELETION_RETENTION_UNSPECIFIED
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: false

counts:
google_apigee_organization: 1
3 changes: 2 additions & 1 deletion tests/modules/apigee/organization_retention.tfvars
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ organization = {
database_encryption_key = "123456789"
analytics_region = "europe-west1"
retention = "MINIMUM"
}
disable_vpc_peering = false
}
1 change: 1 addition & 0 deletions tests/modules/apigee/organization_retention.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ values:
retention: MINIMUM
runtime_database_encryption_key_name: '123456789'
runtime_type: CLOUD
disable_vpc_peering: false

counts:
google_apigee_organization: 1
6 changes: 4 additions & 2 deletions tests/modules/apigee/tftest.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,9 @@ tests:
env_only_with_api_proxy_type:
env_only_with_deployment_type:
envgroup_only:
instance_only:
instance_only_psc_mode:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add this additional tests as examples in the readme?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

instance_only_vpc_mode:
no_instances:
organization_only:
organization_only_psc_mode:
organization_only_vpc_mode:
organization_retention: